Categories
JavaScript Best Practices

Bad Smells in JavaScript Code

Writing software is hard. There’re only a few ways to do it right and there’re many ways to do it wrong and make our lives hard.

In this article, we’ll look at some ways to write bad code by looking at a few codes smells.

Alternative Classes with Different Interfaces

2 classes that do the same thing but with different interfaces aren’t good because of duplication.

We don’t want to have that. Therefore, we may want to create a superclass with the shared code and then have subclasses that have differing methods.

Incomplete Library Class

Reuse isn’t overrated.

Library builders have a tough job. They may make incomplete classes but they don’t let us modify them to do what we want them to do.

Therefore, this makes the library class useless to us unless we can add the functionality that we want.

We may have to add new methods to these classes directly to solve this problem.

For instance, if we have a class that we imported, we can add own methods to it by writing:

const mixin = {
  foo() {
    //...
  },
  bar() {
    //...
  }
}
`
Object.assign(Foo.prototype, mixin);

In the code above, we merged the code from the Foo class’s prototype with the methods from the mixin object to incorporate more methods into the class with the Object.assign method.

Data Class

Data classes are classes that only have fields.

These are classes that are probably manipulated in too much detail by other classes.

Therefore, we should encapsulate the public fields if they’re all exposed as public.

We can also encapsulate collection fields if necessary.

To encapsulate them, we can make the fields private and add methods to access and set them.

Refused Bequest

Subclasses inherit methods that parent classes give access to them.

If we don’t need those classes in the parent, then we can just push them down to the subclasses.

Then not all the subclasses will inherit the methods from the parent class while they can still stay in the subclasses that need to have them.

Comments

Comments are good for some things. We can comment on why we’re doing something, but we don’t need to explain how we’re doing something in the comments since we’re already doing it in the code.

Also commented code is bad. We should take them our since they aren’t run, to begin with.

Conditionals That Aren’t in Their Own Line

We should break conditionals into their own line so that we can read them easier.

So instead of writing:

if (foo) {
  //...
} if (bar) {
  //...
}

We write:

if (foo) {
  //...
}
if (bar) {
  //...
}

Annotate Optional Parameters

Optional parameters should have a default value in JavaScript. For instance, we can write the following code to indicate that a parameter is optional:

const foo = (a = 1) => {
  //...
}

Watch out for “Dead Stores”

Dead stores are when we assign a value to a variable but it’s reassigned without using the original value.

Therefore, we never actually need the original value, so we can remove that line.

So instead of writing:

let x = 1;
x = 8 * 10;

We write:

let x = 8 * 10;

Don’t Invert Our Booleans

Double negatives are always harder to read than direct conditional expressions.

Therefore, we should write in a more direct way is possible. For instance instead of writing:

if (!(x > 10)) {
  //...
}

We write:

if (x <= 10) {
  //...
}

As we can see, the second if statement is much easier to read than the first and it’s also shorter. And they’re both the same.

Use Templates Strings

Template strings are the best kind of JavaScript strings. We can interpolate expressions in it, which we can’t do with any other kind of strings.

Also, we can create multiline strings with it by just typing in the line breaks inside this string.

Since backticks are used as delimiters for template strings, quotations can be used inside strings without escaping them.

Conclusion

Template strings are great. They let us do many more things that we can’t do with old-style strings.

To add more methods to incomplete library classes, we can use Object.assign to add our own methods to them.

Also, we need to encapsulate data classes so that our code isn’t too tightly coupled with data classes.

Categories
JavaScript Best Practices

JavaScript Clean Code — Smells and Heuristics

Bad code has lots of unique characteristics. In this article, we’ll look at each one and what they are. We look at writing comments, functions, and general code smells and heuristics.

Comments

Inappropriate Information

Information that shouldn’t be in the comments like author and changelogs are in the comments. They should be in source control systems, bug trackers, and other record-keeping systems.

Change histories should be in source control systems for example. It has metadata for authors, code changes, change date, etc. These shouldn’t be in the comments.

Comments should be for technical notes about the code.

Obsolete Comment

Comments that are old, irrelevant, or wrong are misleading. They get old quickly. The code should be clean enough to not need so many comments.

They become outdated quickly, so they should be avoided.

Redundant Comment

If the code adequately explains itself, then we don’t need comments explaining it. JSDoc that says nothing more than the signature isn’t also very useful.

They should say things that can’t be shown by the code.

Poorly Written Comments

Comments that are worth writing should be written well. We should make sure that they’re the best comments that we can write.

Commented-Out Code

Commented out code can be misleading. Why are they still there if they’re commented out?

We should delete the code if they aren’t needed. They can also be reverted from the source control system’s change record.

Environment

Build Requiring More Than One Step

Builds shouldn’t require more than one step. The more things we have to do manually, the worse that everyone suffers.

We shouldn’t have to do manual operations like checking code out from source control or run a bunch of commands and scripts every time we have to run a build.

There’re so many build pipeline solutions that the button should be a one-click process.

Tests Requiring More Than One Step

Running tests should also be easy. All tests should be run with one command. Either we can run commands with one click on an IDE or by typing in one command.

Functions

Too Many Arguments

Functions should have a few arguments as possible. No arguments are best. More than 3 is questionable.

Output Arguments

We shouldn’t be returning arguments straight at the end of the function as-is. It just makes no sense.

Flag Arguments

Flag arguments mean that a function does more than one thing, so they should be eliminated.

Dead Function

Functions that aren’t called should be removed. Dead code takes up space and misleads people. We can always get it back from source control history.

General

Multiple Languages in One Source File

One file should only be one language. The more language is in a file, the more confusing it is.

Clean separation of logic and markup is always good. JSX is just a different syntax for JavaScript, so it’s actually one language.

Obvious Behavior That’s Unimplemented

Obvious behavior should be implemented. If it’s so useless that it can stay unimplemented, then we can probably eliminate it.

We should implement obvious functionality as described by a function so that the name isn’t misleading.

Incorrect Behavior at the Boundaries

Developers often trust their intuition when they write their functions and think that everything works. We often ignore corner and boundary cases.

We should check our code by writing tests for these conditions and don’t assume that it’ll work properly with them.

Overriding Safety Mechanism

Safety mechanisms in our code shouldn’t be overridden when we write code. Risky changes should be minimized. The end might be lots of bugs and lots of debugging.

Turning off failing tests are bad and we should think about the possible consequences when we do.

Duplication

Code duplication is bad. Every time we have to change duplicate code, we have to change them in multiple places.

We can remove them by abstracting the code and putting them in a central location.

Coding becomes faster and less error-prone since we only have to change things in one place.

The most obvious form is clumps of identical code.

Another form is conditional statements that appear multiple times in different parts of the code. We can replace them with polymorphic code.

Most design patterns are well-known ways to eliminate duplication. They’re structured to eliminate them.

Code at Wrong Level of Abstraction

When we abstract code, we should make them completely. The separation is complete. All the higher-level concepts to be in the base class.

Constants, variables, and utility functions shouldn’t be in the base class.

Source files, components, and modules should at a higher level of abstraction also be separated from ones with lower levels of abstraction.

We don’t wrong higher-level and lower-level code mixed together.

For example, if we have an Account class:

class Account {  
  saveAccountToDb() {}  
  getAccount() {}  
  setAccount() {}  
}

Then we have code at 2 levels of abstraction because we have saveAccountToDb which is a helper method to save data to the database.

We want to move it to a helper class or function.

Conclusion

Comments should be minimized and are useful and up to date when they’re there.

Functions should have as few arguments as possible and only do one thing. This also means we shouldn’t have flag arguments.

When we write code, we should check for boundary and corner cases to avoid bugs. Also, we should override safety features like removing important tests. They’re probably there for a reason.

Finally, code duplication is bad.

Categories
JavaScript Best Practices

JavaScript Code Smells — Functions

In programming, a code smell is a characteristic of a piece of code that indicates there may be deeper problems. It’s a subjective characteristic used for judgment of whether the code is decent quality or not by looking at it.

In this article, we’ll look at some code smells of JavaScript functions, including too many parameters, long methods, identifier length, returning too much data, and long lines of code.

Too Many Parameters

Functions can have too many parameters. If they have too many, it makes the function more complicated when reading it and calling it. It probably means that we can clean up the code in some way to make this easier to read.

For example, the following function takes many parameters:

const describeFruit = (color, name, size, price, numSeeds, type) => {
  return `${fruitName} is ${fruitColor}. It's ${fruitSize}. It costs ${price}. It has ${numSeeds}. The type if ${type}`;
}

