current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

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());
}
share|improve this question
1  
A controller is supposed to control the Model layer, and not be a Model's service itself. – Kid Diamond Aug 12 '15 at 10:11
    
Thanks, I still got a lot to learn. I will have to check what I should put in the model then :) – Ron Brouwers Aug 12 '15 at 10:14
up vote 3 down vote accepted

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);
}
share|improve this answer
1  
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. – Ron Brouwers Aug 12 '15 at 10:11

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.