I use NHibernate to search for data in the table Register. It contains monetary registers, incomes and outcomes. Some registers are repetitive, so I have managed to implement a few properties that my C# will check if a determined register is repetitive and loop through its repetition in order to add more registers to the result list. Basically, the repetitive registers will have all of the same fields, including the PK Id, but the date will vary according to its repetition. If it's daily, the main loop will repeat until it met the Repetition End Date of the Register or the Final Date specified in the Method's header; otherwise it would go on looping infinitely if any Register has Repetition End Date equals to null. The code below does exactly what I said up above, but it's slow. I need to make it process faster, since it's handling and comparing so many dates:

Here's my Register Class:

public class Register
{
    public Register()
    { }

    // This constructor will create a new Register giving an Register Object, and will set the Date of this new Register.
    public Register(Register r, DateTime d)
    {
        this.Id = r.Id;
        this.Description = r.Description;
        this.Value = r.Value;
        this.Date = d;
        this.Type = r.Type;
        this.IsRepetitive = r.IsRepetitive;
        this.IsOnlyOnWeekdays = r.IsOnlyOnWeekdays;
        this.RepetitionType = r.RepetitionType;
        this.RepetitionEndDate = r.RepetitionEndDate;
    }

    public virtual Int32 Id { get; set; }

    public virtual String Description { get; set; }

    public virtual Decimal Value { get; set; }

    /// <summary>
    /// 0 - Revenue
    /// 1 - Expense
    /// </summary>
    public virtual Int32 Type { get; set; }

    public virtual Boolean IsRepetitive { get; set; }

    public virtual Boolean IsOnlyOnWeekdays { get; set; }

    /// <summary>
    /// 0 - Daily repetition.
    /// 1 - Weekly repetition.
    /// 2 - Monthly repetition.
    /// 3 - Annual repetition.
    /// </summary>
    public virtual Int32 RepetitionType { get; set; }

    public virtual DateTime? RepetitionEndDate { get; set; }

    public virtual DateTime Date { get; set; }

    public virtual IList<ExcludedDate> ExcludedDate { get; set; }
}

Here's how I'm processing everything:

