2
\$\begingroup\$

I have this class that has these two methods that are so closely related to the each other. I do not want to pass the flags so I kept them separate. I was wondering if there is a way to rewrite it so that I do not have to repeat so closely!

  class Test extends Controller
    {
        public static function nonFormattedData($param)
        {
            $arr = array();
            if (is_array($param)) {
                $i = 0;
                $sql = "
                    select *
                    from table1
                    where
                    ";


                if (isset($param['startDate'])) {
                    $sql .= "   date_created between ? AND  ?";
                    $arr[] = $param['startDate'];
                    $arr[] = $param['endDate'];
                    $i++;
                }
                if (isset($param['amount']) && !empty($param['amount'])) {

                    if ($i > 0) $sql .= " AND ";
                    $sql .= " balance= ?";
                    $arr[] = $param['amount'];
                    $i++;
                }
                if (isset($param) && !empty($param['amount'])) {

                    if ($i > 0) $sql .= " AND ";
                    $sql .= " balance= ?";
                    $arr[] = $param['amount'];
                    $i++;
                }
                if (isset($param['createdBy']) && !empty($param['createdBy'])) {

                    if ($i > 0) $sql .= " AND ";
                    $sql .= " column2 like '%Created By: " . $param['createdBy'] . "%'";
                }
                $sql .= ' group by id.table1 ';
                $rs = Query::RunQuery($sql, $arr);
                foreach ($rs as $row) {
                    $records = new Account();
                    $results[] = $records;
                }
                return $results;
            }
        }




        public static function formattedData($serArray, $orderBy = "giftcardaccount_id desc", $offset = 0, $limit = 10)
        {
                $arr = array();
            if (is_array($param)) {
                $i = 0;
                $sql = "
                    select *
                    from table1
                    where
                    ";


                if (isset($param['startDate'])) {
                    $sql .= "   date_created between ? AND  ?";
                    $arr[] = $param['startDate'];
                    $arr[] = $param['endDate'];
                    $i++;
                }
                if (isset($param['amount']) && !empty($param['amount'])) {

                    if ($i > 0) $sql .= " AND ";
                    $sql .= " balance= ?";
                    $arr[] = $param['amount'];
                    $i++;
                }
                if (isset($param) && !empty($param['amount'])) {

                    if ($i > 0) $sql .= " AND ";
                    $sql .= " balance= ?";
                    $arr[] = $param['amount'];
                    $i++;
                }
                if (isset($param['createdBy']) && !empty($param['createdBy'])) {

                    if ($i > 0) $sql .= " AND ";
                    $sql .= " column2 like '%Created By: " . $param['createdBy'] . "%'";
                }
                $sql .= ' group by id.table1 ';
                $rs = Query::RunQuery($sql, $arr);

                return array("data" => $rs);
            }
        }


    }
\$\endgroup\$
3
\$\begingroup\$

Just judging by the size you can tell that this one method is doing entirely too much. The Single Responsibility Principle states that a method/function should be responsible for one thing to the exclusion of all others. It can request other methods/functions to help it, but it should only be concerned with its purpose, which should be evident by its name.

So, the first thing I would say to do is break this up into even more methods first. The how should be pretty obvious in most cases. In this case, however, it may not be, so let's take a look at what you are doing to get an idea of how we should break this up.

Each if statement sets up an additional part of this growing SQL statement. If you look closely at each of these blocks, you may begin to notice a pattern. When a pattern arises we know that we can apply the "Don't Repeat Yourself" (DRY) Principle to it so that it can be reused. So our pattern here is to append a SQL statement, add to an array, and finally increment a counter. Sounds simple enough.

private
    $sql = '',
    $arr = array()
;

//constructor, etc...

private function append( $sql, $params ) {
    if( strlen( $this->sql ) ) {
        $sql = ' AND ' . $sql;
    }

    $this->sql .= $sql;
    $this->arr = array_merge( $this->arr, $params );
}

