1
\$\begingroup\$

I am writing a program that updates bonds in an external system that I have database access to. The program is run in an 8-minute interval.

Bonds are stored in a table called Debentur. Bonds need to be calculated if they are new or it's calculation method is changed. The calculation method is stored in the column CalledIn with possible values 1 (active) and null (not active).

  1. How do you like my method of finding what is changed?
  2. In my opinion it is not obvious what the class CalculateClaimsBondsChangedCommand is not doing. It is finding changes, updating them and then inserting the changes so they won't be updated in the next update. Do you have any suggestions for improvement?
  3. My repository DebenturChangesRepository has a lot of code duplication which I have not yet refactored, how would you refactor it?
  4. Is there anything else that could be improved?

public class CalculateClaimsBondsChangedCommand : ICalculateCommand
{
    private readonly IDebenturChangesRepository debenturChangesRepository;
    private readonly IClaimsCalculatorService claimsCalculatorService;

    public CalculateClaimsBondsChangedCommand(
        IDebenturChangesRepository debenturChangesRepository,
        IClaimsCalculatorService claimsCalculatorService)
    {
        if (debenturChangesRepository == null) throw new ArgumentNullException("debenturChangesRepository");
        if (claimsCalculatorService == null) throw new ArgumentNullException("claimsCalculatorService");
        this.debenturChangesRepository = debenturChangesRepository;
        this.claimsCalculatorService = claimsCalculatorService;
    }

    public void Execute()
    {
        var newDebenturs = debenturChangesRepository.CreateForNewDebenturs();
        var changedDebenturs = debenturChangesRepository.CreateForChangedCalledIn();

        claimsCalculatorService.CalculateClaims(newDebenturs.Concat(changedDebenturs).Select(d => d.ClaimNo));

        foreach (var debentur in newDebenturs)
        {
            debenturChangesRepository.InsertDebenturChange(debentur);
        }
        foreach (var debentur in changedDebenturs)
        {
            debenturChangesRepository.UpdateDebenturChange(debentur);
        }
    }
}

public class DebenturChangesRepository : IDebenturChangesRepository
{
    private readonly string connectionString;

    public DebenturChangesRepository(string connectionString)
    {
        if (connectionString == null) throw new ArgumentNullException("connectionString");
        this.connectionString = connectionString;
    }

    public List<DebenturChange> CreateForNewDebenturs()
    {
        var sql = "Select ClaimNo, CalledIn " +
            "From Debentur " +
            "Where ClaimNo Not In (Select ClaimNo From DebenturChange) ";

        using (var connection = new SqlConnection(connectionString))
        {
            connection.Open();

            var command = connection.CreateCommand();
            command.CommandText = sql;

            using (var reader = command.ExecuteReader())
            {
                return GetDebentursFromReader(reader);
            }
        }
    }

    public List<DebenturChange> CreateForChangedCalledIn()
    {
        var sql = "Select d.ClaimNo, d.CalledIn " +
            "From Debentur d join DebenturChange c on d.ClaimNo = c.ClaimNo " +
            "Where IsNull(d.CalledIn, 0) <> IsNull(c.CalledIn, 0) ";

        using (var connection = new SqlConnection(connectionString))
        {
            connection.Open();

            var command = connection.CreateCommand();
            command.CommandText = sql;

            using (var reader = command.ExecuteReader())
            {
                return GetDebentursFromReader(reader);
            }
        }
    }

    private List<DebenturChange> GetDebentursFromReader(IDataReader reader)
    {
        var debenturs = new List<DebenturChange>();
        while (reader.Read())
        {
            debenturs.Add(ParseDebenturChange(reader));
        }

        return debenturs;
    }

    private DebenturChange ParseDebenturChange(IDataReader reader)
    {
        return new DebenturChange()
        {
            ClaimNo = (string)reader["ClaimNo"],
            CalledIn = ParseColumnToNullableInt(reader["CalledIn"])
        };
    }

    private int? ParseColumnToNullableInt(object o)
    {
        if (o == DBNull.Value) return null;
        return Convert.ToInt32(o);
    }

