## DEV Community is a community of 871,998 amazing developers

We're a place where coders share, stay up-to-date and grow their careers.

Tiffany White

Posted on

This post was originally posted to my blog.

One of the biggest lessons I am trying to learn as a mid-level dev is keeping my code simple.

I was working on a few algorithms for a bootcamp I planned to attend.

I tweeted this algorithm I was stuck on a couple weeks ago:

Clean two-liner. Nice, right?

Let's take a look below:

``````function longestString(strs) {
// is x string length greater than y string length? If so
// return x
// if not return y
// add an empty string as an accumulator
// as a callback to the reduce function
const longest = strs.reduce((x, y) => x.length >= y.length ? x : y, '' );
return longest;
}
``````

Here I am trying to get the longest string in an array. I thought I could accomplish this easily with a functional array method so I searched around. I read the MDN for `map`, `filter` and `reduce` to jog my memory, and then settled on `reduce`.

Someone on StackOverflow had a similar algorithm they were trying to solve. I adapted my algorithm based on that.

As the Twitter embed shows I had a bit of trouble with the comparison as my test wasn't passing. I added the appropriate operator and all was well.

This is as clean as it gets.

## This is What Mentors Are For

A friend of mine and mentor DM'd me on Twitter about this and the solutions people offered me on Twitter. He said that one solution was a mess and that if he had written anything like that he would have gotten chewed out by his boss.

My immediate response was to chuckle at the guy who gave me the nested ternary.

But he wasn't talking about that. He was talking about my clever two liner. Well...

## A Lesson in Dumb Code

My friend and I talked at length about cleverness and writing code for other humans to read. The variables I use in the two line solution don't have any context. Instead, I should have broken them out into something like this:

``````let lenX = str1.length;
let lenY = str2.length;

const longest = strs.reduce((str1, str2) => lenX >= lenY ? str1 : str2, '');
``````

This is still terse but more readable and easier to understand.

I could have used a traditional for loop but wanted to look in the know and get in easily. I wanted to look clever and smart and in the process made the code unreadable, the hallmark of a mid-level dev.

## Keep It Simple, Stupid

When I was a newbie dev, I never understood why anyone would write a variable declaration like `x` or `y`. I disliked functional programming methods like `reduce` for that reason: most examples I found used these variables. I never understood what `x` was referencing. As someone who better understands the language I have fallen into that clever mid-level trap. Don't do it. While yes, it makes you look as if you know what you're doing, it also makes the code unreadable and you start to look less and less appealing as a potential dev candidate for X company.

As a friend of mine used to say:

Keep it simple, stupid.

If you enjoyed this, perhaps you'd also like to see some stuff from my newsletter. No spam. 50% dope content 50% dope links. No worries if that isn't your thing.

https://buttondown.email/tiffanywhite

## Discussion (56)

Nested Software • Edited on

Anonymous lambdas can be okay, but I often create a named function to make things a bit more clear. For example, here you could write something like:

``````const strs = ['hello', 'goodbye', 'farewell', 'aloha']

const longerOfPair = (str1, str2) => str1.length >= str2.length ? str1  : str2

const longest = strs.reduce(longerOfPair, '') // > farewell
``````
JavaScript Joel • Edited on

In your example `let lenX = str1.length;` is confusing to me because it is declared outside of the `reduce` where `str1` is not accessible.

was your intent to write it like this?

``````const longest = strs.reduce((str1, str2) => {
const lenX = str1.length;
const lenY = str2.length;

return lenX >= lenY ? str1 : str2
}, '');
``````

I also like how Nested Software broke out the reducer into it's own function.

theMightiestT

it's accessible because of scope. a variable declared inside the reduce wouldn't be accessable outside its scope; however, the other way around is fine(ish). depends if it should be mutable or not

JavaScript Joel

No. What i am saying is `let lenX = str1.length` will produce a null reference exception because `str1` is not available outside the reducer.

theMightiestT

yes sorry you're right... I misread it

Nando • Edited on

imho the external assignment cannot however work ... i think that it isn't evaluated at each cycle of reduce ... or am I wrong ?

Avalander • Edited on

I don't think your first function is hard to read at all. If anybody finds it unreadable enough to say

that one solution was a mess and that if he had written anything like that he would have gotten chewed out by his boss.

I would chuckle and dismiss their opinion as them probably not being used to working with `reduce`.

That being said, if I were to improve that code I would probably extract a function that would compare two strings and return the longest, like Nested Software suggests. `lenX` doesn't really provide more information than `x.length`.

Regarding short variable names, there is a case for them in certain contexts, and `reduce` and the like are one of those. Let's consider this.

