current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite
1

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.
share|improve this question

2 Answers 2

up vote 5 down vote accepted

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.

share|improve this answer
    
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. –  Caster Troy Aug 6 '14 at 9:56
up vote 2 down vote

  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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.