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.
• 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.

// Represents an initial loan that needs to be repaid.
{
// 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<decimal> DecreaseAmountBy { get; set; }
public bool IsUnlimited { get; set; }
public Nullable<int> NbrOfOccurrences { get; set; }
public Nullable<decimal> GoalAmount { get; set; }
public System.DateTime EffectiveOnWeekEndingDate { get; set; }
}


// Represents a change to the payment plan for a loan.
{
public long ID { get; set; }
public long RecurringHeaderID { get; set; }
public string AdjustmentType { get; set; }
public Nullable<decimal> MonthlyPaymentAmount { get; set; }
public decimal PaymentAmount { get; set; }
public int PostingKey2 { get; set; }
public Nullable<decimal> DecreaseAmountBy { get; set; }
public bool IsUnlimited { get; set; }
public Nullable<int> NbrOfOccurrences { get; set; }
public Nullable<decimal> GoalAmount { get; set; }
public System.DateTime EffectiveOnWeekEndingDate { get; set; }
public System.DateTime CreatedOn { get; set; }
public string CreatedBy { get; set; }
}


Schedule:

// Represents a single scheduled payment for a loan.
public partial class RecurringSchedule
{
public long ID { get; set; }
public long PaymentAccountID { get; set; }
public long RecurringHeaderID { get; set; }
public System.DateTime PaymentDate { get; set; }
public decimal PaymentAmount { get; set; }
public int OccurrenceNbr { get; set; }
public Nullable<decimal> RemainingBalance { get; set; }
public System.DateTime CreatedOn { get; set; }
public string CreatedBy { get; set; }
public Nullable<System.DateTime> ModifiedOn { get; set; }
public string ModifiedBy { get; set; }
public string Comment { get; set; }
}


My real troubles are applying the adjustments to the existing schedules. I've done it, but it's a very large function that's kind of a mess (imo).

/// <summary>
/// Given all of the header's existing schedules will apply the adjustment to them.
/// This works for Adjustments, Replacements, and Zeroes alike.
/// </summary>
{
// DATE-RANGE: Get the effective date ranges for the adjustment.

// PARTITION: Get all of the existing schedule for the header.
// 2) Get a list of schedules whose PaymentAmount & RemainingBalance will be affected (During Adjustment).
// 3) Get a list of schedules whose RemainingBalance will only be affected (After Adjustment).
IEnumerable<RecurringSchedule> allSchedules = allHeaderSchedules.OrderBy(x => x.OccurrenceNbr);

IList<RecurringSchedule> schedules = new List<RecurringSchedule>(allSchedules.Where(x => x.PaymentDate < adjEffectiveStart).OrderBy(x => x.OccurrenceNbr));

// REPLACEMENT:
Int32 occurrenceShift = adjEffectiveStart.Subtract(allSchedules.OrderBy(x => x.OccurrenceNbr).First().PaymentDate).Days / 7;
Int32 startingOccurrence = lastExistingSchedule.OccurrenceNbr + 1;
Int32 endingOccurrence = Math.Max((this.NbrOfOccurrences ?? 0) + occurrenceShift, allHeaderSchedules.Count());

// SETUP: Determine the initial values for updated schedules.
Decimal paymentAmount = this.PaymentAmount;
Decimal decreaseAmountBy = this.DecreaseAmountBy ?? 0;
Decimal remainingBalance = this.GoalAmount ?? schedules.Last().RemainingBalance.Value;

// ADJUST: Update each schedule, decrementing remainingBalance and paymentAmount as necessary.
{
// RemainingBalance is the value AFTER payment amount has been taken out.
remainingBalance = remainingBalance - paymentAmount > 0
? remainingBalance - paymentAmount
: 0;

schedule.PaymentAmount = paymentAmount;
schedule.RemainingBalance = remainingBalance;

// Payment Amount is decreased after the payment, if applicable.
paymentAmount = paymentAmount - decreaseAmountBy > 0
? paymentAmount - decreaseAmountBy
: 0;

if (remainingBalance != 0)
{
// THRESHHOLD: If the balance after the next payment is under
// the threshhold then add it to the current payment and let
// remaining payment schedules be a payment of nothing.
{
paymentAmount = 0;
remainingBalance = 0;
decreaseAmountBy = 0;
}
}

}

{
{
remainingBalance -= schedule.PaymentAmount;

schedule.RemainingBalance = remainingBalance;

// THRESHHOLD: If the balance after the next payment is under
// the threshhold then add it to the current payment and let
// remaining payment schedules be a payment of nothing.
if (remainingBalance != 0 && schedule.AdjustForThreshhold(decreaseAmountBy))
{
paymentAmount = 0;
remainingBalance = 0;
decreaseAmountBy = 0;
}

}
}
// EXTENSION: Replacements Only.
{
String createdBy = HttpContext.Current != null &&
HttpContext.Current.User != null &&
HttpContext.Current.User.Identity != null
? HttpContext.Current.User.Identity.Name
: "System";

for (int i = startingOccurrence; i <= endingOccurrence; ++i)
{
// RemainingBalance is the value AFTER payment amount has been taken out.
remainingBalance = remainingBalance - paymentAmount > 0
? remainingBalance - paymentAmount
: 0;

RecurringSchedule newSchedule = new RecurringSchedule() {
PaymentAccountID = this.PaymentAccountID,
PaymentDate = startingDate,
OccurrenceNbr = i,
PaymentAmount = paymentAmount,
RemainingBalance = remainingBalance,
CreatedBy = createdBy,
CreatedOn = DateTime.Now
};

// Increment the date by a week.

// Payment Amount is decreased after the payment, if applicable.
paymentAmount = paymentAmount - decreaseAmountBy > 0
? paymentAmount - decreaseAmountBy
: 0;

if (remainingBalance != 0)
{
// THRESHHOLD: If the balance after the next payment is under
// the threshhold then add it to the current payment and let
// remaining payment schedules be a payment of nothing.
{
paymentAmount = 0;
remainingBalance = 0;
decreaseAmountBy = 0;
}
}

}
}

return schedules;
}


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 IList<RecurringSchedule> _schedules;
// etc.