public IEnumerable<Register> ObtainRegisters(DateTime EndDate)
{
    ICriteria query = session.CreateCriteria(typeof(Register));

    // Get all the registers using NHibernate.
    var Registers = query.List<Register>().Where(r => r.Date <= EndDate);

    var returnList = new List<Register>();

    // Loop on all the registers.
    foreach (var register in Registers)
    {
        // Check if it's repetitive. If it's not, just add to the return list. If it is, run through the repetition logic.
        if (register.IsRepetitive)
        {
            // Checks the repetition type and runs logic according to it.
            #region Add daily registers

            if (register.RepetitionType == 0)
            {
                for (int i = 0; i <= (EndDate - register.Date.Date).TotalDays; i++)
                {
                    // Obtains this loop date.
                    var loopDate = register.Date.AddDays(i);

                    // Check if it's only on weekdays.
                    if (register.IsOnlyOnWeekdays)
                    {
                        if (loopDate.DayOfWeek != DayOfWeek.Saturday && loopDate.DayOfWeek != DayOfWeek.Sunday)
                        {
                            // Adds the repetitive registers only if it's date is not in any Excluded Dates.
                            if (!register.ExcludedDate.Any(d => d.Date.Date == loopDate))
                            {
                                returnList.Add(new Register(register, loopDate));
                            }
                        }
                    }
                    else
                    {
                        // Adds the repetitive registers only if it's date is not in any Excluded Dates.
                        if (!register.ExcludedDate.Any(d => d.Date.Date == loopDate))
                        {
                            returnList.Add(new Register(register, loopDate));
                        }
                    }


                    if (register.RepetitionEndDate.HasValue)
                    {
                        // Checks is the final date is equals to the loop date.
                        if (register.RepetitionEndDate.Value.Date == loopDate)
                        {
                            break;
                        }
                    }
                }
            }

            #endregion

            #region Add weekly registers

            if (register.RepetitionType == 1)
            {
                if (register.RepetitionEndDate.HasValue)
                {
                    // Checks if the repetition end date is greater or equal to the main end date.
                    if (register.RepetitionEndDate.Value.Date > EndDate.Date)
                    {
                        break;
                    }
                }

                for (int i = 0; i <= (EndDate - register.Date.Date).Days; i = i + 7)
                {
                    // Obtains the loop date.
                    var loopDate = register.Date.AddDays(i);

                    // Checks if the loop date is beyond the repetition end date and finishes the loop if it's the case.
                    if (register.RepetitionEndDate.HasValue)
                    {
                        if (loopDate.Date > register.RepetitionEndDate.Value.Date)
                        {
                            break;
                        }
                    }

                    // Finishes the loop if the loop date is greater then the main end date.
                    if (loopDate.Date > EndDate.Date)
                    {
                        break;
                    }


                    // Adds the repetitive registers only if it's date is not in any Excluded Dates.
                    if (!register.ExcludedDate.Any(d => d.Date.Date == loopDate))
                    {
                        returnList.Add(new Register(register, loopDate));
                    }


                    if (register.RepetitionEndDate.HasValue)
                    {
                        // Checks if the repetition end date equals to the loop date. If true, finishes the loop.
                        if (register.RepetitionEndDate.Value.Date == loopDate)
                        {
                            break;
                        }
                    }
                }
            }

            #endregion

            #region Add monthly registers

            if (register.RepetitionType == 2)
            {
                if (register.RepetitionEndDate.HasValue)
                {
                    // Checks if the repetition end date is equal to the main end date. If true, breaks the loop.
                    if (register.RepetitionEndDate.Value.Date == EndDate.Date)
                    {
                        break;
                    }
                }

                // Runs some loop magic.
                for (int i = 0; i <= (EndDate.Year * 12 + EndDate.Month) - (register.Date.Year * 12 + register.Date.Month); i++)
                {
                    // Obtains the loop date.
                    var loopDate = register.Date.AddMonths(i);

                    // Checks if the repetition end date is greater than the loop date. If true, finishes the loop.
                    if (register.RepetitionEndDate.HasValue)
                    {
                        if (loopDate.Date > register.RepetitionEndDate.Value.Date)
                        {
                            break;
                        }
                    }

                    // Finishes the loop if the loop date is greater then the main end date.
                    if (loopDate.Date > EndDate.Date)
                    {
                        break;
                    }


                    // Adds the repetitive registers only if it's date is not in any Excluded Dates.
                    if (!register.ExcludedDate.Any(d => d.Date.Date == loopDate))
                    {
                        returnList.Add(new Register(register, loopDate));
                    }


                    if (register.RepetitionEndDate.HasValue)
                    {
                        // Checks if the repetition end date equals to the loop date. If true, finishes the loop.
                        if (register.RepetitionEndDate.Value.Date == loopDate)
                        {
                            break;
                        }
                    }
                }
            }

            #endregion

            #region Add annual registers

            if (register.RepetitionType == 3)
            {
                if (register.RepetitionEndDate.HasValue)
                {
                    // Checks if the repetition end date is greater than the main end date. If true, breaks the loop.
                    if (register.RepetitionEndDate.Value.Date > EndDate.Date)
                    {
                        break;
                    }
                }

                for (int i = 0; i <= EndDate.Year - register.Date.Year; i++)
                {
                    // Obtains the loop date.
                    var loopDate = register.Date.AddYears(i);

                    // Checks if the loop date is beyond the repetition end date and finishes the loop if it's the case.
                    if (register.RepetitionEndDate.HasValue)
                    {
                        if (loopDate.Date > register.RepetitionEndDate.Value.Date)
                        {
                            break;
                        }
                    }

                    if (loopDate.Date > EndDate.Date)
                    {
                        break;
                    }


                    // Adds the repetitive registers only if it's date is not in any Excluded Dates.
                    if (!register.ExcludedDate.Any(d => d.Date.Date == loopDate))
                    {
                        returnList.Add(new Register(register, loopDate));
                    }


                    if (register.RepetitionEndDate.HasValue)
                    {
                        // Checks if the repetition end date equals to the loop date. If true, finishes the loop.
                        if (register.RepetitionEndDate.Value.Date == loopDate)
                        {
                            break;
                        }
                    }
                }
            }

            #endregion
        }
        else
        {
            // Adds the non-repetitive register to the return list.
            returnList.Add(register);
        }
    }

    return returnList.OrderBy(r => r.Date);
}

