132

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?

closed as primarily opinion-based by gnat, Glenn Nelson, GrandmasterB, Thomas Junk, Andy Jun 10 '17 at 20:01

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 13
    as someone who only has 10 years self-teachings in JS, I would consider myself a Jr. and you lost me at Babel ES6 – RozzA Jun 8 '17 at 20:18
  • 26
    OMG - been active in industry since 1999 and coding since 1983, and you are the most harmful kind of developer there is. What you think is "clever" is really called "expensive" and "hard to maintain" and "a source of bugs" and it has no place in a business environment. The first example is simple, easy to understand and it works while the second example is complex, hard to understand, and not provably correct. Please stop doing this kind of thing. It is NOT better, except maybe in some academic sense that doesn't apply to the real world. – user1068 Jun 9 '17 at 20:26
  • 15
    I just want to quote Brian Kerninghan here: "Everyone knows that debugging is twice as hard as writing a program in the first place. So if you're as clever as you can be when you write it, how will you ever debug it?" - en.wikiquote.org/wiki/Brian_Kernighan / "The Elements of Programming Style", 2nd edition, chapter 2. – MarkusSchaber Jun 10 '17 at 17:55
  • 7
    @Logister Coolness is no more a primary objective than simplicity. The objection here is to gratuitous complexity, which is the enemy of correctness (surely a primary concern) because it makes the code harder to reason about and more likely to contain unexpected corner cases. Given my earlier-stated skepticism of claims that it actually is easier to test, I have not seen any convincing argument for this style. In analogy with the rule of least privilege wrt security, perhaps there could be a rule of thumb that says one should be wary of using powerful language features to do simple things. – sdenham Jun 12 '17 at 12:56
  • 6
    Your code looks like junior code. I would expect a senior to write the first example. – sed Jun 16 '17 at 15:07
321

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

  • 2
    From a symmetry point of view, what does let pages = Identity(pagesList) have different from parseFoo(foo)? Given that, I would probably have... {Identity(pagesList), parseFoo(foo), parseBar(bar)}.flatMap(x -> x). – ArTs Jun 6 '17 at 9:29
  • 8
    Thank you for explaining that having three nested lambda expressions for collecting a mapped list (to my untrained eye) might be a bit too clever. – Thorbjørn Ravn Andersen Jun 6 '17 at 20:07
  • 2
    Comments are not for extended discussion; this conversation has been moved to chat. – yannis Jun 8 '17 at 8:03
  • Maybe a fluent style would work good in your second example? – user1068 Jun 9 '17 at 20:30
225

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.

  • 156
    "Novice developers are prone to overly complex and clever code, because it requires a more experience to be able to see the simplest and clearest solution" can't agree more with you. Excelent answer! – Bonifacio Jun 6 '17 at 11:16
  • 23
    Overly complex code is also quite passive-aggressive. You're deliberately producing code that few others can read or debug easily...which means job security for you, and utter hell for everyone else in your absence. You may as well write your technical documentation entirely in Latin. – Ivan Jun 6 '17 at 20:39
  • 14
    I don't think clever code is always a show off thing. Sometimes it feels natural, and only looks ridiculous on a second inspection. – user69767 Jun 7 '17 at 7:43
  • 5
    I have removed the phrasing about "showing off" since it sounded more judgmental than intended. – JacquesB Jun 7 '17 at 9:17
  • 11
    @BaileyS - I think that emphasises the importance of code review; what feels natural and straightforward to the coder, especially when gradually developed that way, can easily seem convoluted to a reviewer. The code then doesn't pass review until refactored / rewritten to remove the convolution. – Myles Jun 8 '17 at 16:15
21

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.

  • 2
    +1 for pointing out this wide-spread attitude (does not necessarily apply to the OP): "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.". – Giorgio Jun 10 '17 at 8:24
  • +1. Just a small pedantic remark, you can use the spread (...) operator instead of _.concat to remove that dependency. – YoTengoUnLCD Jun 11 '17 at 5:10
  • 1
    @YoTengoUnLCD Ah, good catch. Now you know that me and my team are still on the journey of unlearning some of our lodash use. That code can use spread operator, or even [].concat() if one wants to keep the shape of the code intact. – b0nyb0y Jun 11 '17 at 9:15
  • Sorry, but this code listing is still much less obvious to me than the original "junior code" in OP's post. Basically: Never use ternary operator if you can avoid it. It's too tense. In a "real" functional language, if-statements would be expressions and not statements, and therefore more readable. – Olle Härstedt Jun 12 '17 at 20:15
  • @OlleHärstedt Umm, that's an interesting claim you made. The thing is, Functional Programming paradigm, or any paradigm out there, is never tied down to any particular "real" functional language, much less its syntax. Thus, dictating what conditional constructs should be or should "never" be used just doesn't make any sense. A ternary operator is as valid as a regular if statement, whether you like it or not. The readability debate between if-else and ?: camp is never-ending, so I won't get into it. All I will say is, with trained eyes, lines like these are hardly "too tense". – b0nyb0y Jun 13 '17 at 16:51
5

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.

3

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.