Simple Repository Pattern without ORM and Unit Tests

Question

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

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; }
}

Answer 1

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.

Answer 2

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.