3
\$\begingroup\$

I have to compare variables to obtain coordinates.

           l
upleft     l     upright
-----------l--------------
downleft   l     downright
           l

Here is what I am doing now:

protected function check_if_quadrant_is_valid($variable)
    {
        $middle_of_x = 297;
        $middle_of_y = 421;

       $heigth =  $variable["y"] <= $middle_of_y ?   "up" :  "down";        

       $side =  $variable["x"] <= $middle_of_x ?  "left" : "right";

            $variable_location = $heigth . $side;

            if($this->item->$variable_location == 1)
            {
                return true;
            }
         return false;
    }

The variable $variable is an array with x and y as key and their coordinates as value.

The variable $item is an object. It contains whether the variable should be in the quadrant. It contains $item->upleft, $item->upright, $item->downleft, $item->downright that are booleans.

The point of this function is to retrieve the quadrant of the $variable and check if it is valid within the $item.

The code works perfectly, however it feels very cheap.

Here is the same that can be copypasted and their values changed to test:

function check_if_quadrant_is_valid()
    {
        #Change at will
        $item["upleft"] = 0;
         $item["upright"] = 1;
          $item["downleft"] = 1;
           $item["downright"] = 1;

        $variable["y"] = 90;
        $variable["x"] = 20;

        $middle_of_x = 297;
        $middle_of_y = 421;

       $heigth =  $variable["y"] <= $middle_of_y ?   "up" :  "down";        

       $side =  $variable["x"] <= $middle_of_x ?  "left" : "right";

            $variable_location = $heigth . $side;

            if($item[$variable_location] == 1)
            {
                return true;
            }
         return false;
    }
\$\endgroup\$
  • \$\begingroup\$ @Carcigenicate Sorry I wanted to change the variable names so I messed up a bit. Also I added a working example that can be copypasted. \$\endgroup\$ – prgrm Apr 7 '17 at 18:26
  • \$\begingroup\$ I don't understand what you are wanting us to review. You have two versions of the same method with different parameters being passed in. Which are we supposed to review? Can you clean up the post to make it clear what you want reviewed? \$\endgroup\$ – Mike Brant Apr 7 '17 at 20:40
1
\$\begingroup\$

I find the original code overly complicated.

Here is a revised version of the original test code.

class Test {

    private $item = [ [ false, true ], [ true, true ] ];

    const VERTICAL_MIDDLE = 421;
    const HORIZONTAL_MIDDLE = 297;

    public function is_quadrant_valid($variable) {
        $down = $variable['y'] > self::VERTICAL_MIDDLE;
        $right = $variable['x'] > self::HORIZONTAL_MIDDLE;

        return $this->item[$down][$right];
    }

}

$t = new Test();
echo $t->is_quadrant_valid([ 'y' => 90, 'x' => 20 ]) ? 'Valid' : 'Invalid';

This includes a class, which seems to be consistent with the original code.

This version defines $item as a private property of the class. Rather than making keys by concatenating strings, I use a two dimensional array system. This also allows us to arrange $item such that the quadrant organization matches.

Rather than storing 0s and 1s that we have to convert into false and true values, this just stores true and false directly.

Instead of using regular variables to store the coordinates of the middle, this uses constants.

I changed the name of the function to be shorter, but I don't think that it is any less clear.

Since the original takes a function parameter, it seems that the test version should as well. This way we could run multiple tests just calling with different values.

Since we index the arrays with 0 for false and 1 for true, we can simplify the variables. I renamed them to better match what they hold.

I find this simpler and easier to read. You should be able to easily transfer this back to your original code if desired.

As a general rule, if (is_true()) return true; else return false; can be replaced by return is_true(); which is much shorter.

\$\endgroup\$

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.