2
\$\begingroup\$

I use this function in my wordpress site to add genres to an album: What is it that makes this code so bulky? Is it have for the server or it just looks like?

/**
 * Creates a genere. If one already exists it will return an error 
 * @author Junaid Qadir Baloch <shekhanzai.baloch@gmail.com>
 * @global type $wpdb
 * @param type $genreName
 * @param bool $returnFalse Return False if genre already exists insted of
 * returning setatus and message. Defaults to FALSE
 * @return type 
 */
function createGenre($genreName, $returnFalse = FALSE) {
    $funtionName = __FUNCTION__;
    global $wpdb;
    $genreName = trim($genreName);
    if (empty($genreName)) {
        $status = ReturnValue::STATUS_ERROR;
        $value = "";
        $message = 'Genere name cannot be empty';
        return ReturnValue::doReturn($funtionName, $status, $value, $message);
    } elseif (!is_string($genreName)) {
        $status = ReturnValue::STATUS_ERROR;
        $value = "";
        $message = 'Genere name must be a text string';
        return ReturnValue::doReturn($funtionName, $status, $value, $message);
    }
    $genreName = ucwords($genreName);
    $genreSlug = text2Slug($genreName);
    $query = "SELECT COUNT (*) as count FROM `genres` WHERE `name`='" . $genreName . "' AND  `slug`='" . $genreSlug . "' ";
    $result = $wpdb->get_results($query);
    if ($result[0]->count <= 0) {
        $formData = array(
            'name' => $genreName,
            'slug' => $genreSlug
        );
        $result = $wpdb->insert('genres', $formData);
        if ($result) {
            $status = ReturnValue::STATUS_SUCCESS;
            $value = "";
            $message = 'Genere ' . $genreName . ' has been created.';
            return ReturnValue::doReturn($funtionName, $status, $value, $message);
        } else {
            $status = ReturnValue::STATUS_ERROR;
            $value = "";
            $message = 'Failed to create a genere as ' . $genreName . '.';
            return ReturnValue::doReturn($funtionName, $status, $value, $message);
        }
    } else {
        if ($returnFalse) {
            return FALSE;
        }
        $status = ReturnValue::STATUS_ERROR;
        $value = "";
        $message = 'Genere ' . $genreName . ' already exists.';
        return ReturnValue::doReturn($funtionName, $status, $value, $message);
    }
}

This a static class to handle return values. It provides a unified way for other functions to return values.

/**
 * ReturnValue is an helper class to handle
 * return values, status codes and messages
 * returned from different functions 
 *
 * @author JQ
 */
class ReturnValue {

    const STATUS_SUCCESS = "SUCCESS";
    const STATUS_FAIL = "FAILED";
    const STATUS_OK = "OK";
    const STATUS_ERROR = "ERROR";

    public static function doReturn($funtionName, $status = NULL, $value = NULL, $message = NULL) {
        if ($status == NULL) {
            $status = self::STATUS_FAIL;
        }
        $return = array(
            action => $funtionName,
            status => $status,
            message => $message,
            returnValue => $value
        );
        return (object) $return;
    }

}
\$\endgroup\$
1
\$\begingroup\$

Remove global (this shouldn't need to be explained).

Your code depends on the class ReturnValue with method doReturn. This is a tight coupling that makes it hard to test your code. For your code to be reused you must also include the ReturnValue. This is almost ok, when it is only 1 extra file, but it gets out of hand very quickly.

Your code has also become bloated by the error code handling. Consider rewriting this with exceptions:

/**
 * Creates a genere. If one already exists it will return an error
 *
 * @param type $wpdb        Wordpress database?
 * @param type $genreName   What is this?
 * @return type 
 */
function createGenre($wpdb, $genreName) {
    if (empty($genreName) || !is_string($genreName))
    {
       throw new InvalidArgumentException(
          // Choose either __METHOD__ or __FUNCTION__ depending if this is OO.
          __METHOD__ . ' genreName must be a non-empty string');
    }

    $genreName = ucwords($genreName);
    $genreSlug = text2Slug($genreName);
    $query = "SELECT COUNT (*) as count FROM `genres` WHERE `name`='" .
        $genreName . "' AND  `slug`='" . $genreSlug . "' ";
    $result = $wpdb->get_results($query);
    if ($result[0]->count > 0) {
       throw new RuntimeException(__METHOD__ . ' genre already exists.');
    }

    $formData = array(
       'name' => $genreName,
       'slug' => $genreSlug
       );

    $result = $wpdb->insert('genres', $formData);

    if (!$result) {
       throw new RuntimeException(
           __METHOD__ . ' failed to create genre: ' . $genreName);
    }
}

The code is a lot shorter and flatter (which makes it easier to test and maintain. Arrow code is harder to debug, maintain and test - see here.

Here is the usage with exceptions:

try
{
   $genreName = 'slimy ';
   createGenre($wpdb, trim($genreName));

   return array('action'      => 'createGenre',
                'message'     => 'Genre ' . $genreName . ' has been created.',
                'returnValue' => '', // If needed even.
                'status'      => 'SUCCESS');
}
catch (Exception $e)
{
   return array('action'      => 'createGenre',
                'message'     => $e->getMessage(),
                'returnValue' => '', // If needed even.
                'status'      => 'ERROR',

}
| improve this answer | |
\$\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.