Standards for "Professional Level" Code Quality

1

Hey there FreeCodeCamp

I’ve been working through the new curriculum for just over 6 weeks now and have so far completed Responsive Web Development and JS Data Structures and Algorithms certifications.

Now that I’ve got a pretty decent amount of code that I’ve built up through these projects, I’ve started thinking about what it would take to ‘go pro’

What are some commonly accepted standards for ‘professional quality code’?

I tend to write pseudocode comments as I work to help me structure my thoughts and as a result, often end up with some pretty lengthy code.

Example: my solution for the Roman Numeral Converter algorithm:

function convertToRoman(num) {
    /*
    Converts int to roman numerals.

    ** Only able to handle ints 1 - 399,999 **
    */

    // handle errors
    if (num >= 400000 || num < 1) {
        console.error("convertToRoman is only able to handle integers between 1 and 400000, you passed" + str(num));
        return false;
    } else if ( num % 1 !== 0) {
        console.error("convertToRoman is only able to handle whole numbers, you passed" + str(num));
        return false;
    }

    let numeralsKey = [
        ["I", "V"],            // Ones place:  1:"I", 5:"V"
        ["X", "L"],            // Tens place  10:"X", 50:"L"
        ["C", "D"],            // Hundreds place: 100 : "C", 500  : "D"
        ["M", "V\u0305"],      // Thousands place: 1000: "M", 5000: "V\u0305" == V̅
        ["X\u0305", "L\u0305"], // Ten Thousands place: 10000: "X\u0305", 50000: "L\u0305"
        ["C\u0305"]             // Hundred Thousands place: 100000: "C\u0305"
    ];

    // convert number to an array of single digits, representing each "place" value
    let places = num.toString().split('').reverse(); // array of single digits, reversed so that ones place comes first rather than last

    // for each place value, convert to correct roman numerals
    places.forEach( function (item, index) {

        let x = Number(item);
        let placeNumerals = '';

        if (x == 0) {
            // 0 = nothing; empty string
            places[index] = placeNumerals + '';

        } else if (x > 0 && x <=3) {
            // 1, 2, 3 = "1 value" for the given place, repeated up to 3 times
            for ( let j = 0; j < x; j++) {
                placeNumerals = placeNumerals + numeralsKey[index][0];
            }
            places[index] = placeNumerals;

        } else if (x == 4) {
            // 4: "1 value" then "5 value" for given place. Ex: 4 == "IV"
            places[index] = placeNumerals + numeralsKey[index][0] + numeralsKey[index][1];

        } else if (x == 5) {
            // 5: "5 value" for given place.
            places[index] = placeNumerals + numeralsKey[index][1];

        } else if (x > 5 && x <= 8) {
            // 6, 7, 8: "5 value" for the given place, followed by "1 value" repeated up to 3 times.
            placeNumerals = placeNumerals + numeralsKey[index][1];
            let remainder = x % 5; // how many "1 value" chars are needed?
            for ( let j = 0; j < remainder; j++) {
                placeNumerals = placeNumerals + numeralsKey[index][0];
            }
            places[index] = placeNumerals;

        } else if (x == 9) {
            // 9: "1 value" for the givem place followed by "1 value" for the NEXT place up
            places[index] = placeNumerals + numeralsKey[index][0] + numeralsKey[index + 1][0];

        }
    });

    // now that all the places have been converted to roman numeral equivalents, revert the order back to original order
    places.reverse();
    // combine placeNumerals for each place into one big string and return
    return places.join("");
}

Now as you can see - this thing is a whopper. 70 + lines of code (with lots of whitespace + comments thrown in) .

I’ve seen other solutions on the web that are barely 70 characters:

function romanize(num) {
  var lookup = {M:1000,CM:900,D:500,CD:400,C:100,XC:90,L:50,XL:40,X:10,IX:9,V:5,IV:4,I:1},
      roman = '',
      i;
  for ( i in lookup ) {
    while ( num >= lookup[i] ) {
      roman += i;
      num -= lookup[i];
    }
  }
  return roman;
}

(Ok maybe not 70 chars, but you know what I mean).

Comparing the two, I can tell right off the bat that the “shorter” solution is:

  • Obviously, shorter
  • Simpler, in some ways - based on many repetitions of a simple loop, subtracting from a value and adding to a string, vs mine which decomposes the string, creates and concatenates arrays, etc.
  • More efficient? (not sure)
  • Maybe fancier?

My question is - if my solution looked more like the other one, would that be better? Would either solution be preferable in a professional development context ?

Bubbling up to first principles - beyond things like:

  • Code has to do what it’s supposed to do reliably, and be reasonably resilient
  • DRY - do not repeat yourself
  • Functional programming - creating functions that explicitly state external dependencies, and don’t mutate anything else in the program

What makes good, professional code?

2

This is the kind of thing that will come with practice. Your solution has quite a few comments that I’d consider redundant (e.g. explaining what reverse() and join() do, both of which are common JavaScript methods with self-explanatory titles). One interesting example is this comment:

let numeralsKey = [
    ["I", "V"],            // Ones place:  1:"I", 5:"V"
    ["X", "L"],            // Tens place  10:"X", 50:"L"
    ["C", "D"],            // Hundreds place: 100 : "C", 500  : "D"
    ["M", "V\u0305"],      // Thousands place: 1000: "M", 5000: "V\u0305" == V̅
    ["X\u0305", "L\u0305"], // Ten Thousands place: 10000: "X\u0305", 50000: "L\u0305"
    ["C\u0305"]             // Hundred Thousands place: 100000: "C\u0305"
];

