Should we create a new function for every multiple conditions?

Question

I have the existing code provided below:

if (!$existing_records && !$has_required_field) {
    return 'Skip Update, no records found';
} elseif (!$existing_records && $has_required_field) {
    syslog(LOG_ERR, "some field is required but no regulatory records found. id=$id");
    return 'Error some field is required but no records found';
} elseif ($client_records == $existing_records) {
    syslog(LOG_INFO, "skipped, the record is unchanged. id=$id");
    return 'Skip Update record, the record is unchanged';
} elseif ($unfilled_required_fields) {
    syslog(LOG_ERR, " one of required fields is not filled id=$id");
    $additional_message = ' but not all required field is filled';
}

//bla bla bla (logic to saving the record)

The above code contains conditions with logging and return statements but no actual logic inside them, just exactly like above.

My senior pointed out that the above code contains multiple conditions, which can make it burdensome for readers to understand each condition. As a suggestion, they provided a similar code change, shown below:

$update = determine_to_skip_update($id, $existing_records, $has_required_field, $client_records, $unfilled_required_fields);
if($update['is_skipped']){
    return $update['message'];
}else {
    $additional_message = $update['message'];
}

 function determine_to_skip_update(int $id, array $existing_records, bool $has_required_field, array $client_records, array $unfilled_required_fields)
 {  
     $result = ['is_skipped'=>false, 'message'=>''];
     if (!$existing_records && !$has_required_field) {
         $result['message'] = ' Skip Update, no records found';
     } elseif (!$existing_records && $has_required_field) {
         syslog(LOG_ERR, "field is required but no regulatory records found. id=$id");
         $result['message'] = ' | Error some field is required but no records found';
     } elseif ($client_records == $existing_records) {
         syslog(LOG_INFO, "skipped, the record is unchanged. id=$id");
         $result['message'] = ' | Skip Update record, the record is unchanged';
     } elseif ($unfilled_required_fields) {
         syslog(LOG_ERR, " one of required fields is not filled. id=$id");
         $result['message'] = ' not all required field is filled';
     }
     return $result;
 }

Is the suggested code change truly an improvement? I feel it is more maintainable, but somehow it seems a bit more complicated to understand than my initial code.

Are there any other better alternatives to improve it further? Alternatively, should I consider using a new class to handle this simple logic?

Answer 1

I would suggest a different approach. IMO there's nothing wrong with the early-return exits, but I would not bury them in elseif blocks. One thing that improves readability is to focus on a single condition in an if instead of combining multiple conditions, possibly using ! to invert them.

So this would be my version of your original snippet:

if (!$existing_records) {
  if ($has_required_field) {
    syslog(LOG_ERR, "some field is required but no regulatory records found. id=$id");
    return 'Error some field is required but no records found';
  } else {
    return 'Skip Update, no records found';
  }
}
if ($client_records == $existing_records) {
    syslog(LOG_INFO, "skipped, the record is unchanged. id=$id");
    return 'Skip Update record, the record is unchanged';
}
if ($unfilled_required_fields) {
    syslog(LOG_ERR, " one of required fields is not filled id=$id");
    $additional_message = ' but not all required field is filled';
}

//bla bla bla (logic to saving the record)

One could remove the else as it is also redundant due to the return in the corresponding if block, this is probably a matter of taste.

Answer 2

In my opinion, the key distinction here is

• early returns because it's a skippable record, and
• additional messages, but not skipping.

Mixing the two is what makes the code so complicated, both in the original version (it's easy to overlook that one of the branches doesn't have a return) and in the refactored version (function needs to return two pieces of information).

I would therefore go with this:

$skip_message = should_skip($id, $existing_records, $has_required_field, $client_records);
if ($skip_message) return $skip_message;
$additional_message = additional_message($id, $unfilled_required_fields);
// ...


function should_skip(int $id, array $existing_records, bool $has_required_field, array $client_records) {
  if (!$existing_records && !$has_required_field) {
    return 'Skip Update, no records found';
  }
  if (!$existing_records && $has_required_field) {
    syslog(LOG_ERR, "some field is required but no regulatory records found. id=$id");
    return 'Error some field is required but no records found';
  }
  if ($client_records == $existing_records) {
    syslog(LOG_INFO, "skipped, the record is unchanged. id=$id");
    return 'Skip Update record, the record is unchanged';
  }
  return false;
}

function additional_message(int $id, array $unfilled_required_fields) {
  if ($unfilled_required_fields) {
    syslog(LOG_ERR, " one of required fields is not filled id=$id");
    return ' but not all required field is filled';
  }
  return "";
}

Answer 3

As always with coding it depends on the context and on your personal likeing.

I do not like if / else and try to avoid it if possible. The reason is, that an if/else feels like ONE statement and my mind has to keep the whole statement in memory.

When each if case will leave the function, then i can rewrite an if/else to:
"If this then return"
"If that then return"
So each if is one statement and could "leave" my mind, as soon as i reach the end of it.

Therefore i would use:
if( ...) { log and return };
if( ...) { log and return };
// do the main stuff

If there are quite a lot of checks before the main stuff could be done, i like to extract that into a function.
Thats similar to the approach of your seniors, just that i would still omit the if/Else cascade

errorMessage = validateStuff(...); 
if( errorMessage exists){
  return errorMessage
}
do magic and return 'Everything is fine'

function validateStuff(...){
if( firstCheck) { return 'Failed on First' }
if( secondCheck) { return 'Failed on Second'}
...
// if no check kicked in
return null;

Answer 4

I guess you are using some crazy language without exceptions here?

It seems to me that these checks are guard clauses on your update function and as such its fine to have multiple of them, and early returns, without refactoring.

The 'bad' thing is not throwing an exception when you hit one, and perhaps the elseifs rather than just following up with more ifs, which obviously makes sense when you can throw and hence return early.

As it stands with the elses, missing early returns on some blocks and the different things happing in each block, its hard to see this and discount it as "oh that's just guard clauses, i don't have to worry about understanding it", so it is confusing.

If you have a whole bunch of these validations to perform, and errors to return it might make sense to refactor them out into their own IsValid function as per the suggestion. But at least return a bool, or a list of ValidationFailure or something.