4
\$\begingroup\$

Gentlemen!

I am hoping to find a more efficient (less hairy) way to do the following code.

The public interface will call calculateListItemValue and pass the following variables

  • $source_list: stdClass() with a structure similar to List JSON below.
  • $target_list: stdClass() with a structure similar to List JSON below.
  • $item_index: integer index pointing to the index of the array (list)->items[$item_index]
  • $type: string value containing the controlling type $source_list->attribute->type to be compared to

calculateListItemValue should check the value of attribute->value and calculate the final value of the operation based on range and operator

  • If value >= 0 result should be positive
  • If value < 0 result should be negative
  • If range is 0 apply Operator() to self
  • If range is 4 apply Operator() to all items in source_list (or target_list if value < 0)
  • Else apply to items with attribut->attrNo == range

Code follows.

/* List JSON example
 * {
 *  "attributes": {
 *      "operator": "+",
 *      "value": "10",
 *      "range": "2,3",
 *      "type": "ATK",
 *      "leader": true,
 *      "attrNo": 2
 *      }
 * } 
 * 
 */

private function calculateListItemValue(&$source_list, &$target_list, $item_index, $type = 'ATK')
{
    $power    = array();

    $operator = $source_list->items[$item_index]->attribute->operator; 
    $value    = intval($source_list->items[$item_index]->attribute->value);   
    $leader   = $source_list->items[$item_index]->attribute->leader ? 1 : .3;
    $item_type = $source_list->items[$item_index]->attribute->affect;

    $range = explode(',', $source_list->items[$item_index]->attribue->range);

    if($value >= 0)
    {
        if(array_search('0', $range) !== false)
        {
            if($item_type == $type)
            {
                $power['power_up'] = $this->Operator($operator, ($type == 'ATK' ? $source_list->items[$item_index]->ATK : $source_list->items[$item_index]->DEF), $value, $leader);                
            }
        }
        elseif(array_search('4', $range) !== false)
        {
            foreach($source_list->items as $item)
            {            
                if($item_type == $type)
                {
                    $power['power_up'] += $this->Operator($operator, ($type == 'ATK' ? $item->ATK : $item->DEF), $value, $leader);
                }
            }
        }
        else
        {
            foreach($source_list->items as $item)
            {
                if(isset($item->attribute->attrNo))
                {
                    if(array_search($item->attribute->attrNo, $range) !== false)
                    {
                        if($item_type == $type)
                        {
                            $power['power_up'] = $this->Operator($operator, ($type == 'ATK' ? $item->ATK : $item->DEF), $value, $leader);
                        }
                    }
                }
            }
        }
    }

    elseif($value < 0)
    {
        if(array_search('0', $range) !== false)
        {
            return $power;                
        }
        elseif(array_search('4', $range) !== false)
        {
            foreach($target_list->items as $item)
            {
                if($item_type == $type)
                {
                    $power['power_down'] += $this->Operator($operator, ($type == 'ATK' ? $item->ATK : $item->DEF), $value, $leader);
                }
            }
        }
        else
        {
            foreach($target_list->items as $item)
            {
                if(isset($item->attribute->attrNo))
                {
                    if(array_search($item->attribute->attrNo, $range) !== false)
                    {
                        if($item_type == $type)
                        {
                            $power['power_down'] += $this->Operator($operator, ($type == 'ATK' ? $item->ATK : $item->DEF), $value, $leader);                             
                        }   
                    }
                }
            }
        }
    }

    return $power;
}

private function Operator($operator, $base, $value, $mul)
{
    $result = 0;

    if($operator == '*')
    {
        $result = abs(ceil(($base * ($value * $mul)) - $base));        
    }

    elseif($operator == '+')
    {
        $result = abs(ceil(($base + ($value * $mul)) - $base));
    }

    return $result;
}

Edit: Let me say that the code itself works fine, no problems.. but it's a nightmare to look at and explain to other people.

\$\endgroup\$
  • \$\begingroup\$ it is good to have algorithm along with the code, specifically when the code long. \$\endgroup\$ – Kinjal Oct 10 '12 at 7:21
1
\$\begingroup\$

