3
\$\begingroup\$

I'm trying to implement the repository pattern in the following manner:

Interface:

public interface ISurveyRepository
{
    Survey Add(Survey survey);
    IEnumerable<Survey> GetAll();
    Survey GetById(int id);
    bool Update(Survey survey);
    bool Delete(int id);
}

Implementation:

public class SurveyRepository : ISurveyRepository
{
    private readonly ISurveyRepository _dbContext;

    public SurveyRepository(ISurveyRepository dbContext)
    {
        _dbContext = dbContext;
    }

    public Survey Add(Survey survey)
    {
        return _dbContext.Add(survey);
    }

    public IEnumerable<Survey> GetAll()
    {
        return _dbContext.GetAll();
    }

    public Survey GetById(int id)
    {
        return _dbContext.GetById(id);
    }

    public bool Update(Survey survey)
    {
        return _dbContext.Update(survey);
    }

    public bool Delete(int id)
    {
        return _dbContext.Delete(id);
    }
}

And here are the creation of 2 different contexts, this could be oracle database, sql server or whathever, each one of those with a separate Context class

public class SurveyMemoryContext : ISurveyRepository
{
    private List<Survey> _surveys;

    public SurveyMemoryContext()
    {
        _surveys = new List<Survey>();
    }

    public Survey Add(Survey survey)
    {
        _surveys.Add(survey);
        return survey;
    }

    public IEnumerable<Survey> GetAll()
    {
        return _surveys;
    }

    public Survey GetById(int id)
    {
        return _surveys.Find(p => p.Id == id);
    }

    public bool Update(Survey survey)
    {
        var updatedSurvey = _surveys.Find(p => p.Id == survey.Id);

        if (updatedSurvey == null) return false;

        _surveys.Remove(updatedSurvey);
        updatedSurvey.Id = survey.Id;
        updatedSurvey.Name = survey.Name;
        updatedSurvey.Description = survey.Description;

        _surveys.Add(updatedSurvey);
        return true;
    }

    public bool Delete(int id)
    {
        var deletingSurvey = _surveys.Find(p => p.Id == id);
        _surveys.Remove(deletingSurvey);
        return true;
    }
}

Oracle Database context:

public class SurveyDbContext : ISurveyRepository, IDisposable
{
    private IDbConnection _connection;
    private IDbCommand _command;
    private IDbTransaction _transaction;

    public SurveyDbContext()
    {
        _connection = new OracleConnection(Properties.Settings.Default.OracleConnectionString);

        if (_connection.State != ConnectionState.Open)
            _connection.Open();

        _command = _connection.CreateCommand();
    }

    public Survey Add(Survey survey)
    {
        try
        {
            using (_command = _connection.CreateCommand())
            {
                _transaction = _connection.BeginTransaction();

                _command.CommandText = "INSERT INTO PREFIX_TABLE " +
                                       " (Id, Name, Description) " +
                                       " VALUES(:Id, :Name, :Description)";

                _command.AddOracleParameter(":Id", survey.Id);
                _command.AddOracleParameter(":Name", survey.Name);
                _command.AddOracleParameter(":Description", survey.Description);

                _command.ExecuteNonQuery();

                _transaction.Commit();
            }
        }
        catch (Exception ex)
        {
            _transaction.Rollback();
            ex.Log();
            throw;
        }

        return survey;
    }

    public IEnumerable<Survey> GetAll()
    {
        throw new System.NotImplementedException();
    }

    public Survey GetById(int id)
    {
        throw new System.NotImplementedException();
    }

    public bool Update(Survey survey)
    {
        throw new System.NotImplementedException();
    }

    public bool Delete(int id)
    {
        throw new System.NotImplementedException();
    }

    private void Dispose(bool isDisposing)
    {
        if (isDisposing)
        {
            if (_connection != null)
            {
                _connection.Close();
                _connection.Dispose();
                _connection = null;
            }
        }
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
}

Finally, the methods are called like that (in unit testing only at the moment)

[TestClass]
public class SurveyContextTest
{
    private SurveyRepository _repository;

    [TestInitialize]
    public void TestInitialize()
    {
        //_repository = new SurveyRepository(new SurveyDbContext());
        _repository = new SurveyRepository(new SurveyMemoryContext());
    }

    [TestCleanup]
    public void TestCleanup()
    {
        _repository = null;
    }

    [TestMethod]
    public void AddNewSurveyReturnsInsertedSurvey()
    {
        // Arrange
        var survey = new Survey
        {
            Id = 100,
            Name = "Name",
            Description = "Description"
        };

        // Act
        var insertedSurvey = _repository.Add(survey);

        // Assert
        Assert.AreEqual(survey, insertedSurvey);
    }