So, you may notice that we are doing a few things differently here. First, we add some new properties to our class. The $sql and $arr properties take the place of their respective variables. This is so that our new methods can communicate with each other. Next, we removed the counter $i. The counter is pretty much useless, even in its current form. If we remove the initial value of $sql then we can just check the string's length. If it is empty then of course we don't prepend an AND, but if it isn't then we should. Don't worry, we'll prepend the initial value when we are done. The last thing we do is merge our arrays. Since we are using a standard sequential array there should be no problems here, we just have to be sure to append everything in the right order, which we've just done. So, now to use it.

public function nonFormattedData( $param ) {
    if( ! is_array( $param ) ) {
        return FALSE;
    }

    if( isset( $param[ 'startDate' ] ) ) {
        $this->append( ' date_created between ? AND ?', array(
            $param[ 'startDate' ],
            $param[ 'endDate' ]
        ) );
    }

    //etc...

    $this->sql = '
        select *
        from table1
        where
        ' . $this->sql . '
        group by id.table 1 '
    ;

    //etc...
}

So, there's a few new things to cover here as well. The first is the removal of the static keyword. Static methods and properties are counter to OOP. For the most part you will probably never need them. But specifically in this case they prevent us from using our new method and properties. There are ways around this, but again, that is counter to OOP. The next thing I did was reversed your initial if statement and returned early. This is a good habit to get in to. First it reduces the amount of indentation, meaning we are no longer violating the Arrow Anti-Pattern (even as slight as the violation was), and second it reduces the amount of processing necessary for PHP to determine that it doesn't need to do anything. Not much mind, but still more efficient. The next if statement should look pretty familiar, its exactly the same, but the inner block is different. Here we are using the new method we just created. This should be pretty self-explanatory. Add the custom SQL and pass in the appropriate parameters. Repeat for each if statement. Finally we come to our new SQL statement. I promised you we'd get back to that initial value. Well, here it is, combined with appending the final value, resulting in our complete SQL statement, ready to use. The rest is the same.

Now, we've cleaned up this first method and, as you've already mentioned, the second method is pretty similar. If we apply the above to it it will be better, but we can still do more. As sky py has pointed out, the only difference is that foreach loop in the first method. The second method returns the results the first uses to loop over, or something VERY similar. So the obvious move here would be to call the second method from the first, and use the return to loop from. So our new first method should look like this.

public function nonFormattedData( $param ) {
    $results = array();

    $rs = $this->formattedData( $param );
    foreach( $rs[ 'data' ] AS $row ) {
        $records = new Account();
        $results[] = $records;
    }

    return $results;
}

The only thing new here is that I initialized the $results array, which you should always do before trying to manipulate it. There's a couple of reasons for this. The first, and most obvious, is what happens if $rs[ 'data' ] is empty? The loop never happens and $results is never defined. Resulting in an undefined error and a potential crash. The not so obvious is what would happen if $results was accidentally set somewhere previously, or god forbid, is a global. If that variable happens to be an array you'll just get some funny results. But if it happens to be a string you will be modifying the string using string-array notation, which will definitely cause problems.

So this answers your question, but still isn't as efficient as it could be. To be the most efficient we will want to limit how many times we query the server. Assuming that we always call the second method before the first, then we can have the second method save its results as a new property and access that property in the first method instead of querying the server again. Following that logic we can do the same thing to the first method and then use getters to return the appropriate property instead of calling those methods again and again.

I hope this helps. If you have any questions or need some clarification, don't hesitate to ask.

\$\endgroup\$
2
\$\begingroup\$

You have two methods, one is nonFormattedData and other one is formattedData. I don't see the arguments for formattedData are being used.

The difference between these two methods are how the result is returned. formatdata method returns array("data" => $rs) and nonFormatData does a foreach on the $rs and returns array of results

foreach ($rs as $row) {
 $records = new Account();
 $results[] = $records;
}

If this is the only difference then there is no point of duplicating the code. One option is to make client code deal with how it wants data or create a separate method which will take query result set and formats the required output.

\$\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.