Does this unusual data access pattern create any problems?

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

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.

StackExchange Code Review Q#31543, answer score: 7