You’ve essentially implemented a more appropriate data structure in pseudocode (a map, which in JavaScript you’d usually implement with an object), but haven’t followed through with your actual code, which uses an array of arrays. This implies you’re cognizant of the structure of your data, which is a good thing — all you need to do is implement a structure that’s more optimal.

Some solid general rules for commenting:

  1. Give variables, functions, modules, etc. descriptive names. Combined with good design, this helps to make your code “self-documenting”, reducing the need for comments.
  2. Now that your code is “self-documenting”, you can comment on the why, not the how.
  3. Disregard rule #2 when the how is especially complicated or counter-intuitive and requires explanation.

Meanwhile, there is at least one way in which your code definitely beats the other solution you found — it handles numbers all the way up to 399,999, rather than just 3,999. That’s not required to pass the tests for the challenge, but it’s a nice touch. You’ve also niftily avoided the trap of reverse()-ing strings containing combining characters, such as \u0305, by reversing the array before joining it into a string.

2 Likes
3

Hi @pseudospencer,

After reading other people code (mostly in the project feedback section of the forum) I think[0] the minimum is:

  • a function signature[1]

  • and a some type of syntax convention[2]

But more complex projects needs more complete (and longer) comments.

Example (Reactjs)

/src/isomorphic/modern/class/ReactBaseClasses.js (line 33-70)


/**
 * Sets a subset of the state. Always use this to mutate
 * state. You should treat `this.state` as immutable.
 *
 * There is no guarantee that `this.state` will be immediately updated, so
 * accessing `this.state` after calling this method may return the old value.
 *
 * There is no guarantee that calls to `setState` will run synchronously,
 * as they may eventually be batched together.  You can provide an optional
 * callback that will be executed when the call to setState is actually
 * completed.
 *
 * When a function is provided to setState, it will be called at some point in
 * the future (not synchronously). It will be called with the up to date
 * component arguments (state, props, context). These values can be different
 * from this.* because your function may be called after receiveProps but before
 * shouldComponentUpdate, and this new state, props, and context will not yet be
 * assigned to this.
 *
 * @param {object|function} partialState Next partial state or function to
 *        produce next partial state to be merged with current state.
 * @param {?function} callback Called after state is updated.
 * @final
 * @protected
 */
ReactComponent.prototype.setState = function(partialState, callback) {
  invariant(
    typeof partialState === 'object' ||
      typeof partialState === 'function' ||
      partialState == null,
    'setState(...): takes an object of state variables to update or a ' +
      'function which returns an object of state variables.',
  );
  this.updater.enqueueSetState(this, partialState);
  if (callback) {
    this.updater.enqueueCallback(this, callback, 'setState');
  }
};

Cheers and happy coding :slight_smile:

Note:

[0] Take it with a grain of salt because I am currently trying to get my first developer job.

[1] I wrote a blog post about comments

[2] JSDoc for example

4

Yes, definitely: a combination of the simplicity of the other solution + your additions would be ideal. I just want to emphasise that I’m not crapping on your code, your code looks perfectly fine.

The other set of code isn’t fancy at all, it just does the dumbest thing possible. Well-written code often looks basic. That’s just something that comes through experience: find the simplest, clearest general solution that uses the minimum amount of code.

There is an important difference between production code and the code you write in these challenges. With the latter, it is completely isolated, and the code you can continue to polish over time. Production code has to deal with the real world, has deadlines, and often looks [and often is] horribly messy.

If I need to fulfill some real-world coding task, the options I have, in order from most to least attractive, are:

  1. I do not write any code and the task is still completed. No code, no chance of anything going wrong, nothing else in the codebase to maintain.
  2. I delete code from the codebase and the task is complete. This possibly needs tests to confirm I won’t break anything, but no actual code and the codebase has less code [overall] to maintain.
  10. I have to add code to the codebase to complete the task. This is by far the least attractive option, as there are now multiple avenues of risk, more code to maintain, and I need to write new tests.

So assume it is the last option. I want to write as little code as possible, and I want that code to do as little as possible. Say I want to modify your code: I’m probably going to end up with something very much like the other set of code. The comments will be useful at first, but I’ll need to keep updating them. After the second or third time doing that it’ll start to drive me crazy, so I’ll just delete them. I need to run the program in my head, so I want as little conditional logic as possible (otherwise I’ll lose track). So all those if statements ideally need to get stripped out. And so on.

I strive to write code that is easy to delete and completely rewrite/replace. Complexity and size are enemies here.

It’s important to make sure that the question being asked is the correct one as well, to not get stuck down a rabbit-hole. As an example, there’s a challenge in the algorithm section, Where do I belong. The description:

Which looks quite complicated, and it can take quite a while to reach a satisfactory solution. But what if you were to change the phrasing of the question:

That’s a bit simpler, and it turns out to be exactly the same

This is all well and good, and should always be considered of paramount importance. But reality tends to translate itself into a lot of tiny edge cases. So there always needs to be an assessment of whether it is worthwhile to spend time figuring out an elegant general solution, or whether just copy-pasting some existing code will do. Is the tradeoff worthwhile?

I could go on, but

1 Like