Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

Let's say I have a not-so-intuitive if statement in my code, but only if you're new to the codebase:

def set_markets(markets=None):
    """
    Will accept 'all' as markets to set all markets on.
    For individual markets, pass in a list of ints.
    """
    if markets:
        if type(markets) == list:
            markets = ','.join(map(str, markets))
        self._set_markets(markets)

For those of you wondering:

The existing system I'm interacting with (a database) expects a list of integers from the front end interface. This code does it from the backend, so a list of ints was chosen since it best represents what a user might feed in otherwise.

The markets stuff is handed down from another class, so even though it appears like it's taking place over 5 lines, it's more like ~50 lines.

My question is, would you consider this more readable? Why or why not?

def set_markets(markets=None):
    """
    Will accept 'all' as markets to set all markets on.
    For individual markets, pass in a list of ints.
    """
    if markets:
        if type(markets) == list:
            markets = ','.join(map(str, markets))
      # else:
          # markets are set to "all"
        self._set_markets(markets)

EDIT:

This code looks worse than it is because I left off a detail: if None is fed in for markets (the default parameter), then it will not modify the markets settings the database.

Here's what I have now:

    if markets is not None:
        if isinstance(markets, list):
            markets = ','.join(map(str, markets))
        else:
            markets = 'all'
        self._set_new_header_markets(markets)
share|improve this question
2  
Just a tip, use isinstance(markets, list) as the proper way to check the type of an object in Python. –  Greg Hewgill Mar 4 '13 at 21:42
    
Interpreting pretty much anything as "all" strikes me as very bad. I think it'd be way better to only accept "all" for that. –  Winston Ewert Mar 4 '13 at 22:23
add comment

2 Answers

up vote 13 down vote accepted

Firstly, putting code in comments makes me assume it's dead code. It'll just be really confusing if you do that.

Secondly, the fact that you need a comment is because your code is unreadable. Try for making the code readable before adding a comment.

If you accept 'all' or a list, you should really do it like this:

if markets == 'all':
   self._set_markets('all')
else:
   self._set_markets(','.join(map(str, markers))

Then its abundantly clear what is going on and you avoid type checking, and you don't accept invalid data so long as it isn't a list.

share|improve this answer
    
I updated my question based on your observations. –  Droogans Mar 4 '13 at 21:53
    
Accepted answer for "the fact that you need a comment is because your code is unreadable". –  Droogans Mar 4 '13 at 21:55
add comment

To me, the fact that you've formatted the else: to look like regular code is misleading; something more commenty seems like it'd be appropriate.

Right now it looks like the else is commented out because there is no code for that block yet, and the inner comment is a placeholder for code to come, which is not the case (?). I'd inline both comments, bump up the indentation on the hash and maybe remove the else's formatting:

if markets:
    if type(markets) == list:
        markets = ','.join(map(str, markets))
    # else: markets are set to "all"
    self._set_markets(markets)
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.