Me and my colleague had recently a discussion on good practices in EF. So i show one of mine.
He said it is a little bit muddler.
My practice consist in modify the autogenerated class in a specific way. This is my starting model:
namespace PCServer.Data
{
using System;
using System.Collections.Generic;
public partial class Post: IEntity
{
public int Id { get; set; }
public string Title { get; set; }
public string Message { get; set; }
public DateTime Date { get; set; }
public virtual Post ParentPost { get; set; }
public virtual AspNetUser Author { get; set; }
}
}
I like to extend it in this way:
public partial class Post
{
//This class "do" something, like adding a post or deleting a post
public static class Do
{
public static void AddPost(ref ApplicationDbContext context, string postMessage)
{
//Create a post
Post p = new Post();
p.Title = "This is an example!";
p.message = postMessage;
p.Date = DateTime.UtcNow;
//Adding to context
BaseService.Add(post, out context);
}
public static void DeletePost(ref ApplicationDbContext context, int postId)
{
PostRepository postRepo = new PostRepository(context);
postRepo.GetById(postId);
//Removing from context
BaseService.Remove(post, out context);
}
}
//This class "Get" something, like all posts
public static class Get
{
public static void GetPosts()
{
using(ApplicationDbContext context = new ApplicationDbContext())
{
PostRepository postRepo = new PostRepository(context);
return postRepo.GetAllPosts();
}
}
}
//This class "Set" something, like title of the post or the post itself maybe
public static class Set
{
public static void Title(ref ApplicationDbContext context, int postId, string title)
{
PostRepository postRepo = new PostRepository(context);
Post post = postRepo.GetById(postId);
post.Title = title;
BaseService.Update(post, out context);
}
public static void ChangePost(ref ApplicationDbContext context, int postId, Post post)
{
PostRepository postRepo = new PostRepository(context);
Post dbPost = postRepo.GetById(postId);
dbPost = post;
BaseService.Update(dbPost, out context);
}
}
}
So, when i must to do something with an Entity, i can (for example only):
ApplicationDbContext c = new ApplicationDbContext();
Post.Do.AddPost(ref c,"Hi!");
IEnumerable<Post> posts = Post.Get.GetPosts();
Post.Set.Title(ref c,100,"Changing title!");
And after all:
await BaseService.CommitAsync<Post>(c);
SO. What do you think? Would you use it? Why?
Thank you and sorry for long post.
One of the advantages of any design pattern is that of comprehensibility to others. Me: How are you designing your Web Services? You: We're using the repository pattern. Me: Ok, I understand that.
Whilst this is by no means a comprehensive answer, it goes a long way to understanding where to look for things and where things need changing when you're asked to make an update.
What you have is a design pattern that you like to use, but you should ask yourself whether it is easily comprehensible by others.
For example, you have a class named "Do". As a name, this gives no indication whatsoever as to what the class is for. Your comment indicates that it "does something", but is anyone else going to intuitively understand what goes in there? Adding and Deleting are obviously "doing" things, but why isn't Getting or Setting? This may make sense to you, but others may see things differently.
I'm assuming you are working in a team. If your team is going to adopt this as a pattern, then everyone needs to agree to use it, and to do that, everyone needs to understand intuitively how it works, and more importantly, how to extend it. If they don't, then you will start getting methods in your Do class that you don't think should be there, and you will be putting methods in other classes that other people can't find because they don't have the same mental model as you as to how this works. It does not take long for things to get messy if everyone is doing things differently.
With respect to extending Entities with partial classes, I would avoid this myself. Entities have a clearly defined responsibility, and by adding code to them, you are assigning extra responsibilities (c.f. the Single Responsibility Principle). Admittedly, your extra code is all separated into separate static sub classes, but if that is the case, why not simply make them classes in their own right and move them somewhere else in your application?
In terms of the classes themselves, static classes with static methods are going to make unit testing extremely difficult, and there's no reason why they should be static, so I would definitely change that. Of course now you have to instantiate them to use them, which presents more of an argument for moving them somewhere else.
On a more technical point, what is BaseService doing? You are creating a context, passing it by reference (why?) to AddPost (for example), and then using it as an out parameter in BaseService.Add. I'm not sure what's going on here, but it looks wrong.
As a pattern, I would say it needs work, but simply by attempting to have a pattern, you are heading in the right direction.
You haven't mentioned what your colleague thinks is 'good practice' but I suggest that you continue discussing this until you agree. Decide what you are trying to achieve, and what pattern best fits that. Maybe there is an established pattern that fits, but try not to overengineer it. My experience tells me that the more I try to abstract away from Entity Framework, the harder it can be to use some of it's more powerful features.