{
/// 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
}
}


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<RecurringSchedule> allHeaderSchedules, which you then enumerate multiple times. Which isn't a good thing (ReSharper warns about this). I'd suggest using an ICollection<T> instead.

• I agree. From the looks of it the main issue with the original code is the lackluster object model. Splitting huge multi-purpose entities into smaller classes with fewer responsibilities would be a major imrovement, IMHO. – Nikita B Aug 11 '15 at 11:17

Code style

It isn't easy to follow this code, because you are switching styles a lot, sometimes from one line of code to the other.

IList<RecurringSchedule> schedules = new List<RecurringSchedule>(allSchedules.Where(x => x.PaymentDate < adjEffectiveStart).OrderBy(x => x.OccurrenceNbr));

• Sometimes you are using int, decimal and then you are using Int32 and Decimal.
• Sometimes you are checking if the Count property of the IList ==0 and sometimes you are using Any() which by the way for types/interfaces implementing ICollection<T>, which IList<T> does, will add a little overhead by having a null check on the passed IEnumerable<TSource> and a cast to ICollection<T> together with a null check.

Extracing this paymentAmount, decreaseAmountBy and remainingBalance to a separate class like so

public class PaymentParameter
{
public decimal PaymentAmount { get; set; }
public decimal DecreaseAmountBy { get; set; }
public decimal RemainingBalance { get; set; }
}


will help to refactor this method.

So let us start by extracting the first loop to a separate method

private IList<RecurringSchedule> UpdateSchedules(IList<RecurringSchedule> schedulesDuringAdj, PaymentParameter paymentParameter, bool hasSchedulesAfterAdjustment)
{
IList<RecurringSchedule> schedules = new List<RecurringSchedule>();
decimal remainingBalance = paymentParameter.RemainingBalance;
decimal paymentAmount = paymentParameter.PaymentAmount;
decimal decreaseAmountBy = paymentParameter.DecreaseAmountBy;

{
// RemainingBalance is the value AFTER payment amount has been taken out.
remainingBalance = Math.Max(remainingBalance - paymentAmount, 0);

schedule.PaymentAmount = paymentAmount;
schedule.RemainingBalance = remainingBalance;

// Payment Amount is decreased after the payment, if applicable.
paymentAmount = Math.Max(paymentAmount - decreaseAmountBy, 0);

if (remainingBalance != 0)
{
// THRESHHOLD: If the balance after the next payment is under
// the threshhold then add it to the current payment and let
// remaining payment schedules be a payment of nothing.
{
paymentAmount = 0;
remainingBalance = 0;
decreaseAmountBy = 0;
}
}

}

paymentParameter.DecreaseAmountBy = decreaseAmountBy;
paymentParameter.PaymentAmount = paymentAmount;
paymentParameter.RemainingBalance = remainingBalance;

return schedules;
}


As you will see, I have replaced the tenary conditions by simple calls to Math.Max().

The next method to extract is for the case that this.AdjustmentType != "Replacement" like so

