Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a controller. It functions as follows:

A user imports a file and decides whether the files contents contain new entries to be placed into a database, or existing entries entries to be updated.

The user clicks on the import button and is sent to the controller below. If there are any warnings or errors, the validation results screen is shown - to help the users correct their data.

If the file doesn't contain any errors or warnings, the validation screen will be skipped and their file contents will be inserted/updated into the database.

This is clearly far too much logic for a controller as I have read that the controller is simply meant to decide which view to send the data to. I am having a rather difficult time determining how to extract out the data.

I am looking for suggestions on what my next step should be. Thank you to anyone who takes time to help me.

public ActionResult ValidationResultsScreen(HttpPostedFileBase file, DatabaseAccessMode databaseAccess)
{
    try
    {
        var fileLocation = Path.Combine(Path.GetTempPath(), string.Join(Guid.NewGuid().ToString(), file.FileName));

        Session["TempFileLocation"] = fileLocation;
        Session["DatabaseAccessMode"] = databaseAccess;

        file.SaveAs(fileLocation);

        var importer= new Importer(_connectionString, _logManager, databaseAccess,Server.MapPath("..\\bin"));

        var parsedValues= importer.ParseFile(fileLocation);

        var failedValues = ParsedValues.Where(x => x.ParsedState != ParsedState.Success);

        if (failedValues.Any())//if there were any warnings or errors display the validation results screen
        {
            return View(processedValues);
        }
        //else import the file to the database

        var transactionResults = importer.UploadToDatabase(parsedValues);

        var failedDBTransactions= RetrieveFailedResults(transactionResults);

        return failedDBTransactions.Any() 
            ? View("DatabaseImportErrorScreen", failedDBTransactions) 
            : View("DatabaseImportResults", transactionResults);
    }
    catch (Exception exception)
    {
        ViewBag.ExceptionMessage = exception.Message;
        return View("DefaultErrorScreen");
    }
}
share|improve this question

migrated from stackoverflow.com Dec 13 '12 at 0:40

This question came from our site for professional and enthusiast programmers.

1  
If you're uncomfortable with the method size, just refactor as much of the logic as you can into another class, perhaps your repository class. –  Robert Harvey Dec 13 '12 at 0:19
    
Hello Robert, that is exactly what I want to do, hence the question. I am unsure how to extract the logic out. –  SamuelDavis Dec 13 '12 at 0:23
    
Why do you use var every time you create a variable? That makes it more difficult to tell what that variable contains. For example, what type is parsedValues? –  Bob Horn Dec 13 '12 at 0:56
3  
It's not necessary; it's nice. It's a readability thing. IMO, beautiful code isn't filled with vars. It would be nice to see it and instantly know what it is, without having to look somewhere else or hover over it. Beautiful code should read like a book. –  Bob Horn Dec 13 '12 at 2:58
1  
I really would like to know why you are attempting to use the session in your controller. Or are we upgrading old code to mvc and this hasn't been handled yet? –  JayC Dec 13 '12 at 4:50

2 Answers 2

up vote 1 down vote accepted

Maybe something like this? (commentary included):

//Do file locations really need to be saved in the session? I would hope not.
protected string TempFileLocation {get; set;}
//This assumes DatabaseAccessMode is an class of some kind.
protected DatabaseAccessMode DatabaseAccess {get;set;}

//place wherever your private methods go...
private WhateverTypeORInterfaceParsedValuesIs GetParsedValues(HttpPostedFileBase file)
{
        TempFileLocation  = Path.Combine(Path.GetTempPath(), 
            string.Join(Guid.NewGuid().ToString(), 
            file.FileName));

        file.SaveAs(TempFileLocation);
        var importer= new Importer(_connectionString, 
            _logManager, 
            DatabaseAccess ,Server.MapPath("..\\bin"));
        var parsedValues= importer.ParseFile(TempFileLocation);

        return parsedValues;
}

//and of course, your new Action
public ActionResult ValidationResultsScreen(HttpPostedFileBase file, DatabaseAccessMode databaseAccess)
{
    try
    {
        //save it for later. 
        //(we still need to figure out a proper lifetime
        // for your Database Access mode)
        DatabaseAccess = databaseAccess; 

        //grab parsed values.
        var parsedValues = GetParsedValues(file)

        //if there were any warnings or errors display the validation results screen
        if (parsedValues.Where(x => x.ParsedState != ParsedState.Success).Any())
        {
            return View(processedValues); // what are the processedValues???
        } 
        else
        { 
            //import the file to the database
           var transactionResults = importer.UploadToDatabase(parsedValues);
           var failedDBTransactions= RetrieveFailedResults(transactionResults);

           return failedDBTransactions.Any() 
            ? View("DatabaseImportErrorScreen", failedDBTransactions) 
            : View("DatabaseImportResults", transactionResults);
        }
    }
    catch (Exception exception)
    {
        ViewBag.ExceptionMessage = exception.Message;
        return View("DefaultErrorScreen");
    }
}

