[irishsultan]

While I still would add a comment about the why, your last bit of code probably should be written without magic constants.

    # Some countries have sales tax rules dependent on the day of the week
    return nil if country_code==KERPLAKISTAN and day_of_week==MONDAY

The exact comment here could probably be more specific (e.g. where do you find these rules), but it also most likely shouldn't repeat the code (and the code should make clear what it represents).

[mikekchar]

If you do the substitution as you suggest and then add a unit test, then you have something ;-) Something on the lines of "describe countries with sales tax dependent upon the days of the week => Kerplakistan doesn't have sales tax on Mondays" So now it's self documented and self testing.

But I agree with your statement that there should be a pointer to the business rules somewhere. Otherwise it's difficult to have a meeting with the business side and ask, "Has anything here changed?" I think that's the biggest thing people miss out -- It's not that hard to find the thing in the code if things change. It's super hard to make sure you are on top of all the business requirement changes.

[arethuza]

But don't do what one memorably awful project I had to maintain did - to use that example they would have done:

country_code==FIFTY_FIVE and day_of_week==ONE

[spuz]

But what if the definition of 55 changes? You'll be glad to have your table of constants then.

[arethuza]

The project also defined HTTP, COLON, SLASH, WWW and DOT so that you would have:

   string url = HTTP + COLON + SLASH + SLASH + WWW + DOT ...

I swear I'm not making this up....

[chessturk]

Reminds me of http://pk.org/rutgers/notes/pikestyle.html

> "There is a famously bad comment style: ...

Don't laugh now, wait until you see it in real life."

[snovv_crash]

Well at least a typo sould give a compile time error for some subset of typos.

But in the trade-off in code readability was probably the cause of many other mistakes, so probably ended up further behind.

[rpmisms]

Sounds like a PHP codebase I'm currently working in. I shit you not, $LI = '<li>' is in the functions file, along with $LI_END.

[arethuza]

It was a very enterprisey Java codebase from the bad old days of J2EE - it had somewhere over 30 layers of abstractions between the code in a JSP and a web service call.

[NB 30 isn't an exaggeration - I think the vast team who wrote it were paid by the abstraction or something].

[nitrogen]

But how else will your compiler tell you that you mistyped 55? /s

[JohnBooty]

That's a very good point. Avoiding magic numbers would have removed the need for an explanatory comment in my example.

[richardwhiuk]

Comments are often a code smell. In lots of examples, better variable naming, breaking something out into a function, or constants often reduces the need for a code comment.

[kickopotomus]

I disagree. Code ages and people move on. 2 years down the line some new guys are maintaining the code base. Some new guy is testing the system and notices that sales tax values seem to be "strange" for Kerplakistan on certain days of the week so they create a ticket for it. Then that goes through the typical pipeline. Another member of the team gets assigned the issue and looks into it. They come across the line:

  return nil if country_code==KERPLAKISTAN and day_of_week==MONDAY

Hmm.. Well that's strange. I don't have a background in Kerplakistan monetary policy so I don't know why we aren't assessing sales tax on Monday. Perhaps Kerplakistan is a special case. Is that being handled somewhere downstream? Then 1-2 hours later, after shuffling through source and eventually just Googling Kerplakistan sales taxes, you discover what someone found out 2 years ago when they wrote that line. Now you resolve the ticket and move on with your day but you just wasted a couple man-hours on a non-issue that could have been resolved instantly from a code comment.

Comments are as much for the next guy as they are for you.

[richardwhiuk]

Without a more concrete example, it's difficult to suggest what the better fix would be.

Code smell doesn't mean you should never do it, just that often there's a better way.

[JohnBooty]

Here's a more real-world example.

I worked on an enterprisey line of business app that assigned sales leads to salespeople.

The algorithm to do this was a multi-step process that was (1) rather complex (2) constantly being tweaked (3) very successful (4) contained a number of weighting factors that were utterly arbitrary even to veterans of this app.

It was full of many `if country_code==KERPLAKISTAN && day_of_week==MONDAY` -style weighting factors. Each represented some hard-won experience And when I say "hard-won" I mean "expensive" -- generating leads is expensive business.

We had a strong culture of informative commit messages, but this file had hundreds if not thousands of commits over the years.

It was the kind of code that resisted serious refactoring or a more streamlined design because it was a recipient of frequent change requests.

A few human-readable comments here and there went a loooong way toward taming the insanity and allowing that module to be worked on by developers besides the original author.

Knowing the why for many of these rules made it much easier to work with, and also allowed developers to be educated about the business itself.