Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Self-Documenting Code

lackofimagination.org

Post reply on HN
Print view View original

121–130 of 131 posts

Re: Self-Documenting Code

#121

Post by fenomas »

jonathanlydall wrote: post #105
fenomas wrote: post #92

Earlier quoted context omitted.

It definitely depends, but personally I find early returns to be a bit of an antipattern IF they're based on business logic. If a function has lots of ifs, you can glance at the nesting to see which ones affect the line you want to edit, and ignore the others. But if the function has lots of returns, you have to check every one before a given line in order to know what constraints are true at that point. OTOH early r…

Calling it an anti pattern (as opposed to a subjective preference) is in my opinion super dangerous and reeks of cargo-culting as it implies an active avoidance of it which can result in deep nesting or contorted and hard to read logic. There is no hard rule about early returns being always good or always bad, it depends on the particular situation.

> > It definitely depends, but personally I find..

> ..(as opposed to a subjective preference) is in my opinion super dangerous and reeks of cargo-culting..

o_O

Re: Self-Documenting Code

#122

Post by jansommer »

OptionOfT wrote: post #46
johnfn wrote: post #34

My cut: const passwordRules = [/[a-z]{1,}/, /[A-Z]{1,}/, /[0-9]{1,}/, /\W{1,}/]; async function createUser(user) { const isUserValid = validateUserInput(user); const isPasswordValid = user.password.length >= 8 && passwordRules.every((rule) => rule.test(user.password)); if (!isUserValid) { throw new Error(ErrorCodes.USER_VALIDATION_FAILED); } if (!isPasswordValid) { throw new Error(ErrorCodes.INVALID_PASSWORD); } cons…

My issue with this is that you're using exceptions for control flow. A user not being valid is expected (duplicate username). A password not matching a regex is also expected. Then, in general (not seen here as there are no types), I like to use a lot of types in my code. The incoming user would be of type UnvalidatedUser, whereas the return type of this function would be StoredUser or something like that to distingu…

It's a good point, especially because different return types is well supported in TS.

I made the same in plpgsql recently and opted for returning an implicit union of UserSession | Error, by returning UserSession in the signature and raising errors in the function. The alternative was to return json where you'd have to look at the body of the function to figure out what it returns (when successful), as opposed to the signature.

I'm not sure if I'm striking the right balance. Yes, the signature is "self-documenting" - until you hit an error!

Re: Self-Documenting Code

#123

Post by jansommer »

> The first change I would make is to use named constants instead of cryptic error codes.

But he keeps the cryptic error codes that will go into the logs, or in the frontend where the developer will have to look up the error code. Don't map an error name to u105, just return the actual string: "userValidationFailed".

Re: Self-Documenting Code

#124

Post by kazinator »

fenomas wrote: post #111
kazinator wrote: post #109

Earlier quoted context omitted.

> you can glance at the nesting to see which ones affect the line you want to edit, and ignore the others. Only if all the cases return! Only then is it obvious that you have independent cases. E.g. suppose we have three Boolean inputs x, y, z and want to do something for each binary combination: if (x) { if (y) { if (z) { return 7; } else { return 6; } } else { // x && !y if (z) { return 5; } else { return 4; } } }…

> Only if all the cases return! My comment was about using if blocks as opposed to early returns . I.e. where the nested ifs run exhaustively and return afterwards. Also, obviously deep nested ifs aren't good , so I wasn't advocating them - I just think it's better to fix them by splitting functions or simplifying control flow, than by adding early returns.

So it is more about multiple returns, versus setting some local return variable and returning in one place.

However, setting that return variable can be recognized as a simulated return. If we know that after "ret = 42", there are no other state changes; that the whole if/else mess will drop out to the end where there is a "return ret", then we can just read it as "return 42".

Re: Self-Documenting Code

#125

Post by fenomas »

kazinator wrote: post #124
fenomas wrote: post #111

Earlier quoted context omitted.

> Only if all the cases return! My comment was about using if blocks as opposed to early returns . I.e. where the nested ifs run exhaustively and return afterwards. Also, obviously deep nested ifs aren't good , so I wasn't advocating them - I just think it's better to fix them by splitting functions or simplifying control flow, than by adding early returns.

So it is more about multiple returns, versus setting some local return variable and returning in one place. However, setting that return variable can be recognized as a simulated return. If we know that after "ret = 42", there are no other state changes; that the whole if/else mess will drop out to the end where there is a "return ret", then we can just read it as "return 42".

Sure, in the narrow case where the function only calculates a single return value and has no side effects.

Re: Self-Documenting Code

#126

Post by kazinator »

fenomas wrote: post #125
kazinator wrote: post #124

Earlier quoted context omitted.

So it is more about multiple returns, versus setting some local return variable and returning in one place. However, setting that return variable can be recognized as a simulated return. If we know that after "ret = 42", there are no other state changes; that the whole if/else mess will drop out to the end where there is a "return ret", then we can just read it as "return 42".

Sure, in the narrow case where the function only calculates a single return value and has no side effects.

Or where it produces an effect (or effect group) in every case just before returning, without multiple effects interspersed among multiple condition tests.

Re: Self-Documenting Code

#127

Post by Chris_Newton »

tetha wrote: post #119
Chris_Newton wrote: post #86

If I were reviewing the original code, the first thing I’d question is the line user.password = await hashPassword(user.password); 1. As a rule, mutations are harder to understand than giving new names to newly defined values. 2. The mutation here apparently modifies an object passed into the function, which is a side effect that callers might not expect after the function returns. 3. The mutation here apparently cha…

> At least three of those problems could trivially be avoided by naming the result hashedPassword and, ideally, using TypeScript to ensure that mixing up plain text and hashed passwords generates a type error at build time. Going that path further ends up what a few code bases I've worked with do: Pull the two domains apart into a "UserBeingCreated" and an existing "User". This felt a bit weird at first, but the more…

I’ve had this debate a few times too. Personally I am in the camp that says you’re talking about two interfaces — your external UI or API, and your internal database schema — so even though you’ll often have a lot of overlap between types representing analogous entities in those two interfaces, they aren’t really the same concept and coding as if they will or should always have identical representations is a trap. I would almost always prefer to define distinct types and explicit conversion between them, even though it’s somewhat more verbose, and the password hashing here is a good example of why.

I wrote a more about this in a Reddit post a while back if anyone’s interested: https://www.reddit.com/r/Python/comments/16w97i6/flask_300_r...

Re: Self-Documenting Code

#128

Post by einpoklum »

callc wrote: post #72
einpoklum wrote: post #66

Earlier quoted context omitted.

You're not wrong, but if your code is full of bespoke types that are only relevant to a couple of places where they're used, you hurt interoperability; you lock yourself in to how things are done now; and you may just be shifting the burden of making sense to someplace else in the code. If you are able to formulate types which can be grasped without reading and re-reading their code - that's a win; and if you are abl…

> if your code is full of bespoke types that are only relevant to a couple of places where they're used, you hurt interoperability; you lock yourself in to how things are done now; and you may just be shifting the burden of making sense to someplace else in the code What is an example of bespoke types? Is is all compound types (structs, classes)? If you need interop or extensibility, make an API. Feel free to use wha…

> Is is all compound types (structs, classes)?

Well, plain-old-data structs are not something I would call bespoke types. Classes for which you write code, I suppose.

> All non-primitive types are essentially

If you only use plain structs, then maybe.

Re: Self-Documenting Code

#129

Post by fenomas »

kazinator wrote: post #126
fenomas wrote: post #125

Earlier quoted context omitted.

Sure, in the narrow case where the function only calculates a single return value and has no side effects.

Or where it produces an effect (or effect group) in every case just before returning, without multiple effects interspersed among multiple condition tests.

That's isomorphic to what I said, so... also yes :D

Re: Self-Documenting Code

#130

Post by rjbwork »

skydhash wrote: post #85
rjbwork wrote: post #68

Earlier quoted context omitted.

>1 This is one of my pet peeves. I had an engineer recently wrap Dapper up in a bunch of functions. Like, the whole point of dapper to me is that it gets out of your way and lets you write very simple, declarative SQL database interactions and mapping. When you start wrapping it up in a bunch of function calls, it becomes opaque. DRY has been taken as far too much gospel.

I always prefer having loadBooks than “select * from books” everywhere. I prefer my code to be just the language version of how I would write an explanation if you ask me one specific question. Not for DRY, but for quick scanning and only diving into details when needed

Obviously there's a method called LoadBooks. The implementation of LoadBooks is what I'm talking about.
Post reply on HN