Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm having concerns about readability of this piece of code:

messages_dict = {'error':errors, 'warning':warnings}[severity]
messages_dict[field_key] = message

and I'm to use this instead:

if severity == 'error':
    messages_dict = errors
elif sevierty == 'warning':
    messages_dict = warnings
else:
    raise ValueError('Incorrect severity value')
messages_dict[field_key] = message

But it looks too verbose for such a simple thing.

I don't care too much if it's more efficient than constructing a dictionary for just two mappings. Readability and maintainability is my biggest concern here (errors and warnings are method arguments, so I cannot build the lookup dictionary beforehand and reuse it later).

share|improve this question
    
I don't see the readability concern with the first case. –  Lattyware Jan 25 '13 at 14:43
    
I thought that it might puzzle an unaware reader... –  fortran Jan 25 '13 at 14:52
    
Unaware of what? Dictionaries? They are a core Python data structure - if someone can't read that, they don't know Python. You can't write code that is readable for someone who have no idea. –  Lattyware Jan 25 '13 at 15:08
    
I meant unaware of the 'pattern' (using a dict to choose between two options). I've seen complaints in peer reviews about using boolean operators in a similar way (like x = something or default, maybe a little bit more complex) because it was obscure :-/ –  fortran Jan 25 '13 at 17:21
    
That's a different situation, as it's relatively unclear what is going on (and is better replaced by the ternary operator). This isn't particularly a pattern, it's just a use of dictionaries. –  Lattyware Jan 26 '13 at 14:14

1 Answer 1

Since you mention maintainability I think that the consideration can be taken in terms of what next changes may look like. If further changes will take more statements to be executed with the result of one of those specific options, or if more options will be added, I would stick to the dictionary approach.

Looking into the future can certainly be tricky, but you need to also think how much will the code need to be change to assert a specific new need. If you need to add a new level of reporting, would you prefer to add a new else if or just a value in a dictionary?

Readability then starts to backfire, since if ... else is quite common. What happens when you arrive at 3, 4 or 5 branches?

share|improve this answer

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.