refactoring nested if-else to handle cross cutting concerns like logging

Question

It is a classic problem to handle ever changing requirements in a cleaner way without using too many nested if statements. Here is my current code in javascript.

fetchApiData(url){
//log before start
  Logger.logFetchStartedEvent();
  try {
    data = backendApi.get(url);
    Logger.logFetchSucceededEvent();
    return data;
  } catch (error) {
    Logger.logFetchFailedEvent();
  }
}

Everything was going on a happy path. But I received a requirement that for some specific URLs we do not want to log at all.

No problem. I added a flag and switch and called it a day.

fetchApiData(url, shouldLog){
//log before start
  if(shouldLog) {
    Logger.logFetchStartedEvent();
  }
  try {
    data = backendApi.get(url);
    if(shouldLog) {
      Logger.logFetchSucceededEvent();
    }
    return data;
  } catch (error) {
    if(shouldLog) {
      Logger.logFetchFailedEvent();
    }
  }
}

And it didn't stop there. New requirement drops in and asked to change to accommodate following requirements

• some url will log everything
• some url will log only error
• some url will log only if the API call url is an external site
• in some cases logging of fetchSucceeded event is needed, in some cases it is not needed.

I think you got the point. I can add countless nested if/else conditionals and get it done but now I am sure there must be a better way for this type of issue. Now I feel like one method will become a whole if/else state machine god method.

This is what I came up with

fetchApiData(url,logOnStart, logOnSuccess, logOnFailure, logOnlyExternalLink){
  //log on start
  if(logOnStart) {
    if(logOnlyExternalLink) {
      if(isExternalLink(url)) {
        Logger.logFetchStartedEvent();
      }
    } else {
      Logger.logFetchStartedEvent();
    } 
  }
  try {
    data = backendApi.get(url);
    //log on success
    if(logOnSuccess) {
      // may need external url check again
      Logger.logFetchSucceededEvent();
    }
    return data;
  } catch (error) {
    if(logOnFailure) {
      if(errorType(error) === TimeOut)
      {
        Logger.logFetchFailedTimeOutEvent();
      } else if (errorType(error) === 404) {
        Logger.logFetchFailed404Event();
      } else {
        Logger.logFetchFailedEvent();
      }
    }
  }
}

I did read a lot of questions about nested if/else problem but most of them end up with a foo/bar type examples and vague explanation which make no practical sense to me due to lack of experience.

Please point me to the right direction.

Answer 1

You function should probably look like this:

fetchApiData(url, onEvent){
  //log on start
  onEvent(event.Start);
  try {
    data = backendApi.get(url);
    //log on success
    onEvent(event.Success, url);
    return data;
  } catch (error) {
    onEvent(event.Failure, url, error);
  }
}

Writing the onEvent callback is something that should be handled separately.

Also, instead of an onEvent funciton, you may inject a class instance (eventHandler) into your function.