Repository implementation

I have a repository called PostsRepository:

public class PostsRepository : IPostsRepository
{
    private readonly DatabaseContext context = new DatabaseContext();

    public IEnumerable All()
    {
        return
            context.Posts
                   .OrderBy(post => post.PublishedAt);
    }

    public IEnumerable 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 thi

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.

StackExchange Code Review Q#59210, answer score: 5