Case: Leave the Code Better Off Than You Left It

Writing clean, maintainable code is an art and a science---it is a challenging skill that is difficult to learn except through experience. We have collected a number of coding examples that demonstrate good coding practices.

Background

It is easy, when fixing software bugs, to make the smallest possible fix to get some code working. However, unless there are severe time constraints, it is important to understand the context in which the code occurs, and to view the bug fix as a chance to clean up the code. This case is an example of a “quick fix” that could have been a nice chance to clean up the code.

Problem

Here is a function in “src/components/NodeDescription.js” where a bug originated:

  export const getApiUrlFromHost = () => {
      const host = window.location.host
      var apiUrl = ""
      switch(host) {
          case "localhost:8080":
          case "localhost:3000":
          case "127.0.0.1:3000":
              apiUrl = 'http://0.0.0.0:5000/sections/'
              break;
          case "dicom.innolitics.com":
              apiUrl = 'http://dicom.innolitics.com:5000/sections/'
              break;
          default:
              console.log("Cannot resolve API URL from host.")
      }
      return apiUrl
  }

Here is another version of the code that has fixed the error:

  export const getApiUrlFromHost = () => {
      var host;
      if (typeof window === 'undefined') {
          host = "server-render"
      } else {
          host = window.location.host
      }
      var apiUrl = ""
      switch(host) {
          case "localhost:8080":
          case "localhost:3000":
          case "127.0.0.1:3000":
              apiUrl = 'http://0.0.0.0:5000/sections/'
              break;
          case "dicom.innolitics.com":
          case "server-render":
              apiUrl = 'http://dicom.innolitics.com:5000/sections/'
              break;
          default:
              console.log("Cannot resolve API URL from host.")
      }
      return apiUrl
  }

There are several serious problems with this fix. Can you spot them?

Solution

Although the change in the code fixes the immediate problem, it is overly complicated and has poor variable naming.

The variable name host is imprecise. The variable does not store the “host”—it either stores the host, or if executed on the server, a magical string.

In cases like this, it is better to take a step back and see if there is another way to simplify the existing code, instead of hacking in a “quick fix”. Of course, if this was a live production server with many users, it may be appropriate to apply a quick fix, but afterwards a “proper fix” should be submitted.

There are many problems with the existing code that could have been cleaned up. In particular

If the default statement is reached, the function will simply return undefined
This is a very generic utility function, yet it is included inside a Component—this is very illogical, and could easily lead to circular imports
The switch statement is overly complicated and has several redundant cases
The API URL should probably be a constant, and not a function

Here is a simpler solution that moves the code to src/utils/api.js

let apiUrl
if (typeof window === 'undefined' || window.location.host === 'dicom.innolitics.com') {
        apiUrl = 'http://dicom.innolitics.com:5000/sections/'
} else {
        apiUrl = 'http://0.0.0.0:5000/sections/'
}

export const baseApiUrl = apiUrl

Other modules can import the baseApiUrl constant from the utility library.