HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Scheduled adjustable payments

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
paymentsadjustablescheduled

Problem

Short Description:

I'm working on a "small" project for our finance department. The problem I'm having is managing this large function (at the bottom). It's so large because I'm having troubles reducing the function into smaller parts without either having to pass many parameters along or recalculate some things. Any times or suggestions in accomplishing this? (I've been trying to avoid any functions with more than 3 parameters, hopefully this isn't too irrational a desire).

Entity Details

  • The header is simply put, the initial loan. When this is created so are all of its schedules (individual payments for a specific date).



  • Adjustments can be done changing the PaymentAmount, GoalAmount (effectively the RemainingBalance), and DecreaseAmountBy (Amount to decrease payment amount after each payment).



  • Schedules need to be created ahead of time for various reasons.



  • Adjustments have AdjustmentType. The three valid adjustment types are "Adjustment", "Replacement", and "Zero".



  • An "Adjustment" is just a changing of existing schedules.



  • "Replacements" can be extensions or compression (add/remove schedules).



  • "Zero" is just an adjustment where PaymentAmount, DecreaseAmountBy, and RemainingBalance is set to 0 for the remainder of the existing schedules.



RecurringHeader:

```
// Represents an initial loan that needs to be repaid.
public partial class RecurringHeader
{
// Completely irrelevant properties removed.
public long ID { get; set; }
public long PaymentAccountID { get; set; }
public string NoteNbr { get; set; }
public string TransactionType { get; set; }
public string AcctCode { get; set; }
public decimal PaymentAmount { get; set; }
public int PostingKey2 { get; set; }
public Nullable DecreaseAmountBy { get; set; }
public bool IsUnlimited { get; set; }
public Nullable NbrOfOccurrences { get; set; }
public Nullable GoalAmount { get; set; }
public System.DateTime EffectiveOnWeekEndingDate { get

Solution

Considering allSchedules = allHeaderSchedules.OrderBy(x => x.OccurrenceNbr), why then do you do allSchedules.OrderBy(x => x.OccurrenceNbr) later on?

This method is 140+ lines long, and does a lot. The next person who needs to maintain it will have to spend a lot of time to figure out what it all does, and your idea of splitting the method into smaller, more manageable ones is the right one. My advice: make it a class of its own. That way you can store variables like schedules, schedulesDuringAdj and schedulesAfterAdj at class level, and you don't need to pass them as parameters to other methods:

internal class ScheduleAdjuster
{
   private readonly IEnumerable _allHeaderSchedules;
   priavte readonly IEnumerable _allSchedules;

   private IList _schedules;
   // etc.

   public ScheduleAdjuster(IEnumerable allHeaderSchedules)
   {
      _allHeaderSchedules = allHeaderSchedules;
      _allSchedules = _allHeaderSchedules.OrderBy(x => x.OccurrenceNbr);
      /// etc.
   }

   public IEnumerable Execute()
   {
      // call various methods 
      // these can use the "global" variables like _allHeaderSchedules and _allSchedules etc.
      // without you needing to pass numerous parameters
   }
}


There are a dozen or so places where you use a value from the class that contains AdjustSchedules: is it possible to pass this class as a parameter to ScheduleAdjuster? Quite frankly I wonder how big that parent class is, considering this single method is 140+ lines long.

There are several typos in your comments: scedules, threshhold.

The initial parameter of your method is IEnumerable allHeaderSchedules, which you then enumerate multiple times. Which isn't a good thing (ReSharper warns about this). I'd suggest using an ICollection instead.

Code Snippets

internal class ScheduleAdjuster
{
   private readonly IEnumerable<RecurringSchedule> _allHeaderSchedules;
   priavte readonly IEnumerable<RecurringSchedule> _allSchedules;

   private IList<RecurringSchedule> _schedules;
   // etc.

   public ScheduleAdjuster(IEnumerable<RecurringSchedule> allHeaderSchedules)
   {
      _allHeaderSchedules = allHeaderSchedules;
      _allSchedules = _allHeaderSchedules.OrderBy(x => x.OccurrenceNbr);
      /// etc.
   }

   public IEnumerable<RecurringSchedule> Execute()
   {
      // call various methods 
      // these can use the "global" variables like _allHeaderSchedules and _allSchedules etc.
      // without you needing to pass numerous parameters
   }
}

Context

StackExchange Code Review Q#100523, answer score: 6

Revisions (0)

No revisions yet.