How can I improve CreateGenre() function?

Question

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 <[email protected]>
 * @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;
    }

}

Answer 1

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',

}