current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Software Engineering Stack Exchange is a question and answer site for professionals, academics, and students working within the systems development life cycle who care about creating, delivering, and maintaining software responsibly. 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 0 down vote favorite

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.

share|improve this question
    
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. – Robert Harvey 2 days ago
    
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. – HSharma 2 days ago
    
Sounds reasonable to me. What's the problem? – Robert Harvey 2 days ago
    
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. – HSharma 2 days ago
    
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 2 days ago
up vote 2 down vote

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.

share|improve this answer
    
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. – HSharma 2 days ago
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. – Arseni Mourzenko 2 days ago

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.