Well, first off, you should be wary of using referenced parameters. It sometimes makes legibility difficult, but in this case it is not even necessary. The method you are using these referenced variables in is private, therefore the class is the only thing that will ever use these values. Why not use properties instead? It accomplishes the same thing, is more legible, and is more extensible.

private
    $source_list,
    $target_list
;

private function calculateListItemValue( $item_index, $type = 'ATK' ) {
    $this->source_list;//do something to source list
    $this->target_list;//do something to target list
    //etc...
}

Second, why is this method so long? Break this up into multiple methods based on functionality. If we follow the Single Responsibility Principle, then our methods should only do just enough to fulfill their purpose. No more, no less. Methods can call other methods to accomplish their task, but they should not need to know how those other methods do so.

There are a lot of violations of the "Don't Repeat Yourself" (DRY) Principle. The following is a pretty basic example of it. You should also notice that I'm using type casting instead of intval(). This makes your code just a bit easier to read by removing the need to wrap the entire line in parenthesis.

$attribute = $source_list->items[ $item_index ]->attribute;
$operator = $attribute->operator;
$value = ( int ) $attribute->value;
$leader = $attribute->leader ? 1 : .3;
$item_type = $attribute->affect;

Why are you using array_search() here? array_search() returns the array key if found, FALSE otherwise. Because of this you are forced to explicitly check for FALSE. If that is all you want, why not use in_array() instead? It is cleaner because it only ever returns TRUE/FALSE. It's either found or it isn't. BTW: there is no need to treat "0" as a string. Just use the integer, PHP is loosely typed and will alow it.

if( in_array( 0, $range ) ) {
    //etc...
}

Asides from those principles I've already mentioned, you are also violating the Arrow Anti-Pattern. By this I mean that your code is too heavily indented. The two principles I mentioned above will help. For instance, each if/else statement terminates in a final check ($item_type == $type). This could be done once, just before the first if statement and return early. This follows the DRY principle, helps the Arrow Anti-Pattern by removing a level of indentation across the entire method, and increases efficiency, meaning your program will run faster.

if( $item_type != $type ) {
    return $power;//empty array because nothing was done with it.
}

Let's talk about ternary statements for a moment. Ternary is a very powerful tool, and is wonderful if used correctly. However, it quickly becomes a pain and makes your code illegible if used incorrectly. If you find that your statements become too long, then you should opt for a full if/else structure. If you find that you are opting for multiline ternary to make them more legible, then you should opt instead for a full if/else structure. If you find the statement too complex, making it difficult to read, then you should opt for the full if/else structure. So...

$power['power_up'] = $this->Operator($operator, ($type == 'ATK' ? $source_list->items[$item_index]->ATK : $source_list->items[$item_index]->DEF), $value, $leader);
//compared to
if( $type == 'ATK' ) {
    $power[ 'power_up' ] = $this->Operator( $operator, $source_list->items[ $item_index ]->ATK ), $value, $leader );
} else {
    $power[ 'power_up' ] = $this->Operator( $operator, $source_list->items[ $item_index ]->DEF, $value, $leader );
}

You should notice an immediate difference in how this looks. In the first example, that could easily be mistaken for multiple statements (or so it appeared in my editor), or mistaken for a really long single statement as the ternary is hard to spot. In the second you instantly know what's going on. Of course, you'll notice that the above is still violating the DRY principle and is still a little difficult to read. So let's modify it a little more.

$item = $source_list->items[ $item_index ];
if( $type == 'ATK' ) {
    $power_up = $item->ATK;
} else {
    $power_up = $item->DEF;
}
//or ternary now
$power_up = $type == 'ATK' ? $item->ATK : $item->DEF;

$power[ 'power_up' ] = $this->Operator( $operator, $power_up, $value, $leader );

There, much better. The rest of your code pretty much follows the same logic. All of it can benefit from the above suggestions, so I will stop here and allow you to do the rest. If you need any clarification, let me know.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks for reviewing both of my submissions, they are really informative and helped me out quite a bit to change my approach to problems. \$\endgroup\$ – Nicholas Oct 12 '12 at 8:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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