Similarly, I've seen some developers who seem to think that any inline string or numeric constant is evil. In one PR, I saw:
HTTPS_SCHEME = 'https'
DOMAIN = 'www.example.com'
url = HTTPS_SCHEME + '://' + DOMAIN
I’m a big fan of closeness in code. I prefer defining things as closely to where it’s used as possible. This is a big pet peeve for me!
Do not put regex at the top of the file either! Put it where you use it. Languages are smart, they’ll probably be able to tell that it’s constant anyway.
Also for tiny functions just use a lambda. Please don’t make a one line function a million miles away that you use once or twice.
Amen! The existence of 'helpers.js', 'utils.cc', makes me twitch.
If multiple things use the same regex, which one should it be close to? Or do you propose duplicating it?
Having the constants at the top is more easily customizable, especially should this file get duplicated. If devs need to switch to http instead of https for testing or staging, it makes sense to separate the scheme from the domain and put the constants up top or even in another file. It also matters whether ‘url’ was constructed in multiple places or a single place. Having named constants at the top of the file is a very common style, and sometimes is part of the group coding standards.
Anyway, maybe there are other reasons too, so see Chesterton’s Fence. In any case, it’s never a good idea to assume cargo culting. Someone could easily say the same thing about using inline literals. If it looks weird, ask around and maybe you’ll find out there are good reasons, or maybe you’ll find out nobody cared and that people will like it if you refactor and embed the constants.
In my opinion (and it's just that - an opinion, and mine - yours may differ), it's better to make the code as stupid simple as possible. When you build it, don't assume future change in that spot, because you could be terribly wrong about exactly what would change, so you're adding complexity for no benefit. The first time you need to change the scheme from `https` to `http`, just change it inline in the URL building code. The second time you need to do it, make a constant or an env var.
Over time, everyone develops their own intuition and opinions on what sorts of change and refactors are likely, and which sorts will never come. It's not perfect - it's part of the art of software, not the science. I'm not going to claim I've never written an interface that had a useless cut-point - I definitely have written many.
In my opinion, it's better to err on the side of simplicity, understandability, and closeness than to prematurely factor out those constants.
It's a judgement call - every situation and every practitioner will have a different response. In this case, knowing this product, and as the code reviewer for this patch made by a newer engineer (senior but new to the team), I was very confident that we would never need to change the scheme, and that the extra line and extra cognitive load of groking `HTTPS_SCHEME + "://"` over just `"https://"` or `BASE_URL + blah` was not worth it.
I suggested that the engineer either rewrite it with an inline constant, or refactor the whole base URL out without separating the scheme.
That particular example doesn't quite fit, but I've certainly seen cases where otherwise perfectly ordinary fixed strings needed to be broken up to meet linting rules.
Ah yeah those are bad linting rules then. That's one of my pet peeves. Inconsistent style is better than style forced into jankiness. Also... linters should auto-format whenever possible. I'll make a few exceptions, but broadly I like to set the enforcement of linting rules to only those cases that are honestly confusing or misleading.
If your linter is making you do something stupid fire your linter or learn how to exempt a certain line from a certain rule.
I ran into this as well. If an Event has a name, you can instantly grep across a giant monolith (or a big folder of microservice repos) and find every file that is concerned with that event.
If you pull it out into a constant, you're back to opening up projects one-by-one to 'find usages'
grepability is underrated. This is one of the reasons I dislike passing functions into interfaces - it makes it harder to follow the call chain when the functions are constantly renamed.
Luckily there are often solutions that just come down to which arbitrary labels that you assign. e.g. compare these two blocks
versus The first is much more greppable - search for "\bdo_thing" all over the code base. The second requires realizing that the processor function renames the internal function to "thing_processor".