Granny's Programming Pearls
"inside of every large program is a small program struggling to get out"
JavaRanch.com/granny.jsp

Gerard Charles

Ranch Foreman
+ Follow
since Jan 21, 2010
Cows and Likes
Cows
Total received
8
In last 30 days
3
Total given
0
Likes
Total received
2
Received in last 30 days
1
Total given
8
Given in last 30 days
4
Forums and Threads
Scavenger Hunt
expand Ranch Hand Scavenger Hunt
expand Greenhorn Scavenger Hunt

Recent posts by Gerard Charles

Junilu Lacar wrote:One pair I worked with at Global Day of Coderetreat last Saturday had code like this:

That pair was actually working in Python but the code they had was essentially the same as that Java code.

My question to folks here is this: Would you see this code and think, "Oh, code smell..." or would you think it's a reasonable API?

I'd like to get some other opinions/perspectives before sharing what mine are.

Thanks!



Nope, doesn't smell at all.

Code can't "smell." It's conceptual, inorganic text that doesn't emit molecules. As near as I can tell, "smell" means "I don't like it / I wouldn't have done it this way." It's an opinionated, pejorative term that tells me the speaker is unable or unwilling to communicate in a professional manner.

As far as the method call, from looking at it, I have  pretty good idea what it's going to do. It mirrors the pattern found in the JDK graphics API (e.g. fillText("Cow", 42,54). If I have any doubt, I look at the declaration, because that's where functions are described in Java. Good IDEs will jump you there quickly and/or show the API in a hover. JetBrain's Intellij will fill in the parameter names for you.
3 weeks ago

Liutauras Vilda wrote:

Gerard Charles wrote:some of those neighborLocation values will be negative, but because the code calls worldWrapped they become positive. Logically, I'd expect setAlive to do the same thing.


This is what it does.


My bad, was looking at the old version.
3 weeks ago

Liutauras Vilda wrote:

Gerard Charles wrote:because if the GUI's calculated location of a point is negative


Wondering how that could happen? Does it work like x and y axis where 0 is in a center? My model follows rows, columns idea starting from the upper left corner where 0's are.


Well, it happens in:

some of those neighborLocation values will be negative, but because the code calls worldWrapped they become positive. Logically, I'd expect setAlive to do the same thing.
4 weeks ago

Piet Souris wrote:We have arrived at a point where the discussion is hard to understand, if at all.

Anyway: what I was wondering: if incorporating some GUI makes it necessary to adjust the World code in places, then what is wrong with, or missing from, the design?



Well, probably easiest to run my proposed variant to see what the interface is doing first and then look at how GraphicDisplay is trying to allow CShape to place a pattern on the world.

But it wasn't necessary to adjust the World, nor is their necessarily anything wrong with the design -- I just playing around with merging two different designs. Personally I thought the question: Hey, world, what's the coordinates of the Location four to the left of this one? was a reasonable one to ask.
4 weeks ago

Liutauras Vilda wrote:

Gerard Charles wrote:It include's a drop down to include a couple gliders. The implementation of that needs to be able to add
locations modulo the grid, which is why the code is calling worldWrapped.


I don't think so. API does that already, you don't need to do that in a front-end. You just need to pass pattern's alive cells locations.

Gerard Charles wrote:The GUI intentionally messes with the world -- it allows the user to click on the grid and toggle cells on or off


Understand that, but since my nextGeneration() always construct a new world, for me it makes sense to have all in tact and don't rely on GUI implementor's vision. If GUI wants to change something, it can change through aliveAt(), deadAt(), that way I can assure that world is in consistent state. So getWorldMap() actually supposed to return immutable map.


Actually I didn't need a public worldWrapped since I added an "add" method, but that does need worldWrap, because if the GUI's calculated location of a point is negative (e.g. when placing a glider) the world API doesn't wrap the left side to the right (or top to bottom)-- actually fails silently due to the Math.abs in the implementation.



4 weeks ago

Liutauras Vilda wrote:
World.java Cell at(Location) as well as worldWrapped(Location) instead private became both public. For what reason?
...

2. Map<Location, Cell> getWorldMap(), and that would be a copy of original, so front end couldn't mess up with world's state.


The GUI intentionally messes with the world -- it allows the user to click on the grid and toggle cells on or off (apparently incorrectly with the incorrect deadAt).
It include's a drop down to include a couple gliders. The implementation of that needs to be able to add
locations modulo the grid, which is why the code is calling worldWrapped.

4 weeks ago

Liutauras Vilda wrote:

Gerard Charles wrote:I'm am curious as to your thought process in the static Location.of instead of simply having a public Location(int rowIndex, int columnIndex) constructor?


If you get a chance, read an Item 1 from Effective Java by Joshua Bloch. It has comprehensive chapter about that, advantanges and dis