    public void InsertDebenturChange(DebenturChange debenturChange)
    {
        var sql = "Insert Into DebenturChange (ClaimNo, CalledIn) " +
            "Values (@ClaimNo, @CalledIn)";

        using (var connection = new SqlConnection(connectionString))
        {
            connection.Open();

            var command = connection.CreateCommand();
            command.CommandText = sql;
            command.Parameters.AddWithValue("ClaimNo", debenturChange.ClaimNo);
            command.Parameters.AddWithValue("CalledIn", ParseNullableIntToColumn(debenturChange.CalledIn));

            command.ExecuteNonQuery();
        }
    }

    public void UpdateDebenturChange(DebenturChange debenturChange)
    {
        var sql = "Update DebenturChange " +
            "Set CalledIn = @CalledIn " +
            "Where ClaimNo = @ClaimNo";

        using (var connection = new SqlConnection(connectionString))
        {
            connection.Open();

            var command = connection.CreateCommand();
            command.CommandText = sql;
            command.Parameters.AddWithValue("ClaimNo", debenturChange.ClaimNo);
            command.Parameters.AddWithValue("CalledIn", ParseNullableIntToColumn(debenturChange.CalledIn));

            command.ExecuteNonQuery();
        }
    }

    private object ParseNullableIntToColumn(int? value)
    {
        if (value == null) return DBNull.Value;
        return value.Value;
    }
}
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

Why aren't you encapsulating your SqlCommands in using statements?


My main problem is that you'll need to maintain a class that's currently 100+ lines long for a few simple actions that an ORM like NHibernate can easily handle in far less code. Sure, your "WHERE xxx NOT IN" would require a named query, but that's still not that much extra effort.

Based on your code right now using an ORM is looking like overkill. But I doubt the above will remain the only functionality. Suppose you'll need to maintain additional tables: that would require you to add another 100+ line class featuring much of the same functionality, so you copy-paste the existing class and change things... But then you feel that perhaps you can make this more abstract and pretty soon you're writing your own ORM which you really should not do.

Imagine how much code you'll need to change if Debentur will get an additional field.

Using Dapper could already reduce your code, although I suppose it isn't that useful if you need to do INSERTs and UPDATEs -- though that's possible of course.

\$\endgroup\$
-1
\$\begingroup\$

I made some changes after suggestions from @BCdotWEB

For the class: DebenturChangesRepository I changed the name of CreateForChangedCalledIn to CreateForNewDebenturs so that if I need to monitor additional fields than I only need to change my repository.

I change the sql to use Dapper which shortens the code by aprox 50%!.

Here the new code:

public class DebenturChangesRepository : IDebenturChangesRepository
{
    private readonly string connectionString;

    public DebenturChangesRepository(string connectionString)
    {
        if (connectionString == null) throw new ArgumentNullException("connectionString");
        this.connectionString = connectionString;
    }

    public List<DebenturChange> CreateForNewDebenturs()
    {
        var sql = "Select ClaimNo, CalledIn " +
            "From Debentur " +
            "Where ClaimNo Not In (Select ClaimNo From DebenturChange) ";

        using (var connection = new SqlConnection(connectionString))
        {
            return connection.Query<DebenturChange>(sql).ToList();
        }
    }

    public List<DebenturChange> CreateForChangedDebenturs()
    {
        var sql = "Select d.ClaimNo, d.CalledIn " +
            "From Debentur d join DebenturChange c on d.ClaimNo = c.ClaimNo " +
            "Where IsNull(d.CalledIn, 0) <> IsNull(c.CalledIn, 0) ";

        using (var connection = new SqlConnection(connectionString))
        {
            return connection.Query<DebenturChange>(sql).ToList();
        }
    }

    public void InsertDebenturChange(DebenturChange debenturChange)
    {
        var sql = "Insert Into DebenturChange (ClaimNo, CalledIn) " +
            "Values (@ClaimNo, @CalledIn)";

        using (var connection = new SqlConnection(connectionString))
        {
            connection.Execute(sql, debenturChange);
        }
    }

    public void UpdateDebenturChange(DebenturChange debenturChange)
    {
        var sql = "Update DebenturChange " +
            "Set CalledIn = @CalledIn " +
            "Where ClaimNo = @ClaimNo";

        using (var connection = new SqlConnection(connectionString))
        {
            connection.Execute(sql, debenturChange);
        }
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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