Stephan van Hulst wrote:I'll tell you the same thing I've told Liutauras. If you create your work on separate branches and then make pull requests when you want your code to be reviewed, it's much easier to review and keep the comments in one place.
Let us know what commit on master you want to use as a baseline for an initial review.
salvin francis wrote:Offsets are always going to be the same for square block systems (unless you're planning for a hexagonal system or similar) . Maybe it should be a static final variable ?
salvin francis wrote:Nitpick ...
World.java --> setNeighbors
Offsets are always going to be the same for square block systems (unless you're planning for a hexagonal system or similar) . Maybe it should be a static final variable ? I know that you are just calling that method in the constructor iteration. But, each iteration will create a new array.
I am looking forward to run your code on my machine and see it in action !! Will update you soon.
Liutauras Vilda wrote:I'm not going to question design in general, I consider it just as one of the possible designs. Certainly there are many, some are more flexible, some are less, some are more concise, some less. So will go through what you have for now.
Gerard Charles wrote:2. Cell.setNeighbors(Collection<Cell> neighbors)
No parameter validation.
* What would this look like?
Gerard Charles wrote:5. ClassicCell.compute() very ciphered implementation. Without knowing rules, would have hard times deciphering cell lifecycle's idea.
5.1 Would avoid fall-through.
* Good thing the rules are posted as a comment right above then?
Gerard Charles wrote:8. World constructor. Inconsistent use of 'this', cell assignment is missing that one.
* "this" is required when a parameter (or local variable) of the same name is present. It's not needed
Gerard Charles wrote:9. World constructor. Allows create world in inconsistent state. i.e.: negative width and height. Actually that would result in crashing system. Also missing function parameter validation which could result in NPE.
* What alternative would you suggest?
Liutauras Vilda wrote:..That would look dangling in a World class with no clear indication why is it for. ... Maybe misassigned responsibility?
Gerard Charles wrote:
Junilu Lacar wrote:I will leave it to you guys to smell out the rest of the World constructor, particular the parts where iterate is being called.
Let me give you a hint though. Consider this code and what the JavaDocs declares it does:
How is someone supposed to see how a Cell method is being called on each cell here? Where is the reference to the Cell whose method is invoked? Where is the reference to the method being called? If you can't tell me where those are in this snippet of code, then that means this code is blatantly displaying an inappropriate intimate knowledge of implementation details that are elsewhere in the code.
Junilu Lacar wrote:Names like WorldConsumer and CellConsumer are smelly.
Why not Consumer<World> or Consumer<Cell> instead?
See the difference?
Junilu Lacar wrote:I'm going to walk through the code to explain in detail why I think the World class is very smelly.
In Demo.java, we see
I have already expressed my objection to this code before but let me reiterate: The semantics of the lambda is unclear. It smells of inappropriate knowledge of intimate details of the implementation of World. I get the first two parameters, they define the boundaries of the new World object. But WTH is this lambda expression trying to say? It looks like it is returning a new ClassicCell() instance but what's with the (x, y) parameters? And why aren't they even used? This is a red flag to me that the design is not coherent. There should be no elements in the design that are not used.
Gerard Charles wrote:Generics don't work with primitives.
What's the definition of smelly in the context of code?
Piet Souris wrote:The objection is that these are not used in the right part of the lambda. Why is that a code smell?