how to reduce multiple levels of if staement

Recently I had to use a multiple IF staement to check variaty of conditions and return string if something is found

It end up with 4 indentation , I know I can switch or while loop but still not elegant to look at, I would like get some clean tips or refector this. thanks

 public function check()
    {
        //TO DO REFACTOR UGLY IF

        // check job not started
        if( $this->recruit->started()  ){

            $have_employee = $this->recruit->gotEmployee();
            return $this->gotEmployees[$have_employee];

        }else{

            //if ever employee applied ths job
            if( $this->recruit->appliedBy( $this->user)  ){

                // hired and approved by employer
                if( $this->recruit->workingBy( $this->user)  ){


                    if($this->recruit->noPunchIn() && $this->recruit->needAcceptance())
                    {
                        if( $this->recruit->started()  ){
                            return "wait for acceptance";
                        }
                        return "wait for begin";

                    }else if ( $this->recruit->noPunchIn() && $this->recruit->noNeedAcceptance() )
                    {
                        if( $this->recruit->started()  ){
                            return "wait for payout";
                        }
                        return "wait for begin";


                    }else if ( $this->needToSign() )
                    {
                        return "punching in";

                    }else if ( $this->recruit->hiringCompleted() )
                    {
                        return "full";
                    }
                    else{
                        return "waiting for start";
                    }

                }else{

                    //if employyer are fired
                    if( $this->fired()  ){
                        return "fired";
                    }
                    elseif ( $this->cancelBy() ){
                        return "cancel";
                    }
                    elseif ( $this->rejected() ){
                        return "rejected";
                    }
                    else{
                        return "pending approval";
                    }
                }

            }else{
                return "apply job";
            }
        }


    }

Tippin

4 years ago

Level 13

@andiliang One thing to keep in mind, and a good case to avoid nesting IFs, CHUNK big sections to individual methods. Also, when you are always returning something, that is a breakpoint and the code will not go any further, so you already do not need ALOT of if/else if. Try like this:

public function check()
{
    // check job not started
    if ($this->recruit->started()) {
        return $this->gotEmployees[$this->recruit->gotEmployee()];
    }

    //if ever employee applied ths job
    if ($this->recruit->appliedBy($this->user)) {
        return $this->appliedBy();
    }

    return "apply job";
}

public function appliedBy()
{
    // hired and approved by employer
    if ($this->recruit->workingBy($this->user)) {
        return $this->workingBy();
    }

    //if employyer are fired
    if ($this->fired()) {
        return "fired";
    }

    if ($this->cancelBy()) {
        return "cancel";
    }

    if ($this->rejected()) {
        return "rejected";
    }

    return "pending approval";
}

public function workingBy()
{
    if ($this->recruit->noPunchIn()) {
        if ($this->recruit->needAcceptance()) {
            return $this->recruit->started() ? "wait for acceptance" : "wait for begin";
        }

        if ($this->recruit->noNeedAcceptance()) {
            return $this->recruit->started() ? "wait for payout" : "wait for begin";
        }
    }

    if ($this->needToSign()) {
        return "punching in";
    }

    if ( $this->recruit->hiringCompleted() ) {
        return "full";
    }

    return "waiting for start";
}

*edit to add some inline ternaries and condense one more if

1 like

andiliang

4 years ago

Level 39

yeah I had thought about seperate to several method but its not so clean

Snapey

4 years ago

Level 122

look for ways to return early. You can remove the else conditions

https://youtu.be/0vJ_SkuBgbo

1kr1

4 years ago

Level 8

If you want to perform different actions based on whatever rules, you could implement the strategy pattern. However, based on your code, that's an overkill. The other option I would suggest, is the use of lookup table (plenty of examples online).

andiliang

4 years ago

Level 39

yes I know look up table and I can implements but it needs like a lot array . if use the strategy plattern it just multipl class doing have 1 or 2 checks

tylernathanreed

4 years ago

Level 14

There are actually a number of simplifications here, more than just moving code around.

1) For example, you have a check for $this->recruit->started(), an else case, then additional $this->recruit->started() checks inside that else, which will always return false.

2) When an if statement includes a return statement, you don't need an else statement.

3) I've made the assumption that the needAcceptance and noNeedAcceptance methods are opposites of each other.

4) The wait for begin value is returned in two conditions that could be joined together.

5) Inversing a statement often allows for a quicker return.

6) Certain conditions can be reordered for a cleaner look (using the rules above)

That takes us down to this:

public function check()
{
    if ($this->recruit->started()) {
        return $this->gotEmployees[$this->recruit->gotEmployee()];
    }

    if (! $this->recruit->appliedBy($this->user)) {
        return 'apply job';
    }

    if ($this->fired()) {
        return 'fired';
    }

    if ($this->cancelBy()) {
        return 'cancel';
    }

    if ($this->rejected()) {
        return 'rejected';
    }

    if (! $this->recruit->workingBy($this->user)) {
        return 'pending approval';
    }

    if ($this->recruit->noPunchIn()) {
        return 'wait for begin';
    }

    if ($this->needToSign()) {
        return 'punching in';
    }

    if ($this->recruit->hiringCompleted()) {
        return 'full';
    }

    return 'waiting for start';
}

Please or to participate in this conversation.