9 ms·
I prefer imperative code over collector like APIs for two reasons, both visible in this example: 1. The intention of the code is obscured by more layers of abs
by Laremere 5y ago
I prefer imperative code over collector like APIs for two reasons, both visible in this example:
1. The intention of the code is obscured by more layers of abstraction, increasing complexity.
2. Small changes to what the code is conceptually doing tend to lead to larger changes to the actual code than with imperative code.
For the first point, reading through this post and the previous post linked, the code is doing the following:
Taking the entries of a map of product to count. Turning those entries into a different class, a row of each product and count. Then it collects this list on one hand. On the other hand, it sums the total, based on the per product cost and the count of the product in the cart. Then it combines the row list and the total cost into one object, returning it.
Deconstructed, half of this code is just unnecessary complexity. A map of products to count, and a list of unique product/count tuples are theoretically identical. You can iterate over them, find specific products, etc. There /might/ be a reason to specifically desire a list; However why would that code be coupled with code to sum the cost of the cart?
All in all, why is the code not just:
public BigDecimal sumPrice(Cart cart) {
BigDecimal sum = BigDecimal.ZERO;
for (Map.Entry<Product,Integer> entry : cart.getProducts.entrySet()) {
sum = sum.add(entry.getKey().getPrice().multiply(new BigInteger(entry.getValue())
}
return sum;
}
For the second point, briefly: Consider how code would have to change to calculate a deal, such as Buy One Get One Free. Such code to do this calculation would add another layer to the collector with another function defined somewhere (or hidden in some other existing abstraction, such as OP's CartRow::getRowPrice) instead of just visible in the function that calculates a cart subtotal. If the deal relied upon concepts not limited to one row at a time, eg buy any two flavors of chips for %25 off, the proposed solution would have to be completely rewritten.
- gnud 5y agoYou don't want the "buy one get one free" to apply directly to the cart subtotal. You want to show the customer which entry is free (or discounted), so they can see that the rebate is applied correctly. You probably want to check all 'rebate rules' attached to all products in the cart whenever one is added/removed. An active rebate will then modify the price of the cart item, and the sum method can be stupidly simple.
- hathym 5y agostreams are best used against data with an unknown size (potentially infinite) that isn’t necessarily all held in memory or cannot fit the memory limits.
- grumpyprole 5y agoThe "teeing" combinator is giving you separation of concerns. There are two separate calculations, potentially defined in separate functions, that are composed into a single operation that is performed in a single pass of the stream. Sure, you could write a monolithic imperative for-loop that does it all, but such an approach will not scale.
- voidifremoved 5y agoThe original is almost impossible to debug when something inevitably goes wrong too. I have encountered dozens of places where streaming API calls have been reworked into imperative by whoever ends up maintaining it just so they can figure out why the hell it is breaking in some unforseen edge case.
- willtim 5y agoSide-effects do not mix with lazy on-demand streams! This is unfortunately a problem with bringing functional programming constructs to a language with idiomatic pervasive mutation.
- lostmsu 5y agoBTW, this is a non-issue with C# 1.0 and later due to `yield return` syntax.
- twhitmore 5y agoSorry, I'm skeptical -- how exactly does a 'yield return' resolve the divergences between functional programming & mutable data structures? My layman's impression was that 'yield return' was largely syntactic sugar around an iterator, rather similar to Java.
- lostmsu 5y agoThe comment you responded too were talking about debug experience.
- kitd 5y agoI think your point is valid with a trivial example as shown in the article. In a more complex example, with multiple steps and potentially multiple streams, using the Streams API allows the algorithmic and business logic parts of the whole calculation to be kept separate, more visible and more maintainable/adaptable as a result. Imperative code in such cases usually involves factoring out chunks of the for-loop into separate functions or classes which IME goes in the opposite direction.
- kimi 5y agoWhile I understand the reason of Streams and collectors, the Java implementation is really ugly and obscure. It is a language in itself. I want to like them, but I really can't. On the other hand, in Clojure they are totally natural.
- vincnetas 5y agobeauty is in the eye of the beholder
- deleted 5y ago[deleted]
- deleted 5y ago[deleted]
- deleted 5y ago[deleted]
- bloat 5y agoTrue, sometimes imperative loops are easier than streams. But your code is not a suitable substitute in this case where the constraints are clear. He does want the total sum, and he does want the list, and he does only want to iterate once.
- agilob 5y ago>All in all, why is the code not just: Because it's hard to review, too many things are happening on one line. What if you have to negate multiplication? You're messing that line even more, with streams, you're adding one line that's easy to read, easier to review, track changes.
- watwut 5y ago> What if you have to negate multiplication? Add sum *= -1; on the next line if you think the line is already too complex. Or just do return -sum;
- agilob 5y agoThe example is using BigDecimal class where arithmetic operators don't work, you need to call .negate() on the object, or multiply again BigDecimal("-1"). Negating inside a loop will give different result than multiplying at the end in return line. Nevertheless, you're adding ANOTHER operation to the line which is already complex and hard to read.
- watwut 5y agoThe exactly same thing can be said about stream version. There is no meaningful difference between the two when it comes to this change - either you do it in the middle of loop or add line for it. Except that the stream version is harder to debug and read, but more effective on potentially unlimited stream.
- throwaway4good 5y agoWhat you write could be written as: ...stream(). map(e -> e.getKey().getPrice().multiply(new BigDecimal(e.getValue()))). reduce(BigDecimal.ZERO, BigDecimal::add) The problem here is not the use of streams but that the author goes at the problem in a confusing and round about way.
- theshrike79 5y agoThat's very pretty to look at and easy to read. But it's a huge pain in the ass to debug when getPrice starts failing. You get a 42000 line stack trace and the only information is that it failed somewhere in that long singular line. The first thing you do is unravel all that crap to a normal loop so you can debug it properly. And when you're done, you really don't want to go back to the stream way of doing just in case it breaks again and you need to start debugging once more.
- throwaway4good 5y agoWhy would getPrice start failing? If you have side effects or anything other than a trivial implementation inside getPrice, you have more fundamental problems with what you are building than whether to use a for loop or streams.
- mbreese 5y agoWhy does any code start failing? That’s not the point they were making. The point is that when you start chaining together so many calls, it can become difficult to debug. Prettier to write, but more difficult to debug. But that’s okay, you need to find the balance that’s appropriate for the particular project.
- throwaway4good 5y agoFirst. If getPrice fails, the stack trace will start there. If it returns null (which it should not) and trigger a npe, then the line: map(e -> e.getKey().getPrice().multiply(new BigDecimal(e.getValue()))). Is just as dense as the original: sum = sum.add(entry.getKey().getPrice().multiply(new BigInteger(entry.getValue()) And the stack trace would be just as confusing. Second. The key thing with streams is to borrow from the functional programming paradigm: Split data and functions, avoid or isolate side-effects. Do this correctly and there is a quite real plus in productivity.
- IceDane 5y agoYour advice is OK for when you are doing a one-off, simple thing. In general, though, streaming abstractions leave that paradigm in the dust once things get become just a tad more complicated. Using streaming abstractions, you can reason about the flow of your data at a much higher level and create abstractions which would require 100s or 1000s of lines of code to duplicate. For example, if he decides he needs to buffer the stream, then perform processing on it, then send it out into to the world, he can do all of this super easily by using a few streaming combinators while everything is super clear and type-safe.
- watwut 5y agoAll of that is good when you need it. None of that is good when you don't actually need higher level reasoning about data.
- shock 5y agoWhy do you need to program at a higher level than assembly?
- watwut 5y agoToo much abstraction harms as much as too little abstraction. And java is exactly environment that made "too much abstraction to the point of hurting maintainability" in the past. If you need higher level abstraction, you should use it. If you dont need it, you absolutely should not use it.
- le-mark 5y agoThis x1000, we are not writing code for ourselves, we write code for our employers, and it behooves us all to keep it as simple and maintainable as possible. Will you be there in 10 years to explain it, will you want to if you are? If this is your open source project or whatever do as you please, otherwise follow the principal of least abstraction.
- skohan 5y ago
- davewritescode 5y agoThe first point I won't refute. Some people like imperative style and that's fine. I personally don't and find the stream based approaches much simpler. As for your second point, you could easily add such functionality to the code with a stateful mapping operation that sets the price to 0 for every other item encountered. In your code, you'd have to add another pass of the loop or you'd have to stick the logic for computing the price inside your for loop. Personally, I've found that decomposing problems into stream based pipelines makes it much easier to decorate additional functionality than imperative code but that's just my personal experience. public PriceAndRows getPriceAndRows(Cart cart) { DiscountApplier discounts = new DiscountApplier() return cart.getProducts() .entrySet() .stream() .map(CartRow::new) .map(cartRow -> discounts.apply(cartRow)) // Stateful discount application logic .collect(Collectors.teeing( Collectors.reducing(BigDecimal.ZERO, CartRow::getRowPrice, BigDecimal::add), Collectors.toList(), PriceAndRows::new )); }
- a_imho 5y agoAnother point to consider that is strictly superior is how streams offer ~effortless parallelism.
- nindalf 5y agoNot Java, but an example of streams enabling parallelism in another language - https://developers.redhat.com/blog/2021/04/30/how-rust-makes-rayons-data-parallelism-magical/ https://developers.redhat.com/blog/2021/04/30/how-rust-makes.... Change 4 characters and it goes from saturating one core to saturating all.
- g051051 5y ago> All in all, why is the code not just: In the other code, you could substitute parallelStream for stream and have it execute in parallel for "free".
- stickfigure 5y ago> Consider how code would have to change to calculate a deal, such as Buy One Get One Free. There's nothing wrong with passing in a function other than `CartRow::getRowPrice`. That's one of the beautiful things about functional programming, you can alter behavior by passing different functions as parameters. However, as someone who writes ecommerce code that deals with exactly this situation all the time, I can tell you that the question doesn't really make sense. Discounts are usually represented by data in the cart - either a special field, or a negative line item that gets summed along with the others. You don't just mysteriously have a second cheaper item in the cart.