Compact or Verbose Codestyle?

Which code style do you prefer and why?

Compact

const get = (store, key) => 
    Reflect.has(window[store], key) && 
        JSON.parse(window[store][key]) ||
        new Error(`Key "${key}" not found in window.${store}.`);

Verbose

const get = (store, key) => {
    if (!Reflect.has(window[store], key)) {
        return new Error(`Key "${key}" not found in window.${store}.`);
    }

    const jsonStr = window[store][key]
    const dataObj = JSON.parse(jsonStr);
    return dataObj;
};

Comments

[Blaine Osepchuk]

I don't concern myself with the number of lines (compact vs verbose).

Code must first be readable (by a new programmer, at 5pm on Friday, operating on four or less hours of sleep). You're going to run into trouble with anything else.

I've never encountered code that was too readable. But I have encountered plenty of puzzle/mystery/deceptive/spaghetti code.

[Josh Sommer]

I agree with you 100%. Readable code will pay your team and you back a million times over.

I would suggest going one step farther and banning abbreviations. They can cause needless confusion. In the above example is jsonStr equal to jsonString or jsonStore? It’s much better to not have to make anybody think.

[George Offley]

Same. Compacting your code, to me, is an edge case practice. Where every single millisecond counts is when that is really needed. To me anyway. Making sure your code is readable to everyone who reads it is far more important.

[Blaine Osepchuk]

I completely agree.

If you're compacting your code because you want it to run faster, you are deep inside micro-optimization land. 99.9% of the time for normal programming, those kind of optimizations are not required.

And they are definitely not required throughout your code base to 'make it all run a little faster'.

[Nicolas Bailly]

I'm not a js developer, but I would use this middle ground (hoping it actually does the same thing) :

const get = (store, key) => {
    if (!Reflect.has(window[store], key)) {
        throw new Error(`Key "${key}" not found in window.${store}.`);
    }

    return JSON.parse(window[store][key])
};

[F1LT3R]

One thing you get from The verbosity of the ending variables, is that you don't have to break apart the line to debug the code. You could identify whether your bug resides in the JSON parse or the store getter without changing any lines of code.

I think generally, developers are more comfortable compacting things we understand well, especially when they are stable and behave in expected ways (like JSON.parse and window.localStorage).

I believe I observe people compacting calls to their own code more often, (because they understand it well), and use more verbosity calling their peers' code (because they understand it less).

Personally I lean more towards universal verbosity for the ability to debug without changing lines of code. In my opinion, this also fosters self documenting code, especially when calling internal code and not common environment/library APIs.

[Vincent Dedo]

I was going to make a similar code suggestion, the variables at the end don't add anything.

[Bernhard Webstudio]

Well, one little use case of the variables for newbies: easier to debug (slide in a console.log(var) easily).

[Elliot Derhay]

Can confirm it would run.

[Gal Schlezinger]

I guess you would have want to use the ternary operator to make it more readable, instead of adding the || and && which are harder to understand: the ternary operator accepts a nicer and more predictable form of (value, ifTrue, ifFalse) instead of constructing it on your own with boolean operator magic

const get = (store, key) =>
  Reflect.has(window[store], key)
    ? JSON.parse(window[store][key])
    : new Error(`Key ${key} not found`)

I guess this is readable enough, but I'll probably use the "verbose" one (it really depends on the project standards): I like early returns, or "guard" clauses - can help debugging by understanding that you can stop evaluating the function in your head 😺

[Frank Puffer]

There are two completely independent idioms that make the 2nd example more verbose:

1. The use of if instead of boolean operators for conditions. I tend to use if as the default and sometimes refactor when I think that the code is more readable when using a boolean operator. I also know some people who always use if and consider the other variant as a misuse of boolean operators.

2. The use of intermediate constants for function returns instead of function chaining. This can be useful for two reasons: You can use meaningful names for these constants that provide additional information about what is happening and thus make the code more readable. In this example, I would say that the names jsonStr and especially dataObj do not provide much useful information. Another reason not to chain function calls is to make debugging easier.

I personally have a certain bias towards compactness and like the 1st example slightly more. In real code, the optimum is probably somewhere in between.

[Mike Chen]

If forced to choose between the two, definitely "Verbose". I think it's TOO verbose though. I don't see why it needs all the intermediate variables after the early throw if statement.

I can almost guarantee that coming back to either of these examples 6 months after writing it, it will take less time to understand the explicit if statement and behavior in the "Verbose" example. Even if it only takes an extra few seconds to read the "Compact" example, that still adds up if you're reading through an entire code path and trying to understand it.

[Jon Randy 🎖️]

They appear to do not have the same behaviour?

[Mihail Malo]

The short one returns the error instead of throwing it.

[F1LT3R]

It's true, the behavior is slightly different. I will update this as it adds confusion. The throw will stop the code and unwind the execution stack until the error is caught.

[Mihail Malo]

The problem is that when not using a Result type this would actually be hard to use in JS.
You would have to check

const result = get('a','b')
if (result instanceof Error) throw result
doStuff(result)