However, you may have noticed I removed the utilization of a Session object

I could see some value to having a Database access mode in session. Even so, using a session to cache data can cause a multitude of problems if you don't have a session timeout page or ways to refill your session on session timeout. Also, using the session directly might introduce race issues.

One attempt to still use the session while avoiding race issues might be the following:

private DatabaseAccessMode _DataAccess;
protected DatabaseAccessMode DatabaseAccess
{
     get{
         //save the first non-null value retrieved and use that every time. 
         _DataAccess = _DataAccess 
              ?? (DatabaseAccessMode) HttpContext.Current.Session["DatabaseAccessMode"]
         return _DataAccess ;
     }
     set{
         _DataAccess = value;
         HttpContext.Current.Session["DatabaseAccessMode"] = value;
     }
}

but if you have a whole set of these values, and changing any one of them might invalidate all the others, it would probably be best to isolate that logic somewhere else.

share|improve this answer
    
Thank you for your answer, how would you suggest persisting "protected string TempFileLocation {get; set;}" though? The value would be lost as soon as the controller is next called –  SamuelDavis Dec 13 '12 at 19:46
    
And could you possibly provide a little insight as to why the session is such a bad place to persist data? It seems to me that this is the entire point of the session... –  SamuelDavis Dec 13 '12 at 20:21
    
The more state you introduce into your application via session, and the more pages you have to visit to set appropriate session variables, the harder your application is to debug and the easier it is to introduce bugs. I especially don't like direct use of the Session object because, if I were required to change something relating to that session object, I'd first have to scan all the files to determine where that session variable is used, and then reverse engineer the use cases possibly affecting or effected by that variable. Not fun. Hiding session access makes that job easier. –  JayC Dec 15 '12 at 2:55
    
Ahh yes that makes sense. Yes I would never want to get into that situation. Thank you so much for the help :) –  SamuelDavis Dec 16 '12 at 20:21

Based on your case your controller seems to be mixing multiple things at the same time. The total context is not clear but it seems this is a controller method in a controller that does more than this, correct?

In general I would suggest to see it different. An import is an object, so create a seperate controller for it: ImportsController. That can be a very standard controller:

Add: User add a new file, controller just sends it to Import model -> save. The model saves it in a temp file or whatever it needs to do. You can implement validation here already but you can also say: "A upload and saved file is ok. That is a correct creation of an import."

Pseudo code:

if(dataIncoming) {
    this->Import->save(data)
    //handle file save errors, no data received etc. All coming back from model which should take care of the validation.
    //redirect to edit
}

Edit: Next step is to review the import. Here you can ask the model to validate.

Pseudo code:

ON SAVE (if this is requested without a save/submit action just show data and validation results)
this->Import->parseFile(data) //validate

//show parse file issues to user if any
return View(processedValues);

if(no errors in parsefile) {
  //try database save
  Import->saveInDatabase
  //show validation errors if needed

  //if all ok you can redirect (or have user interaction) to delete
}

View: Always shows you the data of the import

Delete: Can be used to remove a wrong import upload (user uploaded file twice) or when import is done. You can validate this.

Here you can implement also a soft delete for example.

Pseudo code:

Import->delete

So as you see I would move all that file, tmpfile and other parts to the model by seeing an import as a standalone object. That makes it possible to have all encapsulated and also well testable. A controller method doing all steps is much harder to test. For example you would need to catch all different views rendered etc. I prefer to split it up.

share|improve this answer
    
Hello, I was instructed to not show a "file successfully validated" screen as it would just be an extra annoying button to click. If I were not told this, I would definitely do this in two steps (and originally tried to until told otherwise). Also I still have the same problem of persisting the file location across two controller calls. Due to these problems, my code can't really be changed to your suggestion unfortunately. –  SamuelDavis Dec 13 '12 at 19:51
1  
There is no need to show that screen. If no errors you can just redirect to the next method. Most important what I explained it to see an Import as a separate object. If you handle saving the tmp file in your Import model instance your file location issue is also taken care of. –  Luc Franken Dec 14 '12 at 10:02

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.