1
\$\begingroup\$

The code works as it is but it's very messy. Is there a way to clean up the null checking in the if statement?

public function submitCoil ($data) {
    $db = Database::getInstance();

    //Define certain query values
    $uniqueID = $this->hash("id");
    $queryData = array(
        'fields' => array(
            'uniqueid',
            'uploaded',
            'name',
            'resistance',
            'wrapped',
            'category',
            'description',
            'wraps',
            'images',
            'wire_one',
            'wire_two',
            'wire_three',
            'wire_four',
            'wire_five',
            'wire_six'
        ),
        'values' => array(
            'uniqueID' => $uniqueID,
            'uploaded' => date("F j, Y", time()),
            'name' => $data['name'],
            'resistance' => $data['resistance'],
            'wrapped' => $data['wrapped'],
            'category' => $data['category'],
            'description' => $data['description'],
            'wraps' => $data['wraps'],
            'images' => "coils/" . $uniqueID . "/",
            'wire_one' => $data['wire_one'],
            'wire_two' => $data['wire_two'],
            'wire_three' => $data['wire_three'],
            'wire_four' => $data['wire_four'],
            'wire_five' => $data['wire_five'],
            'wire_six' => $data['wire_six']
        )
    );

    //Make sure mandatory datas are filled out
    if (!empty($queryData['values']['name']) && !empty($queryData['values']['resistance']) && !empty($queryData['values']['wrapped']) && !empty($queryData['values']['description']) && !empty($queryData['values']['wraps'])) {
        $db->insertFew("coils", $queryData['fields'], $queryData['values']);
    } else {
        echo "Please fill all fields";
    } 
\$\endgroup\$
  • 3
    \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. For examples of good titles, check out Best of Code Review 2014 - Best Question Title Category You may also want to read How to get the best value out of Code Review - Asking Questions. \$\endgroup\$ – BCdotWEB Nov 12 '15 at 10:46
  • \$\begingroup\$ @BCdotWEB except for the winner/accepted answer that first link is IMO not a list of good question titles. They are good examples of on-topic humor - which are enticing but give no real indication of what the question is about. \$\endgroup\$ – AD7six Nov 12 '15 at 10:55
  • \$\begingroup\$ @AD7six It's a standard comment copy-pasted from here: meta.codereview.stackexchange.com/a/4957/10582 \$\endgroup\$ – BCdotWEB Nov 12 '15 at 10:58
  • \$\begingroup\$ Coolio - my sentiment stands though. \$\endgroup\$ – AD7six Nov 12 '15 at 10:59
5
\$\begingroup\$

Use a validation function

There are many possible solutions, one thing you can do is use a separate function to validate your data:

public function submitCoil($data)
{
    ...
    if (!$this->validates($queryData['values'])) {
        return false;
    }

    return $db->insertFew("coils", $queryData['fields'], $queryData['values']);
}

protected function validates($values)
{
    $mandatoryFields = ['name', 'resistance', 'description', 'wraps'];

    $return = true;

    foreach ($mandatoryFields as $field) {
        if (empty($values[$field])) {
            // Consider doing something like this so you can know/say
            // what the problem is.
            // $this->validationErrors[$field] = "This is empty";
            $return = false;
        }
    }

    return $return;
}

This function simply loops on all the keys that you specify as needing to be not-empty and returns true only if they are all populated.

Many simple functions

Always strive to have simple functions, that do one thing - for example:

public function submitCoil($data)
{
    $queryData = $this->translateData($data);
    if (!$this->validates($queryData['values'])) {
        return false;
    }
    return $this->insert($queryData);
}

protected function translateData($data)
{
    $uniqueID = $this->hash("id");
    return [
        'fields' => [
            'uniqueid',
            'uploaded',
            'name',
            'resistance',
            'wrapped',
            'category',
            'description',
            'wraps',
            'images',
            'wire_one',
            'wire_two',
            'wire_three',
            'wire_four',
            'wire_five',
            'wire_six'
        ],
        'values' => [
            'uniqueID' => $uniqueID,
            'uploaded' => date("F j, Y", time()),
            'name' => $data['name'],
            'resistance' => $data['resistance'],
            'wrapped' => $data['wrapped'],
            'category' => $data['category'],
            'description' => $data['description'],
            'wraps' => $data['wraps'],
            'images' => "coils/" . $uniqueID . "/",
            'wire_one' => $data['wire_one'],
            'wire_two' => $data['wire_two'],
            'wire_three' => $data['wire_three'],
            'wire_four' => $data['wire_four'],
            'wire_five' => $data['wire_five'],
            'wire_six' => $data['wire_six']
        ]
    ];
}

protected function validates($values)
{
    $mandatoryFields = ['name', 'resistance', 'description', 'wraps'];

    foreach ($mandatoryFields as $field) {
        if (empty($values[$field])) {
            return false;
        }
    }

    return true;
}

protected function insert($data)
{
    $db = Database::getInstance();
    return $db->insertFew("coils", $data['fields'], $data['values']);
}

This allows the building-blocks to be changed - or used individually. It also aides readability/maintainability as it is much clearer at a glance what a function does (assuming methods are well-named).

Avoid repetition

The fields and array keys for the values key in $queryData look to serve the same purpose - although they are different. You don't need 'fields' if the keys can be used i.e.:

$queryData = [
    'uniqueid' => $uniqueID,
    'uploaded' => date("F j, Y", time()),
    'name' => $data['name'],
    'resistance' => $data['resistance'],
    'wrapped' => $data['wrapped'],
    'category' => $data['category'],
    'description' => $data['description'],
    'wraps' => $data['wraps'],
    'images' => "coils/" . $uniqueID . "/",
    'wire_one' => $data['wire_one'],
    'wire_two' => $data['wire_two'],
    'wire_three' => $data['wire_three'],
    'wire_four' => $data['wire_four'],
    'wire_five' => $data['wire_five'],
    'wire_six' => $data['wire_six']
];

$db->insertFew("coils", array_keys($queryData), array_values($queryData));

This alone makes the method body 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.