However, my main reason was readability in this case. I really like that way, which seem to go in line what newer Java version follow as as naming convention, i.e.: List.of(...), Map.of(...).... Yes, new Location() is also fairly readable in this case.

One of other advantages could be, that I'm still left with an option to cache Location objects if I wanted to in a future, while habitual way to create object through constructor does not  give you such flexibility, whenever you write "new" you create a new object.

Don't want to list other advantages and repeat what is way better written in a book, but there are quite a few advantages over constructors.



Interesting read. (found a copy Effective Java). (Of course, we no longer have to specify all the types in generic creation, a development Bloch seemed to anticipate).

I'm just used to the standard Java library conventions (e.g. Point2D constructor), so that's what I'd expect to find in a "point" class.
4 weeks ago
So I was curious at to what it would take to connect my JavaFX front end to this implementation. (See pull request on github.) Fairly straightforward, except a a couple tweaks:
* I needed a deadAt method to mirror aliveAt.
* Had to make some methods public to set / access the state of the World.

I'm am curious as to your thought process in the static Location.of instead of simply having a public Location(int rowIndex, int columnIndex) constructor?
4 weeks ago

Junilu Lacar wrote:Names like WorldConsumer and CellConsumer are smelly.

Why not Consumer<World> or Consumer<Cell> instead?

See the difference?



Consumer is already defined in the JDK. Generics don't work with primitives.

What's the definition of smelly  in the context of code?
1 month ago

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.


Oh, that's obviously wrong. Updated master:
1 month ago

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.


Replies interpersed below
1.
Cell field states[]
For me this variable name is misleading. It really represent not just states, but current and next generation's state, for me that is subtle, but very important distinction.
* Changed it to generations
2. Cell.setNeighbors(Collection<Cell> neighbors)
No parameter validation.
* What would this look like?

3.
For me such method gives an impression that Clock has no clear use. So once assert passes, use clock's cycle to access index, not hard code it.
* Good idea -- in fact, adding an explicit method to Clock can make the intent clearer. Revised code:



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? Personally, I'm happy with the full functionality of the switch statement, including fall-throughs where they make sense, but I understand opinions vary.
6. ClassicCell. Why it is public and non final? Same as with most of other classes.
* For me, that's going to be the default implementation unless there's a compelling reason to make it otherwise.


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 for cell. (I understand some programmers don't like C++/Java's implicit this but I'm fine with it.)

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?

10. Knowledge duplication by passing in x, y as coordinates. Why not to have wrapped in a class? If new coordinate would appear, you'd need to re-design whole system instead of just potential class.
* I'm going to ponder that one.

12. World.setNeighbors, World.adjacent(). Too complex personally for me. Poor variable names, i.e.: nx, ny, xypar,  Wouldn't be obvious from maintenance point of view. No parameters check.
* nx is scratch variable name with a scope of three lines. A rose by any other name...

13. ArrayList<Cell> neighbors = ... Keep List<Cell> neighbors = ... as implementing class may change in a future, would be less hassle to update.
* Good point, fixed.

I've pushed  new version with suggested change to the master branch: https://github.com/Gerardwx/conway/tree/master/conway
1 month ago

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.


That's a good point. I find myself wondering whether a JVM could / would optimize the initialization somehow (see https://shipilev.net/jvm-anatomy-park/6-new-object-stages/ for some interesting discussion of what the JVM can /can't do). But more importantly your post suggests an optimization path and an opportunity for making the design more extendable. I changed setNeighbors to take the offsets as a parameter (and incorporated other good suggestions in the thread; declare neighbors as List instead of ArrayList and adding the missing <>)

and then changed the World constructor to protected, with an offsets parameter:

I then added subclass ClassicWorld as shown, using a static method to create the block array one time:
1 month ago

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.



Good idea. I created a branch at  https://github.com/Gerardwx/conway/tree/review for the review.
1 month ago
Following up on thread https://coderanch.com/t/40/700311/code-reviews/engineering/version-Conway-Game-Life, I've implemented an alternate version, mostly because I find it easier to express my thoughts in code rather than prose. The
problem is stated at https://en.wikipedia.org/wiki/Conway%27s_Game_of_Life#Rules

https://github.com/Gerardwx/conway/

There's a console main in Demo.java, and a JavaFX based on in GraphicDemo.java .
1 month ago

Liutauras Vilda wrote:

Gerard Charles wrote:

Think for a moment how would you extend your system with more states than just alive/dead. I'm still bogged with this hypothetical future requirement


I'd most likely replace the boolean with an enum, depending on what the additional states turn out to be. A sometimes alternative is replacing a boolean with a Boolean (nullable, so it has three possible states) -- maybe if we have Schrodinger cat cells that are neither alive nor dead until queried? But dealing with nulls is often more trouble than its worth, so a enum is more likely.

1 month ago