``````const pony_names = ponies.map(x => x.name)
``````

Could I write in a way that avoids naming a variable `x`? Sure.

``````const pony_names = ponies.map(pony => pony.name)
``````

However, does it add anything? Since I'm mapping over an array named `ponies`, I know that `x` is going to refer to a pony, and it works much like a pronoun in a natural language. In natural languages, when the immediate context makes the subject of a sentence clear, it is commonly replaced by a pronoun (or in some languages skipped altogether). So instead of I found a book. The book has a blue cover and the book has 307 pages, we usually say I found a book. It has a blue cover and 307 pages. Naming a variable `x` would be much like using it in English.

As I've said, to be able to do that effectively, what `x` refers to needs to be obvious by the immediate context. I still need to give `pony_names` a sensible name because I'm going to use it in another context detached from the `ponies` array and then I wouldn't know what `x` refers to. But for short functions where the immediate context makes it clear what the variable is, longer names are not really a must.

What about `reduce` then?

``````ponies.reduce((x, y) => ({ ...x, [y.name]: y }), {})
``````

I agree that `x` and `y` might be somewhat confusing here. We have one topic (`ponies`), but two pronouns (`x`, `y`), what do they refer to? We know that reduce passes the element it is iterating over in the second parameter, and the result of the last invocation in the first parameter, so let's give that parameter a more meaningful name.

``````ponies.reduce((prev, x) => ({ ...prev, [x.name]: x }), {})
// or
ponies.reduce((acc, x) => ({ ...acc, [x.name]: x }), {})
``````

No more having to check the docs again to see which parameter was the accumulated result and which one was the current element of the array.

Let's consider another case, the function that takes two strings and returns the longest.

``````const longestString = (a, b) => a.length >= b.length ? a : b
``````

Could I have named my variables `string_1` and `string_2`? Sure, but I would argue that those are not meaningful names either, they are just longer. For all I know, `a` and `b` could be pony names or science fiction novels. And it's fine, because `longestString` is a generic function that should work for any kind of string. Do I need to give the variables a name that indicates they are strings? Again, the function name is just a few characters to the left and it already talks about strings, so the immediate context is enough to understand that `a` and `b` are strings.

## TL;DR

To summarise my point, I think one-character function parameter names are alright when two conditions are fulfilled.

• The name of the function, or the context where an anonymous functions is invoked, makes the nature of the parameter obvious.
• The body of the function is short.

And your first code fulfils both conditions. If anything, I would replace your reducer function's signature from `(x, y)` to `(prev, x)` or extract it into a named function that takes two strings and returns the longest.

mnivoliez

I totally agree. I would add that when we do code, we got a semantic, an intention and I think that's the important part. We should use all tools in our arsenal to achieve the goal while letting the semantic be visible.

Danny Smith • Edited on

I'm on board with this!

`<shares comment with students>`

Nando

I totally agree with you.

Heiker

A long time ago I read a post here on dev.to about the danger of writing clever code. The main idea of the article was this:

If you write clever code, the bugs that will come out of it will be clever.

I always try to write code that is "clean" but I know that is impossible. My perception of what is simple and clean will never be the same as other people (or even my future self).

What I do is avoid clever solutions. Most of the time the "cleverness" in the code is unnecesary complexity.

JavaScript Joel

A long time ago I read a post here on dev.to about the danger of writing clever code.

Ben Halpern

This is a really great example of this concept. Example two is so much cleaner, and really passes the squint test.

Small functions are good but cramming too many things on one line is definitely not.

Ben Sinclair

I'll tell you straight off why I think the first one is tricky to read:

``````const longest = strs.reduce((x, y) => x.length >= y.length ? x : y, '' );
``````

We've got `=>` followed by `>=`. My brain gets easily confused and I flick between bits of the line thinking which is the comparison and which is the fat arrow.

I think that the `x.length` parts are quite straightforward and don't think that breaking them out into their own variables aids readability. The line here:

``````let lenX = str1.length;
``````

makes me think that `lenX` is going to be modified later, because otherwise it would be `const`. The definition outside the `reduce` also means that since you've decoupled the lengths from the strings, you can't reduce more than two array elements. By the time the reduce gets to the third element in your array, well, `lenX` is still the length of `str1` (whatever that was). Unless I don't get how `reduce` is working here (which is entirely possible...)

There's another tiny problem I spotted too: your comment says "is x string length greater than y string length?" but your code tests `>=` so if both strings are the same length, your function would return one but your documentation says it should return the other one.

I think that I'd probably go with:

``````const longest = strs.reduce((str1, str2) => str1.length > str2.length ? str1 : str2, '');
``````

