It is not a mistake. One literal 3 is not distinguishable from another literal 3. If you change it in one place but forget about another - that can lead to the inconsistent behaviour. Configuration variables are much less error prone
Configuration variables make the code ever-so-slightly longer, and introduces additional failure points (what if the config file is missing or corrupted?) and considerations for cases that will never arise with hard-coded values (should we check that the limit is always positive? what if it is 10000 months, should we do something?).
Yes, in many cases they are an improvement, but not always. Every "best practice" must be judged individually with the question "does it apply here?".
Years ago I was working with some very small piece of a web-crawling framework, and I had to do something special for www.naver.com, Korea's biggest portal. Because the string "www.naver.com" appeared multiple times in the same function, we put that into a string constant at the beginning: something like
static const string kNaverUrl = "www.naver.com";
Because having "magic constant" sprinkled in the code is bad!
...it was only much later that I realized the stupidity. As if Naver, Korea's #1 portal, will change its domain name at a whim! Even in the unlikely case that happens (say, they get acquired), so many other things will also change that my code will be surely unusable after that.
kNaverUrl was an unnecessary detour that only served to make the code longer and (slightly) less obvious, guarding against a theoretical possibility that will never come true.
Code Review has this nasty tendency of accumulating "best practices" beyond breaking point, because nobody objects to best practices. Once in a while, these practices turn out not to be that best.
I don't know how does your editor work, but my Vim diligently helps me with
identifier completion, so kNaverUrl wins every time with "www.naver.com", even
if it doesn't make the code more flexible.
It also defends against typos, as serge2k pointed.
It's also easier to refactor code without magic constants, as you have compile time bugs mispelling kNaverUrl instead of runtime bugs because "ww.naver.com" in one of 10 places.
You have a point, but on the other hand, it is much easier to write (kNaverUrl + "index.html") than to write "www.naver.comindex.html". So it just opens door for a different kind of bugs.
> I had to do something special for www.naver.com, Korea's biggest portal. Because the string > "www.naver.com" appeared multiple times in the same function, we put that into a string
> constant at the beginning: something like
> static const string kNaverUrl = "www.naver.com";
> Because having "magic constant" sprinkled in the code is bad!
You have changed your magic string into a magic constant. It's a step towards better code, but the next step is to give the thing a proper name. In this case, something like DoSomethingSpecialUrl. Now you have a bit of self-documenting code.
> As if Naver, Korea's #1 portal, will change its domain name at a whim
Maybe they won't change their domain name, but what if you end up needing to move from http to https? Or what if you want to crawl their staging server at staging.naver.com? Or if you wanted to bypass the DNS?
Avoiding the magic constant achieves more than just guarding against name changes. It also guards against typos, you reduce n potential failure points to 1.
It was a hard-coded value before. The OP just wanted to leave it as a hard-coded value. The world would have been just as hard-coded before his change as after, and it would have been improved as little by the change. The policies in place seem to set the perfect against the good.
My take on this is modifying the hard coded constant was not a functional change to the code base. Making it configurable was a functional change. One shouldn't make functional changes to old battle tested code unless you have to. If you do that it's just risk.
Configuration variables are, in my experience, considerably more error prone. If something is in a configuration file, some day, somebody is going to want to configure it, not knowing how it works, what the valid values are, or what it even does. Moving something to a configuration variable means you have to do all the work to sanitize that input and defend against invalid values.
Hearing "Oh, make setting XYZ configurable" or "Stick a flag to turn FooBar'ing on or off" also fills me with worry, as all too often it has been the harbinger of terrible feature creep and piling on of technical debt.
Nobody set it needs to be a magic number. A descriptive constant name will address that. However, a setting in a config file invites tampering, which in turn means more complicated tests to account for that. It might make sense if frequent changes are anticipated.
However, not every hard coded constant should or needs to be in a config file.