6 parameters are probably too many. We can clean this up by passing in an object instead:

const describeFruit = (fruit) => {
  return `${fruit.name} is ${fruit.color}. It's ${fruit.size}. It costs ${fruit.price}. It has ${fruit.numSeeds}. The type if ${fruit.type}`;
}

As we can see, it’s much cleaner. We don’t have to worry about passing in many arguments.

It also fits better on the screen since it’s shorter.

5 parameters are probably the maximum that should be in a function.

Excessively Long Identifiers

Identifiers that are too long are hard to read. If they can be shorter without losing any information then make them shorter.

For example, we can shorten the following variable declaration:

let colorOfAFruit = 'red';

into:

let fruitColor = 'red';

by removing the Of and A to make it shorter. It doesn’t change the meaning or remove any information. However, it’s shorter so we type less and get the same results.

Excessively Short Identifiers

Identifiers that are too short are also a problem. This is because identifiers that are too short don’t capture all the meaning of the entity that we define.

For example, the following variable name is too short:

let x = 'red';

In the code above, x is too short since we have no idea what it means by looking at the variable name.

Instead, we should write:

let fruitColor = 'red';

So that we know the variable is the color of a fruit.

Excessive Return of Data

Functions that return data we don’t need isn’t good. A function should only return what’s needed by outside code so that we don’t expose extra stuff that isn’t needed.

