As @sgrove mentioned, the guard clauses did not match all possible inputs (which I assume is why it’s a bad style). You can call the function with a -1 and it’ll throw an match error at runtime. There was an assumption based on the domain, that no one will have a negative price.
Anyway… back to the code… you can add a catch all case to throw an exception when a negative number is entered or a price over 200 in your example above. Generally I always avoid the _ catch all clause unless it’s going to throw because it’s a source of a lot of bugs (for example defaulting to a certain plan and then forgetting about it 2 years later when you modify something else). I’ve also added the suffix Exn to the end to indicate to future maintainers that this function will throw a runtime exception if the data does not match the expectation.
let priceTierCalcExn = price => {
let base =
switch (price) {
| price when price > 0 && price < 50 => 0.33
| price when price > 51 && price < 100 => 0.29
| price when price > 101 && price < 200 => 0.26
| _else => raise(Not_found)
};
base * float_of_int(price);
};
I agree with @sgrove on the variants… In an ideal situation you can use the compiler for more than just null checks and to make sure you pass in a string instead of an int. You can embed business logic in the type system to make sure that when you refer to a PricePlan22 it will easier to pattern match against in all of your program (and can likely be changed in one spot).
I’ve came up with a rough example below. You would most likely use a plan ID from your database or Stripe plan ID and run that through a parser to return the subscription variants. Instead of hard coding you could also use ENV variables but then you’ll lose some type safety as the program has to assume they’re present (unless you check on startup I guess).
The purpose here is that you can now pass around Plan1, Plan2 and if you change anything related to the plans it can show you helpful errors. It also removes the need to have a raise branch in the switch since it will not compile if the variants don’t match.
My guess is that the most useful thing about this change would be the case of adding a new variant to Subscription.plan. Then you’ll see a bunch of warnings/errors about a non exhaustive match where you’ve forgotten a case… then you can manage the logic/handling for this new plan in the areas sprinkled throughout the app. Ideally you want the exhaustive match warning option changed to a compile error to keep things sound since you can still pass CI and deploy with warnings.
Unrelated to Reason, but I would also stay away from making variants like GoldPlan, SilverPlan, BronzePlan, because you’re going to end up changing the gold plan to have different features but you don’t want to change existing customers. Plan1 and Plan22 may both be gold but Plan22 may include features that the early adopters (Plan1) currently have.
/* Subscription.re */
module Subscription = {
type plan =
| Plan1
| Plan2
| Plan3;
let parsePlanFromStripeIdExn = (id) =>
switch (id) {
| "j39xd" => Plan1
| "khx3d" => Plan2
| "js7fn" => Plan3
| _ => raise(Not_found)
};
};
/* User.re */
module User = {
type t = {
id: int,
name: string,
plan: Subscription.plan,
};
};
/* when you parse user data into a user record, choose plan */
let currentUser : User.t = {id: 22, name: "Jane", plan: Subscription.Plan2};
let priceTierCalc = (price, plan: Subscription.plan) => {
let base =
switch (plan) {
| Plan1 => 0.33
| Plan2 => 0.29
| Plan3 => 0.26
};
base *. float_of_int(price);
};
Js.log(
priceTierCalc(1, currentUser.plan)
);
Good catch!