Which isn't idiomatic.

[F1LT3R]

Typically I would throw inside the get function here. I did not do this in the example because you can't throw inside the expression and I wanted to keep the logic identical between the two.

However, I do sometimes respond to the caller with something like:

{error: {
  msg,
  origin: new Error(msg)
}};

... then I handle that up the stack.

{
  setupView () {
    const something = await fetch ('something');

    if (something.error) {
      // handle error (throw or return  error depending on level of fatality)
      return something;
    }

    // Guarded logic
    updateView(something);
  }
}

I tend to bubble recoverable errors back up the stack.

Your comment about being idiomatic interested me. Can you elaborate? I might learn something valuable if you have the time to share an idiomatic example.

[Mihail Malo]

I don't think "Idiomatic JavaScript" is something to be particularly proud of, good patterns aren't the most popular here.

The approach with .error seems more conventional though, since this is similar to what you'd get in a JSON response on a remote error. (I only noticed the second example is literally await fetch after I finished writing this. haha)

Promises are already essentially Future<Result<T,E>>, with await really meaning (await promise).unwrap(), which IMHO is a big design flaw. This means that there isn't a built in Result type which we could use rich combinators with, while we can't properly express infalible futures like timeouts, which will never reject. And once await is added to the mix, we end up having to do the traditional, "idiomatic" try-catch around it.

What if you have a function that can throw, and on success returns a promise that might reject? Today you have to do:

try {
  const promise = beginThang()
} catch (err) {
  // handle E1
}
let /* wow, so glad this will be a mutable binding now */ result
try {
  result = await promise
} catch (err) {
  // handle E2
}
return makeUseOf(result)

While it could have been:

beginThang()
  .mapErr(err => {/*  handle E1  */}) // line optional, can just bubble error
  .andThen(async res => {
    (await res)
      .mapErr(err => {/*  handle E2  */}) // line optional, can just bubble error
      .map(makeUseOf)
  })

Note how we get a Result<Future<Result<T,E2>>,E1> from beginThang and ourselves return a Result<Future<Result<T2,E4>>,E3> (or Result<Future<Result<T2,E2>>,E1> if we just bubble errors), so immediate errors can be handles synchronously instead of wrapping our bad outcomes in a Promise.reject(err) so they wait until the next round of the event loop.

[Mihail Malo]

Another thing that is not used at all in "idiomatic JavaScript" is dependency injection.

[Alain Van Hout]

Between those two choices, I'd definitely go for the second one: the time you save writing less characters will not make up the hours you might lose in bug hunting.

As others have already pointed out though, there are two approaches used here:

• The guard clause (the if statement) helps separate the concerns within the method, since validation and data retrieval are indeed separate concerns.
• Extracting intermediate variables can be really helpful when the variable names communicate their (domain) purpose. In the example, that's however not really the case.

I think the important thing to keep in mind is that while the first example is much shorter (in amount of characters), it has exactly the same logic and the second example. That's to say: it's much more dense, meaning that you have to mentally de-compress it when you want to make changes to it. That doesn't help maintainability, and might even slow you (or your colleagues) down in the long run.

[Templar++]

Always program as if the next person, who will be supporting your software is a psychopath, who knows where you live.

When your code isn't easy to understand and obvious from first sight - it's cr*p

[homer1994]

Two, hands down, with dataObj variable inlined and the method called something more descriptive. One is too obscure. I've been doing JavaScript development for over a decade and every time I have to look up the bizarre JavaScript && and || return logic

[Frank Puffer]

I would not call it bizarre and it is not specific to Javascript. You can do this in lots of languages like Java, C++ and even Perl.

The fact that you can use && and || as control structures is a consequence of short circuit evaluation and that is something every programmer should understand.

I do agree that these operators have not been introduced for this purpose and that this style can be less readable.

[Nomæd]

They are quite handy when you need to get a property nested in an object, like let foo = obj && obj.child && obj.child.foo || "", especially when a default fallback value is needed.
But when it's used as a condition, it's a bit annoying and IMO much less readable at first glance.

[Harry Park]

I don't believe it length of the code matters as much now as long as the algorithm is correct and the code is readable. There are plenty of optimizers and minifiers that a project can use to reduce production code.

[Jesse M. Holmes]

Isn’t it strange how the former seems practical if you’ve spent all day working in React using conditional rendering?

If I walked into that piece of code, it would churn my brain for a while, and yet the same expressed as a ternary operation would make sense to me immediately.

If I were a new developer, only the latter would give me hope.

I think compact code is beautiful, but I prefer verbose code because it is quickly understood. There is a balance, and if I feel like compact is the way to go, I’ll balance with comments. In the case of a ternary operator, I explain it once per file so that a more junior developer will have a clue to cling to.

[Réda Housni Alaoui]

The second with typescript

[phantomread]

Verbose, although you can combine the second and third last lines into one.

[Muntadhar Haydar]

depending on my knowledge of the language (and the framework) but will probably go for the least amount of code that's readable

[Javi]

As long as it's readable, make it the most compact possible.

Verbose can be unreadable too.