For example, if we have the following function:

const getFruitColor = (fruit) => {
  return {
    ...fruit,
    size: 'big'
  };
}

We have getFruitColor function with the size property, which isn’t relevant to the color of the fruit. Therefore, it isn’t needed and shouldn’t be returned with the object.

Instead, we should return a string with the fruit color as follows:

const getFruitColor = (fruit) => fruit.color;

The code above is much cleaner and only returns the fruit color as suggested by the name of the function.

Excessively Long Line of Code

Lines of code that are too long make the codebase hard to read, understand and debug.

Code that is so long that they don’t fit in the screen probably should be broken into multiple lines.

For example, instead of writing:

$("p").html("This is a paragraph").css("color", "red").css("text-align", "center").slideUp(1000).slideDown(1000);

We should instead write:

$("p")
  .html("This is a paragraph")
  .css("color", "red")
  .css("text-align", "center")
  .slideUp(1000)
  .slideDown(1000);

This is much cleaner and doesn’t overflow the screen.

Each line of code shouldn’t be more 100 characters so that they can be read without scrolling on any screen.

Code formatters can rearrange the lines so that they’re shorter in most cases.

Conclusion

Having too many parameters in a method makes passing in data hard since it’s easy to miss some items. It also makes the method signature excessively long.

Identifiers should just be long enough to identify the information we need. Also, it shouldn’t be so short that we don’t get enough information from the identifier.

A function should only return the items that we need and no more. This makes using the function easy since there’s less data to handle and not expose extra information that we don’t want to expose.

Finally, long lines of code should be broken into multiple lines so that they’re easier to read and change. Code formatters can break code into multiple lines automatically.

Categories
JavaScript Best Practices

Code Smells in JavaScript Classes

In programming, a code smell is a characteristic of a piece of code that indicates there may be deeper problems. It’s a subjective characteristic used for judgment of whether the code is of decent quality or not by looking at it.

In this piece, we’ll look at some code smells of JavaScript classes, including lazy classes, excessive use of literals, too many conditionals and loops, orphaned variables and constants, and using too many variables instead of objects.


Lazy or Freeloader Class

A lazy or freeloader class is a class that does too little. If it doesn’t do much, it probably shouldn’t be added since it’s mostly useless.

We should find a way to put whatever is in the lazy class into a place that has more stuff. A class that has only one or two methods probably isn’t too useful.


Excessive Use of Literals

Using literals too much isn’t a good idea because repeating them will bring in more chances for errors. This is because we have to change each of them when we change code if there are too many of them.

Literals should be externalized into their own files and scripts. More dynamic data should be stored in databases where possible. This makes localization easy if needed.

For example, if we need to place the URL of Medium in multiple places in our code, we should have one constant with the URL rather than repeating it in multiple places.

So instead of:

