4 ms·
> there's still many loops where writing it as a loop makes it more clear what's going on I think that really depends on your language and what you are used to
by valenterry 5y ago
> there's still many loops where writing it as a loop makes it more clear what's going on
I think that really depends on your language and what you are used to. For me, I would disagree.
> I'm not sure how to implement the consolidation only in terms of map, filter and friends in a way which would be more clear.
Tbh, when I read this, I'm thinking: is the data-structure wrong? Maybe you should have something like a "bundle" that contains orderItems and their invoice. It doesn't seem to make so much sense to keep an invoice ID in each orderItem. That makes sense in a relational db, but not when using objects.
I would model it like that (for lack of a better name than "bundle"):
Order(bundles: Map[InvoiceId, Bundle(invoice: Invoice, orderItems: List[OrderItem]))
Now orderItems and the invoice-amount are logically bundled together and identified by the invoice-id. Then, combining two orders becomes really simple: combinedOrder = order1 + order2
No loops needed, simply treating the data as monoids.
Here's an executable example: https://scastie.scala-lang.org/DDIRbiC5TTuOxmkzEFxTqA https://scastie.scala-lang.org/DDIRbiC5TTuOxmkzEFxTqA
You might argue that it is not clear how that works and you are right for everyone who is not used to how monoids work. But everyone who is, independent of the language, will understand exactly what happens. On the other hand, someone who is not used to loops would probably argue that your loops are hard to understand.
It really boils down to what techniques one is familiar with.
- magicalhippo 5y agoI think I get your point, however the real world is muddy... > I would model it like that How does your model take into account orders where order lines do not map to an invoice, or which has invoices which no order lines references? edit: interesting example btw, I do like a lot of aspects and it was fun seeing some "practical" Scala, haven't had a chance to dig in much. Much appreciated!
- valenterry 5y ago> How does your model take into account orders where order lines do not map to an invoice, or which has invoices which no order lines references? If I understand you right, the latter seems easy: the list of orderItems would just be empty. If there can be orderItems without invoice (why would that happen?) then I'd just add another field "plainOrderItems: List[OrderItem]" next to the bundles. The rest of the code would not change. That being said, there can surely be situations where modeling the data is not trivial. It's always exciting to model it in the best way to describe the problem at hand. And with this approach, sometimes you need to model different "views" to make combining things work. > edit: interesting example btw, I do like a lot of aspects and it was fun seeing some "practical" Scala, haven't had a chance to dig in much. Much appreciated! You are welcome :)
- magicalhippo 5y ago> If there can be orderItems without invoice (why would that happen?) Because people mess up... In this case they'd then fix the missing invoice references post-consolidation. Stuff like this usually happens either due to stress, lack of experience or training (summer temps are a menace) or just negligence. In addition to our consolidation procedure, I've naturally had to write a procedure to undo the consolidation. > then I'd just add another field "plainOrderItems: List[OrderItem]" next to the bundles. That could work, would be more tedious when you want to operate over all items (we do this fairly often), but not terribly so.
- valenterry 5y agoOkay, interesting. > That could work, would be more tedious when you want to operate over all items (we do this fairly often), but not terribly so. Hm, not really. I've updated the code with a convenience method to see how it feels and I don't think it impacts the ergonomics at all: https://scastie.scala-lang.org/QG1VYuFBQ4qRwfJSjq7bjA https://scastie.scala-lang.org/QG1VYuFBQ4qRwfJSjq7bjA