Code quality is meaningless

@timruffles @sidekicksrc

WTF is code quality?

Not application quality

• e.g Slow
• Doesn't work
• Poor design
• Basically: IE6

Everything the user can't see

• and doesn't want to
• like 'plumbing quality': just want the toilet to flush
• can have good app with bad code, and reverse

Who does it anger?

• app quality: users
• code quality: programmers

Code quality is for programmers

WTF is good code?

WTF is a good film?

How can a film be 4/5 stars for everyone?

Film quality...

...depends on the person

• I'm not going to rate 'Fast n Furious 6' 5/5
• Might have done when I was 14

...and doesn't

• e.g out of focus, mistakes in plot
• well made, but of a genre I don't like
• wish film critics realised this

What 'quality' means depends on us

Unfortunately...

...as programmers are faddish and emotional people

Quality is the most subjective bit of coding

Idiomatic Haskell

go a@(Internal al ar abyte abits) ak b@(Internal bl br bbyte bbits) bk
  | (dbyte, dbits) < min (abyte, abits) (bbyte, bbits) = fork a ak b bk
  | otherwise =
       case compare (abyte, abits) (bbyte, bbits) of
         LT -> splitA a ak b bk
         GT -> splitB a ak b bk
         EQ -> link a (go al ak bl bk) (go ar (minKey ar) br (minKey br))

Obvious hey?

Or 'good OOP'

@garybernhardt For example: Looking at this code is a great way to demonstrate how terrible it is to treat AR as a DAO: http://t.co/NkxWODIb

Can we leave taste aside...?

• OOP or FP or procedural

...what's left?

• "This does the same things 3 ways"
• "I can change anything!"
• "This looks like Java!"
• "WTF?! Why did that happen?!"

So code quality can be given a meaning

We can agree on the adjectives for good code

...or bad

Aspects of quality code

• Concise
• Simple
• Readable
• Extensible
• Testable

Concise

A sentence should contain no unnecessary words, a paragraph no unnecessary sentences.

William Strunk Jr. in Elements of Style

Flabby code

function email(string) {
  if(emailRe.test(string)) {
    return true;
  } else {
    return false;
  }
}

Don't repeat yourself

DRY

Programming is pretty much DRY applied to life

Problems of non-DRY code

• fragile: change in n-1 places = bug
• maintainance overhead

How to track?

• flay (hack it to do JS with esprima), flay-js?
• structural duplication detection

Flay in Sidekick

Single point of truth

SPOT

Related to DRY

Each fact about the system captured once

e.g

• How old does a user need to be?
• Scatter in view, model etc: bad
• Capture the knowledge in one place
• More complex things: capture in one function

$("form").submit(function(el) {
  if($(el).find("[name=age]").val() > 18) {
  }
})

Simple

KISS

Actually, nobody tries to write complex code

So this is a tricky word

We should probably abandon 'simple' and 'complex'

Simple

from Latin simplus. Originally referred to a medicine made from one constituent

• vs complex, something made from many parts
• Rich Hickey's definition
• 'doing one thing well'

Instead

Flavours of simple

• low number of concepts
• low detail
• few responsibilies
• low difficulty
• few dependencies/interactions

Low number of concepts

• no OOP, FP, advanced language features
• ORM vs SQL

function main() {
  // ... 5000 lines
}

open("tcp://google.com")
open("tcp://some-socket.sock")
open("/dev/tty18")
open("path/to/some-file")

Low detail

• attempt to abstract everything

function Account() {
}
Account.prototype.transfer = function(anotherAccount) {
  // very little work per method
  return new AccountTransfer(this,anotherAccount).run();
}

<h2>{{ framework.name }} is HTML made awesome!</h2>

Few responsibilies

Low difficulty

Few dependencies

function sprintf(format,...args) {
  // completely depends on input
}
function UserView() {
  // depends on everything
  this.el = $("#user");
  this.user = $.get("/users/" + window.user.id);
}

Can we measure these?

Mesurable?

• low number of concepts
• low detail
• few responsibilies
• low difficulty
• few dependencies/interactions

Low difficulty

• code that doesn't have much to do is easy to understand
• global vs local

Global - low difficulty

• "write a static site generator with markdown"
• simple code - straight transformation

Global - high difficulty

• "integrate the UK's medical records"
• OMG complex, operations, integration, special cases

Local

• 500 LOC functions: doing a lot
• 10 LOC functions: each function easy to understand
• 15 variables with many branches
• no branches, simple transform

Measuring global difficulty

• How frightened are you of your code/deploy/monitoring?

Measuring local difficulty

• how many paths?

if(x) {
} else {
}
if(y) {
} else {
}

Branching factor

• 4 paths: tt, tf, ft, ff
• Cyclomatic complexity: 3

if(x) {
} else {
}
if(y) {
} else {
}

Variable count

$digest: function() {
  var watch, value, last,
      watchers,
      asyncQueue = this.$$asyncQueue,
      postDigestQueue = this.$$postDigestQueue,
      length,
      dirty, ttl = TTL,
      next, current, target = this,
      watchLog = [],
      logIdx, logMsg, asyncTask;

...continued

  // Insanity Warning: scope depth-first traversal
  // yes, this code is a bit crazy, but it works and we have tests to prove it!
  // this piece should be kept in sync with the traversal in $broadcast
  if (!(next = (current.$$childHead || (current !== target && current.$$nextSibling)))) {
    while(current !== target && !(next = current.$$nextSibling)) {
      current = current.$parent;
    }
  }
} while ((current = next));

How many things can you keep in your head at once?

