Refactoring Conditional PHP

Question

I'm wondering if there is another way to refactor my conditionals if/else statements. I feel I'm repeating the same thing over and over,

Here is a snippet (Keep in mind is wayyyy longer than that, but it follows the same principle) I could do a switch statement but it does not reduce the total code quantity.

I just want to get a second opinion of the best approach for this code to go into production. Also important to mention that the statements that I'm comparing $screen->id; are most likely to be dynamically generated if the user selects the checkbox, but that is outside of the scope of the question.

    //check admin screen
    $screen = get_current_screen();
    if ( $screen->id === 'topic') {

        $in['content_css'] = get_template_directory_uri() . "/build/styles/tiny-mce-editor.css";
        $in['block_formats'] = $topics_blocks;
        return $in;
    }
    elseif ( $screen->id === 'provider-jobs')  {

        $in['content_css'] = get_template_directory_uri() . "/build/styles/tiny-mce-editor.css";
        $in['block_formats'] = $providers_blocks;
        return $in;
    }
    //for all the page options
    else {

        $in['block_formats'] = $global_blocks;
        return $in;
    }

}

Thanks! Any guidance is appreciated.

UPDATE! Here is the refactored code, a little bit more cleaner! And complete so the whole context shows.

//check admin screen
$screen = get_current_screen();

//global ones
$in['block_formats'] = $global_blocks;
$in['toolbar1'] = $global_toolbar;

if ( $screen->id === 'topic') {
    $in['block_formats'] = $topics_blocks;
    $in['toolbar1'] = $topics_toolbar;

} elseif ( $screen->id === 'forum')  {
    $in['block_formats'] = $forums_blocks;
    $in['toolbar1'] = $forums_toolbar;

} elseif ( $screen->id === 'post')  {
    $in['block_formats'] = $blogs_blocks;
    $in['toolbar1'] = $blogs_toolbar;

} elseif ( $screen->id === 'jobs')  {
    $in['block_formats'] = $jobs_blocks;
    $in['toolbar1'] = $jobs_toolbar;

}
elseif ( $screen->id === 'provider-jobs')  {
    $in['block_formats'] = $providers_blocks;
    $in['toolbar1'] = $providers_toolbar;

}

return $in;

You may post this question on the Code Review website (for reference: Which computer science / programming Stack Exchange do I post in?). — A.L
– A.L, Commented Feb 25, 2016 at 23:01
This may be on-topic for Code Review, as long as it A works, B isn't hypothetical or incomplete in any way. Please read the on-topic guide before posting. — Quill
– Quill, Commented Feb 25, 2016 at 23:03
The else is pretty redundant, you can go with just the if and elseif and then let the code just drop through to the $in['block_formats'] = $global_blocks; return $in; — Mark Baker
– Mark Baker, Commented Feb 25, 2016 at 23:04
"I could do a switch statement but it does not reduce the total code quantity." However, switch statements would make it more readable. — vascowhite
– vascowhite, Commented Feb 25, 2016 at 23:08

cFreed · Accepted Answer · 2016-02-26 00:23:16Z

2

You might register key/value pairs all known options/blocks. So your code becomes straight reduced, like this:

  $options = [
    'topic'         => $topics_blocks,
    'provider_jobs' => $providers_blocks,
    // ...
  ];
  //check admin screen
  $screen = get_current_screen();
  if (array_key_exists($screen->id, $options) {
    $in['content_css'] = get_template_directory_uri() . "/build/styles/tiny-mce-editor.css";
    $in['block_formats'] = $options[$screen->id];
  } else {
    $in['block_formats'] = $global_blocks;
  }
  return $in;

Edit, applies to the 2nd version of the OP

Here the problem appears a bit different, so here is another solution:

$options = [
  'topic' => 'topics',
  'forum' => 'forums',
  'post'  => 'blogs',
  'jobs'  => 'jobs',
  'provider_jobs' => 'providers',
];
//check admin screen
$screen = get_current_screen();

//global ones
$in['block_formats'] = $global_blocks;
$in['toolbar1'] = $global_toolbar;
// variable ones
if (array_key_exists($screen->id, $options) {
  $in['block_formats'] = ${$options[$screen->id] . '_blocks'};
  $in['toolbar1'] = $options[$screen->id] . '_toolbar'};
}
return $in;

edited Feb 26, 2016 at 0:23

answered Feb 25, 2016 at 23:19

cFreed

4,4641 gold badge26 silver badges35 bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

mmarquez Over a year ago

Woah!! thank you for this answer, this might be exactly what I'm looking for! Makes sense and it reduces the amount of code. Thank you! Will test shortly

mmarquez Over a year ago

Your answer is correct but it does not work in my case as I have two $variables for each of the conditionals. Whenever the condition is true I need to pass $array_toolbar and $array_blocks and in some instances even $array_style. To my knowledge, keys are unique. I can nest the variables in the same key as multidimensional arrays, but I think that unnecessary elevates the complexity!

cFreed Over a year ago

@mmarquez So your given example was simplified. From your comment I can't totally get certainty about what are the additional variations, but I'm pretty sure it 'll be possible to adapt the solution without too much complexity. Please edit your question with all needed precisions, and I'll tell you what I think about.

cFreed Over a year ago

@mmarquez Stop! Don't mind: I discovered your edit just after writing my above comment :) Let me examine it...

cFreed Over a year ago

@mmarquez I edited my answer with a solution for your edited question. But I'm puzzled: it seems yet different from what you cited in your comment ?

|

Collectives™ on Stack Overflow

Refactoring Conditional PHP

1 Answer 1

6 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

6 Comments

Your Answer

Sign up or log in

Post as a guest

Related