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