Metrics for difficulty

• logical lines of code
• cylomatic complexity: branching
• halstead complexity: operands and operations
• ComplexityReport offers both, JSHint has CC cutoff

Measuring dependencies

The more interconnected the code, the harder to change

Worst type: temporaral

• e.g app.js touches global variable user, user_view.js relies on it
• very hard to see

DOM = global

• $(".class") is a global
• always moduarlise DOM, passing in element to components

Modularity

• doesn't just mean having a module system
• unless the code is largely independent, it's not modular

// global variable - regardless of the 'module' in there
require("models/user.js").userCount += 1;
// exactly the same
(global || window).require("models/user.js").userCount += 1;

Tools

• ComplexityReport - entanglement metric
• detects nth level dependencies (e.g A -> B -> C)

Readable

Idiomatic

Means specific to the language

e.g Javascript

;(function(undefined) {

  window.something = something;

  function something(n,opts) {
    var user = opts && opts.user;
    var n = run == null ? 0 : n;
    var names = [].slice.call(arguments,2);
    for(var p in x) {
      if(x.hasOwnProperty(p)) // ...
    }
  }
})();

...and not bringing other languages with you

The obvious tool to use is...

ESLint

• uses esprima parser
• much nicer code-quality than JSHint's Pratt parser

JSHint

• featureful

AngularLint?

• would be nice to have library/framework linters

Good naming

A recipe

• longer the bigger the context
• no single letters beyond i, k, and v
• avoid abbreviation, hard to be consistent
• no types
• replace trivial comments with variables or functions

Good comments

Comment when

• intention not clear from code
• a variable or function name not enough
• there's a surprise

Comments are not documentation

Documentation is...

• example code (tests are good, cf. doctest.js)
• explanations of the system at high-level

Reusable

Happens a lot less than you think

• overpromised
• how often is what you're writing generic enough to be reused?

Is not good per se

• AbstractSingletonProxyFactoryBean
• Where does your code want extension?

If you’re willing to restrict the flexibility of your approach, you can almost always do something better

John Carmack

More generic, more reusable

• without effort
• e.g lodash
• FP achieves 'reuse in the small', OOP hasn't (too specific)

Extension/changeable

What prevents change?

Coupling

Large APIs

• the more methods a module has, the more dependencies it'll grow

Preventing ad-hoc reuse

• Javascript has VERY primitive privacy

var Public = (function() {
  var cantSeeMe = //;
  function Public() {
    privateA();
  }
  function privateA() {
    cantSeeMe;
  }
  return Public;
})();

Allowing ad-hoc modification

• the C/python way: _somethingPrivate
• good for internal code, more risky for external

var Public = (function() {
  function Public() {
    privateA();
  }
  Public._canSeeMe = //;
  Public._privateA = function () {
    this._canSeeMe;
  }
  return Public;
})();

Testable

Test 0

Can you test your code?

Sometimes, easily

function sprintf(str) {
  var params = [].slice.call(arguments,1)
  var types = {
    "%s": function(x) { return x + "" }
  }
  var matches = 0
  return str.replace(/(%[is])/g,function(match) {
    return types[match](params[matches++])
  })
}

it("formats correctly",function() {
  assert.equal("MU!",sprintf("%s","MU!"));
})

Often this simple?

Get real

'generateResetToken': function (req, res) {
    var email = req.body.email;

    api.users.generateResetToken(email).then(function (token) {
        var siteLink = '<a href="' + config().url + '">' + config().url + '</a>',
            resetUrl = config().url.replace(/\/$/, '') +  '/ghost/reset/' + token + '/',
            resetLink = '<a href="' + resetUrl + '">' + resetUrl + '</a>',
            message = {
                to: email,
                subject: 'Reset Password',
                html: '<p><strong>Hello!</strong></p>' +
                      '<p>A request has been made to reset the password on the site ' + siteLink + '.</p>' +
                      '<p>Please follow the link below to reset your password:<br><br>' + resetLink + '</p>' +
                      '<p>Ghost</p>'
            };

        return mailer.send(message);
    }).then(function success() {
        var notification = {
            type: 'success',
            message: 'Check your email for further instructions',
            status: 'passive',
            id: 'successresetpw'
        };

        return api.notifications.add(notification).then(function () {
            res.json(200, {redirect: config.paths().webroot + '/ghost/signin/'});
        });

    }, function failure(error) {
        // TODO: This is kind of sketchy, depends on magic string error.message from Bookshelf.
        // TODO: It's debatable whether we want to just tell the user we sent the email in this case or not, we are giving away sensitive info here.
        if (error && error.message === 'EmptyResponse') {
            error.message = "Invalid email address";
        }

        res.json(401, {error: error.message});
    });
},

Correlates with rest of quality

Anti-testable code is...

• coupled
• closed to modification
• complex (doing > 1 thing)

Measuring tests

• istanbul: measures % of LOC run by tests

Putting it all togther

• CI server running tests and coverage tools
• Also run ComplexityReport, Flay, JSHint
• JSHint fast enough for a pre-commit hook, often good as 'poor man's compiler'

All the tools

• JSHint/ESLint/JSLint
• ComplexityReport
• Flay
• Istanbul

Now you've measured the quality, what now?

Measured == managed?

Yes

That can be bad

Use responsibly

Weigh the airplane

Gaming is easy

So...

∞-junior, idiot-savant

Conversation starter

Wall o’metrics

Stay proud of your team’s code

Tools are good source of information

You’re the expert

Any questions?

@timruffles

sidekickjs.com

Creative commons work