Is it bad practice to use the same method for SAVE and UPDATE?

Question

I'm using laravel but it's not important, when you create a controller with laravel command line tool, it puts 4 default function in there for create and update.

create and store for save

edit and update for well update!

This is what laravel suggest for Shop controller.

class ShopController extends Controller
{

    public function create()
    {
       // return create view
    }

    public function store(Request $request)
    {
      // save a shop
    }

    public function edit($id)
    {
        // find a shop , return edit view
    }

    public function update(Request $request, $id)
    {
        // find the shop with id , update the shop
    }

}

But I like to use the same methods for showing view and store/update my row and avoid writing lots of extra code.

class ShopController extends Controller
{

    public function create($id  = 0)
    {
        return view('shop-create' , ['edit'=> Shop::find($id)]);
    }

    public function store(Request $request , $id = 0 )
    {
        $whitelist = [
            'title'=>'required',
            'phone'=>'present|numeric' ,
            'address'=>'present' ,
        ];
        $this->validate($request, $whitelist );
        $shop = Shop::findOrNew($id) ;
        // find a shop with given id or create a new shop instance
        foreach($whitelist as $k=>$v)
        $shop->$k = $request[$k];

        $shop->save();
     }

}

Naturally I go with what I like (second option), but since laravel suggest the first way, just out of curiosity is there any reason why I shouldn't do it like this? Is this considered bad practice in any way?

Answer 1

Nothing wrong, but you code will be harder to understand, IMHO.

e.g.:

• What does this method do? It's called create, but it also edits?
• The view is called shop-create but it also edits?
• Passing a 0 parameter as default for id and trying to find it every time is unnecessary.

public function create($id  = 0)
{
    return view('shop-create' , ['edit'=> Shop::find($id)]);
}

Although you're thinking that you are simplifying your code, you are turning it more complicated since you are breaking the Single Responsibility principle from SOLID.

It's easier to understand if you have something like the Laravel suggestion.

Also you keep a very common pattern that any Laravel developer will understand, so you can hire someone to take care of your code and do not worry if he will understand.

Answer 2

There is nothing wrong with doing it your way. The "laravel" way you mention is when you create a Restful resource controller and is simply one way to tackle it.

I guess those controller methods were picked because they line up nicely to a "restful" type of controller. If you were to be building a true rest api, then how you do it becomes far more strict from a standards point of view (not imposed by laravel, but line up better to the laravel way).

If you aren't creating a public facing api, or something that is going to be consumed by external entities, then I say design your controllers that work best for you and your team

Answer 3

This is how i usually do it, this way you can still have different validation by using the requests and it's still clear (imo) what the functions do.

public function store(AdminPartnerRequest $request)
{
    return $this->handleCreateOrUpdate($request);
}

public function update(AdminPartnerRequest $request, $id)
{
    return $this->handleCreateOrUpdate($request,true, $id);
}


private function handleCreateOrUpdate($request, $edit = false, $id = null)
{
   if ($edit){
       $partner = Partner::find($id);
   } else{
       $partner = new Partner();
   }

        $partner->name = $request->input('name');
        $partner->picture = $request->input('image');         
        $partner->save();

        return \Redirect::route('admin.partners.index');
}