You're not doing code quality right

I've been itching to write about this topic for ages, and only recently found the space to do so. Turns out, as most people understand it, code quality is measurable through this 10 things:

• code uniformity and self-similarity
• adherence to agreed code formatting
• adherence to naming schemes
• usage of declared variables exclusively
• lack of declared but unused vars
• lack of development artifacts (console.log, debugger, etc)
• lack of easily avoidable repetition
• approval of automated tests
• approval of an automated linting step
• passing of any vanity rules the development community has agreed on, like arity and so on

Some of this variables may or may not apply according to your programming language and environment (ie doing Haskell-based window manager work it's probably really hard to have undeclared variables or development artifacts laying around) but in a broad sense, this are very likely the things a code reviewer will look for in your Pull Requests.

The issue with this is, while this checks prevent code that is nonconformal from entering the broader codebase, nothing of the above will prevent a given piece code from being shit.

It's hard to do reviews of code in big systems, and we're not looking in the right places when we do.

Take the following code for example:

// is/Func.js
(function(fn){
  const isFunction = function(obj){
    return obj.constructor === fn.constructor;
  };
  window.isFunc = isObjFunction;
})(
  (function(fn){ return fn; })(
   (function(obj){ return obj; })(
     (function(){ return arguments[0] })(
       function(){}
     )
   )
  )
);

// tests/is/Func.spec.js
it("returns true when the passed comparable object is a function, false otherwise", function(){
  expect(window.isFunc(function(){})).toBe(true);
  expect(window.isFunc("test")).toBe(false);
});

Is this code "good"?

• it passes all tests
• it implements a well known module pattern
• is self similar
• there is a naming scheme that is being respected
• there are no development artifacts anywhere

... you get the picture: It is still shit code. More importantly, it is conformal shit code, which relies on the Window object being present, the designated name not being important for other things, and, oh, the 3 unnecessary nested IIFEs with different arguments inside all being correct.

It all comes down to what is considered a marker of "quality" in an organizational level. Is the code better if it's simpler or if it's more familiar? Does wether or not the code uses a single kind of quotes make the code better? Has your linter forcing you to use requestAnimationFrame instead of setTimeout ever prevented any bugs?

Reading the code we are reviewing and actually deciding wether or not it is up to a certain quality level is a difficult exercise, especially when dealing with large pull requests and our "actual" workloads (as if the stewardship of quality in our product wasn't part of the workload somehow). But letting shit, conformant code in carries the risk of trapping the project in a downward spiral where, by the same rules defined above, the kind of shit code that I presented ends up being the way things are done.

If you've ever worked with react on a mid to large project, you'll have met folder structures like store/reducers/profile/photo/actualReducer.js, accompanied by store/selectors/profile/photo/current/getCurrentPhoto.js.
The project had code reviews, the files were reviewed, linted, inspected, and a mountain of validation papertrail was produced favoring the terrifying new reality that every goddamn getter needs its own file and test. We've all agreed to do it this way, after all. It passed reviews and was talked about as great in last month's all-hands.

A fear of being "that guy" will have landed the project in a situation where modifying an event handler will have a whole subtask called "Find the getter for the result of this event handler", and code quality will objectively still be shit in terms of maintainability, extensibility, discoverability and more generally, ownership.

Reviewing instead for clarity, economy of resources, ease of maintenance and adequateness is hard, but almost always ends up improving the end product, which is in consequence smaller, easier to own, walk through, and test.

Walking back to the original bit of code and comparing it to:

// is/Func.js

var module = module || {exports: {}};
(function(container){
  function IsFunction(obj){
    return typeof(obj) === 'function'
  }

  if(context.exports){
    context.exports.IsFunction = IsFunction;
  }
  else {
    if(container.isFunc){
      throw new Error("container.isFunc already exists");
    }
    container.isFunc = IsFunction;
  }
)(typeof(window) !== 'undefined'? window : module);

It is obvious a number of things have been changed; however, is it better?

This code is "better" in that:

1. it can live in places where Window is not present
2. it uses a simpler conditional check
3. it cares about trashing the context where it sits.
4. it is less fragile, accounting for the fact that the comparable object (its first parameter) can be undefined
5. declaring a function is less convoluted than assigning an anonymous function declaration

If you and your coworkers can agree those things are better than wether or not all quotes used are either single or double, then the second bit of code is better, and the first is worse.

Plus, as far as units of code go, this one is easy to follow in its entirety.

Yes, the example is entirely artificial, and it's not very common to find such a clear cut scenario, but the judgement calls involved are going to be the same.

Choosing the right parameters for quality and code standards, while a hard discussion, is very much worth it. So maybe try that next time?