[Bugs] #1592 UNSP: NEW FEATURE: enhanced color selector

[Sugar Labs Bugs]

#1592: NEW FEATURE: enhanced color selector
------------------------------------------+---------------------------------
    Reporter:  walter                     |          Owner:  tomeu      
        Type:  enhancement                |         Status:  new        
    Priority:  Unspecified by Maintainer  |      Milestone:  0.88       
   Component:  sugar                      |        Version:  Unspecified
    Severity:  Unspecified                |       Keywords:  r!         
Distribution:  Unspecified                |   Status_field:  Unconfirmed
------------------------------------------+---------------------------------
Changes (by tomeu):

  * keywords:  r? => r!


Comment:

 > +            [self.strole, self.fill] = self.get_random_color()

 Typo

 > +        # save an index to our color in the list

 Superfluous comment, see
 http://wiki.sugarlabs.org/go/Development_Team/Code_guidelines about
 comments.

 > +        self.n = self.find_index()

 Better use descriptive identifiers, here I would use self._current_color
 or self._current_color_index, note the leading underscore because it's a
 private member.

 > +            logging.debug('Color string is not valid: %s, '
 > +                          'fallback to default', color_string)

 Are we really falling back to a default? Better logging.error instead of
 logging.debug or even better, raise a ValueError exception.

 > +        [self.stroke,self.fill] = _parse_string(color_string)

 Watch out the space after ',', see PEP-08 about whitespace.

 > +    def get_random_color(self):

 Why is this an instance method? Shouldn't be a function in the module
 scope as it has nothing to do with any particular XoColor instance?

 > +        my_n = int(random.random() * (len(colors) - 1))

 Every time you create a new identifier, you have a chance to make your
 code more readable by choosing descriptive names. I would use color_index,
 random_color, etc.

 > +        [my_stroke, my_fill] = colors[my_n]

 stroke, fill = colors[color_index] should be enough

 > +        my_n = self.n
 > +        my_n += 1

 Suggest doing instead next_index = self._current_color + 1

 > +    def get_prev_color(self):

 Try to avoid abbreviations.

 > +    def find_index(self):

 If this method is not used outside its class, it should be private (see
 PEP-08 about scopes).

 > +        for c in range(0,len(colors)):

 Maybe s/c/color?

 > +        # if the color is not found, then return 0

 Not necessary

-- 
Ticket URL: <http://bugs.sugarlabs.org/ticket/1592#comment:1>
Sugar Labs <http://sugarlabs.org/>
Sugar Labs bug tracking system