zlacker

Good article, but one (very minor) nit I have is with the PizzaOrder example.

    struct PizzaOrder {
        size: PizzaSize,
        toppings: Vec<Topping>,
        crust_type: CrustType,
        ordered_at: SystemTime,
    }

The problem they want to address is partial equality when you want to compare orders but ignoring the ordered_at timestamp. To me, the problem is throwing too many unrelated concerns into one struct. Ideally instead of using destructuring to compare only the specific fields you care about, you'd decompose this into two structs:

    #[derive(PartialEq, Eq)]
    struct PizzaDetails {
        size: PizzaSize,
        toppings: Vec<Topping>,
        crust_type: CrustType,
        … // additional fields
    }

    #[derive(Eq)]
    struct PizzaOrder {
        details: PizzaDetails,
        ordered_at: SystemTime,
    }

    impl PartialEq for PizzaOrder {
        fn eq(&self, rhs: &Self) -> bool { 
            self.details == rhs.details
        }
    }

I get that this is a toy example meant to illustrate the point; there are certainly more complex cases where there's no clean boundary to split your struct across. But this should be the first tool you reach for.

>>stouse+(OP)
You have a good point there, that is better. But it is still, well honestly, wrong. Two orders ordered at different times are just not the same order, and using a typeclass approach to say that they most definitely are is going to bite you in the back seat.

PartialEq and Eq for PizzaDetails is good. If there is a business function that computes whether or not someone orders the same thing, then that should start by projecting the details.

>>hurril+db
You can solve this in the general case by implementing the typeclass for the coarser equality relation over an ad-hoc wrapper newtype.

>>hurril+db
I do agree that implementing PartialEq on orders in this way is a bad fit. But it is a synthetic example to make a point, so I tried to keep it in the spirit of the original article (while ironically picking nits in the same vein myself).

>>hurril+db
Yeah, I immediately twitched when I saw the PartialEq implementation. Somebody is going to write code which finds the "correct" order and ends up allowing someone to order the same pizza but get yours, while you have to wait for it to be made and cooked again.

It's not difficult to write the predicate same_details_as() and then it's obvious to reviewers if that's what we meant and discourages weird ad-hoc code which might stop working when the PizzaDetails is redefined.

>>zozbot+2l
Well it isn't a good call. This is the kind of code that OOP makes people write.

>>stouse+(OP)
Decomposing things just to have different equality notions doesn't generalize.

How would you decompose a character string so that you could have a case-insensitive versus sensitive comparison?

:)

>>kazina+VD
> How would you decompose a character string

With a capitalization bit mask of course!

And you can speed up full equality comparisons with a quick cap equality check first.

(That is the how. The when is probably "never". :)

>>Neverm+Vb1
Don't forget to store the locale used for capitalization, too.

>>hurril+db
> But it is still, well honestly, wrong. Two orders ordered at different times are just not the same order

I probably don't have enough context but whatever identity makes up "your order" goes in the PizzaOrder and not the PizzaDetails. The delivery address, for example, goes in the PizzaOrder.

>>kazina+VD
Right, I did note that this decomposition isn’t always applicable. But it often is, and you should default to that when possible.

>>stouse+(OP)
While better, a person modifying PizzaDetails might or might not expect this change to affect the downstream pizza deduplication logic (wherever it got sprinkled throughout the code). They might not even know that it exists.

Ideally, imho, a struct is a dumb data holder - it is there to pass associated pieces of data together (or hold a complex unavoidable state change hidden from the user like Arc or Mutex).

All that is to say that adding a field to an existing struct and possibly populating it sparsely in some remote piece of code should not changed existing behavior.

I wonder whether there's a way to communicate to whoever makes changes to the pizza details struct that it might have unintended consequences down the line.

Should one wrap PizzaDetails with PizzaComparator? Or better even provide it as a field in PizzaOrder? Or we are running into Java-esq territory of PizzaComparatorBuilderDefaultsConstructorFactory?

Should we introduce a domain specific PizzaFlavor right under PizzaDetails that copies over relevant fields from PizzaDetails, and PizzaOrder compares two orders by constructing and comparing their flavours instead? A lot of boilerplate.. but what is being considered important to the pizza flavor is being explicitly marked.

In a prod codebase I'd annotate this code with "if change X chaange Y" pre submit hook - this constraint appears to be external to the language itself and live in the domain of "code changes over time". Protobufs successfully folded versioning into the language itself though. Protobufs also have field annotations, "{important_to_flavour=true}" field annotation would be useful here.