1

I am using a MVC framework and I have developed a module where .csv file is being imported into the database, I am little unsure about certain things which I'll try to explain below:

Step 1:

.csv file is uploaded and I do all the initial checks like valid .csv file, size etc

Step 2:

Next I go through the main headers of the .csv file and check if there are any new fields added to the file and if there are then I create an array of those fields. Before checking I already get list of existing fields from the db as an array. Then I create a string of all table field names, after properly sanitizing and escaping them using php

Step 3:

I go through the .csv file line by line, sanitize & escape values using php and create a string that is used as VALUES for my insert query

Step 4:

I pass the string of fields and values to the model for query execution.

Now thing is I am executing Steps 1 to 3 in controller and Step 4 in model, so question is should I sanitize & escape fields n values in controller or should I do it in the model? Which is technically & logically much better way of doing things in the MVC framework.

5
  • 1
    For the most part, the only thing you should be doing in the controller is deciding which methods in the model to call and providing some basic coordination. Everything else should happen in the model. The model is where your Business Domain is implemented. Commented Nov 18, 2016 at 18:47
  • Well I have three methods in my model, one to get all fields, two to add new fields and third to execute the final insert query.
    – Just_Do_It
    Commented Nov 18, 2016 at 18:49
  • 1
    Sounds reasonable to me. What's the problem? Commented Nov 18, 2016 at 18:52
  • 1
    Problem is during code review I was told that fields n values should be sanitized & escaped in the model not in controller as it will cause errors .... but I don't understand why would it cause any error or is it just that best practice is to handle such stuff in the model not in controller.
    – Just_Do_It
    Commented Nov 18, 2016 at 19:00
  • By default you want to make sure your code remains as DRY as possible. By adding this logic in the model you make sure that if any other places eventually need to handle CSV uploads, all they need to do is to use the model to do the right things. With your current implementation, if at some point you end up adding a CSV upload somewhere else, you'll need to make sure the same sanitizing happens, which is more error prone and will likely lead to copy/pasting code..
    – Alberto
    Commented Nov 19, 2016 at 0:28

2 Answers 2

2

What you put in a controller and what finds its way in the model depends on the language and framework you use. Some prefer small models which consist of data structures and practically no logic; others prefer using a controller just as a glue, and moving as much logic as possible to the models. Given the code review you got, it seems that the habit in your company/ecosystem is to put data validation in the model.

This, by the way, makes perfect sense. Imagine a model Price which contains the price of a product. Would it be better to put the validation logic such as “the price amount could not be inferior or equal to zero” in the actual Price class, or do it in a controller? In OOP, you'd usually put this logic in the Price class.

Discussing validation specifically, for instance ASP.NET MVC where usually most of the code can be found in the controller, uses a special mechanism where input validation (and more specifically the validation of forms) is done through the attributes which decorate the members of the model. The controller is not involved.

but I don't understand why would it cause any error

It won't. The problems with your approach are only that:

  • It makes difficult to follow the logic: the data belongs to a given class, but the validation of this data is made in a different class, which doesn't even have this responsibility in the first place.

  • The model can be used by multiple controllers, leading to the duplication of the validation logic.

Probably both cases don't apply well or at all in your current case; however, they apply for the remaining part of the code base, and keeping your code consistent is important.

2
  • I completely agree with what you stated above but if we consider a case where a form is being submitted and data needs to be inserted in the db, the validation/checks part is normally done in the controller and then the final entity is passed on to the model for insertion, which I see is the normal way things are done in MVC frameworks.
    – Just_Do_It
    Commented Nov 18, 2016 at 19:16
  • 1
    @HSharma: not necessarily. For instance in ASP.NET MVC where usually most of the code can be found in the controller, forms validation is done through attributes decorating the model. The controller is not involved. Commented Nov 18, 2016 at 21:12
1

I prefer controllers and models (nowadays mostly Entities that are mistakenly referred to as Models) to be 'slim'. Do garbage collection and other gibberish elsewhere, but not there.

So you have to let the controller delegate the work:

(Controller object whispering) "I could do it, but that's too much trouble. I'll let my minion do that hard work and I'll just sit back, relax and wait for him to finish. When he's finished, I have another minion waiting to do finish the hard work!"

So I recommend to introduce a 'Helper' that would be called from the controller.

What it would basically do is:

  • build the basic output and sanitize it in the process
  • return the result

The result is a collection of values that can be parsed by your model (this is an important feature of the helper!).

I theory you'd now have two steps:

  • The controller sends and receives data from and to the helper
  • The controller sends data to the model and stores it

Makes it a lot cleaner and your code is more readable!

When dealing with really large heaps of data, this approach may not always work. You may have to integrate step 2 in step 1 (you helper acts without controller intervention an stores data autonomously).

1
  • Placing sanitation & validation into a separate collaborator of the presentation / UI layer follows separation of concerns and fosters reuse💡
    – hc_dev
    Commented Feb 9, 2021 at 17:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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