Javascript - I have 300 if/else statements. Should I be using switch instead?-CodePudding

I have a js file that looks for paths and rewrites the paths, and it has over 300 if/else statements. I am looking for ways to clean it up, but I also need to make sure that it is readable for the next person who will work on it. My first thought was to convert all of these if statements to either ternaries or switch statements, but I am not sure if that is the best approach. I am trying to balance the urge to write "better" code, but also I need to make sure it stays readable, and the current if/else structure reads pretty well.

Example:

if (request.uri.match(^\/path1\/.*$)) {
     request.uri = request.uri.replace(/^\/path1\/(.*)$/) "newPath"   "$1");
} else if (request.uri.match(^\/path2\/.*$)) {
     request.uri = request.uri.replace(/^\/path2\/(.*)$/) "newPath"   "$1");
} else if (request.uri.match(^\/path3\/.*$)) {
     request.uri = request.uri.replace(/^\/path3\/(.*)$/) "newPath"   "$1");
}

I would say about 75% of the 300 if statements look similar. The path1, path2 examples are overly simplistic, the regex for each of these range from simple to complex. The other 25% incorporate other logic aside from just rewriting the URL. Think injecting headers, or in some cases one more level of nested if/else statements.

Here is an example of what some of those might look like:

else if (request.uri.match(^\/path3(-new)?\/.*$)) {
     request.headers['header'] = {"value": "something"};
     if (request.uri.match(^\/path3\/.*$)) {
          request.uri = request.uri.replace(/^\/path3\/(.*)$/) "/newPathA/"   "$1");
     } else if (request.uri.match(^\/path3-new\/.*$)) {
          request.uri = request.uri.replace(/^\/path3-new\/(.*)$/) "/newPathB/"   "$1");
     }
}

I went down the path to build functions to handle different things, like the uri replace lines. The challenge is the code that the functions are replacing aren't difficult, and it just feels like a bit of a waste. I can either have a line that does this:

request.uri = request.uri.replace(/^\/path3-new\/(.*)$/) "/newPathB/"   "$1");

Or I can have this:

function pathModifier(newPath) {
    request.uri = reqUri.replace(/^(\/.*?[\/])(.*)$/, newPath); 
    return;
}

pathRewriter("/newPathA/"   "$1");

I am not saving a lot, and it makes it more difficult to read for the person I pass this on to.

So, what would you do?

CodePudding user response：

I'd use a declarative approach for the majority of cases and still expose a way to extend it by passing a custom function.

This way you have low amounts of repetition in your code while still offering the needed flexibility.

Something like this:

const rewrites = {
  '/path1/': ['/newPathA/'],
  '/path2/': ['/newPathB/'],
  '/path3/': ['/newPathC1/', { headers: { name: 'value' } }],
  '/path3-new/': ['/newPathC2/', { headers: { name: 'value' } }],
  '/path4': ['/newPathD', { headers: { name: 'value' }, callback: req => {
    req.customProperty = 123
    req.doCustomStuff()
  } }]
}

function handleRewrites (req, rewrites) {
  const [newPath, options] = Object.keys(rewrites)
    .find(oldPath => req.uri.startsWith(oldPath)) ?? []

  if (newPath) {
    req.uri = newPath   req.uri.slice(oldPath.length)
    if (options?.headers) Object.assign(req.headers, options.headers)
    options?.callback?.(req)
  }
}

Of course you are free to design the "domain-specific language" of how to declare the rewrites as it best fits your use case. I chose an object with old path as key and [newPath, optionalOptionsObject] as value because the majority of the time I'd expect there to be just an oldPath=>newPath transformation but it could be different of course.

If you would need for example actual regular expressions in some cases, you could of course change it and do something like an array where you then use values of the format [oldPathOrRegex, newPath, optionalOptionsObject]. You could then allow the easy string path but also allow regexes (depending on whether it is instanceof RegExp you'd apply one logic or the other, allowing you to specify both types of path selectors with little effort when declaring the rewrites).

There is of course still the thing that you would specify the common headers of several routes multiple times. One of the possible ways to allow specifying a (possibly even nested) group of rewrites with common properties that get merged with any per-route properties would be creating a function that takes the common settings plus another rewrite list and returns an expanded version of each given rewrite, and to then spread the result of the function with ...:

function withOptions (commonOptions, rewrites) {
  for (const rewrite of Object.values(rewrites)) {
    if (!rewrite[1]) rewrite[1] = {}
    const rewriteOptions = rewrite[1]
    
    if (commonOptions.headers || rewriteOptions.headers) {
      rewriteOptions.headers = {
        ...commonOptions.headers,
        ...rewriteOptions.headers
      }
    }
    
    if (commonOptions.callback || rewriteOptions.callback) {
      const childCallback = rewriteOptions.callback
      rewriteOptions.callback = req => {
        commonOptions.callback?.(req)
        childCallback?.(req)
      }
    }
  }
    
  return rewrites
}

const rewrites = {
  '/path1/': ['/newPathA/'],
  '/path2/': ['/newPathB/'],
  ...withOptions({ headers: { name: 'value' } }, {
    '/path3': ['/newPathC2/'],
    '/path3-new': ['/newPathC3/']
  }),
  '/path4': ['/newPathD', { headers: { name: 'value' }, callback: req => {
    req.customProperty = 123
    req.doCustomStuff()
  } }]
}

Note that in this example I wrote withOptions so it mutates the given rewrites, because it was the simplest way and I assumed it would be passed object literals anyway. If this isn't true, it can be changed to return copies of course.

CodePudding user response：

If you have many cases, I would suggest using a HashMap/Object, to map between the lookup key and their value.

const paths = {
  '/old/path/to/a': '/new/path/to/a',
  '/old/path/to/b': '/new/path/to/b',
  '/old/path/to/c': '/new/path/to/c'
}

function getNewPath(oldPath) {
  return paths[oldPath] || '/fallback/not/found'
}

Or if you have to handle different business logic, you could also return a function

const paths = {
  '/old/path/to/a': () => { return modifyA() },
  '/old/path/to/b': () => { return modifyB() },
  '/old/path/to/c': () => { return modifyC() },
}

function getNewPath(oldPath) {
  return paths[oldPath]()
}