Hexworks / mixite

A GUI agnostic hexagonal grid library. Supports a multitude of grid layouts including hexagonal, triangular, rectangular and more.
Apache License 2.0
191 stars 31 forks source link

SatelliteData class not encouraging immutability #43

Open aaiezza opened 5 years ago

aaiezza commented 5 years ago

This might not actually be desirable for other implementations right now, but it might be good to include in the next major version release. Basically SatelliteData currently requires that you override setter methods if you implement the interface directly. If the calculators require this benefit of mutability, then there's not a nice way around this. However, I'm trying to define my own class of data for a hex, and I want those objects to be immutable. If the board state changes, I want to be able to place newly constructed satellite data objects on those hexes in the grid.

So, this is barely a big deal since I can still just override them to have them do nothing anyway, but it does look like this:

public class MySatelliteData implements SatelliteData
{
    . . .

    public void setPassable( final boolean passable )
    {
        return;
    }

    public void setOpaque( final boolean opaque )
    {
        return;
    }

    public void setMovementCost( final double movementCost )
    {
        return;
    }

    . . .
}

Definitely just being nit-picky with this one. Immutability is definitely a good practice to be eliciting. It does result in a tad less flexibility from this library, but you are probably more than likely saving a consumer from potentially getting themselves into trouble via mutability.

adam-arold commented 5 years ago

Why do you think that it gets less flexible because of the mutability? I agree nevertheless. It is not necessary for these properties to be mutable. I'll fix this in the next release!

aaiezza commented 5 years ago

Hey @adam-arold, sorry. I'm it is more flexible to leave these fields mutable. It just seems less than ideal since immutability is more favorable.

Cool to hear! I look forward to the release. Ping me if you'd like a code reviewer.

drekbour commented 4 years ago

I had a think about this as I made some other changes and came to the following conclusion: #52 SatelliteData inteface should be optional.

WRT this issue: mutability is a user choice, do they wish to store a single instance of the data (and mutate it) or keep re-storing immutable classes. This is a HUGE design decision in the modern multi-core world and depends on context that Mixite cannot know. I really like that Mixite is entirely linear, entirely read-only. Any threading concerns only relate to the storage backend and should be the user's problem to handle. This should be well documented in the readme / source.

Apart from those docs, IMO #52 supercedes this issue.