private IList<RecurringSchedule> UpdateNonReplacements(IList<RecurringSchedule> schedulesAfterAdj, PaymentParameter paymentParameter)
{
IList<RecurringSchedule> schedules = new List<RecurringSchedule>();
decimal remainingBalance = paymentParameter.RemainingBalance;
decimal paymentAmount = paymentParameter.PaymentAmount;
decimal decreaseAmountBy = paymentParameter.DecreaseAmountBy;

{
remainingBalance -= schedule.PaymentAmount;

schedule.RemainingBalance = remainingBalance;

// THRESHHOLD: If the balance after the next payment is under
// the threshhold then add it to the current payment and let
// remaining payment schedules be a payment of nothing.
if (remainingBalance != 0 && schedule.AdjustForThreshhold(decreaseAmountBy))
{
paymentAmount = 0;
remainingBalance = 0;
decreaseAmountBy = 0;
}

}
paymentParameter.DecreaseAmountBy = decreaseAmountBy;
paymentParameter.PaymentAmount = paymentAmount;
paymentParameter.RemainingBalance = remainingBalance;

return schedules;
}


quite easy, isn't it ?

Because the else if targets some more parameters, we will extend the PaymentParameter class to take them into account too.

public class PaymentParameter
{
public decimal PaymentAmount { get; set; }
public decimal DecreaseAmountBy { get; set; }
public decimal RemainingBalance { get; set; }
public DateTime StartingDate { get; set; }
public int StartingOccurance { get; set; }
public int EndingOccurance { get; set; }
}


so we can extract the method as follows

private IList<RecurringSchedule> UpdateReplacements(PaymentParameter paymentParameter)
{
IList<RecurringSchedule> schedules = new List<RecurringSchedule>();

if (paymentParameter.EndingOccurance < paymentParameter.StartingOccurance)
{
return schedules;
}

string createdBy = HttpContext.Current != null &&
HttpContext.Current.User != null &&
HttpContext.Current.User.Identity != null
? HttpContext.Current.User.Identity.Name
: "System";

decimal remainingBalance = paymentParameter.RemainingBalance;
decimal paymentAmount = paymentParameter.PaymentAmount;
decimal decreaseAmountBy = paymentParameter.DecreaseAmountBy;

for (int i = paymentParameter.StartingOccurance; i <= paymentParameter.EndingOccurance; ++i)
{
// RemainingBalance is the value AFTER payment amount has been taken out.
remainingBalance = Math.Max(remainingBalance - paymentAmount, 0);

RecurringSchedule newSchedule = new RecurringSchedule()
{
PaymentAccountID = this.PaymentAccountID,
PaymentDate = startingDate,
OccurrenceNbr = i,
PaymentAmount = paymentAmount,
RemainingBalance = remainingBalance,
CreatedBy = createdBy,
CreatedOn = DateTime.Now
};

// Increment the date by a week.

// Payment Amount is decreased after the payment, if applicable.
paymentAmount = Math.Max(paymentAmount - decreaseAmountBy, 0);

if (remainingBalance != 0)
{
// THRESHHOLD: If the balance after the next payment is under
// the threshhold then add it to the current payment and let
// remaining payment schedules be a payment of nothing.
{
paymentAmount = 0;
remainingBalance = 0;
decreaseAmountBy = 0;
}
}

}
return schedules;
}


As you maybe have seen, there isn't a check for schedulesAfterAdj.Count == 0 anymore. This check had been superflous because one of the conditions to enter the else if had been !schedulesAfterAdj.Any() so it will be entered only if schedulesAfterAdj.Count == 0.

The former AdjustSchedules() will then look like so

public IEnumerable<RecurringSchedule> AdjustSchedules(IEnumerable<RecurringSchedule> allHeaderSchedules)
{
// DATE-RANGE: Get the effective date ranges for the adjustment.

// PARTITION: Get all of the existing schedule for the header.
// 2) Get a list of schedules whose PaymentAmount & RemainingBalance will be affected (During Adjustment).
// 3) Get a list of schedules whose RemainingBalance will only be affected (After Adjustment).
IEnumerable<RecurringSchedule> allSchedules = allHeaderSchedules.OrderBy(x => x.OccurrenceNbr);

List<RecurringSchedule> schedules = allSchedules.Where(x => x.PaymentDate < adjEffectiveStart).ToList();

// REPLACEMENT:
int occurrenceShift = adjEffectiveStart.Subtract(allSchedules.First().PaymentDate).Days / 7;

PaymentParameter paymentParameter = new PaymentParameter()
{
PaymentAmount = this.PaymentAmount,
DecreaseAmountBy = this.DecreaseAmountBy ?? 0,
RemainingBalance = this.GoalAmount ?? schedules.Last().RemainingBalance.Value,
StartingDate = lastExistingSchedule.PaymentDate,
StartingOccurance = lastExistingSchedule.OccurrenceNbr + 1,
EndingOccurance =Math.Max((this.NbrOfOccurrences ?? 0) + occurrenceShift, allHeaderSchedules.Count())
};

{
}
// EXTENSION: Replacements Only.
{
}

return schedules;
}