and not declare any other variables. I might split it over a couple of lines too, because ternaries inside fat-arrow functions make for a busy line and make my brain work more than I like.

Tiffany White

Yes. My friend told me I should break them out into separate variables so I tried going with that. He gave me a solid example. The difference between what I do and what he does are magnitudes different though: he's responsible for the lives of train passengers and I am just a web dev. I can understand him wanting to simplify it into separate variables as his code needs to keep people physically safe and a bad bug could cost lives. But this reads a bit better.

Will Vincent • Edited on

Even if you're "just a web dev" your code really ought to be given the same amount of care as if it were responsible for people's lives. :)

But if you really want to be clever, and keep it readable, it can be dramatically shortened with lodash's maxBy method...

``````const _ = require('lodash')

const strings = [
'12345',
'123',
'1234567',
'12',
'1',
'123456',
'1234',
]

const result = _.maxBy(strings, string => string.length)
``````

I'm sure the code challenge requires no external libraries, but that's not necessarily anything like a real-world requirement.

Interestingly, lodash uses a for loop for this, for what it's worth..

Iรฑaki Arroyo • Edited on

Not trying to be rude here, just giving my point of view, but, I am so far to be agreed with you here, as a web dev or just as a dev you should always consider writing your code in a way that it doesn't break "the train", hiding the possibility of making bugs due to you are just a web dev is a very poor excuse. From my point of view, thinking in that way ends in having bugs over bugs into the projects.

Alex Lohr

Sometimes, there's an even simpler solution:

``````const strs = ['hello', 'goodbye', 'farewell', 'aloha'];

strs.sort((str1, str2) => str2.length - str2.length);

const longestString = strs[0];
``````
Lars Gyrup Brink Nielsen • Edited on

If you turn that into a reusable function, you will be mutating the array parameter, i.e. sorting it in-place. You would have to clone the array before sorting it to prevent side effects from passing an array into the function.

Also, you made a mistake. You compare `str2.length` to `str2.length` (both are `str2`).

``````function longest(xs) {
// Shallow clone
xs = [...xs];
xs.sort((a, b) => b.length - a.length);

return xs[0];
}
``````

Or with reusable functions

``````function compareLength(a, b) {
return b.length - a.length;
}

return first;
}

function longest(xs) {
xs = shallowClone(xs);
xs.sort(compareLength);

}

function shallowClone(xs) {
return [...xs];
}
``````
Alex Lohr

You're right.

Nando

I don't think this is a simpler solution, albeit from a logical point of view: it may be understood only considering the outcome of the sorting process, then getting the first item as the one with the minor size...

theMightiestT

i thought the same... and if you have strings with numbers, then add a return (a-b) or something...

Evaldas Buinauskas

Seems that sort has worse performance compared to reduce. Reduce is O(n) while sort is O(n long)

Alex Lohr

Yes, the performance is worse. But unless you need the solution to scale to arrays with more than a few thousand words, this will hardly be an issue.

Evaldas Buinauskas

True. Just wrote that because it wasn't mentioned anywhere and someone might care about it.

theMightiestT

valid point but I agree with Alex... not sure it would really be a thing in an implementation like this

tbodt

I would probably end up using a for loop for this, but here's the most readable version using `reduce` I could think of:

``````function longestString(strs) {
return strs.reduce((x, y) => {
if (x.length > y.length)
return x;
else
return y;
}, '');
}
``````
Tiffany White

Makes sense. I was trying to piece together CL's function from bits and pieces of a Twitter DM when I wrote this article. It's definitely not optimal. I should have asked him to clarify however he's a low level programmer not as familiar with Javascript.

Nando

imho this solution is cleaner only for guys not used to ternary expressions, otherwise the first proposed solution is the better one; I don't think that the "if" solution does add any bit of clarity.

Memoria

As @avalander said, as an experienced developer, I don't think the function is hard to read at all.
Skipping the fact that your corrected code is wrong (first pointed by @joelnet ), which is very interesting because it shows that you still not mastering this bit of code.

I also do not think the rewritten function adds more value.
In my opinion, the difficulty of understanding what this one-liner does resides in the "Array.reduce" function.
While it should be understood since it belongs to the language, I tends to find new programmers having difficulties with it because they're not use to see it nor to use it, hence why they don't understand it.

A better demonstration of the KISS principle would have been to see another alternative to .reduce() until it's readable enough to remove this big comment.
A comment is an apology for a poor written code, it's usually better to explain why you do something instead of what.

Tiffany White

The reason there are comments in the first example was because I was applying to a bootcamp and wanted to explain my process to the admissions office.

Memoria • Edited on

