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

Small project, do whatever you want. Large with other developers, follow the conventions.

Coding conventions are a set of guidelines for a specific programming language that recommend programming style, practices, and methods for each aspect of a program written in that language. These conventions usually cover file organization, indentation, comments, declarations, statements, white space, naming conventions, programming practices, programming principles, programming rules of thumb, architectural best practices, etc. These are guidelines for software structural quality. Software programmers are highly recommended to follow these guidelines to help improve the readability of their source code and make software maintenance easier. Coding conventions are only applicable to the human maintainers and peer reviewers of a software project. Conventions may be formalized in a documented set of rules that an entire team or company follows, or may be as informal as the habitual coding practices of an individual. Coding conventions are not enforced by compilers. -- https://en.wikipedia.org/wiki/Coding_conventions

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');
}

Answer 4

Nothing wrong, but in that case you have to maintain proper comments that specify that your function perform add / edit and for that you are using some variable like $id or some thing else. If it is available than you can update the record otherwise insert it.

Answer 5

I used this method in a last project of mine, we called the store() and update() function manage() instead and had a getManage() which would use the same view for creating and editing. I liked this method a lot yet came across a few things worth noting. Sadly the cons outway the pros if you ever have to face those issues :(

Pros:

• Smaller code - No longer do you have duplicate lines of code in your store() and update() function.
• Faster to re-use with basic models - ctrl+c ctrl+v ctrl+f ctrl+r if you know what I mean.
• Easier to add/change input values - An extra value would not mean having to change store() and update() to make sure they both utilize the extra input.
• One function to rule them all - As long as you are not doing anything special, you can even define one function for everything. Need to change something? You've got one function, no worries.

Cons:

• Code is harder to understand for others (or an older you) - If someone is new to this method or hasn't used it in a while, understanding what happens within your function is a little harder than having two separate ones.
• Validation is a nuisance - As stated in this answer validation may be different for create and update. Meaning you may sometimes have to write two validations which will eventually lead to messy code and we don't want that!
• Value insertion wasn't as cool as I thought - If you want to use the same predefined array to create or update then you may run into the problem of wanting to insert values on create yet never want to update them. This eventually led to either ugly if statements or two predefined arrays.

Eventually it's up to what you're going to make and what you want to do. If you have a basic website which will manage blog posts and pages then have no worries going for a shared store() and update() function. Yet if you're creating a huge CMS with many models, relations and different create and update input values (which may mean different validation) then I'd go with what Laravel recommends. It would be much easier to maintain in the long run and you won't have to deal with headaches, hacky fixes and unclean code.

Whatever you do, don't do both in different controllers! That would be confusing.

By the way, if you're wondering what kind of project I had - it was a huge CMS. So even though it was very useful and easy in some cases, it was sadly not worth it.

Answer 6

Using same function for save() and update() is good idea but at the same time it will increase complexity .. One point is If in future you want to change anything you need to change it only at one place. But at the same time you need to take some extra care.

As your function should be more dynamic.

1) Multiple records manipulation : you may require to update more than one raws at the same time so your function should be enough flexible to insert/update single/multiple values by the same function. Meaning , single query should be fired for multiple records in both the cases.

2) Validation if value already exist : When you are going to check some validation ... in insert case you only need to check if the value is exist in db or not when in update case you need to check with exclusion of current id e.g.

for insert case

 $this->validate($request, [
        'email' => 'required|string|email|unique:tablename,email'
    ]);

for update case

  $this->validate($request, [
        'email' => 'required|string|email|unique:tablename,email,'.$id.',id'
    ]);

And at last very small point but need to be considered ..

3) Success message : At the time of insertion message should be "added successfully" and at updation time Record "updated successfully"

Answer 7

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