Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SatelliteData class not encouraging immutability #43

Open
aaiezza opened this issue Jan 11, 2019 · 3 comments
Open

SatelliteData class not encouraging immutability #43

aaiezza opened this issue Jan 11, 2019 · 3 comments

Comments

@aaiezza
Copy link

aaiezza commented Jan 11, 2019

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
Copy link
Member

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
Copy link
Author

aaiezza commented Jan 12, 2019

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants