Repository implementation

Question

I have a repository called PostsRepository:

public class PostsRepository : IPostsRepository
{
 private readonly DatabaseContext context = new DatabaseContext();
 public IEnumerable<Post> All()
 {
 return
 context.Posts
 .OrderBy(post => post.PublishedAt);
 }
 public IEnumerable<Post> AllPublishedPosts()
 {
 return
 context.Posts
 .OrderBy(post => post.PublishedAt)
 .Where(post => !post.Draft);
 }
 public Post Find(string slug)
 {
 return context.Posts.Find(slug);
 }
 public void Create(Post post)
 {
 post.Slug = SlugConverter.Convert(post.Slug);
 post.Summary = Summarize(post.Content);
 post.PublishedAt = DateTime.Now;
 AttachTags(post);
 if (context.Posts.Find(post.Slug) != null)
 {
 throw new Exception("tag already exists. choose another.");
 }
 context.Posts.Add(post);
 context.SaveChanges();
 }
 public void Update(Post post)
 {
 post.Slug = SlugConverter.Convert(post.Slug);
 post.Summary = Summarize(post.Content);
 AttachTags(post);
 if (context.Posts.Find(post.Slug) != null)
 {
 throw new Exception("tag already exists. choose another.");
 }
 context.Posts.Add(post);
 context.SaveChanges();
 }
 public void Delete(Post post)
 {
 context.Posts.Remove(post);
 context.SaveChanges();
 }
 private void AttachTags(Post post)
 {
 foreach (var tag in post.Tags)
 {
 if (context.Tags.Any(x => x.Name == tag.Name))
 {
 context.Tags.Attach(tag);
 }
 }
 }
 private static string Summarize(string content)
 {
 // contrived.
 return content;
 }
}

I am worried that I might have wound up with a design that is not very testable as it is not apparent to me how to test this code.

I am going to ask another question on SO as to how to unit test this class soon but before I peruse this implementation I would like to ask that you please review my repository implementation.

Particular areas of concern:

• Testability
• I have read countless opinions about what a repository should do. Are there any pragmatic reasons why my implementation might be bad?
• The PostsRepository needs to access the Tags database set. Am I allowing this in the correct way? Know that I plan to implement a TagsRepository in the future.
• I throw an exception when the slug (which must be unique) is occupied. Should I return a bool to indicate failure instead? Would this not violate the command-query segregation principle?
• I am aware that the Update method is hard to reason about and I am working on that. It is for this reason that my code does not currently adhere to DRY.

Answer 1

Testability

Your repository implements an interface which will allow it to get stubbed out easily, so that's a very testable thing. However if you want to also (unit-test) your repositories themselves then you are stuck because you have a hardcoded dependency on DatabaseContext.

You should move that up one layer by abstracting it out as well and providing a custom DbContext for testing purposes or by mocking it out. More information on that here.

I have read countless opinions about what a repository should do. Are there any pragmatic reasons why my implementation might be bad?

One thing that comes to mind is the long-lived DbContext object. Some say it isn't an exact necessity, others say you definitely should avoid it but generally accepted is the notion that you should dispose of your DbContext. A unit of work corresponds to one method inside your repository to wrap a using statement around it.

Note that this would interfere with testing the repository itself so it's worth considering using integration-tests to test your repository in particular.

The PostsRepository needs to access the Tags database set. Am I allowing this in the correct way? Know that I plan to implement a TagsRepository in the future.

I am not familiar enough with EF to answer that.

I throw an exception when the slug (which must be unique) is occupied. Should I return a bool to indicate failure instead? Would this not violate the command-query segregation principle?

It does not violate it because it is just the result of your command, it is not the result of a query. It holds exactly the same value as an exception.

There is a more interesting way though: create a "result"-object. This can be as simple as

class CallResult {
 bool Success { get; set; }
 string Message { get; set; }
}

Which will be more descriptive than bool since you can also send a message along. Whether you choose this or an exception depends on your own preference: using an exception will force you to have a try-catch around it somewhere which is rather ugly but then again it will prevent you from accidentally leaving out validation on the return type (either a try-catch or if(result.Success)) which is not enforced with a custom type.

I am aware that the Edit method is hard to reason about and I am working on that. It is for this reason that my code does not currently adhere to DRY.

Since I don't see an Edit method I'll assume you haven't written it yet. DRY is nice and all but for some niche situations (CRUD actions, tests, ..) I would argue that readability is more important than having a few lines similar to eachother.

I suppose you could create a situation like this, but that depends on how different each action is.

void Process(Post post) {
 post.Slug = SlugConverter.Convert(post.Slug);
 if (context.Posts.Find(post.Slug) != null)
 {
 throw new Exception("tag already exists. choose another.");
 }
 post.Summary = Summarize(post.Content);
 AttachTags(post);
}
void Create(Post post) {
 Process(post);
 post.PublishedAt = DateTime.Now;
 context.Posts.Add(post);
 context.SaveChanges();
}
void Update(Post post) {
 Process(post);
 context.Posts.Add(post);
 context.SaveChanges();
}

Note how I performed the validation before doing the other work.

---

A method should be written as [action][context]. AllPublishedPosts makes me think it's a property, not a method. I would change this to GetAllPublishedPosts. I'm not speaking out about All since that's a special situation, I suppose.

• Caster Troy: You make some good points. I completely forgot to give consideration to the lifetime of the database context for example. And my apologies. I meant to say that the Update method is hard to reason about - there is no method called Edit. I have updated my answer.

Answer 2

1. One thing I noticed is that you have a fair amount of code duplication. It's something I try to avoid because it means if you find a problem in a piece of code which is duplicated then you have to go and them all and fix them. If you just have one place where you perform a certain operation then there is only one place to fix/extend.

   Some examples:

   • Shared code between All and AllPublishedPosts. Can be refactored into:

     public IEnumerable<Post> All()
     {
      return context.Posts.OrderBy(post => post.PublishedAt);
     }
     public IEnumerable<Post> AllPublishedPosts()
     {
      return All().Where(post => !post.Draft);
     }
     

   • Create and Update are pretty much exactly the same method except for a single line. I'd refactor it into:

     public void Create(Post post)
     {
      CreateOrUpdate(post, true);
     }
     public void Update(Post post)
     {
      CreateOrUpdate(post, false);
     }
     private void CreateOrUpdate(Post post, bool isNewPost)
     {
      post.Slug = SlugConverter.Convert(post.Slug);
      post.Summary = Summarize(post.Content);
      if (isNewPost)
      {
      post.PublishedAt = DateTime.Now;
      }
      AttachTags(post);
      if (context.Posts.Find(post.Slug) != null)
      {
      throw new Exception("tag already exists. choose another.");
      }
      context.Posts.Add(post);
      context.SaveChanges();
     }
     

   • You already have a Find method but you duplicate the code in Create and Update. Use Find there instead.

2. Avoid throwing generic Exception - be specific. In Create and Update it would probably make more sense to throw an ArgumentException.

3. You should try to bail early. Right now in Create and Update you perform a bunch of operations just to potentially reject the post later. Something like this would seem to make more sense:

    private void CreateOrUpdate(Post post, bool isNewPost)
    {
    post.Slug = SlugConverter.Convert(post.Slug);
    if (Find(post.Slug) != null)
    {
    throw new ArgumentException("tag already exists. choose another.");
    }
    post.Summary = Summarize(post.Content);
    if (isNewPost)
    {
    post.PublishedAt = DateTime.Now;
    }
    AttachTags(post);
    context.Posts.Add(post);
    context.SaveChanges();
    }
   

4. DatabaseContext should probably be an interface so you can inject it through the repository constructor. For unit testing you can mock out the interface. If the repository is a transient entity (i.e. constructed per web-request for example) then injecting the context through the constructor is probably the most straight forward way. If the repository is longer lived (like a singleton lifestyle) then you might want to consider the unit of work pattern in order to limit the lifetime of your context.