Unity Repository Best Practices

I am now developing a product, which will use Unity & Repositories when it comes to data management. I will demonstrate on a simple example. What we want to do, is to create an app with calendar control, where user (e.g. manager) will be able to store holidays for his employees.

First define our models:

We have a class Employee which looks like following:

```
public class Employee :ViewModelBase, IEditableObject
{
public int EmployeeID { get; set; }

public string FullName
{
get
{
return $"{this.Name} {this.Surname}";
}
}

private string _Name;
public string Name
{
get { return _Name; }
set
{
_Name = value;
OnPropertyChanged("Name");
}
}

private string _Surname;
public string Surname
{
get { return _Surname; }
set
{
_Surname = value;
OnPropertyChanged("Surname");
}
}

private DateTime _DateOfBirth;
public DateTime DateOfBirth
{
get
{
if (_DateOfBirth == DateTime.MinValue)
_DateOfBirth = DateTime.Now;
return _DateOfBirth;
}
set
{
_DateOfBirth = value;
OnPropertyChanged("DateOfBirth");
}
}
//hierachy level
private int _EmployeeLevelID;
public int EmployeeLevelID
{
get { return _EmployeeLevelID; }
set
{
_EmployeeLevelID = value;
OnPropertyChanged("EmployeeLevelID");
}
}

private ObservableCollection _Holidays;
public ObservableCollection Holidays
{
get
{
if (_Holidays == null)
_Holidays = new ObservableCollection();
return _Holidays;
}
set
{
_Holidays = value;
OnPropertyChanged("Holidays");
}
}
}

public class Holidays : ViewModelBase, IClonea

Your Employee class is a ViewModel, concerned with your application's business-level logic: it's much more than a DTO/POCO class, and using it as such sounds very much like mixing up data and business concerns.

That said...

OnPropertyChanged("Surname");

I'm surprised of seeing literal strings in C# 6.0 code, when nameof is available to make your code refactoring-friendly and strongly-typed:

OnPropertyChanged(nameof(Surname));

Your private fields are not following the naming conventions; they should be _camelCase, not _PascalCase. I like the underscore prefix though, because it eliminates the need to use this as a qualifier.

I'm also a bit surprised to see IClonable implemented in C# 6.0 code, since that interface has been controversial for just about as long as it has existed.

From MSDN:

Because callers of Clone cannot depend on the method performing a predictable cloning operation, we recommend that ICloneable not be implemented in public APIs.

Besides, why clutter up client code with an object when you could just implement a copy constructor?

public Holiday(Holiday existing)
{
    Start = existing.Start;
    End = existing.End;
    Reason = existing.Reason;
    Description = existing.Description;
}

Notice I renamed the type Holiday - you should keep pluralized type names for IEnumerable things (collections, lists, dictionaries, etc.). If I write code against your API, I expect Holidays to represent a bunch of Holiday objects.

Back to higher-level architecture. I think what you're calling your repository is actually just a service that's sitting between your actual data access and your business logic - and IDataManager (really? out of all names?) is your actual repository. And its responsibilities are mixed, too; you can tell just by looking at the interface - here, I've rearranged the members to illustrate:

public interface IDataManager
{
    ObservableCollection LoadEmployees();
    ObservableCollection LoadEmployeeLevel();

    void SaveEmployees(ObservableCollection employees);
    void SaveEmployeeLevels(ObservableCollection levels);

    void DeleteEmployee(Employee person);     
    void DeleteEmployeeLevel(EmployeeLevel level);
}

An interface shouldn't be designed to change. What happens when (note: not if) you need a new entity type? You need to change the interface and modify all implementations (of which, I suspect there's only one, right?). It's literally screaming:

"My cohesion! My cohesion is suffering! My cooooheeeesiooooon!!!"

A more cohesive interface would look like this:

public interface IDataManager
{
    IEnumerable Load();
    void Save(IEnumerable entities);
    void Delete(T entity);     
}

Notice the coupling just went down as well: the interface is no longer tied to any specific concrete types.

High cohesion, low coupling: that's what you should be striving for.

I can't help but notice the similarities:

public interface IRepository
{
    IEnumerable GetAll();
    void Insert(T entity);
    void Update(T entity);
    void Delete(T entity);     
}

That's why I said earlier:

[...] and IDataManager (really? out of all names?) is your actual repository.

– Mat's Mug 5 minutes ago

I would suggest you get your ViewModel classes out of the data layer, and introduce a set of entity classes for your data layer to work with: then your service layer can "translate" entities into view models (and vice-versa) for the presentation layer to play with.

StackExchange Code Review Q#121966, answer score: 3