As you may noticed I have

• changed the type of schedules from IList<RecurringSchedule> to List<RecurringSchedule> to use the AddRange() method.
• removed the checking for endingOccurance >= startingOccurance because this is used in the UpdateReplacements() method as a guard clause.
• removed the redundant OrderBy(x => x.OccurrenceNbr) calls
• changed Int32 to int
• I actually ended up doing a pleasant mix of both answers. I created a ScheduleAdjuster class that contains a PaymentModel class. The payment model is very similar to yours with additional functions to make it look a bit more "fluent". My adjuster has two private functions (Adjust and Replace) and then a public AdjustSchedules function that just calls one or the other. Thank you both! – Shelby115 Aug 11 '15 at 14:13

Note: this post is after the OP selected the preferred answer.

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.

@NikitaBrizhak is absolutely right on.

It's the lack of class design that is the problem. Classes that exhibit the Single Responsibility Principle will naturally distribute the busy-ness and complexity such that most of your existing function will disappear - the details will be in appropriate classes.

I imagine you're too far along with this project to re-work design. However, thoughtful refactoring the function itself as suggested by the (currently) 2 answers will help if you want to tackle the design.

NOTE: these are inspirational examples. Don't cut-n-paste this stuff into your code!

## There and Back Again - a Refactor tale

The basic game plan is to keep client code from having to know class details to do things. You want the client to "ask" an object to do something, not do it for the object.

These first few ideas should be direct substitutions into your existing function - meaning that the function will not break (this is the definition of "refactor").

Once the classes have some functionality you can pull out snippets of processing from the function.

### Version Control CYA

STOP! Do this first. Hear me know and believe me later.

Put version control software on your computer; Subversion Mercurial, GIT... whichever you prefer. I use Mercurial.

I have version control on my computer even though we use version control as team. I can experiment at will, recover, and never affect the o-fishal code base.

### Basic Class Functionality

Implement IComparable Then this:

_allHeaderSchedules.OrderBy(x => x.OccurrenceNbr); // doing it for the class


will become this:

_allHeaderSchedules.Sort(); // asking the class to sort itself.


Override Equals (and GetHashCode at the same time). This will allow you to Find() any given payment object w/in a collection; or prevent duplicate entries -

if(myCollection.Contains(thisPayment))


### Collection functionality

 allSchedules.Where(x => x.PaymentDate >= adjEffectiveStart && x.PaymentDate < adjEffectiveEnd).OrderBy(x => x.OccurrenceNbr).ToList();


becomes

allSchedules.InAdjustmentRange(adjEffectiveStart, adjEffectiveEnd);


Once you get some functionality into your collection many interim code lines in the existing function go away too.

implement IComparable for the collection to make it sort by date order.

allSchedules.Sort();


### Next Level Refactoring

The previous refactoring make the next step possible. You must have basic class functionality first or you will be hacking at code all over the place all at once and you'll break everything.

### DateRange Class ?

The initial refactorings has me using start/end date pairs, especially for method parameters. Maybe there should be a simple class for this

### Extract Actual Loan Processing

THIS:

  foreach (RecurringSchedule schedule in schedulesAfterAdj)
{
remainingBalance -= schedule.PaymentAmount;

schedule.RemainingBalance = remainingBalance;

// THRESHHOLD: If the balance after the next payment is under
// the threshhold then add it to the current payment and let
// remaining payment schedules be a payment of nothing.
if (remainingBalance != 0 && schedule.AdjustForThreshhold(decreaseAmountBy))
{
paymentAmount = 0;
remainingBalance = 0;
decreaseAmountBy = 0;
}

}
}


Could Become something like THIS:

 remaingBalance = allSchedules.InAdjustmentRange(dateRangeObject).CalculateBalance();


Wow. Just a hint at the power of good class design.

### Every class does its part

The "payment"/"paymentTransaction" class should be calculating its own balance, and the scheduleCollection should leverage this:

public class scheduleCollection {
public decimal CalculateBalance() {