Skip to main content
Code Review

Return to Revisions

4 of 4
Commonmark migration

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.

AD7six
  • 1.4k
  • 9
  • 11
default

AltStyle によって変換されたページ (->オリジナル) /