I reviewed my code from my first job in 2015

Mar 07, 2025

I am fortunate that the project for the first job I had was actually a non-profit's project, and it is open source!

I found it just recently and I am very happy to be able to go through it and find out what I've learned in my 8-9 years since the Summer of 2015.

I contributed +35,000 and -20,000 lines of code professionally during that year, so I'm looking forward to it. The commits.

All right let's start with the basics, this is more about code style as opposed to engineering.

Comments

They were a mixed bag. A lot were shit. Some were useful.

Take for example this simple commit, with a clear commit message.

fix sort in findTrending

  // sort
  result.sort(function (a, b) {
-   return b - a
+   return b.followersCount - a.followersCount
  })

An enlightening comment indeed by the way (// sort).

A more useful comment would've been a quick clarification return b.followersCount - a.followersCount // descending

Reminds me of the old category of memes:

Sometimes the comments were actually harmful!

var amounts = {}, // what we're returning
  result = []

// ...

return result

Oops. The comment points to the wrong variable! We actually return result.

This is likely a copy-paste error. In other functions in commits in around that area, amounts = {} IS actually the value being returned.

But again those comments are not useful.

It is understood it is what we're returning, it's evident at the bottom of the short function with the statement return amounts.

On the other hand some comments were arguably useful, and actually seems to be attempting to follow the Pseudocode Programming Process (PPP, described in Clean Code 2). Attempting to.

If we look at the full function definition, we can see a basic version of PPP taking place. In that context, the comments become more useful. Somebody quickly reading the code (like myself right now) can see the intent with the comments, and confirm quickly that the code is indeed doing that.

var findTrending = function (resultFromKnex) {
  var amounts = {}, // what we're returning
    result = []

  // get all the voice IDs
  var followedVoices = resultFromKnex.map(function (val) {
    return val.voiceId
  })

  // find out how many times each number repeats
  followedVoices.forEach(function (voiceId) {
    if (amounts[voiceId]) {
      amounts[voiceId] += 1
    } else {
      amounts[voiceId] = 1
    }
  })

  // convert to array
  for (var voiceId in followedVoices) {
    result.push({ id: voiceId, followersCount: followedVoices[voiceId] })
  }

  // sort
  result.sort(function (a, b) {
    return b - a // [of course this is wrong, and fixed in a later commit]
  })

  return result
}

Not a bad piece of code for probably some of my first 1000 lines of code. Certainly professionally.

Sadly later in my professional life, PPP waned off. The habit disappeared. It is something I'm working on to do again.

Callbacks

This isn't particularly an issue. This was the meta back then.

Code like this handles a particular Express route handler:

FeaturedVoice.all(function (err, allFeaturedVoices) {
  if (err) { return next(err) }
     
  EntitiesPresenter.build(allFeaturedVoices, req.currentPerson, function (err, presenterVoices) {
    if (err) { return next(err) }
     
    res.format({
      html: function () {
        res.locals.featuredVoices = presenterVoices
        req.featuredVoices = presenterVoices
        res.render('browse/featured/voices')
      },
      json: function () {
        res.json(presenterVoices)
      },
    })
  })
})

So I'm sure there are readers that are no longer familiar with this style of coding, it is very rare in modern JavaScript.

It's a method to achieve asynchronous execution of code, by taking advantage of the JS event loop in the most straightforward way: just pass the result along to a callback function.

This is what we did before Promises became standard practice with the coming of ES2015 (ES6).

Personally I think it's a shame that we no longer see this kind of code. We now rely on Promises and more importantly the async/await syntax, which is based on Promises.

If the code above was written with callbacks but with modern syntax and library usage, following the conventions of that code, and my personal preferences on some things, it'd probably look something like:

// Somewhere above
// const async = require('async')

async.waterfall([
  FeaturedVoice.all,
  (featuredVoices, cb) => EntitiesPresenter.build(featuredVoices, req.currentPerson, cb),
], (err, result) => {
  if (err) { return next(err) }

  res.format({
    html() {
      res.locals.featuredVoices = req.featuredVoices = presenterVoices
      res.render('browse/featured/voices')
    },
    json() {
      res.json(presenterVoices)
    },
  })
})

The code with async/await equivalent would look something like:

const featuredVoices = await FeaturedVoice.all();
const presenterVoices = await EntitiesPresenter.build(featuredVoices, req.currentPerson);

res.format({
  html() {
    res.locals.featuredVoices = req.featuredVoices = presenterVoices
    res.render('browse/featured/voices')
  },
  json() {
    res.json(presenterVoices)
  },
})

The main issue I have with async/await, although it is very ergonomic, is that the event loop is no longer evident, even though it is what is actually driving the mechanism.

I normally write this code though, because callback patterns are not the norm anymore.

Commit messages

See for yourself, these are the oldest commits: https://github.com/alshafei/crowdvoice.by/commits/master/?author=greduan&after=1388dbd9d06125847cf1790334021d679d293e68+629

No standard.

All over the place.

There is an argument that these messages don't really matter in reality if you don't intend to make it part of some kind of semantic release pipeline.

Nowadays I either follow Conventional Commits, if it makes sense for the project because it's policy or because it's primarily technical individuals reading the commit messages. If non-technical users will be looking at it, I use normal commit messages.

And when I use "normal commit messages" I try to follow these basic rules described in an article titled How to Write a Git Commit Message:

Separate subject from body with a blank line
Limit the subject line to 50 characters
Capitalize the subject line
Do not end the subject line with a period
Use the imperative mood in the subject line
Wrap the body at 72 characters
Use the body to explain what and why vs. how

The bolded points are the ones I particularly focus on when writing my commit messages. A focus on semantics. The rest are stylistic points and while I follow them, they're not worth talking about.

The semantic point are also relevant when thinking of Conventional Commits.

Credit where it is due

For what it's worth, there is some good quality code produced, that doesn't look too dissimilar from what I would produce now, albeit with different syntax or paradigms and perhaps clearer logic.

For example if we take a look at this commit b04ad8cc9ccffae3805cb7c51f76b42530e813b5, the only points I can really criticize are:

The console.log() statements which were leftover.
I would've used async.waterfall() and instead of setting ids = inside of the series, I would'v done return callback(result.map(function (val) { return val.entityId })).
notification is a function-scope variable for some reason. It is only used at the end, it didn't need to be declared at the top of the function. It violates the Locality of Behaviour principle.

A big migration

One of the larger pieces of work I recall was a migration.

We had a fundamental shift of how the messaging system made its relations. But we wanted to keep all the previous data too, is what I recall. But apparently there was also an issue with duplicate messages, related to this.

So I recall spending around one week figuring out this one commit: 3b170e246d24001589840c2d36bb35b6ab692456

In particular I recall this migration took a while: 20160121174029_merge_duplicate_MessageThreads_records.js

By this point Promises were in use. Although async/await wasn't there yet.

For the record, I did not write the code with the bug, was written before I started working. I was, however, assigned to fix it.

My first large feature

I was tasked with implementing the endpoint that the frontend sends a video to be the homepage's hero section, that video has to be compressed properly, uploaded to AWS, and be made available.

Not that hard right?

Get file
Store in AWS raw
Convert to other formats with ffmpeg
Upload other formats to AWS
Record all in DB

It was the first feature I implemented with a deadline. Christmas 2015.

Looking at the code, I'd say it's quite decent.

It doesn't split things into services or function calls. That's fine for me, it's only used here, and it makes it very clear to work with and debug (just read top to bottom).

Testing

My lead got sick of me pushing code that the frontend guy would test and it wouldn't work, would return easy errors that could've been avoided by just testing. Nowadays I could probably avoid those errors just by reading the code, they were elementary errors.

So my lead said "I will not merge code from you that doesn't have tests". I recall thinking after a few months they should know I push good code, and given me a break, but alas.

I would cheat.

I would submit the code and specify which test commands to run.

I spent like a week trying to get ALL tests to pass at once, but I couldn't manage it, so I just optimized for the tests that I needed him to run in that moment, for the commands that I gave him. I don't know if he ever figured that out.

The largest difference

All nice and well, a lot of these things are superficial.

The largest change is that now my mindset around programming is very different.

Back then I was just code monkeying, implementing requirements. Barely. I didn't have a huge spirit for quality assurance and the such.

Nowadays I'm thinking of the project on a much longer time span. And I don't push code unless I'm 99%+ that it works.

As well now I focus on Data-Oriented Design (as defined by Mike Action). How can I make data flows clear, easy to follow, and easy to change? This trumps any other concern. I'm VERY wary of abstraction and "architecture" for projects that aren't handling thousands of requests per second, or at least per minute.