Figuring out what the code is doing is not the hard part. Documenting the reason you want it to do that (domain knowledge) is the hard part.
Agreed.
And sometimes code is not the right medium for communicating domain knowledge. For example, if you are writing code the does some geometric calculations, with lot of trigonometry, etc. Even with clear variable names, it can be hard to decipher without a generous comment or splitting it up into functions with verbose names. Sometimes you really just want a picture of what's happening, in SVG format, embedded into the function documentation HTML.
TempleOS: Hold my communion wine
Yeah. I advocate for self explanatory code, but I definitely don't frown upon comments. Comments are super useful but soooo overused. I have coworkers that aren't that great that would definitely comment on the most basic if statements. That's why we have to push self explanatory code, because some beginners think they need to say:
//prints to the console
console.log("hello world");
I think by my logic, comments are kind of an advanced level concept, lol. Like you shouldn't really start using comments often until you're writing some pretty complex code, or using a giant codebase.
Comments are super useful but soooo overused
I think overusing comments is a non-issue. I'd rather have over-commented code that doesn't need it, over undocumented code without comments that needs them. If this over-commenting causes some comments to be out of date, those instances should hopefully be obvious from the code itself or the other comments and easily fixed.
Sometimes when I don't leave comments like that, I get review comments asking what the line does. Code like ThisMethodInitsTheService()
with comments like "what does this do?" in the review.
So now I comment a lot. Apparently reading code is hard for some people, even code that tells you exactly what it does in very simple terms.
Fair. I guess in this case, it's a manner of gauging who you're working with. I'd much rather answer a question once in a while than over-comment (since refactors often make comments worthless and they're so easy to miss..), but if it's a regular occurrence, yeah it would get on my nerves. Read the fuckin name of the function! Or better yet go check out what the function does!
Worse, refactors make comments wrong. And there is nothing more annoying then having the comment conflict with the code. Which is right? Is it a bug or did someone just forget to update the comments... The latter is far more common.
Comments that just repeat the code mean you now have two places to update and keep in sync - a pointless waste of time and confusion.
One upvote is not enough.
I once wrote a commit message the length of a full blog post comparing 10 different alternatives for micro optimization, with benchmarks and more. The diff itself was ten lines. Shaved around 4% off the hot path (based on a sampling profiler that ran over the weekend).
I can't recall the exact change but a coworker did something five years very intentionally. The comments, the commit and everything described what they did but not why.
I think it was with side effects: true and I fixed a certain way we bundled things and I believe that could have solved the issue but I don't know for sure :/
Ew no.
Abusing language features like this (boolean expression short circuit) just makes it harder for other people to come and maintain your code.
The function does have opportunity for improvement by checking one thing at a time. This flattens the ifs and changes them into proper sentry clauses. It also opens the door to encapsulating their logic and refactoring this function into a proper validator that can return all the reasons a user is invalid.
Good code is not "elegant" code. It's code that is simple and unsurprising and can be easily understood by a hungover fresh graduate new hire.
Agreed. OP was doing well until they replaced the if statements with ‚function call || throw error’. That’s still an if statement, but obfuscated.
Don't mind the ||
but I do agree if you're validating an input you'd best find all issues at once instead of "first rule wins".
Short circuiting conditions is important. Mainly for things such as:
if(Object != Null && Object.HasThing) ...
Without short circuit evaluation you end up with a null pointer exception.
I agree, this is an anti-pattern for me.
Having explicit throw
keywords is much more readable compared to hiding flow-control into helper functions.
This is the most important thing I've learned since the start of my career. All those "clever" tricks literally just serve to make the author feel clever at the expense of clarity and long-term manintainability.
100% un-nesting that if would have been fine.
I mean, boolean short circuit is a super idiomatic pattern in Javascript
I think that’s very team/project dependent. I’ve seen it done before indeed, but I’ve never been on a team where it was considered idiomatic.
Good code is not “elegant” code. It’s code that is simple and unsurprising and can be easily understood by a hungover fresh graduate new hire.
I wouldnt go that far, both elegance are simplicity are important. Sure using obvious and well known language feaures is a plus, but give me three lines that solve the problem as a graph search over 200 lines of object oriented boilerplate any day. Like most things it's a trade-off, going too far in either direction is bad.
Why the password.trim()
? Silently removing parts of the password can lead to dangerous bugs and tells me the developer didn't peoperly consider how to sanitize input.
I remember once my password for a particular organization had a space at the end. I could log in to all LDAP-connected applications, except for one that would insist my password was wrong. A trim()
or similar was likely the culprit.
Another favorite of mine is truncating the password to a certain length w/o informing the user.
Saving the password truncates but validation doesn't. So it just fails every time you try to log in with no explanation. The number of times I have seen this in a production website is too damn high.
The password needs to be 8 letters long and may only contain the alphabet. Also we don't tell you this requirement or tell you that setting the password went wrong. We just lock you out.
Thanks for the tip. password.trim() can indeed be problematic. I just removed that line.
The reason for leaving in the password.trim()
would be one of the few things that I would ever document with a comment.
In addition to the excellent points made by steventhedev and koper:
user.password = await hashPassword(user.password);
Just this one line of code alone is wrong.
- It's unclear, but quite likely that the type has changed here. Even in a duck typed language this is hard to manage and often leads to bugs.
- Even without a type change, you shouldn't reuse an object member like this. Dramatically better to have password and hashed_password so that they never get mixed up. If you don't want the raw password available after this point, zero it out or delete it.
- All of these style considerations apply 4x as strongly when it's a piece of code that's important to the security of your service, which obviously hashing passwords is.
I would have liked some comments explaining the rules we are trying to enforce or a link to the product requirements for it. Changing the rules requirements is the most likely reason this code will ever be looked at again. The easier you can make it for someone to change them the better. Another reason to need to touch the code is if the user model changes. I suppose we might also want a different password hash or to store the password separately even a different outcome if the validation fails. Or maybe have different ruled for different user types. When building a function like this I think less about "ideals" and more about why someone might need to change what I just did and how can I make it easier for them.
and how can I make it easier for them.
I am wary of this. It is very hard to predict what someone else in the future might want to do. I would only go so far as to ensure nothing I am doing will unnecessarily block a refactor later on but I would avoid trying to add or abstract things in ways that make the current code harder to read because you think it might be easier for someone to add to in the future.
I have needed, far too many times, to strip out some unused abstraction to do something that abstraction was never intended to allow because someone was trying to save me time and predict what might happen to the code in the future and got it completely wrong. It is far easier to add an abstraction to simple code later on when it actually helps then to try and figure out what the abstraction is and remove it when it is found to be wrong.
async function createUser(user) {
validateUserInput(user) || throwError(err.userValidationFailed);
isPasswordValid(user.password) || throwError(err.invalidPassword);
!(await userService.getUserByEmail(user.email)) || throwError(err.userExists);
user.password = await hashPassword(user.password);
return userService.create(user);
}
Or
async function createUser(user) {
return await (new UserService(user))
.validate()
.create();
}
// elsewhere…
const UserService = class {
#user;
constructor(user) {
this.user = user;
}
async validate() {
InputValidator.valid(this.user);
PasswordValidator.valid(this.user.password);
!(await UserUniqueValidator.valid(this.user.email);
return this;
}
async create() {
this.user.password = await hashPassword(this.user.password);
return userService.create(this.user);
}
}
I would argue that the validate routines be their own classes; ie UserInputValidator
, UserPasswordValidator
, etc. They should conform to a common interface with a valid()
method that throws when invalid. (I’m on mobile and typed enough already).
“Self-documenting” does not mean “write less code”. In fact, it means the opposite; it means be more verbose. The trick is to find that happy balance where you write just enough code to make it clear what’s going on (that does not mean you write long identifier names (e.g., getUserByEmail(email)
vs. getUser(email)
or better fetchUser(email)
).
Be consistent:
get*
andset*
should be reserved for working on an instance of an objectis*
orhas*
for Boolean returns- Methods/functions are verbs because they are actionable; e.g.,
fetchUser()
,validate()
,create()
- Do not repeat identifiers: e.g.,
UserService.createUser()
- Properties/variables are not verbs; they are state: e.g.,
valid
vsisValid
- Especially for JavaScript, everything is
const
unless you absolutely have to reassign its direct value; I.e., objects and arrays should beconst
unless you use the assignment operator after initialization - All class methods should be private until it’s needed to be public. It’s easier to make an API public, but near impossible to make it private without compromising backward compatibility.
- Don’t be afraid to use
if {}
statements. Short-circuiting is cutesy and all, but it makes code more complex to read. - Delineate unrelated code with new lines. What I mean is that jamming all your code together into one block makes it difficult to follow (like run-on sentences or massive walls of text). Use new lines and/or
{}
to create small groups of related code. You’re not penalized for the white space because it gets compiled away anyway.
There is so much more, but this should be a good primer.
I would argue that the validate routines be their own classes; ie
UserInputValidator
,UserPasswordValidator
, etc.
I wouldn't. Not from this example anyway. YAGNI is an important paradigm and introducing plenty of classes upfront to implement trivial checks is overengineering typical for Java and the reason I don't like it.
Edit: Your naming convention isn't the best either. I'd expect UserInputValidator
to validate user input, maybe sanitize it for a database query, but not necessarily an existence check as in the example.
I wouldn't. Not from this example anyway. YAGNI is an important paradigm and introducing plenty of classes upfront to implement trivial checks is overengineering…
Classes, functions, methods… pick your poison. The point is to encapsulate your logic in a way that is easy to understand. Lumping all of the validation logic into one monolithic block of code (be it a single class, function, or methods) is not self-documenting. Whereas separating the concerns makes it easier to read and keep your focus without mixing purposes. I’m very-engineering (imo) would be something akin to creating micro services to send data in and get a response back.
Edit: Your naming convention isn't the best either. I'd expect
UserInputValidator
to validate user input, maybe sanitize it for a database query, but not necessarily an existence check as in the example.
If you go back to my example, you’ll notice there is a UserUniqueValidator
, which is meant to check for existence of a user.
And if you expect a validator to do sanitation, then your expectations are wrong. A validator validates, and a sanitizer sanitizes. Not both.
For the uninitiated, this is called Separation of Concerns. The idea is to do one thing and do it well, and then compose these things together to make your program — like an orchestra.
This is abuse of the separation of concerns concepts IMO. You have taken things far too far many made it far less readable overall. The main concern here is password validation - and the code already separated this out from other code. By separating out each check you are just violating another principal - locality of behavior which says related things should be located close to each other. This makes things far easier to read and see what is actually going on without needing to jump through several classes/functions of abstraction.
We need to stop trying to break everything down into the smallest possibly chunks we can. It is fine for a few lines of related code to live in the same function.
A quick glance and this seemed nothing to do with self documenting code and everything to do with the flaws when code isn't strictly typed.
Programming
Welcome to the main community in programming.dev! Feel free to post anything relating to programming here!
Cross posting is strongly encouraged in the instance. If you feel your post or another person's post makes sense in another community cross post into it.
Hope you enjoy the instance!
Rules
Rules
- Follow the programming.dev instance rules
- Keep content related to programming in some way
- If you're posting long videos try to add in some form of tldr for those who don't want to watch videos
Wormhole
Follow the wormhole through a path of communities !webdev@programming.dev