Avoiding if-statements with object oriented design, PHP

Question

I'm basically creating a display-module for an ad-system I've created.

I'm trying to avoid the following construction, with repeated if-statements.

My gut feeling tells me there's a smarter way to do this, maybe with polymorphism?

<?php

class Ad { 
    public $adState = 'active'; 
} 

class AdWriter { 
    public function displayAd(Ad $ad, $viewmode = 'visitor') { 
        if ($viewmode =='visitor') { 
            if ($adState == 'active') {} 

            else if ($adState == 'paused') {} 

            else if ($adState == 'inactive') {} 

        } 

        else if ($viewmode = 'owner') { 
            if ($adState == 'active') {} 

            else if ($adState == 'paused') {} 

            else if ($adState == 'inactive') {} 
        } 

        else if ($viewmode == 'administrator') { 
            if ($adState == 'active') {} 

            else if ($adState == 'paused') {} 

            else if ($adState == 'inactive') {} 
        } 
    } 
}  

?>

Gordon · Accepted Answer · 2011-06-16 11:35:40Z

16

Apply the Refactoring Replace Conditional with Polymorphism and have a look at the State Pattern.

answered Jun 16, 2011 at 11:35

Gordon

318k76 gold badges548 silver badges566 bronze badges

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

1 Comment

Daniel Cull Over a year ago

Does replacing conditional with polymorphism not just mean your putting your conditionals behind creating objects instead of calling methods etc

Wesley van Opdorp · Accepted Answer · 2011-06-16 11:35:56Z

4

You could create an Factory (Pattern), using an switch on viewmode and create an specific Ad implementing an interface having an simple function 'display' for example.

Pseudo example:

class AdFactory { 

    public static function getAd($sType) {
        switch($sType) {
            case "AdOne":
                return new AdOne();
            case "AdTwo":
                return new AdTwo();
        }
    }
    throw new Exception("Unknown ad!");
}

class AdOne implement AdInterface {
    public function display() {
        // All that AdOne does when displaying.
    }
}

interface AdInterface {
    public function display() { }
}

$oAd1 = AdFactory::getAd('typeOne');
$oAd1->display();

$oAd2 = AdFactory::getAd('typeTwo');
$oAd2->display();

edited Jun 16, 2011 at 11:35

answered Jun 16, 2011 at 11:23

Wesley van Opdorp

15k4 gold badges43 silver badges59 bronze badges

Comments

Evgeny Shadchnev · Accepted Answer · 2011-06-16 11:23:27Z

3

Instead of passing $viewmode, pass an object that would encapsulate the logic for this viewmore and call its method that would do the work. This way you'll avoid the need for if-statements.

answered Jun 16, 2011 at 11:23

Evgeny Shadchnev

7,3884 gold badges30 silver badges30 bronze badges

Comments

Lexx Koto · Accepted Answer · 2011-06-16 11:21:35Z

0

I'm sneaking onto StackOverflow at work, so don't have time to write a detailed reply of all your possibilities.

But to 'tidy up' those ifs, you can do this:

switch $viewmode {
  case 'visitor':
    your_code_here;
    break;
  case 'owner':
    your_code_here;
    break;
  default:
    will_run_if_nothing_above_matches;
    break;
}

answered Jun 16, 2011 at 11:21

Lexx Koto

1234 bronze badges

2 Comments

Poyan Over a year ago

That didn't really eliminate anything though? I'm still stuck with repeatedly checking for the states.

MrMesees Over a year ago

Not if all states share an interface

questioner · Accepted Answer · 2011-06-16 11:28:19Z

0

  switch($viewmode){
    case "visitor":
        switch($adstate){
            case "active": 
                //statement
                break;
            case "paused": 
            break;
            case "inactive": 
            break;
        }
        break;
    case "owner":
        break;
    case "administrator":
        break;
}

answered Jun 16, 2011 at 11:28

questioner

1,1824 gold badges14 silver badges22 bronze badges

Comments

score 0 · Accepted Answer · 2011-06-16 11:51:42Z

0

In 8 chapter of this book you can find very detailed answer to your question.

In short: use Composition or Factories. (see answer of Wesley van Opdorp).

Also, avoid using string arguments as enumerable: $viewmode = 'visitor'
with this argument, you will have to keep in memory all possible values of this argument. Or look into code of function to remember them. And these values are strings - good place for typos. Also, it will be very difficult to change values in feature, because all calls of this method will contain hardcoded strings.
Use class-constants:

class AdWriter { 
const view_mode_visitor = 1;
...

Also, $adState - wrong code, should be $ad->state. But using public fields it's bad practice too :)

edited Jun 16, 2011 at 11:51

answered Jun 16, 2011 at 11:37

user680786

Collectives™ on Stack Overflow

Avoiding if-statements with object oriented design, PHP

6 Answers 6

1 Comment

Comments

Comments

2 Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

6 Answers 6

1 Comment

Comments

Comments

2 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related