Best convention/style for pattern matching


#1

Hello fellow Reason friends! :heart:

I am trying to write an expression like this into reason:

function priceTierCalc(price) {
  if (price > 0 && price < 50) {
    return 0.33 * price;
  } else if (price > 51 && price < 100) {
    return 0.29 * price;
  } else (price > 101 && price < 200) {
    return 0.26 * price;
  } 
}

And I have written something like this:

let priceTierCalc = (price) => {
  switch(price) {
  | price when price > 0 && price < 50 => 0.33 *. float_of_int(price)
  | price when price > 51 && price < 100 => 0.29 *. float_of_int(price)
  | price when price > 101 && price < 200 => 0.26 *. float_of_int(price)
  }
};

Which seems to be working fine, but I am getting this message from the compiler:

bad style, all clauses in this pattern-matching are guarded.

Please excuse my newbiness, but what is wrong with having all clauses guarded? If there is a better way to write the expression above, could someone enlighten me? :smiley:

Thanks!


#2

I’d probably move all the conditions to a tuple:

let priceTierCalc = price =>
  switch (
    price > 0 && price < 50,
    price > 51 && price < 100,
    price > 101 && price < 200,
  ) {
  | (true, _, _) => 0.33 *. float_of_int(price)
  | (_, true, _) => 0.29 *. float_of_int(price)
  | (_, _, true) => 0.26 *. float_of_int(price)
  };

Which has the benefit that now bsb will tell you:

Warning 8: this pattern-matching is not exhaustive.
Here is an example of a value that is not matched:
(false, false, false)

(which I think is a problem in both of your examples, but bsb couldn’t tell in the reason version because of the guards)

Alternatively, if these are known tiers, I might make a variant that would return return e.g. HighPriceTier(discount) and switch over that, but that may not match your domain well.


#3

@sgrove thank you for your reply!

Mmm…I see! Because the conditions are guarded the compiler can’t tell if all cases are covered.

In my example I only included 3 tiers, but the function I am writing actually has 9 tiers. Would you still move all your condition to a tuple when there’s nine? That could look pretty verbose, no?

Also what you said about using variants sounds interesting:

This isn’t semantically correct, but would it look something like this? :grimacing:

type = Discount | HigherDiscount | HighestDiscount;

switch (price) {
| Discount  when price > 0 && price < 50 => 0.33 * float_of_int(price)
| HigherDiscount  when price > 51 && price < 100 => 0.29 * float_of_int(price)
| HighestDiscount  when price > 101 && price < 200 => 0.26 * float_of_int(price)
};

I guess I don’t quite understand…can you expound on how to switch over constructors in this case?


#4

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)
);

#5

Hey @SkinnyGeek1010 !
That’s great to know that the special fall-through case from your experience has been a source of a lot of bugs…I will use it sparingly.
And thank you for your example and explanation on what the logic might look like using variants—it’s very helpful!! I wasn’t quite sure how to express that, but this really helps. :heart:


#6

Since it’s my purpose in life to ask silly questions: Wouldn’t code like this –

    switch (price) {
    | price when price > 0 && price < 50 => 0.33
    | price when price > 51 && price < 100 => 0.29

be considered as not covering all cases because a price of 50 is not less than 50 nor greater than 51, so it matches neither the first nor second condition?


#7

@jdeisenberg haha yes you are right. Thanks for pointing that out!


#8

This… this is one of those use cases where property based testing is soooo helpful :slight_smile: Good catch!


#9

Hello :wave:

Just came here to tell what I would probably do in that scenario. If there’s nothing else to check on conditions besides price range I would just have a list with the values and find the one that matches the price.

let priceTierCalc = price => {
  let tiers = [
    (0, 50, 0.33),
    (50, 100, 0.29),
    (50, 200, 0.26)
  ];

  let (_, _, discount) =
    List.find(((l, h, _)) => price > l && price <= h, tiers);

  discount *. float_of_int(price);
}; 

Bonus points, List.find already throws Not_found if no match is found.
I like this better, because adding/updating tiers is now easy.


#10

I love seeing different approaches! Thank you for sharing!!