Fair enough, just keep in mind that documentation is different from comments.

I would just like to add something you might be interested in.
If you wanted your function "longestString" to be available outside of its package as a library function for example, you would want to comment using jsdoc.

More than it allows you to generate an html doc form your code, your IDE knows about jsdoc and can generate friendly popup as you try to use this function.
Have a look at the difference:

Without jsdoc

With jsdoc

To finish, while being old (but definitely not irrelevant) and not JavaScript specific, I invite you to read Clean Code: A Handbook of Agile Software Craftsmanship from Robert C. Martin (Uncle Bob).

Cheers !

Laureline Paris

You might want to change the order of your picture ( first picture showing actully the ' without jsdoc ' and the second showing the ' with jsdoc '

Memoria

Thanks for pointing this out :)!

Sergiu Mureลan

I think this only comes down to what the team is used to. If they know how to search MDN and some functional programming then it's also readable. We should use the tools of a language in our own favour.

I would use this code because:

• It has a small code base (usually easier to maintain)
• It is more robust (since you are not reinventing the wheel and use what the language has to offer)
• It is often the cleanest it can get

Not using the tools your language has to offer leaves you with unnecessarily large codebases.

@joelnet 's version looks to be the most readable, that's what I would use.

Amin • Edited on

I would probably go for a combo prototype/function.

``````// Can be used as-is
function longestString(first, second) {
return second.length > first.length ? second : first
}

// But the prototype makes it more natural to read later
Array.prototype.longest = function() {
return this.reduce(longestString, '')
};

// You know the winner is and will forever be..
const languages = [ 'javascript', 'php', 'ruby', 'python' ]

languages.longest() // "javascript" ๐
``````
theMightiestT

I am wondering why you chose reduce instead of sort for your array method? In the example you provided (comparing 2 strings), sort seems to accomplish what you're after and it's really short... the first index is the longest.

Kushan Joshi • Edited on

I agree that the code you mentioned is terse, but in my humble opinion it is readable.

Everyone has their own definitions of readability. I think it is totally okay to have a terse lambda like the one you mentioned, as long as it is simply doing one thing and is reasonably small. In the example you posted it is simply comparing lengths of two strings and returning the longest. These type of short lambdas are pretty convenient and are pretty common in Javascript these days.
If you compare them side by side, the lambda version wins for me.

``````function longest(strs) {
return strs.reduce((x, y) => {
if (x.length > y.length)
return x;
else
return y;
}, '');
}

const longest = strs => strs.reduce((s, t) => s.length >= t.length ? s : t);
``````

A code is clever when you yourself don't understand what it does a week later. To mitigate that you have to provide a verbose comment describing what the hell it does and pray to god that whoever changes the code also updates the comment, because in real life comments and code go out of sync pretty often.

Another important thing about the more terse option is that it improves readability when you start chaining operators. Let me give you a hypothetical example and you decide for yourself:

``````function longest(strs) {
return strs
.filter(s => {
if (typeof s === 'string') {
return true;
}
})
.map(s => {
if (/^[aeiou]/i.test(s)) return 'an ' + s;
else return 'a ' + s;
})
.reduce((x, y) => {
if (x.length > y.length) return x;
else return y;
}, '');
}

const longest = strs =>
strs
.filter(s => typeof s === 'string')
.map(s => (/^[aeiou]/i.test(s) ? 'an ' : 'a ') + s)
.reduce((s, t) => (s.length >= t.length ? s : t));
``````
• Each individual piece is easy to reason with independently.
• It has more inertia to code change, i.e. a fellow programmer cannot simply introduce a closure variable as easily as in the `if else` version. Hey we all have had that bug which was temporarily fixed by peppering code with global variables.
• Less syntax noise.
• I don't have to constantly fiddle around with closing `{}`.
John Hotterbeekx • Edited on

Cleaning code while using lambda expressions can be challenging with times. How I usually try to clean it, is by extracting the actual expression to a separate function, since this usually has its own responsibility.

I'd refactor it like this:

``````function getLongestStringInArray(stringArray) {
return stringArray.reduce((str1, str2) => getLongestString(str1, str2));
}

function getLongestString(string1, string2) {
if (string2.length > string1.length) return string2;
return string1;
}
``````

By splitting the functionality and also removing the inline if statements and arrowbody functions it becomes a lot more readable in my opinion. It also becomes a lot more testable, since you can easily write you test cases for `getLongestString` testing all comparison options, and afterwards testing array specific behavior only for `getLongestStringInArray`.

Comment deleted
Thomas H Jones II

