86
Self-documenting Code (lackofimagination.org)
top 50 comments
sorted by: hot top controversial new old
[-] Simulation6@sopuli.xyz 136 points 5 days ago

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.

[-] tatterdemalion@programming.dev 34 points 5 days ago

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.

[-] Flamekebab@piefed.social 10 points 4 days ago

TempleOS: Hold my communion wine

[-] hex@programming.dev 7 points 4 days ago* (last edited 4 days ago)

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.

[-] Faresh@lemmy.ml 5 points 4 days ago

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.

[-] hex@programming.dev 2 points 4 days ago

I understand what you're saying and I mostly agree, but those few instances where a line of code is only slightly different and the comment is the same, can really be confusing.

[-] TehPers@beehaw.org 4 points 4 days ago

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.

[-] hex@programming.dev 5 points 4 days ago

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!

[-] nous@programming.dev 3 points 4 days ago* (last edited 4 days ago)

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.

load more comments (5 replies)
[-] steventhedev@lemmy.world 21 points 5 days ago

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

load more comments (1 replies)
[-] steventhedev@lemmy.world 67 points 5 days ago

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.

[-] traches@sh.itjust.works 47 points 5 days ago

Agreed. OP was doing well until they replaced the if statements with ‚function call || throw error’. That’s still an if statement, but obfuscated.

[-] BrianTheeBiscuiteer@lemmy.world 7 points 5 days ago

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

[-] rooster_butt@lemm.ee 3 points 4 days ago

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.

[-] verstra@programming.dev 20 points 5 days ago

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.

[-] YaBoyMax@programming.dev 14 points 4 days ago

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.

[-] hex@programming.dev 6 points 4 days ago

I mean, boolean short circuit is a super idiomatic pattern in Javascript

[-] arendjr@programming.dev 7 points 4 days ago

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.

load more comments (1 replies)
[-] clutchtwopointzero@lemmy.world 2 points 4 days ago

Because on JS the goal is to shave bytes to save money on data transfer rates

[-] hex@programming.dev 2 points 4 days ago

It's not that deep. It looks nice, and is easy to understand.

[-] lmaydev@lemmy.world 8 points 5 days ago

100% un-nesting that if would have been fine.

[-] Womble@lemmy.world 5 points 5 days ago* (last edited 5 days ago)

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.

load more comments (1 replies)
[-] Atlas_@lemmy.world 21 points 4 days ago* (last edited 4 days ago)

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.

  1. 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.
  2. 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.
  3. 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.
load more comments (6 replies)
[-] koper@feddit.nl 39 points 5 days ago* (last edited 5 days ago)

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.

[-] spechter@lemmy.ml 32 points 5 days ago

Another favorite of mine is truncating the password to a certain length w/o informing the user.

[-] NotationalSymmetry@ani.social 15 points 5 days ago

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.

load more comments (2 replies)
[-] Flipper@feddit.org 10 points 5 days ago

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.

[-] Aijan@programming.dev 16 points 5 days ago* (last edited 5 days ago)

Thanks for the tip. password.trim() can indeed be problematic. I just removed that line.

[-] HamsterRage@lemmy.ca 12 points 5 days ago

The reason for leaving in the password.trim() would be one of the few things that I would ever document with a comment.

[-] Kissaki@programming.dev 2 points 3 days ago* (last edited 3 days ago)

Code before:

async function createUser(user) {
    if (!validateUserInput(user)) {
        throw new Error('u105');
    }

    const rules = [/[a-z]{1,}/, /[A-Z]{1,}/, /[0-9]{1,}/, /\W{1,}/];
    if (user.password.length >= 8 && rules.every((rule) => rule.test(user.password))) {
        if (await userService.getUserByEmail(user.email)) {
            throw new Error('u212');
        }
    } else {
        throw new Error('u201');
    }

    user.password = await hashPassword(user.password);
    return userService.create(user);
}

Here's how I would refac it for my personal readability. I would certainly introduce class types for some concern structuring and not dangling functions, but that'd be the next step and I'm also not too familiar with TypeScript differences to JavaScript.