    [TestMethod]
    public void UpdateSurveyReturnsTrue()
    {
        // Arrange
        var survey = new Survey
        {
            Id = 100,
            Name = "Name",
            Description = "Description"
        };

        var insertedSurvey = _repository.Add(survey);

        insertedSurvey.Id = 110;
        insertedSurvey.Name = "Name 2";
        insertedSurvey.Description = "Description 2";

        // Act
        var updatedSurvey = _repository.Update(insertedSurvey);

        // Assert
        Assert.IsTrue(updatedSurvey);
    }

    [TestMethod]
    public void GetAllReturnsAllTheSurveys()
    {
        // Arrange
        CreateMockSurveysInRepository(5);

        // Act
        var insertedSurveys = _repository.GetAll();

        // Assert
        Assert.IsTrue(insertedSurveys.Count() == 5);
    }

    [TestMethod]
    public void GetByIdReturnsTheExistingSurvey()
    {
        // Arrange
        CreateMockSurveysInRepository(5);

        // Act
        var insertedSurvey = _repository.GetById(101);

        // Assert
        Assert.IsNotNull(insertedSurvey);
    }

    [TestMethod]
    public void DeleteExistingSurveyReturnsTrue()
    {
        // Arrange
        CreateMockSurveysInRepository(5);

        // Act
        var isDeleted = _repository.Delete(101);

        // Assert
        Assert.IsTrue(isDeleted);
    }

    private void CreateMockSurveysInRepository(int number)
    {
        for (int i = 0; i < number; i++)
        {
            var survey = new Survey
            {
                Id = 100 + i,
                Name = "Name " + i,
                Description = "Description " + i,
            };

            _repository.Add(survey);
        }
    }
}

Here is the schema for clarity

solution

Is this a good implementation of the repository pattern? The _transaction in the database implementation shall be intended as the Unit of Work. Unit tests on the db could be done by creating a separate class (SurveyMockDbContext) that does everything as the normal DbContext except it always rolls back everything. Is this a principle of Dependency Injection, since the constructor of the SurveyRepository class takes in an interface to perform operations, without actually knowing who's really acting?

The model of course is just a simple poco

public class Survey
{
    public int Id { get; set; }
    public string Name { get; set; }
    public string Description { get; set; }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ What's the // test code? Why even bother include it in the post if you don't want it reviewed? What's the // command code actually doing? A meaningful peer review requires that we review your actual code, not just its skeleton. Also... isn't the interface missing something to select surveys in some way? "code simplified for readability" isn't helping you on this site. Please include your actual code. \$\endgroup\$ – Mathieu Guindon Jun 20 '16 at 16:26
  • \$\begingroup\$ The linter that's built into Visual Studio will tell you that you've not implemented IDisposable properly. See that "Analyze" button on the tool bar? It'll spot all kinds of things. \$\endgroup\$ – RubberDuck Jun 21 '16 at 1:36
  • \$\begingroup\$ @Mat'sMug please check my updated answer \$\endgroup\$ – Alexander D Jun 21 '16 at 7:50
  • \$\begingroup\$ What is the class SurveyRepository for? It looks like a decorator that doesn't add functionallity... Therefore you can drop it. \$\endgroup\$ – JanDotNet Jul 22 '16 at 11:38
1
\$\begingroup\$

SurveyMemoryContext

Is this actually production code, or does it belong in your Test project because it allows you to run tests without hitting an actual database? Either way, I'd consider changing it to a Dictionary<int, Survey>, keyed on the Survey.Id, since Id is your main search criteria.

What happens if an invalid Id is passed into delete? It seems like it should at least return the result of _surveys.Remove(deletingSurvey).

public bool Delete(int id)
{
    var deletingSurvey = _surveys.Find(p => p.Id == id);
    _surveys.Remove(deletingSurvey);
    return true;
}

Unit Tests

Your unit tests feel a bit incomplete. A lot of them don't really seem to test very much. For example DeleteExistingSurveyReturnsTrue could be made to pass by stubbing SurveyRepository.Delete to always return true. There's no counter test to validate that passing a non-present Survey would return false (or throw an exception or ...).

The test GetByIdReturnsTheExistingSurvey simply checks that GetById returns a non null Survey. It doesn't check that the right survey has been returned (at a minimum I would expect it to check that the Id of the survey requested matches that of the survey returned).

When you're writing your tests consider what the minimal implementation needed to make all of your tests pass is. If you could get away with having a series of return true, return new Survey etc methods then you might want to rethink the value of the tests.

SurveyDbContext

In your constructor you're creating a new DbCommand:

_command = _connection.CreateCommand();

However, you're doing the same thing in your Add method:

using (_command = _connection.CreateCommand())

It seems like _command should really be a local variable, rather than a class member, unless you've got another use case you haven't implemented yet that requires it at the class level.

You only check if the connection is open in your constructor:

