3 ms·

One pattern that this guy misses is "better to be clear than to be clever". His "a == x || a == y..." example is a case of verbosity being a good thing, and pro

by erez 15y ago

One pattern that this guy misses is "better to be clear than to be clever". His "a == x || a == y..." example is a case of verbosity being a good thing, and probably the reason why most languages don't have a == (x or y or z) in their syntax. And then, if this isn't convoluted enough, he hides it behind an abstraction, which is another anti-pattern: Whatever you need to hide, you need to refactor. Hiding your cleverness away from the user is never a good idea. He even excuses it with "This refactoring allows others to read the code in context to the domain, without having to comprehend the internal logic." Sadly, this "Don't make me think" attitude is very common.

morsch 15y ago
He proposes to replace if(current_day == "Monday" || current_day == "Wednesday" || current_day == "Friday") with if(["Monday", "Wednesday", "Friday"].include?(current_day)) I've done this before, and I don't think it's any less clear -- I wouldn't have thought of making a public announcement about it, though. I like it because it more closely aligns with the way you're probably thinking in this example: you're wondering if current_day is part of a certain set of days (the discount weekdays, apparently), not if it's equal to "Monday" or equal to "Tuesday", etc. I like the Javascript example with it's indexOf(..) >= 0 a lot less, but I guess people more used to JS idioms auto-translate this sequence to a set-contains operation. It's more sensible if the set of values is larger than just three; and in this instance, you have to wonder why the day isn't available as a numeric variable, but I think you can let it slide for a contrived example. It's even more useful if you reference the same set more than once, because you can just define it once, and as a constant if your language of choice does that kind of thing. This would also let you make your code less wordy (but arguably more literal) without hiding logic in functions, e.g. his later example would be if(discount_weekdays.include?(current_day) && current_date > 20)
- erez 15y ago
  "It's more sensible if the set of values is larger than just three ... It's even more useful if you reference the same set more than once" Both are correct, I am strictly referring to his rationale being it's too verbose.
- Inufu 15y ago
  So like Python's 'in'? if 5 in [1, 2, 5, 6, 8]: pass
- valisystem 15y ago
  case current_day when "Monday", "Wednesday", "Friday" then <do stuff> end
- bingaling 15y ago
  do_stuff() if $current_day ~~ @discount_weekdays;
KevinEldon 15y ago
I think his code choices are clear. The domain logic is "if it is a discount day then...". Keeping the calculation for what is and is not a discount day in the if statement muddles the logic because now you have the domain logic and logic for calculating whether or not today is a discount day in the same block of code. The discount day calculation is complex enough by the end (before memoization) that you'd likely end up adding a comment above the if statement to explain that you're calculating a discount day. His choice to use [].include? is idiomatic ruby so I don't think it's being clever for clever's sake... although I've always kind of stumbled over that construct, I sort of wish there was an 'in' statement... if current_day in [x,y,z]. It would be easier to read (for me at least).
- obtu 15y ago
  > current_day in [x, y, z] Your pseudocode is idiomatic Python code :)
  - gbog 15y ago
    Yes, it did strike me how JS and Ruby are hard to read compared to Python's "if x in [a, b, c]". Moreover, the memoise part is better handled with a decorator: it is a special behavior of the function and don't relate to its logic (its body).

by erez 15y ago

morsch 15y ago

He proposes to replace if(current_day == "Monday" || current_day == "Wednesday" || current_day == "Friday") with if(["Monday", "Wednesday", "Friday"].include?(current_day)) I've done this before, and I don't think it's any less clear -- I wouldn't have thought of making a public announcement about it, though. I like it because it more closely aligns with the way you're probably thinking in this example: you're wondering if current_day is part of a certain set of days (the discount weekdays, apparently), not if it's equal to "Monday" or equal to "Tuesday", etc. I like the Javascript example with it's indexOf(..) >= 0 a lot less, but I guess people more used to JS idioms auto-translate this sequence to a set-contains operation. It's more sensible if the set of values is larger than just three; and in this instance, you have to wonder why the day isn't available as a numeric variable, but I think you can let it slide for a contrived example. It's even more useful if you reference the same set more than once, because you can just define it once, and as a constant if your language of choice does that kind of thing. This would also let you make your code less wordy (but arguably more literal) without hiding logic in functions, e.g. his later example would be if(discount_weekdays.include?(current_day) && current_date > 20)

erez 15y ago
"It's more sensible if the set of values is larger than just three ... It's even more useful if you reference the same set more than once" Both are correct, I am strictly referring to his rationale being it's too verbose.
Inufu 15y ago
So like Python's 'in'? if 5 in [1, 2, 5, 6, 8]: pass
valisystem 15y ago
case current_day when "Monday", "Wednesday", "Friday" then <do stuff> end
bingaling 15y ago
do_stuff() if $current_day ~~ @discount_weekdays;

KevinEldon 15y ago

I think his code choices are clear. The domain logic is "if it is a discount day then...". Keeping the calculation for what is and is not a discount day in the if statement muddles the logic because now you have the domain logic and logic for calculating whether or not today is a discount day in the same block of code. The discount day calculation is complex enough by the end (before memoization) that you'd likely end up adding a comment above the if statement to explain that you're calculating a discount day. His choice to use [].include? is idiomatic ruby so I don't think it's being clever for clever's sake... although I've always kind of stumbled over that construct, I sort of wish there was an 'in' statement... if current_day in [x,y,z]. It would be easier to read (for me at least).

obtu 15y ago
> current_day in [x, y, z] Your pseudocode is idiomatic Python code :)
- gbog 15y ago
  Yes, it did strike me how JS and Ruby are hard to read compared to Python's "if x in [a, b, c]". Moreover, the memoise part is better handled with a decorator: it is a special behavior of the function and don't relate to its logic (its body).