We have an API function that breaks down a total amount into monthly amounts based on given start and end dates.
// JavaScript
function convertToMonths(timePeriod) {
// ... returns the given time period converted to months
}
function getPaymentBreakdown(total, startDate, endDate) {
const numMonths = convertToMonths(endDate - startDate);
return {
numMonths,
monthlyPayment: total / numMonths,
};
}
Recently, a consumer for this API wanted to specify the date range in other ways: 1) by providing the number of months instead of the end date, or 2) by providing the monthly payment and calculating the end date. In response to this, the API team changed the function to the following:
// JavaScript
function addMonths(date, numMonths) {
// ... returns a new date numMonths after date
}
function getPaymentBreakdown(
total,
startDate,
endDate /* optional */,
numMonths /* optional */,
monthlyPayment /* optional */,
) {
let innerNumMonths;
if (monthlyPayment) {
innerNumMonths = total / monthlyPayment;
} else if (numMonths) {
innerNumMonths = numMonths;
} else {
innerNumMonths = convertToMonths(endDate - startDate);
}
return {
numMonths: innerNumMonths,
monthlyPayment: total / innerNumMonths,
endDate: addMonths(startDate, innerNumMonths),
};
}
I feel this change complicates the API. Now the caller needs to worry about the heuristics hidden with the function's implementation in determining which parameters take preference in being used to calculate the date range (i.e. by order of priority monthlyPayment
, numMonths
, endDate
). If a caller doesn't pay attention to the function signature, they might send multiple of the optional parameters and get confused as to why endDate
is being ignored. We do specify this behavior in the function documentation.
Additionally I feel it sets a bad precedent and adds responsibilities to the API that it should not concern itself with (i.e. violating SRP). Suppose additional consumers want the function to support more use cases, such as calculating total
from the numMonths
and monthlyPayment
parameters. This function will become more and more complicated over time.
My preference is to keep the function as it was and instead require the caller to calculate endDate
themselves. However, I may be wrong and was wondering if the changes they made were an acceptable way to design an API function.
Alternatively, is there a common pattern for handling scenarios like this? We could provide additional higher-order functions in our API that wrap the original function, but this bloats the API. Maybe we could add an additional flag parameter specifying which approach to use inside of the function.
6 Answers 6
Seeing the implementation, it appears to me what you really require here is 3 different functions instead of one:
The original one:
function getPaymentBreakdown(total, startDate, endDate)
The one providing the number of months instead of the end date:
function getPaymentBreakdownByNoOfMonths(total, startDate, noOfMonths)
and the one providing the monthly payment and calculating the end date:
function getPaymentBreakdownByMonthlyPayment(total, startDate, monthlyPayment)
Now, there are no optional parameters any more, and it should be pretty clear which function is called how and for which purpose. As mentioned in the comments, in a strictly typed language, one could also utilize function overloading, distinguishing the 3 different functions not necessarily by their name, but by their signature, in case this does not obfuscate their purpose.
Note the different functions don't mean you have to duplicate any logic - internally, if these functions share a common algorithm, it should be refactored to a "private" function.
is there a common pattern for handling scenarios like this
I don't think there is a "pattern" (in the sense of the GoF design patterns) which describes good API design. Using self-describing names, functions with fewer parameters, functions with orthogonal (=independent) parameters, are just basic principles of creating readable, maintainable and evolvable code. Not every good idea in programming is necessarily a "design pattern".
-
24Actually the "common" implementation of the code could simply be
getPaymentBreakdown
(or really any one of those 3) and the other two functions just convert the arguments and call that. Why add a private function that is a perfect copy of one of these 3?Giacomo Alzetta– Giacomo Alzetta2019年09月25日 07:18:45 +00:00Commented Sep 25, 2019 at 7:18 -
@GiacomoAlzetta: that is possible. But I am pretty sure the implementation will become simpler by providing a common function which contains only the "return" part of the OPs function, and let the public 3 functions call this function with parameters
innerNumMonths
,total
andstartDate
. Why keep an overcomplicated function with 5 parameters, where 3 are almost optional (except one has to be set), when a 3-parameter function will do the job as well?Doc Brown– Doc Brown2019年09月25日 07:31:01 +00:00Commented Sep 25, 2019 at 7:31 -
3I didn't mean to say "keep the 5 argument function". I'm just saying that when you have some common logic this logic need not be private. In this case all 3 functions can be refactored to simply tranform the parameters to start-end dates, so you can use the public
getPaymentBreakdown(total, startDate, endDate)
function as common implementation, the other tool will simply compute the suitable total/start/end dates and call it.Giacomo Alzetta– Giacomo Alzetta2019年09月25日 08:13:37 +00:00Commented Sep 25, 2019 at 8:13 -
@GiacomoAlzetta: ok, was a misunderstanding, I thought you were talking about the second implementation of
getPaymentBreakdown
in the question.Doc Brown– Doc Brown2019年09月25日 09:13:28 +00:00Commented Sep 25, 2019 at 9:13 -
I'd go as far as adding a new version of the original method that's explicitly called 'getPaymentBreakdownByStartAndEnd' and deprecating the original method, if you want to supply all of these.Erik– Erik2019年09月25日 12:18:17 +00:00Commented Sep 25, 2019 at 12:18
Additionally I feel it sets a bad precedent and adds responsibilities to the API that it should not concern itself with (i.e. violating SRP). Suppose additional consumers want the function to support more use cases, such as calculating
total
from thenumMonths
andmonthlyPayment
parameters. This function will become more and more complicated over time.
You're exactly correct.
My preference is to keep the function as it was and instead require the caller to calculate endDate themselves. However, I may be wrong and was wondering if the changes they made were an acceptable way to design an API function.
This isn't ideal either, because the caller code will be polluted with unrelated boiler plate.
Alternatively, is there a common pattern for handling scenarios like this?
Introduce a new type, like DateInterval
. Add whatever constructors make sense (start date + end date, start date + num months, whatever.). Adopt this as the common-currency types for expressing intervals of dates/times throughout your system.
-
3@DocBrown Yep. In such cases (Ruby, Python, JS), it's customary to just use static/class methods. But that's an implementation detail, that I don't think is particularly relevant to my answer's point ("use a type").Alexander– Alexander2019年09月24日 21:37:12 +00:00Commented Sep 24, 2019 at 21:37
-
2And this idea sadly reaches its limits with the third requirement: Start Date, total Payment and a monthly Payment - and the function will calculate the DateInterval from the money parameters - and you should not put the monetary amounts into your date range...Falco– Falco2019年09月25日 07:44:21 +00:00Commented Sep 25, 2019 at 7:44
-
3@DocBrown "only shift the problem from the existing function to the constructor of the type" Yes, it's putting time code where time code should go, so that money code can be where money code should go. It's simple SRP, so I'm not sure what you're getting at when you say it "only" shifts the problem. That's what all functions do. They don't make code go away, they move it into more appropriate places. What's your issue with that? "but my congratulations, at least 5 upvoters took the bait" This sounds a lot more assholeish than I think (hope) you intended.Alexander– Alexander2019年09月25日 14:39:29 +00:00Commented Sep 25, 2019 at 14:39
-
@Falco That sounds like a new method to me (on this payment calculator class, not
DateInterval
):calculatePayPeriod(startData, totalPayment, monthlyPayment)
Alexander– Alexander2019年09月25日 14:42:03 +00:00Commented Sep 25, 2019 at 14:42
Sometimes fluent-expressions help on this:
let payment1 = forTotalAmount(1234)
.breakIntoPayments()
.byPeriod(months(2));
let payment2 = forTotalAmount(1234)
.breakIntoPayments()
.byDateRange(saleStart, saleEnd);
let monthsDue = forTotalAmount(1234)
.calculatePeriod()
.withPaymentsOf(12.34)
.monthly();
Given enough time to design, you can come up with a solid API that acts similar to a domain-specific-language.
The other big advantage is that IDEs with autocomplete make almost irrevelant to read the API documentation, as is intuitive due its self-discoverable capabilities.
There are resources out there such as https://nikas.praninskas.com/javascript/2015/04/26/fluent-javascript/ or https://github.com/nikaspran/fluent.js on this topic.
Example (taken from the first resource link):
let insert = (value) => ({into: (array) => ({after: (afterValue) => {
array.splice(array.indexOf(afterValue) + 1, 0, value);
return array;
}})});
insert(2).into([1, 3]).after(1); //[1, 2, 3]
-
8Fluent interface by itself doesn't make any particular task easier or harder. This seems more like the Builder pattern.VLAZ– VLAZ2019年09月25日 06:25:14 +00:00Commented Sep 25, 2019 at 6:25
-
8The implementation would be rather complicated though if you need to prevent mistaken calls like
forTotalAmount(1234).breakIntoPayments().byPeriod(2).monthly().withPaymentsOf(12.34).byDateRange(saleStart, saleEnd);
Bergi– Bergi2019年09月25日 07:52:44 +00:00Commented Sep 25, 2019 at 7:52 -
4If developers truly want to shoot on their feet, there are easier ways @Bergi. Still, the example you put is far more readable than
forTotalAmountAndBreakIntoPaymentsByPeriodThenMonthlyWithPaymentsOfButByDateRange(1234, 2, 12.34, saleStart, saleEnd);
DanielCuadra– DanielCuadra2019年09月25日 17:04:24 +00:00Commented Sep 25, 2019 at 17:04 -
5@DanielCuadra The point I was trying to make is that your answer doesn't really solve the OPs problem of having 3 mutually exclusive parameters. Using the builder pattern might make the call more readable (and raise the probability of the user noticing that it doesn't make sense), but using the builder pattern alone doesn't prevent them from still passing 3 values at once.Bergi– Bergi2019年09月25日 18:23:06 +00:00Commented Sep 25, 2019 at 18:23
-
2@Falco Will it? Yes, it's possible, but more complicated, and the answer made no mention of this. The more common builders I've seen consisted of only a single class. If the answer gets edited to include the code of the builder(s), I'll happily endorse it and remove my downvote.Bergi– Bergi2019年09月25日 21:43:23 +00:00Commented Sep 25, 2019 at 21:43
Well, in other languages, you'd use named parameters. This can be emulated in Javscript:
function getPaymentBreakdown(total, startDate, durationSpec) { ... }
getPaymentBreakdown(100, today, {endDate: whatever});
getPaymentBreakdown(100, today, {noOfMonths: 4});
getPaymentBreakdown(100, today, {monthlyPayment: 20});
-
6Like the builder pattern below, this makes the call more readable (and raises the probability of the user noticing that it doesn't make sense), but naming the parameters does not prevent the user from still passing 3 values at once - e.g.
getPaymentBreakdown(100, today, {endDate: whatever, noOfMonths: 4, monthlyPayment: 20})
.Bergi– Bergi2019年09月25日 18:24:52 +00:00Commented Sep 25, 2019 at 18:24 -
1Shouldn't it be
:
instead of=
?Barmar– Barmar2019年09月25日 19:22:19 +00:00Commented Sep 25, 2019 at 19:22 -
I guess you could check that only one of the parameters is non-null (or is not in the dictionary).Mateen Ulhaq– Mateen Ulhaq2019年09月25日 23:43:39 +00:00Commented Sep 25, 2019 at 23:43
-
1@Bergi - The syntax itself doesn't prevent users from passing nonsensical parameters but you can simply do some validation and throw errorsslebetman– slebetman2019年09月26日 06:07:11 +00:00Commented Sep 26, 2019 at 6:07
-
@Bergi I am by no means a Javascript expert, but I think Destructuring Assignment in ES6 can help here, though I am very light on knowledge with this.Gregory Currie– Gregory Currie2019年09月26日 06:22:13 +00:00Commented Sep 26, 2019 at 6:22
As an alternative you could also break the responsibility of specifying the number of month and leave it out of your function :
getPaymentBreakdown(420, numberOfMonths(3))
getPaymentBreakdown(420, dateRage(a, b))
getPaymentBreakdown(420, paymentAmount(350))
And the getpaymentBreakdown would receive an object which would provide the base number of months
Those would higher order function returning for instance a function.
function numberOfMonths(months) {
return {months: (total) => months};
}
function dateRange(startDate, endDate) {
return {months: (total) => convertToMonths(endDate - startDate)}
}
function monthlyPayment(amount) {
return {months: (total) => total / amount}
}
function getPaymentBreakdown(total, {months}) {
const numMonths= months(total);
return {
numMonths,
monthlyPayment: total / numMonths,
endDate: addMonths(startDate, numMonths)
};
}
-
What happened to the
total
andstartDate
parameters?Bergi– Bergi2019年09月26日 17:31:24 +00:00Commented Sep 26, 2019 at 17:31 -
This seems like a nice API, but could you please add how you would imagine those four functions to be implemented? (With variant types and a common interface this could be quite elegant, but it's unclear what you had in mind).Bergi– Bergi2019年09月26日 17:32:45 +00:00Commented Sep 26, 2019 at 17:32
-
And if you were to be working with a system with discriminated unions/algebraic data types, you could pass it in as, say, a TimePeriodSpecification
.
type TimePeriodSpecification =
| DateRange of startDate : DateTime * endDate : DateTime
| MonthCount of startDate : DateTime * monthCount : int
| MonthlyPayment of startDate : DateTime * monthlyAmount : float
and then none of the problems would occur where you could fail to actually implement one and so on.
-
This is definitely how I would approach this in a language that had types like these available. I tried to keep my question language-agnostic but maybe it should take into account the language used because approaches like this become possible in some cases.CalMlynarczyk– CalMlynarczyk2019年09月30日 16:09:19 +00:00Commented Sep 30, 2019 at 16:09
Date
- you can supply a string and it can be parsed to determine the date. However, this way oh handling parameters can also be very finicky and might produce unreliable results. SeeDate
again. It's not impossible to do right - Moment handles it way better but it's very annoying to use regardless.monthlyPayment
is given buttotal
is not an integer multiple of it. And also how to deal with possible floating point roundoff errors if the values aren't guaranteed to be integers (e.g. try it withtotal = 0.3
andmonthlyPayment = 0.1
).