Using region is discussed controversial, but using region inside a method is clearly a code smell, indicating that the method in question is doing to much and code of the single region's should be extracted to separate methods.


By inverting conditions you can easily reduce codeduplication.

Let us take a look at the #region Add daily registers

Here we could add inside the loop a guard clause which should just continue if register.IsOnlyOnWeekdays and the loopDate is a sunday or a saturday.

if (register.IsOnlyOnWeekdays && (loopDate.DayOfWeek == DayOfWeek.Saturday ||  
                                  loopDate.DayOfWeek == DayOfWeek.Sunday))  
{
    continue;
}

but this wouldn't be quite right, because we still would need to break out of the loop if register.RepetitionEndDate.HasValue and also register.RepetitionEndDate.Value.Date == loopDate so let us extract this check to a method IsValidRegisterDate() like

private bool IsValidRegisterDate(Register register, DateTime date)
{

    return (!register.IsOnlyOnWeekdays ||
              date.DayOfWeek != DayOfWeek.Saturday &&
              date.DayOfWeek != DayOfWeek.Sunday
           )
        && !register.ExcludedDate.Any(d => d.Date.Date == date);
}  

next we need to extract the check wether the code should break out of the loop to a separate method, because this is used in other places too.

private bool IsRepetitionEndDate(DateTime? endDate, DateTime currentDate)
{
    return endDate.HasValue && endDate.Value.Date == currentDate;
}

which leads to this loop

for (int i = 0; i <= (EndDate - register.Date.Date).TotalDays; i++)
{
    var loopDate = register.Date.AddDays(i);

    if (IsValidRegisterDate(register, loopDate))
    {
        returnList.Add(new Register(register, loopDate));
    }

    if (IsRepetitionEndDate(register.RepetitionEndDate, loopDate))
    {
        break;
    }
}

which we will extract to a method GetWeeklyRegisters() like

private IEnumerable<Register> GetWeeklyRegisters(Register register, DateTime endDate)
{
    var registers = new List<Register>();

    int days = (int)(endDate - register.Date.Date).TotalDays;

    for (int i = 0; i <= days; i++)
    {

        var loopDate = register.Date.AddDays(i);

        if (IsValidRegisterDate(register, loopDate))
        {
            registers.Add(new Register(register, loopDate));
        }

        if (IsRepetitionEndDate(register.RepetitionEndDate, loopDate))
        {
            break;
        }
    }
    return registers;
}

which can be called like

if (register.RepetitionType == 0)
{
    returnList.AddRange(GetWeeklyRegisters(register, endDate);
}  

In a similiar way you should refactor the rest of the code.


Because RepetitionType can only hold one value, you should use a swicth instead of multiple if statements. Additional the RepetitionType should be either an enum or you should extract the different values it can hold to some meaningful constants.


If you invert this if condition inside the foreach loop you can just continue after adding the register so you could omit the else part, which saves horizontal spacing which improves readability.

Like

if (!register.IsRepetitive)  
{
    returnList.Add(register);
    continue;
}
switch (register.RepetitionType)
{

    case .....

Based on the naming guidelines method parameters should be named using camelCase casing so EndDate should become endDate.


Comments should describe why something is done. Let the code itself speak for what is done by using meaningful names for classes, methods and variables.

So a comments like

// Loop on all the registers.
foreach (var register in Registers)  

or

// Check if it's repetitive. If it's not, just add to the return list. If it is, run through the repetition logic.
if (register.IsRepetitive)  

are only adding noise to the code.

  • Don't use bare Int32s for your enumerations. That's what enum is for.

    public enum TransactionType { Revenue, Expense };
    
    public virtual TransactionType Type { get; set; }
    
    public enum RepetitionFrequency { Daily, Weekly, Monthly, Annually };
    
    public virtual RepetitionFrequency RepetitionType { get; set; }
    
  • Don't mix languages for variable names (Registros). Either pick English or your native language and stick with it.

  • It's very confusing to use the same identifier for both types and member variables (DayOfWeek), and also confusing to use the same identifier for nested members (Date)!
  • You've got a lot of duplicated code. Whenever you find yourself writing the same, or almost the same thing multiple times, there's got to be some way to remove the duplication.
  • The language mix was because i wrote the whole code in Brazilian Portuguese and translated just for this post's purpose. – Edgar Salazar Mar 10 '15 at 20:10

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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