8
\$\begingroup\$

The background

Two beginners without access to an experienced mentor write an ASP .NET MVC application, mostly simple CRUD, using Linq to SQL for the data access layer. Beginner number 1 writes the model part. I, beginner number 2, start writing controllers and views. When using the web and a textbook for learning best practices, I notice that our code differs from the established patterns in some way. Still, I create a way for the user to edit data without changing my coworker's code, and as far as we have tested, it does what it is supposed to do.

If our slightly unorthodox approach works, we cannot afford to refactor the whole thing right now. But I am afraid that we can have programmed us into a corner and be too inexperienced to notice it. So please tell us: what are the potential downsides of our current implementation?

A big picture of the concept

We save edits to an entity of type Animal line in the following way: On submitting the form with the edits, the model binder returns a viewmodel to the controller. Every time the controller is initialized, it creates a a new repository instance, initializing it with a new instance of a data context. When the user submits edits, the default model binder returns a new viewmodel object to the controller action. The controller calls the viewmodel's UpdateBaseAnimalLine method, which changes the properties of the entity class. Then the controller calls the repository's Update method on the newly changed entity class. It does nothing more than calling SubmitChanges on the repository's data context.

Problems I have seen so far

  • As far as I am aware, the data context is never disposed of in our code. After looking around, it seems that having one data context per repository instance is good practice, so we probably don't want to change that, but I cannot think of a good place to add a dispose call, what am I overlooking? The examples I found on the web use custom-written factories which provide a datacontext, and I hope there is a simpler way to do it right.

  • It looks weird to me that we have to change the state of the actual entity object somewhere, and then just call "SubmitChanges" in the repository. Doesn't this open us to potential race conditions? Or is the framework intelligent enough to take care of that behind the scenes? More to the point, does it take care of it in the way we are using it?

  • Is there a way to get the default model binder to use a viewmodel constructor which takes an int parameter, instead of just initializing all primitive type fields with the values from the form? (I suppose that it is possible if I write a custom one, but as I have a workaround, I don't want to go that deep for now).

Please look into the code for further problems, I suppose there must be more than I can find.

The code

As a shortened example, we have the business entity Animal Line, with the two properties name and database ID.

    [Table(Name = "AnimalLine")]
public class AnimalLine 
{

    [Column(Name = "AnimalLine_ID", IsPrimaryKey = true, IsDbGenerated = true, AutoSync = AutoSync.OnInsert)]
    public int AnimalLineId { get; set; }

    [Column(Name = "FullName", CanBeNull = false)]
    public string FullName { get; set; }
}

There is a class functioning as a repository, called AnimalLineManagement. It can update an existing animal line either from a bunch of properties, or from an existing object.

public class AnimalLineManagement
{
    private DataContext dataContext;
    private Table<AnimalLine> animalLine;

    public AnimalLineManagement(DataContext dataContext)
    {
        // as far as I can see, he has forgotten to dispose of the data context. 
        this.dataContext = dataContext;
        animalLine = dataContext.GetTable<AnimalLine>();
    }

     // other methods left out for brevity

     public bool Update(String fullName, int id)
     {
        try
        {
            var al = animalLine.SingleOrDefault(a => a.AnimalLineId == id);
            if (al != null)
            {
                if (!String.IsNullOrEmpty(fullName))
                {
                    al.fullName= fullName;
                }
            }
            dataContext.SubmitChanges();
            return true;

            }
            else { return Insert(fullName); }
        }
        catch
        {
            return Insert(fullName);
        }
    }

    public void Update(AnimalLine al)
    {
            dataContext.SubmitChanges(); 
    }
}

