Am I too 'clever' to be readable by Jr. devs? Too much functional programming in my JS? [closed]

Question

I'm a Sr. front-end dev, coding in Babel ES6. Part of our app makes an API call, and based on the data model we get back from the API call, certain forms need to be filled out.

Those forms are stored in a doubly-linked list (if the back-end says some of the data is invalid, we can quickly get the user back to the one page they messed up and then get them back on target, simply by modifying the list.)

Anyway, there's a bunch of functions used to add pages, and I'm wondering if I'm being too clever. This is just a basic overview - the actual algorithm is much more complex, with tons of different pages and page types, but this'll give you an example.

This is how, I think, a novice programmer would handle it.

export const addPages = (apiData) => {
   let pagesList = new PagesList(); 

   if(apiData.pages.foo){
     pagesList.add('foo', apiData.pages.foo){
   }

   if (apiData.pages.arrayOfBars){
      let bars = apiData.pages.arrayOfBars;
      bars.forEach((bar) => {
         pagesList.add(bar.name, bar.data);
      })
   }

   if (apiData.pages.customBazes) {
      let bazes = apiData.pages.customBazes;
      bazes.forEach((baz) => {
         pagesList.add(customBazParser(baz)); 
      })
   } 

   return pagesList;
}

Now, in order to be more testable, I've taken all those if statements and made them separate, stand alone functions, and then I map over them.

Now, testable is one thing, but so is readable and I wonder if I'm making things less readable here.

// file: '../util/functor.js'

export const Identity = (x) => ({
  value: x,
  map: (f) => Identity(f(x)),
})

// file 'addPages.js' 

import { Identity } from '../util/functor'; 

export const parseFoo = (data) => (list) => {
   list.add('foo', data); 
}

export const parseBar = (data) => (list) => {
   data.forEach((bar) => {
     list.add(bar.name, bar.data)
   }); 
   return list; 
} 

export const parseBaz = (data) => (list) => {
   data.forEach((baz) => {
      list.add(customBazParser(baz)); 
   })
   return list;
}


export const addPages = (apiData) => {
   let pagesList = new PagesList(); 
   let { foo, arrayOfBars: bars, customBazes: bazes } = apiData.pages; 

   let pages = Identity(pagesList); 

   return pages.map(foo ? parseFoo(foo) : x => x)
               .map(bars ? parseBar(bars) : x => x)
               .map(bazes ? parseBaz(bazes) : x => x)
               .value

}

Here's my concern. To me the bottom is more organized. The code itself is broken into smaller chunks that are testable in isolation. BUT I'm thinking: If I had to read that as a junior developer, unused to such concepts as using Identity functors, currying, or ternary statements, would I be able to even understand what the latter solution is doing? Is it better to do things the "wrong, easier" way sometimes?

Answer 1

In your code, you have made multiple changes:

• destructuring assignment to access fields in the pages is a good change.
• extracting the parseFoo() functions etc. is a possibly good change.
• introducing a functor is … very confusing.

One of the most confusing parts here is how you are mixing functional and imperative programming. With your functor you aren't really transforming data, you are using it to pass a mutable list through various functions. That doesn't seem like a very useful abstraction, we already have variables for that. The thing that should possibly have been abstracted – only parsing that item if it exists – is still there in your code explicitly, but now we have to think around the corner. For example, it's somewhat non-obvious that parseFoo(foo) will return a function. JavaScript doesn't have a static type system to notify you whether this is legal, so such code is really error prone without a better name (makeFooParser(foo)?). I don't see any benefit in this obfuscation.

What I'd expect to see instead:

if (foo) parseFoo(pages, foo);
if (bars) parseBar(pages, bars);
if (bazes) parseBaz(pages, bazes);
return pages;

But that's not ideal either, because it is not clear from the call site that the items will be added to the pages list. If instead the parsing functions are pure and return a (possibly empty) list that we can explicitly add to the pages, that might be better:

pages.addAll(parseFoo(foo));
pages.addAll(parseBar(bars));
pages.addAll(parseBaz(bazes));
return pages;

Added benefit: the logic about what to do when the item is empty has now been moved into the individual parsing functions. If this is not appropriate, you can still introduce conditionals. The mutability of the pages list is now pulled together into a single function, instead of spreading it across multiple calls. Avoiding non-local mutations is a far bigger part of functional programming than abstractions with funny names like Monad.

So yes, your code was too clever. Please apply your cleverness not to write clever code, but to find clever ways to avoid the need for blatant cleverness. The best designs don't look fancy, but look obvious to anyone who sees them. And good abstractions are there to simplify programming, not to add extra layers that I have to untangle in my mind first (here, figuring out that the functor is equivalent to a variable, and can effectively be elided).

Please: if in doubt, keep your code simple and stupid (KISS principle).

Answer 2

If you are in doubt, it probably is too clever! The second example introduces accidental complexity with expressions like foo ? parseFoo(foo) : x => x, and overall the code is more complex which means it is harder to follow.

The purported benefit, that you can test the chunks individually, could be achieved in a simpler way by just breaking into individual functions. And in the second example you couple the otherwise separate iterations, so you actually get less isolation.

Whatever your feelings about functional style in general, this is clearly an example where it makes the code more complex.

I find a bit of a warning signal in that you associate simple and straightforward code with "novice developers". This is a dangerous mentality. In my experience it is the opposite: Novice developers are prone to overly complex and clever code, because it requires more experience to be able to see the simplest and clearest solution.

The advice against "clever code" is not really about whether or not the code uses advanced concepts which a novice might not understand. Rather it is about writing code which is more complex or convoluted than necessary. This makes the code harder to follow for everybody, novices and experts alike, and probably also for yourself some months down the line.

Answer 3

This answer of mine comes a bit late, but I still want to chime in. Just because you're using the latest ES6 techniques or using the most popular programming paradigm doesn't necessarily mean that your code is more correct, or that junior's code is wrong. Functional Programming (or any other technique) should be used when it's actually needed. If you try to find the tiniest chance to cram latest programming techniques into every problem, you will always end up with an over-engineered solution.

Take a step back, and try to verbalize the problem you're trying to solve for a second. In essence, you just want a function addPages to transform different parts of apiData into a set of key-value pairs, then add all of them into PagesList.

And if that's all there is to it, why bother using identity function with ternary operator, or using functor for input parsing? Besides, why do you even think it's a proper approach to apply functional programming that causes side-effects (by mutating the list)? Why all those things, when all you need is just this:

const processFooPages = (foo) => foo ? [['foo', foo]] : [];
const processBarPages = (bar) => bar ? bar.map(page => [page.name, page.data]) : [];
const processBazPages = (baz) => baz ? baz.map(page => [page.id, page.content]) : [];

const addPages = (apiData) => {
  const list = new PagesList();
  const pages = [].concat(
    processFooPages(apiData.pages.foo),
    processBarPages(apiData.pages.arrayOfBars),
    processBazPages(apiData.pages.customBazes)
  );
  pages.forEach(([pageName, pageContent]) => list.addPage(pageName, pageContent));

  return list;
}

(a runnable jsfiddle here)

As you can see, this approach still uses functional programming, but in moderation. Also note that since all 3 transformation functions cause no side effects whatsoever, they are dead easy to test. The code in addPages is also trivial and unassuming that novices or experts can understand by just a mere glance.

Now, compare this code with what you've come up with above, do you see the difference? Undoubtedly, functional programming and ES6 syntaxes are powerful, but if you slice the problem the wrong way with these techniques, you'll end up with even messier code.

If you don't rush into the problem, and applying the right techniques in the right places, you can have the code that is functional in-nature, while still is very organized and maintainable by all team members. These characteristics are not mutually-exclusive.

Answer 4

The second snippet is not more testable than the first. It would be reasonably straightforward to set up all the needed tests for either one of the two snippets.

The real difference between the two snippets is comprehensibility. I can read the first snippet fairly quickly and understand what's going on. The second snippet, not so much. It's far less intuitive, as well as substantially longer.

That makes the first snippet easier to maintain, which is a valuable quality of code. I find very little of value in the second snippet.

Answer 5

TD;DR

1. Can you explain your code to the Junior Developer in 10 minutes or less?
2. Two months from now, can you understand your code?

Detailed Analysis

Clarity and Readability

The original code is impressively clear and easy to understand for any level of programmer. It is in a style familiar to everybody.

Readability is largely based on familiarity, not some mathematical counting of tokens. IMO, at this stage in time, you have too much ES6 in your rewrite. Maybe in a couple of years I'll change this part of my answer. :-) BTW, I also like the @b0nyb0y answer as a reasonable and clear compromise.

Testability

if(apiData.pages.foo){
   pagesList.add('foo', apiData.pages.foo){
}

Assuming that PagesList.add() has tests, which it should, this is completely straightforward code and there is no obvious reason for this section to need special separate testing.

if (apiData.pages.arrayOfBars){
      let bars = apiData.pages.arrayOfBars;
      bars.forEach((bar) => {
         pagesList.add(bar.name, bar.data);
      })
   }

Again, I see no immediate need for any special separate testing of this section. Unless PagesList.add() has unusual issues with nulls or duplicates or other inputs.

if (apiData.pages.customBazes) {
      let bazes = apiData.pages.customBazes;
      bazes.forEach((baz) => {
         pagesList.add(customBazParser(baz)); 
      })
   } 

This code is also very straightforward. Assuming that customBazParser is tested and doesn't return too many "special" results. So again, unless there are tricky situations with `PagesList.add(), (which there could be as I'm not familiar with your domain) I don't see why this section needs special testing.

In general, testing the whole function should work fine.

Disclaimer: If there are special reasons to test all 8 possibilities of the three if() statements, then yes, do split up the tests. Or, if PagesList.add() is sensitive, yes, split up the tests.

Structure: Is it worth breaking up into three parts (like Gaul)

Here you have the best argument. Personally, I don't think the original code is "too long" (I'm not a SRP fanatic). But, if there were a few more if (apiData.pages.blah) sections, then SRP rears it's ugly head and it would be worth splitting. Especially if DRY applies and the functions could be used in other places of the code.

My one suggestion

YMMV. To save a line of code and some logic, I might combine the if and let into one line: e.g.

let bars = apiData.pages.arrayOfBars || [];
bars.forEach((bar) => {
   pagesList.add(bar.name, bar.data);
})

This will fail if apiData.pages.arrayOfBars is a Number or String, but so will the original code. And to me it is clearer (and an overused idiom).