3 ms·

I feel like there a happy medium between the two, the left can easily be made more simple by factoring out one or two functions however the right went too far.

by jpc0 3y ago

I feel like there a happy medium between the two, the left can easily be made more simple by factoring out one or two functions however the right went too far. The prepare and addtoppings functions should be one function, prepare effectively just fills in a struct and calls add toppings, its pointless to seperare them. The Bake function simply prepares the over for cooking, which the author mentioned should be a dependency with a method and then factors 4 lines of code into a new function for no reason. The bake and bake pizza function should be one function. You can then keep the box function as is. That would be both easier to maintain and easier to read.

erhaetherth 3y ago
> You can then keep the box function as is. The box function is broken too. You box the pizza and then return the pizza...but the box is logically a wrapper for the pizza. `box(pizza)` should return a boxed pizza. A box with contents=[pizza]. Maybe some sauce and pepperoncini in there too. Plus all these functions are impure. Which isn't always bad but if you can prevent things like boxing it before baking it, you should. And what even... this entire example is just horrendous. You box the pizza and then slice the pizza? Ready = box.Close()? Can the Close() operation fail? And then the pizza is not ready? Why not throw an error, now the caller has to check if the pizza that got returned to them is even ready...? And that fact is even more hidden on the right side. Same for Sliced and Boxed.
- jpc0 3y ago
  This entire function is clearly a factory for a boxed pizza with toppings which is baked. I'd argue the entire box.Close() method is slideware and wouldn't exist since it likely is just a return true. You can just as easily just say pizza.Ready = true. Reading this code afterwards I would think there was some stupid requirement somewhere for a pizza.Ready property so someone added it and would check a commit log to see if it can just be removed. Decent catch there though, the box can also be a dependency that get's passed in.

by jpc0 3y ago

erhaetherth 3y ago

> You can then keep the box function as is. The box function is broken too. You box the pizza and then return the pizza...but the box is logically a wrapper for the pizza. `box(pizza)` should return a boxed pizza. A box with contents=[pizza]. Maybe some sauce and pepperoncini in there too. Plus all these functions are impure. Which isn't always bad but if you can prevent things like boxing it before baking it, you should. And what even... this entire example is just horrendous. You box the pizza and then slice the pizza? Ready = box.Close()? Can the Close() operation fail? And then the pizza is not ready? Why not throw an error, now the caller has to check if the pizza that got returned to them is even ready...? And that fact is even more hidden on the right side. Same for Sliced and Boxed.

jpc0 3y ago
This entire function is clearly a factory for a boxed pizza with toppings which is baked. I'd argue the entire box.Close() method is slideware and wouldn't exist since it likely is just a return true. You can just as easily just say pizza.Ready = true. Reading this code afterwards I would think there was some stupid requirement somewhere for a pizza.Ready property so someone added it and would check a commit log to see if it can just be removed. Decent catch there though, the box can also be a dependency that get's passed in.