0

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?

4
  • 1
    Does this answer your question? Approaches for checking multiple conditions?
    – gnat
    Commented Jul 7, 2023 at 9:08
  • @gnat Actually, I've already read that question, and considering that the answer depends on the specific scenario, I created a question that aligns with a more specific scenario. Commented Jul 7, 2023 at 9:14
  • It could be my mistake for not fully understanding the answers on that question, but I hope someone can assist me in gaining a better understanding. Commented Jul 7, 2023 at 9:20
  • Tell your senior that if return **else** hurts :-)
    – Laiv
    Commented Jul 8, 2023 at 10:45

4 Answers 4

4

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.

1

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 "";
}
0

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;
0

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.

3
  • 1
    Exceptions are useful when the validation is critical. However, if the validation is not crucial and our main objective is to log missing data/records for future filling, we can simplify the code and refrain from using exceptions. This validation requirement is imposed by the third-party integration, and our priority is to ensure our system remains send the records, even if the records may not be valid within the integration. This situation could be attributed to a flawed business flow. But your suggestion to create an IsValid function also sounds interesting. I will try to do that. Commented Jul 8, 2023 at 1:22
  • Erm, I would still have the exceptions at some level. You just need to separate out the code enough so you can see where it is critical. presumably there is a point where you don't want to update if its missing a required field or something and if you somehow have got to that point in the code without the validation being hit already, you need to throw an exception.
    – Ewan
    Commented Jul 8, 2023 at 9:29
  • It looks that these cases are not exceptions, but regular behaviour.
    – gnasher729
    Commented Jul 8, 2023 at 12:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.