[haiku-bugs] Re: [Haiku] #7951: [LaunchBox] add Open containing folder option (easy)

  • From: "axeld" <trac@xxxxxxxxxxxx>
  • Date: Sat, 24 Mar 2012 21:10:35 -0000

#7951: [LaunchBox] add Open containing folder option (easy)
   Reporter:  diver                   |      Owner:  stippi
       Type:  enhancement             |     Status:  new
   Priority:  normal                  |  Milestone:  R1
  Component:  Applications/LaunchBox  |    Version:  R1/Development
 Resolution:                          |   Keywords:
 Blocked By:                          |   Blocking:
Has a Patch:  1                       |   Platform:  All

Comment (by axeld):

 Thanks for the patch! It looks basically good, and everything seems to be
 at the right place. I have a few suggestions, though, and there are a few
 coding style violations:

 * Please don't use abbreviations that hide the meaning of the word. For
 example, the `CONT` in the message constant should just stay `CONTAINING`.
 * Why add the button as reference if you only ever use its Ref()? You
 could add the entry_ref directly to the message instead.
 * If the method you are calling isn't documented to return a value
 different from B_OK on success, you should only test ==, not >=.
 * We prefer to use `button->Ref() != NULL` in comparisons to make it a
 boolean comparison for clarity.
 * The code you are using does not do any error checking. The only effect
 this will have in this case is that nothing happens, so that's probably
 acceptable. In any case, you might want to only add that menu item if the
 button is not empty, and has a valid entry_ref in the first place.
 * Instead of nesting to if's without any extra code, you could use a &&
 and save an extra indenting level.
 * The variable names you chose aren't coding style compliant; we use camel
 case, and variable names always start with a lower case letter. In other
 words, `Opentarget` would become `openTarget`, `Openmsger` could be called
 `tracker` instead, or, if you prefer `trackerMessenger`, etc.
 * You sometimes use no spacing, like after the second `if`, or around the
 `=` operator.

 Would be great if you could take another look at your code with the
 comments in mind :-)

Ticket URL: <http://dev.haiku-os.org/ticket/7951#comment:6>
Haiku <http://dev.haiku-os.org>
Haiku - the operating system.

Other related posts: