2
\$\begingroup\$

What I have is a grocery list. Once I select a grocery by ticking the checkbox, I can select an amount for that product and a unit (unit is referred to in my below code as group). Once I am done, I click save and the values are updated in the database. But for all checkboxes that are unchecked, the values should be updated to 0. Also, if I DO select a checkbox, but I DO NOT select an amount OR a group, the values should all still be updated to 0.

The code below works flawlessly, but I think it could be written much shorter / better. Any pointers?

public function post()
{
    $checkboxes         = Input::get('checkbox');
    $checkedAmounts         = Input::get('amount');
    $checkedGroups      = Input::get('group');

    /* The below code checks if a checkbox was checked, an amount was selected and a group was selected. If any of those values are true, update the database with the values selected. */

        foreach ($checkedAmounts as $key => $value)
        {
            if($value !== '0')
            {
                Product::where('id', '=', $key) ->update(['amount' => $value]);
            }
        }

        foreach ($checkedGroups as $key => $value)
        {
            if($value !== '0')
            {
                Product::where('id', '=', $key) ->update(['group' => $value]);
            }
        }

        foreach ($checkboxes as $key => $value)
        {
            if($value !== '0')
            {
                Product::where('id', '=', $key) ->update(['selected' => '1']);
            }
        }

    /* The below code checks if a checkbox was NOT checked, an amount was NOT selected or a group was NOT selected. If ANY of those are false, update the database and set everything to 0 for that row. */

        foreach ($checkboxes as $key => $value)
        {
            if($value === '0')
            {
                Product::where('id', '=', $key) ->update(['selected' => '0']);
                Product::where('id', '=', $key) ->update(['amount' => '0']);
                Product::where('id', '=', $key) ->update(['group' => '0']);
            }
        }

        foreach ($checkedAmounts as $key => $value)
        {
            if($value === '0')
            {
                Product::where('id', '=', $key) ->update(['selected' => '0']);
                Product::where('id', '=', $key) ->update(['amount' => '0']);
                Product::where('id', '=', $key) ->update(['group' => '0']);
            }
        }

        foreach ($checkedGroups as $key => $value)
        {
            if($value === '0')
            {
                Product::where('id', '=', $key) ->update(['selected' => '0']);
                Product::where('id', '=', $key) ->update(['amount' => '0']);
                Product::where('id', '=', $key) ->update(['group' => '0']);
            }
        }           

        return Redirect::to('groceries')->with('categories', Category::with('products')->orderBy('name', 'asc')->get());
}
\$\endgroup\$
  • 1
    \$\begingroup\$ A controller is supposed to control the Model layer, and not be a Model's service itself. \$\endgroup\$ – Kid Diamond Aug 12 '15 at 10:11
  • \$\begingroup\$ Thanks, I still got a lot to learn. I will have to check what I should put in the model then :) \$\endgroup\$ – Hardist Aug 12 '15 at 10:14
3
\$\begingroup\$

First of all, you could have less updates if you passed all values you want to set at once:

Product::where('id', '=', $key) ->update(['selected' => '0', 'amount' => '0', 'group' => '0']);

Secondly, you check in many places in your code if value is different than 0 and save the value, but if later if it is equal to 0 you save 0 in the database - you could just simply save the value in the database and skip the checks.

This, including some other enhancements, should work for you:

$data = [];
foreach ($checkedAmounts as $productId => $value)  {
  $data[$productId]['amount'] = $value;
}

foreach ($checkedGroups as $productId => $value) {
  $data[$productId]['group'] = $value;
}

foreach ($checkboxes as $productId => $value) {
  $data[$productId]['selected'] = boolval($value);
}

foreach ($data as $productId => $updates) {
  Product::whereId($productId)->update($updates);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks, this indeed works better and is what I was looking for. I learned a lot from this answer :) For my "other problem" I already know what to do. I think I will check if any value is 0 AFTER the update and then update all other values to 0 as well in that row. To keep my form clean. Thanks again. \$\endgroup\$ – Hardist Aug 12 '15 at 10:11

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.