> On March 29, 2012, 5:28 a.m., Vaalith Jinn wrote: > >
well said, Boroondas Yes, it's clearer to use the enum, and yes an enum parameter can have a default > 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. > > Boroondas Gupte wrote: > 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? Since the enum would be part of the interface to the public function, it shouldn't be private, and belongs right there next to the method: typedef enum { FIRST_USE, REPLACING } UpdateType; bool updateSelf(UpdateType usage); and then the test inside the method becomes: if (FIRST_USE == usage) - Oz ----------------------------------------------------------- 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 > >
_______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