const passwordRules = [/[a-z]{1,}/, /[A-Z]{1,}/, /[0-9]{1,}/, /\W{1,}/]
function validatePassword(plainPassword) => plainPassword.length >= 8 && passwordRules.every((rule) => rule.test(plainPassword))
async function userExists(email) => await userService.getUserByEmail(user.email)

async function createUser(user) {
    // What is validateUserInput? Why does it not validate the password?
    if (!validateUserInput(user)) throw new Error('u105')
    // Why do we check for password before email? I would expect the other way around.
    if (!validatePassword(user.password)) throw new Error('u201')
    if (!userExists(user.email)) throw new Error('u212')

    const hashedPassword = await hashPassword(user.password)
    return userService.create({ email: user.email, hashedPassword: hashedPassword });
}

Noteworthy:

  • Contrary to most JS code, [for independent/new code] I use the non-semicolon-ending style following JavaScript Standard Style - see their no semicolons rule with reasoning; I don't actually know whether that's even valid TypeScript, I just fell back into JS
  • I use oneliners for simple check-error-early-returns
  • I commented what was confusing to me
  • I do things like this to fully understand code even if in the end I revert it and whether I implement a fix or not. Committing refacs is also a big part of what I do, but it's not always feasible.
  • I made the different interface to userService.create (a different kind of user object) explicit
  • I named the parameter in validatePassword plainPasswort to make the expectation clear, and in the createUser function more clearly and obviously differentiate between "the passwords"/what password is. (In C# I would use a param label on call validatePassword(plainPassword: user.password) which would make the interface expectation and label transformation from interface to logic clear.

Structurally, it's not that different from the post suggestion. But it doesn't truth-able value interpretation, and it goes a bit further.

[-] Rogue@feddit.uk 7 points 4 days ago

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.

[-] graycube@lemmy.world 8 points 5 days ago

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.

[-] nous@programming.dev 7 points 4 days ago

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.

load more comments (1 replies)
[-] dohpaz42@lemmy.world 7 points 5 days ago
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:

  1. get* and set* should be reserved for working on an instance of an object
  2. is* or has* for Boolean returns
  3. Methods/functions are verbs because they are actionable; e.g., fetchUser(), validate(), create()
  4. Do not repeat identifiers: e.g., UserService.createUser()
  5. Properties/variables are not verbs; they are state: e.g., valid vs isValid
  6. Especially for JavaScript, everything is const unless you absolutely have to reassign its direct value; I.e., objects and arrays should be const unless you use the assignment operator after initialization
  7. 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.
  8. Don’t be afraid to use if {} statements. Short-circuiting is cutesy and all, but it makes code more complex to read.
  9. 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.

[-] RecluseRamble@lemmy.dbzer0.com 8 points 5 days ago* (last edited 5 days ago)

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.

[-] dohpaz42@lemmy.world 5 points 4 days ago

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.

load more comments (2 replies)
[-] olafurp@lemmy.world 2 points 4 days ago

I like the service but the constructor parameter is really bad and makes the methods less reusable

[-] dohpaz42@lemmy.world 2 points 4 days ago

That’s fair. How would you go about implementing the service? I always love seeing other people’s perspectives. 😊

[-] olafurp@lemmy.world 1 points 3 days ago

More or less the same but the user gets passed as a method parameter each time. Validators would be in my opinion a long function inside the service also with named variables like this because it's just easy to read and there are no surprises. I'd probably refactor it at around 5 conditions or 30 lines of validation logic.

I recommend trying out using the constructor in services for tools such as a database and methods for data such as user. It will be very easy to use everywhere and for many users and whatever

const passwordIsValid = ...
if (!passwordIsValid){
  return whatever
}
load more comments (1 replies)
load more comments
view more: next ›
this post was submitted on 21 Oct 2024
86 points (86.4% liked)

Programming

17269 readers
232 users here now

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



founded 1 year ago
MODERATORS