[opensource-dev] Review Request: STORM-64: Local Bitmaps 3.0 implementation.

[Boroondas Gupte]

> On March 29, 2012, 5:28 a.m., Vaalith Jinn wrote:
> > indra/newview/lllocalbitmaps.h, line 42
> > <http://codereview.secondlife.com/r/566/diff/1/?file=7720#file7720line42>
> >
> >     For clarity's sake i kept private enums block below where it's currently at.
> >     
> >     If i were to add an enum value in here i'd have to either move the whole block up to keep consistency or break consistency and write in a single enum below the main public block..
> >     
> >     Neither is a really pretty solution as far as readability is concerned.
> >     
> >     Is it really worth it?
> 
> Boroondas Gupte wrote:
>     I think it'd be worth it, as it'd make the calling code much more readable. See http://doc.qt.nokia.com/qq/qq13-apis.html#thebooleanparametertrap
>     
>     Note that in the example at hand, the function will only be called with boolean literals, not boolean variables, so we don't have the problem of forcing additional if-else code onto the caller. Also, half if not all of the lengthy comment above the one place where you call the function with "true" could be saved if a well-named constant or enum would be passed instead.
> 
> Vaalith Jinn wrote:
>     Um... several questions:
>     
>     A) Unless i am misreading, are you implying that an enum would make it easier to understand than the existing albeit lengthy comment?
>     
>     B) The way i designed it is actually the way one would see the mechanism: The updateself function updates by default, it's what it does no more no less.
>     The bool is there to indicate a special case, special because in the life of a single local bitmap object it is only used once and that is during it's creation.
>     
>     So to me it seems that forcing the function to take a "run regularly" enum parameter during each regular, non first-run call would be something akin to stating the obvious,
>     which would seem redundant to me to the point of causing confusion.

A) Yes, that's what I'm implying. It might not be true for every reader, just as there is no single writing style that can be considered more understandable to everyone then every other writing style. But for many (me included), switching between reading comments written in natural language and code in a programming language requires a little effort, so one tends to either read just the code, skipping over just the comments, or the comments, skipping over the code. A good use of both seems to try explaining *what* you do with the code itself and *why* you do it (if not obvious) in the comments.

The advantage for self-explanatory code over comments it that it is precise, up-to-date and "true". If code gets changed and comments are not adapted accordingly, remaining out-of-date comments might confuse or even mislead the reader. (Maybe unlikely to become an issue right here, but this is an important point I took away from reading Robert C. Martin's "Clean Code" book.)

B) Can't enum arguments have default values, too?


- Boroondas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/566/#review1199
-----------------------------------------------------------


On March 30, 2012, 4:06 p.m., Vaalith Jinn wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/566/
> -----------------------------------------------------------
> 
> (Updated March 30, 2012, 4:06 p.m.)
> 
> 
> Review request for Viewer, Callum Linden and Richard Nelson.
> 
> 
> Description
> -------
> 
> Local Bitmaps is a mechanism to locally load images into the viewer, track them and optionally (per each image)
> have it check if the image has been overwritten locally and if so - update it in the viewer and inworld.
> 
> This change affects the texture picker - adding radio checks that let the user choose between the "Inventory" (regular inventory) and "Local" tabs (list of locally added files).
> 
> *** DO NOT USE THIS YET ***
> do not implement for live viewers until fully reviewed.
> 
> 
> This addresses bug STORM-64.
>     http://jira.secondlife.com/browse/STORM-64
> 
> 
> Diffs
> -----
> 
>   doc/contributions.txt 96a53bafcd1c 
>   indra/newview/CMakeLists.txt 96a53bafcd1c 
>   indra/newview/lllocalbitmaps.h PRE-CREATION 
>   indra/newview/lllocalbitmaps.cpp PRE-CREATION 
>   indra/newview/lltexturectrl.h 96a53bafcd1c 
>   indra/newview/lltexturectrl.cpp 96a53bafcd1c 
>   indra/newview/llviewertexturelist.h 96a53bafcd1c 
>   indra/newview/llwearable.h 96a53bafcd1c 
>   indra/newview/llwearable.cpp 96a53bafcd1c 
>   indra/newview/skins/default/xui/en/floater_texture_ctrl.xml 96a53bafcd1c 
> 
> Diff: http://codereview.secondlife.com/r/566/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vaalith Jinn
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20120331/d4b5c144/attachment.htm