Keeping Your Code Simple

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'])
)

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];

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.

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

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.

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

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.

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

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.

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];

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.

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

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.

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 {}.