Hacker News Reader

Comment by erez | Hacker News Reader

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.

View on HN

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)

"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.

So like Python's 'in'?

  if 5 in [1, 2, 5, 6, 8]:
      pass

    case current_day when "Monday", "Wednesday", "Friday" then <do stuff> end

do_stuff() if $current_day ~~ @discount_weekdays;

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).

> current_day in [x, y, z]

Your pseudocode is idiomatic Python code :)

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).

Reply on news.ycombinator.com