const mediumRequest = fetch('[http://medium.com'](http://medium.com%27));
//...
const mediumJsUrl = '[https://medium.com/topic/javascript](https://medium.com/topic/javascript)';

We should instead write:

const MEDIUM_URL = '[http://medium.com'](http://medium.com%27);
const mediumRequest = fetch(MEDIUM_URL)
//.,.
const mediumJsUrl = `${MEDIUM_URL}/topic/javascript`;

This is better because we didn’t repeat [https://medium.com](https://medium.com/topic/javascript) in multiple places. Making changes then becomes easier.


Cyclomatic Complexity

Cyclomatic complexity means that there are too many conditional statements and loops in our code.

The complexity can arise in different ways. Loops and conditionals can be nested too deeply. More than two levels of nesting is probably too much and hard to read.

There can also be too many conditionals and loops that aren’t nested.

For example, instead of writing something like:

We should instead write:

We eliminated the nesting and moved some deeply nested code into its own function. This increases readability, and separating code into its own functions makes it easier to test.

Also, using loop control statements — like continue and break— in addition to return can help with controlling the flow of the code a lot without deeply nesting conditional statements with many lines inside.


Orphaned Variable or Constant Class

These are classes that have a collection of constants that belong elsewhere rather than in their own class.

This needs changing because it doesn’t make sense to put them in a place where they aren’t used. It’s not intuitive for anyone reading the code.

For example, if we have a class that has the following variable and we have the following classes:

Then the 'red' in the Color class is a better fit in the Apple class since we’re only using it for Apple instances.

So we can instead write:

class Apple {
  constructor() {
    this.color = 'red';
  }
}

And not bother with having a Color class.


Data Clump

A data clump is a situation where we have too many variables passed around together in various parts of a program. This means that we should group these together into their own objects and pass them together.

For example, if we have a bunch of variables that we pass into a function as follows:

We should rewrite this so that all the variables are in an object instead and change the signature of the function to accept the object. As we can see, there are six parameters, which is too many. More than five is probably too many parameters for a function in most cases.

Also, we had to write out all the variables and they’re related, so we can group them into fields and reference the whole object instead of each variable separately.

This lowers the chance of missing variable references anywhere since grouping the variables into one object means that we only have to deal with one variable instead of six.

We also repeated the word fruit a lot.

We can eliminate all the repetition and reduce the number of variables and parameters we have to deal with by writing the following instead:

Now, we only have one object and parameter to deal with instead of several variables and parameters. It’s much easier on the eyes and more organized. Also, it’s harder for us to forget to reference some variables in our code since we have only one to deal with.


Conclusion

We shouldn’t have classes that do little or orphaned variables and constants. Also, too many loops and conditions — especially if they’re nested deeply — are hard to read and follow.

If we have many variables that are related to each other, they should be grouped into objects so that they’re easy to reference and deal with.

Categories
JavaScript Best Practices

Code Smells in JavaScript

In programming, a code smell is a characteristic of a piece of code that indicates there may be deeper problems. It’s a subjective characteristic used to judge whether code is of decent quality by looking at it.

In this article, we look at more code smells in JavaScript code, including feature envy, and classes that are too intimate.


Feature Envy

Feature envy means that one class uses too many features of another class. This means that most of one class reference something in another class.

This is especially applicable to using fields from one in class in another class excessively

For example, if we have the following:

The ShapeCalculator class uses lots of fields from Rectangle. We get all the fields from Rectangle in the ShapeCalculator class when we don’t have to.

This breaks encapsulation in the Rectangle class since we exposed the fields height and width when we don’t have to.

Instead, we should avoid using the height and width fields from the Rectangle object in the ShapeCalculator class and write a method in the Rectangle class and call it in the ShapeCalculator class as follows:

This is much better because all we reference in the ShapeCalculator from the Rectangle class is the getArea method. We no longer have to worry about changes to the height and width fields since they’re encapsulated in the getArea method.

As long as getArea returns a number with the Rectangle‘s area, we aren’t concerned about the fields or the implementation of the methods in the Rectangle class.

Photo by Wynand Uys on Unsplash


Inappropriate Intimacy

A class that has dependencies on the implementation details of another class is considered to be too intimate.

We don’t want classes that depend on the implementation of the other classes because this means that we have to change both classes when the first class change.

There isn’t enough encapsulation of the fields or implementation of the methods.

The lack of encapsulation may arise from exposing the fields of the class. Letting us set field values directly is probably not good in most cases.

Writing code that adheres to some interface is impossible because it’s very hard to change a class to follow an interface since so many other classes depend on the implementation of the given class.

Too many interactions between classes also cause confusion because the workflow is hard to trace. This is the object-oriented version of spaghetti code.

Any changes in a class that other classes are too dependent on also creates problems when we change them since they’re so tightly coupled, we have to change all the classes that depend on it to change it. This exacerbates the problem of maintainability of code, in addition to spaghetti code.

For example, if we have the following classes:

We can see the ShapeCalculator uses the Box classes fields and methods a lot.

In the ShapeCalculator class, we reference the Box‘s length, width, height properties, and the getSurfaceArea method.

If we change anything in the Box class, we also have to change it in the ShapeCalculator. For instance, if we rename everything in the Box class as follows:

We have to make the same changes in the ShapeCalculator calculator. This is a complete nightmare and it’s not even that complex. Imagine if our classes had more fields and methods — it quickly becomes unmaintainable.

The interaction is also confusing. Some fields are used in ShapeCalculator for calculations and some methods are used straight from the Box class.

What we should do instead is not reference any fields in the Box class in the ShapeCalculator class and instead change the Box class to provide all the methods needed by the ShapeCalculator class, as follows:

Then the ShapeCalculator can be rewritten as:

As we can see, the ShapeCalculator class only references the methods of the Box class, instead of having a mix of field and method references from the Box class.

Now we don’t have to worry whether the field names are changed in the Box class, since we never used them.

When we write classes, we should only expose what we need. Most fields can probably be hidden so they won’t be used by external classes. This reduces the effort to maintain the code since they won’t have to be changed in all the classes that use them.

It keeps everything modular and self-contained. Also, we only expose methods that we need to use and hide others like helper methods from the public for the same reason.

Too much coupling and referencing between classes is no good!