    if (_connection.State != ConnectionState.Open)
        _connection.Open();

This will probably be ok, depending on how long lived your context is likely to be. If your DbContext is going to be a long lasting instance, then it's worth considering how you want your application to behave / recover if the database connection is closed.

\$\endgroup\$
  • \$\begingroup\$ Thanks, my tests are very basic because it is the very first time doing these for me and I am experimenting. Is the repository pattern correctly implemented? Is it right to use it like I am doing in TestInitialize()? And yes, you're right, the MemoryContext shall be moved to the Test project \$\endgroup\$ – Alexander D Jun 22 '16 at 7:52
  • \$\begingroup\$ @AlexanderD It seems like a reasonable implementation of the repository pattern, although I tend to use a different pattern for wrapping my Data access, so you might want a second opinion. The TestInitialize looks fine. \$\endgroup\$ – forsvarir Jun 22 '16 at 8:31
  • \$\begingroup\$ The connection lost is something I've yet to think about, meanwhile I recently updated the using statements to include (_connection.CreateCommand()) because otherwise when performing 2 distinct operations on the db the second operation would throw and error about the _command which would have all the parameters of the first execution. I have to remove it from the constructor, indeed \$\endgroup\$ – Alexander D Jun 22 '16 at 10:44
0
\$\begingroup\$

Is it really necessary to support different data store systems? I would not suggest to use Survey as an EF entity type as it could quickly become a headache while working with different data APIs or when data structure becomes more realistic than just a simple object. I would also split ISurveyRepository to two different interfaces as they implementation have almoust nothing shared:

public interface ISurveyReader
{
    IEnumerable<Survey> ReadAll();
    Survey Read(int id);
}

public interface ISurveyWriter
{
    void Write(Survey survey);
    void Delete(int id);
}

Let’s say for example that our Survey has Questions:

public class Survey
{
    public int Id { get; set; }
    public string Name { get; set; }
    public IList<string> Questions { get; set; }
}

Then:

class SurveyWriter : ISurveyWriter
{
    public void Write(Survey survey)
    {
        var ctx = new EContext();
        var eSurvey = ctx.Surveys
            .Include(e => e.Questions)
            .FirstOrDefault(e => e.Id == survey.Id);

        if (eSurvey == null)
            eSurvey = ctx.Surveys.Create();

        eSurvey.Name = survey.Name;            
        ctx.Questions.RemoveRange(eSurvey.Questions.ToArray());
        eSurvey.Questions.AddRange(from q in survey.Questions
                                   select new EQuestion { Text = q });

        ctx.SaveChanges();
        survey.Id = eSurvey.Id;
    }

    public void Delete(int id)
    {
        var ctx = new EContext();
        var eSurvey = ctx.Surveys
            .Include(e => e.Questions)
            .FirstOrDefault(e => e.Id == id);

        ctx.Surveys.Remove(eSurvey);
        if (eSurvey != null)
            ctx.Questions.RemoveRange(eSurvey.Questions.ToArray());

        ctx.SaveChanges();
    }
}

Where:

class ESurvey
{
    public int Id { get; set; }
    public string Name { get; set; }
    public virtual List<EQuestion> Questions { get; set; } = new List<EQuestion>();
}

class EQuestion
{
    public int Id { get; set; }
    public string Text { get; set; }
}

class EContext : DbContext
{
    public DbSet<ESurvey> Surveys { get; set; }
    public DbSet<EQuestion> Questions { get; set; }
}

P.S. I would suggest to have a look at SRP, Identifing Relationship and Cascade Delete.

\$\endgroup\$
  • \$\begingroup\$ You're right, dividing the ISurveyRepository would follow the CQSRP, I'll do that. I might have confused you using the word DbContext in SurveyDbContext, but it is in no way related to Entity Framework, in contrary, I don't want to use any kind of ORM. I suppose the code in the SurveyWriter is EF related and not valid with raw sql data store mechanism, since it wouldn't derive from EF's DbContext. I'll check those links asap. Indeed, the Surveys will have a list of Questions for sure \$\endgroup\$ – Alexander D Jun 22 '16 at 10:37
  • \$\begingroup\$ @AlexanderD Yep, sorry, it mostly related to EF. I should not do it at 3:00 AM :) \$\endgroup\$ – Dmitry Nogin Jun 22 '16 at 15:58
  • \$\begingroup\$ However, how would you recommend to implement the SurveyWriter and SurveyReader? Shall it be implemented as a SurveyRepository : ISurveyReader, ISurveyWriter ? \$\endgroup\$ – Alexander D Jun 23 '16 at 7:50
  • \$\begingroup\$ They have almost nothing shared in terms of implementation details - there is no any dependency of reader methods on writer methods and vice versa. The same time, I found myself implementing Composite pattern for the writer - to stream updates to multiple targets the same time. Actually, I use this component to create repository implementation as reader and writer might be consumed altogether. \$\endgroup\$ – Dmitry Nogin Jun 23 '16 at 8:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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