There is also a view model class, which wraps an actual animal line class. It provides the properties of the animal line in a way which will not produce an exception (in the real application, a call like AnimalLine.Species.LatinName produces an exception if Species is not set, and I don't want to catch this in the view in the middle of all the HTML), and packs some more info which would have been stuffed in the ViewBag else (not shown here).

public class AnimalLineVM
{
    private AnimalLine animalLine; 
    public string errorMessage = "An error occured while trying to retrieve this information";
    private string fullName;

    //I would have preferred to always initialize the base animal line 
    //in the constructor, but when the instance is created by the model binder, 
    //I don't think I can do this. So I set the base animal line later, 
    //using this variable to ensure that once set, it can't be changed. 
    private bool animalLineAlreadySet;


    public AnimalLineVM(AnimalLine baseAnimalLine)
    {
        this.animalLine = baseAnimalLine;
        animalLineAlreadySet = true; 
    }

    public AnimalLineVM()
    {
        animalLineAlreadySet = false; 
    }

    public AnimalLine BaseAnimalLine
    {
        get { return animalLine; } 
        set
        {
            if (!animalLineAlreadySet)
            {
                animalLine = value;
                animalLineAlreadySet = true;
            }
            else
            {
                throw new InvalidOperationException("The base animal line has already been set. It is not possible to change it."); 
            }
        }
    }

    [Display(Name = "ID")] 
    public int AnimalLineId
    {
        // read only, so we cannot get a discrepancy between the base animal line and the ID in the viewmodel
        get
        {
            if (animalLineAlreadySet)
            {
                int id = BaseAnimalLine.AnimalLineId;
                if (id == null || id < 0)
                {
                    return -1;
                }
                else return id;
            }
            else return -1; 
        }
    }


    [Display(Name = "Full name")] 
    public string FullName
    {
        get
        {
            fullName = fullName ?? errorMessage;
            return fullName; 
        }
        set
        {
            fullName = value; 
        }
    }

    public void updateBaseAnimalLine()
    {
        BaseAnimalLine.FullName = this.FullName; 
    }
}

And this is the controller:

public class AnimalLineController : Controller
{

    private IAnimalLineManagement animalLineManagement;

    public TumorModelsController() :base()
    {
        animalLineManagement = new AnimalLineManagement(new DataContext(ConfigurationManager.ConnectionStrings["TumorModelsDB"].ConnectionString)); 
    }

public ActionResult EditAnimalLine(int animalLineId)
{
    AnimalLine al = animalLineManagement.GetSingleLine(animalLineId);
    return View("EditAnimalLine", new AnimalLineVM(al)); 
} 

    //TODO: implement validation of user input
    [HttpPost]
    public ActionResult EditAnimalLine(AnimalLineVM animalLine)
    {
        int alId; 

        if (int.TryParse(Request.Form["animalLineId"], out alId))
        {
            animalLine.BaseAnimalLine = animalLineManagement.GetSingleLine(alId);
            animalLine.updateBaseAnimalLine();
        }

        return View("AnimalLine", animalLine); 
    }
}
\$\endgroup\$
7
\$\begingroup\$

First of all, your code doesn't compile, you have a poor lonely extra closing bracket in your Update method.

Next, your Update method shouldn't Insert if Update doesn't work. If the entity doesn't exist, you should return false. Otherwise rename your method UpdateOrInsert so that it is clear what you are doing! Also, you shouldn't blindly catch every possible exception. Imagine an SqlException is thrown because the connection cannot be established to the database, you will catch this exception, then try to insert into the "broken" database. I don't think you need to catch any exceptions there because you already check if your entity exists. If you need to catch, you might have another problem in your code.

The second Update method is weird, why does it exists?

In your view model, your errorMessage should be marked const. Also, if your application ever needs to offer multiple languages, you should consider putting this message in a resource file.

This code doesn't compile, an int cannot be null, so your if should only check if the id is greater than 0.

int id = BaseAnimalLine.AnimalLineId;
if (id == null || id < 0)

In your FullName getter, I have a suggestion to make it a one liner, (maybe you won't like it but maybe you'll learn something from this!) you can do :

get { return fullName ?? (fullName = errorMessage); }

Edit You could also return fullName ?? errorMessage if you don't need to use fullName with the errorMessage value anywhere else.

For your problem about the intiialization of the base animal line, I would recommend not to validate that it cannot be set more than once. A view model is almost like a DTO, if I decide to mess up its values, it is the problem of the code's user, you don't have to protect your VM from this.

In your controller, the constructor's signature doesn't fit the controller's name, this doesn't compile.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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