I'm gonna have to plead guilty to resorting to impenetrable regex. More than a few times, over the years, where I've produced code with elements that look like it came from a different galaxy. That said, sometimes, it is the easiest (if not most readable) way to solve a given problem.

Max

In my understanding, using the .reduce() function is somewhat misplaced here. You're not looking to reduce all values in an array to one value, but rather look for one element that has certain properties.

Finding things in arrays (and in life) is easier when there is order in search space.

Here's another approach to finding the longest string in an array:

``````const longestString = arr => arr.sort((a, b) => b.length - a.length)[0];
``````
Benny Powers ๐ฎ๐ฑ๐จ๐ฆ • Edited on

Thanks Tiffany for the post, which has opened up such a lovely discussion.

I'm going to push back a little on your examples. By pulling your data out into the top level of the scope instead of your operations, you're contributing to more complex code by adding to the cognitive load of the reader.

In the first example, the worst it'll get for the reader is parsing a single pure function.

In the second example, the reader will need to parse that function while referencing external state.

Could you have simplified that first example by extracting operations to the top level instead of data?

``````import { maxBy, length } from 'ramda'

// longest :: (Ord a) => (a, a) -> a
const longest = maxBy(length)

const longestString = (strs) =>
strs.reduce(longest, '');
``````

REPL.it

In this case, there's 0 mental parsing. Your reader's cognitive load involves reading the Ramda docs to find out that `maxBy(length)` will return the longest of its two arguments. We even provide a function signature in the comments so they don't have to shlep all the way out to the browser.

Is pulling in Ramda cheating? Heck no! Ramda is a general-use functional library. If we're concerned about dependency creep, we could implement those on our own as well.

The most readable code is the code that you don't have to read. Let's layer general interfaces and abstractions instead of bespoke imperative instructions.

Chris Shepherd

All good stuff. When you write code it is always best to remember that the person who will be looking at it next will probably be you...

A you who has probably forgotten how this app works, has a system down, users screaming and a boss looking over your shoulder.

Be kind to your future self, keep it as simple as possible because future you will really appreciate it.

zeddotes

Maybe I'm missing something, but doesn't passing an `initialValue` after your `reduce`'s callback mean that what you're calling `str1` (in the callback) is actually the `accumulator`? For the sake of verbosity, I'd name it accordingly. Better yet, I'd get rid of the `initialValue` altogether.

Please correct me if I'm wrong.

Sharon DiOrio

Writing for the least clever compiler (the brain) is always preferred.

P. Todd Decker

I don't get it. If you want to keep it simple, keep it simple. There is no need at all for lambdas or reduce functions here. Why not simple? Trying to get it all with single lines of code is silly. Anyway you cut it it is an O(n) problem.

``````function longestString(strs) {
let longest = 0;
for (let i = 0; i < strs.length; i++) {
if (strs[i].length >= strs[longest].length) {
longest = i;
}
}
return strs[longest];
}

console.log(
longestString(['hi', 'there', 'tiffany'])
)
``````
conectionist

"While yes, it makes you look as if you know what you're doing, it also makes the code unreadable"

I couldn't agree more. Nobody should ever write code just to make themselves look good/clever.
There is no place for ego inside a company.
Never forget that you're not writing code (just) for yourself. You're (also) writing for others. And when writing code for others, you should make it as readable and as easy to understand as possible.

Maurice Hayward

I remember when you sent that tweet out!

it also makes the code unreadable and you start to look less and less appealing as a potential dev candidate for X company.

This made me think of this tweet:

Tiffany White

p-mcgowan

I recently built the most glorious map/reduce/filter bohemoth that transforms one complicated object into another complicated json body, and it was beautiful. It was so cool looking, littered with ternaries and fancy remappings. I was so proud.

I had to fix a bug in it about a week later, and I had to re-read it about 10 times to figure out what could possibly be wrong with it. A perfect example of something someone else said on dev.to that you should write code for the next person and not fun, cool-looking code. I've since re-written most of it, but I still feel bad for the next person...

Patrick Waivers

This code does not work. This line throws an error:

``````let lenX = str1.length;
``````
Tiffany White

I'll have to look at it. Like I said in another comment, I took the contents of a Twitter DM with a mentor and tried to piece together what he was saying. He offered some variables broken out of the main function so I was trying to infer from the bit I got. It's probably wrong and I felt that while writing it.

Theofanis Despoudis

Even in a case of the 2 longest strings with the same length the results are partially correct. In that case, you need to return a list of strings.

Michael Rouse • Edited on

The directions are clear, if string x is longer than string y, return x, otherwise return y.

So, if when going through the array you find a string with the same length, you use that one as the new longest string.

Nothing about returning an array of strings.