4
\$\begingroup\$

I am new to "modern" Javascript, and I am reviewing some code from a frontend contractor. I'd like to make the sequential code more descriptive/procedural.

The following code is a from a file called build.js, which is used to start a new project build (via Webpack). I guess we could call that a "build script" of sorts:

import path from 'path';
import webpack from 'webpack';
import fse from 'fs-extra';
import config from './../webpack/webpack.config.production';
import paths from './../paths';

fse.emptyDirSync(paths.distPath, (err) => {
  if (err) console.error(err);
});

fse.copySync(
  paths.staticPath,
  path.join(paths.distPath, 'static'),
  { clobber: true },
  (err) => { if (err) console.error(err); }
);

webpack(config).run((err, stats) => {
  if (err) console.error(err);
  else console.log(stats.toString());
});

This glue code is sort of simple, if you know modern JS. Still, I find that the lack of high-level description or modularity will be a problem later on for junior JS people, or for bigger and more complicated scripts.

It looks like in the JavaScript world, people like to comment, and I often find this version:

Version 1

... imports ...

// Empty build folder
fse.emptyDirSync(paths.distPath, (err) => {
  if (err) console.error(err);
});

// Copy in static files
fse.copySync(
  paths.staticPath,
  path.join(paths.distPath, 'static'),
  { clobber: true },
  (err) => { if (err) console.error(err); }
);

// Build
webpack(config).run((err, stats) => {
  if (err) console.error(err);
  else console.log(stats.toString());
});

This seems to be the "most idiomatic JavaScript" code when looking at other open source codebases.

I am not used to writing scripts, but in my Ruby/Rails community, a lot of people prefer to write self-descriptive code instead of comments, at least for their business logic. I know this is a script and it differs from business logic, but the following makes me more comfortable:

Version 2

... imports ...

function main() {
  emptyBuildFolder();
  copyStaticFilesToBuildFolder();
  build();
}

function emptyBuildFolder() {
  fse.emptyDirSync(paths.distPath, (err) => {
    if (err) console.error(err);
  });
}

function copyStaticFilesToBuildFolder() {
  fse.copySync(
    paths.staticPath,
    path.join(paths.distPath, 'static'),
    { clobber: true },
    (err) => { if (err) console.error(err); }
  );
}

function build() {
  webpack(config).run((err, stats) => {
    if (err) console.error(err);
    else console.log(stats.toString());
  });
}

main();

Basic main() or init() technique. I like the modularity and it feels more maintainable.

Here's an OO version which is the same but with more syntactic sugar:

Version 3

... imports ...

new class {

  constructor() {
    this.emptyBuildFolder();
    this.copyStaticFilesToBuildFolder();
    this.build();
  }

  emptyBuildFolder() {
    fse.emptyDirSync(paths.distPath, (err) => {
      if (err) console.error(err);
    });
  }

  copyStaticFilesToBuildFolder() {
    fse.copySync(
      paths.staticPath,
      path.join(paths.distPath, 'static'),
      { clobber: true },
      (err) => { if (err) console.error(err); }
    );
  }

  build() {
    webpack(config).run((err, stats) => {
      if (err) console.error(err);
      else console.log(stats.toString());
    });
  }

};

(or we could explicitly define a Build class and then call new Build() right away).

I prefer version 2 or 3, but in the JavaScript ecosystem I see a huge amount of code like Version 1. At least that was my feeling after reviewing ~ 50 real world open source JS React+Webpack projects.

I don't want to be the one guy writing non-idiomatic JS, but on the other hand, I find Version 2 or 3 so much better. I would only use comments to describe actually complicated things, or to explain the whys (and let variable/function/class names describe the hows). Why am I the only one thinking like this? Which version should I use?

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Not sure why someone down-voted my question, as I tried to be as thorough as possible \$\endgroup\$ – Jerome Dalbert Jul 4 '17 at 23:48
  • \$\begingroup\$ What problem are you trying to solve here? The build script won't likely to change often and it's readable enough already. \$\endgroup\$ – Pavlo Jul 5 '17 at 4:21
6
\$\begingroup\$

I would say a very big "no" to Version 3. Instantiating an object should not have a side-effect of emptying a directory and kicking off a build — that's very surprising behavior. In OOP, the methods should be the verbs. Here, your noun has gone rogue and is doing all kinds of activities.

Version 2 adds verbosity with questionable benefits. If you want to define functions, then the functions should accept parameters and return results. Here, the functions act on inputs that are essentially global variables, so you still have to read the code within each function to understand what it does.

Version 1 is good, and I would say that the added comments are probably beneficial.

All four of the scripts lack decent error handling. If an error occurs, I would expect the entire script to abort, rather than printing a message and trying to continue.

\$\endgroup\$
1
  • \$\begingroup\$ This is the correct answer. I'd add that even while "named functions over comments" is a good principle in general, they're simply overkill for such a simple script. \$\endgroup\$ – Jonah Jul 6 '17 at 6:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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