6
\$\begingroup\$

I have a Location type:

public abstract class Location implements Locatable {
    private int x, y;

    protected final void setX(int x) {
         this.x = x;
    }

    protected final void setY(int y) {
         this.y = y;
    }

    public final int getX() {
        return x;
    }

    public final int getY() {
        return y;
    }
}

interface Locatable {
    int getX();
    int getY();
}

Location has protected mutator methods, to ensure I can provide an MutableLocation type, but I'm not sure if this violates the Interface Seggregation principle when creating an ImmutableLocation.

I feel it doesn't, since the client will never have access to the setters through ImmutableLocation (cannot call it from an instance, cannot call it from a subclass of ImmutableLocation). So this was my attempt at a SOLID location type, which I plan on using in a project.

class ImmutableLocation extends Location {
    //...
 }

class MutableLocation extends Location {
    public void adjustX(int amount, IncremeantOperation op) {
        int x = op.adjust(x, amount);

        setX(x);
    }

    public void adjustY(int amount, IncremeantOperation op) {
        int y = op.adjust(getY(), amount);

        setY(y);
    }
}

enum IncremeantOperation {
    INCREASE {
        public int adjust(int target, int amount) {
             return target + amount;
        }
    },
    DECREASE {
        public int adjust(int target, int amount) {
            return target - amount;
        }
    };

    public int adjust(int target, int amount) {
        throw new UnsupportedOperationException("Cannot call adjust(int, int) on unknown operation");
    }
}

Please give your thoughts. My main concerns are:

  1. I'm not sure if the enum is an overkill
  2. If the setters are a violation of SOLID; I'm not sure how else I should do it.
  3. If UnsupportedOperationException is best fit for the adjust method

interface Locatable {
    Location getLocation();
}

public abstract class Location {
    //may add fields, which explains the class

    public abstract int getX();
    public abstract int getY();
}

public final class ImmutableLocation extends Location {
    private int x, y;

    public ImmutableLocation(int x, int y) {
        this.x = x;
        this.y = y;
    }

    public int getX() {
        return x;
    }

    public int getY() {
        return y;
    }
}

public final class MutableLocation extends Location {
    private int x, y;

    public MutableLocation(int x, int y) {
        this.x = x;
        this.y = y;
    }

    public void adjustX(int x) {
         //adjust x
    }

    public void adjustY(int y) {
         //adjust y
    }

    public int getX() {
        return x;
    }

    public int getY() {
        return y;
    }
}

Locatable would be implemented by anything with a location, such as Entity:

abstract class Entity implements Locatable {
    private Location location;

    public Location getLocation() {
         return location;
    }
}
\$\endgroup\$
4
\$\begingroup\$

It doesn't make sense to have setters in an immutable location. Consider this alternative:

interface Location {
    int getX();
    int getY();
}

class MutableLocation implements Location {

    private int x;
    private int y;

    public MutableLocation(int x, int y) {
        this.x = x;
        this.y = y;
    }

    public int getX() {
        return x;
    }

    public int getY() {
        return y;
    }

    public void adjustX(int x) {
        //adjust x
    }

    public void adjustY(int y) {
        //adjust y
    }
}

final class ImmutableLocation implements Location {
    private final Location location;

    public ImmutableLocation(int x, int y) {
        location = new MutableLocation(x, y);
    }

    @Override
    public int getX() {
        return location.getX();
    }

    @Override
    public int getY() {
        return location.getY();
    }
}

Your questions

  1. I'm not sure if the enum is an overkill

Not necessarily. Maybe it is, maybe it isn't. It will depend on the rest of the code in your project. From here it can go both ways.

  1. If the setters are a violation of SOLID; I'm not sure how else I should do it.

They violate SOLID, in particular the interface segregation principle: the immutable class inherits setters but doesn't need them. See my proposed cleaner alternatives above.

  1. If UnsupportedOperationException is best fit for the adjust method

For unsupported operations, this is fine, but your purpose is not clear. Do you intend to add values in this enum without implementing adjust? If yes, then your solution is fine. But if you never intend to add values without implementing adjust, then it would be better to make the method abstract. Also you should use @Override in the implementations:

enum IncrementOperation {
    INCREASE {
        @Override
        public int adjust(int target, int amount) {
            return target + amount;
        }
    },
    DECREASE {
        @Override
        public int adjust(int target, int amount) {
            return target - amount;
        }
    };

    public abstract int adjust(int target, int amount);
}
\$\endgroup\$
  • \$\begingroup\$ Abstract methods in an enum? Magical. Not sure why I didn't try it before. I changed Locatable to force Location getLocation() instead of getX() and getY(), removed the setters from Location so both types can extend it (avoiding alternative classes with different interfaces) as well as the fields. But this requires MutableLocation and ImmutableLocation to declare their own x and y fields. Something about that is screaming out "code smell"; I could be wrong. I'll add the changes to the bottom of my question, incase my explanation was bad. \$\endgroup\$ – Vince Emigh May 17 '15 at 16:40
  • \$\begingroup\$ Edited my post, please take a look :) Would that be considered a code smell? It seems pretty tedious that every Location has an x and y property, but you must manually declare the fields in subclasses. It seems "off", as in I shouldn't need to define the fields for every Location subclass \$\endgroup\$ – Vince Emigh May 17 '15 at 16:50
  • \$\begingroup\$ I thought about this some more, and added an alternative that seems to fit the bill, and maybe even ergonomic. \$\endgroup\$ – janos May 17 '15 at 20:15
  • \$\begingroup\$ Of course! A proxy pattern, not sure why I didn't think of it, seeing how I answered a question for ImmutableMap on SO suggesting it. Although I believe interfaces should end with an adjective, describing the ability to perform some behavior. It's more of a philosophical thing, so I'll stick with Location being a class, unless there's a good reason not to. I know it doesn't allow the specific locations to extend something else, but it doesn't seem as if they should anyways. To me, it makes sense to use adjectives for interfaces and nouns for classes. Any final thoughts? \$\endgroup\$ – Vince Emigh May 17 '15 at 20:24
  • \$\begingroup\$ Nope. Try it and see how it works for your case. \$\endgroup\$ – janos May 17 '15 at 20:27
3
\$\begingroup\$

To ensure immutability, you must make the class final or define a package-private constructor (it's assumed that somebody sneaking in your package is a sin freeing you from all your promises).

The big problem with your ImmutableLocation is that it's not thread-safe as an immutable class should be. No final fields, no safe publishing.

interface Locatable {

I don't see what you need it for. A Location as an abstract class is good enough, unless you want to let people add their own implementations.

  1. I'm not sure if the enum is an overkill

And you won't be sure until it's used a lot. There's typo in its name (IncremeantOperation).

  1. If the setters are a violation of SOLID; I'm not sure how else I should do it.

Not having them in the immutable class is the best option. You could make the fields protected and add the setters in the mutable subclass... but this isn't better either.

  1. If UnsupportedOperationException is best fit for the adjust method.

It should be abstract, unless there's a very good reason for not implementing it in a member. But what should be such a member good for if it does not implement the only operation it should implement?

However, UOE is for me always a sign of an imperfection. But we don't live in a perfect world and the alternatives to UOE are often worse (class hierarchy blow up). OTOH with mere two concrete classes, there should be a better way.


When I wrote a similar class, I left everything out of the superclass what wasn't really common. An immutable class has final fields while a mutable one has non-final fields, so there are none in common.

abstract class Location {
    abstract int getX();
    abstract int getY();
}

@Getter @Setter
final class MutableLocation extends Location {
    private int x;
    private int y;

    adjust...

    // I find this pretty useful....
    ImmutableLocation toImmutableLocation() {...}
}

@Getter @RequiredArgsConstructor
final class ImmutableLocation extends Location {
    private final int x;
    private final int y;

    // ... and this too.
    MutableLocation toMutableLocation() {...}
}

The Lombok annotations do exactly what you think they do. If you don't want to use them, you can write the boilerplate manually.

You can replace the abstract superclass by an interface if you prefer. But I guess, you don't need both.

\$\endgroup\$
  • \$\begingroup\$ A class doesn't NEED final fields to be immutable, as long as the client can't change the state. Since the client doesn't have access to the setter methods, there's no way for them to modify an ImmutableLocation (please correct me if I'm wrong). Will most definitely look into Lombok though. Any framework that lowers verbosity through annotations is a framework for me :) And thanks for the heads up on the typo! As for Locatable, you're right, I didn't need such abstractions. But after a few changes, I actually found a good use for the Locatable interface after changing it's contract \$\endgroup\$ – Vince Emigh May 17 '15 at 16:56
  • \$\begingroup\$ @VinceEmigh "A class doesn't NEED final fields to be immutable" - that's perfectly true, but the big problem remains: One thread creates a Location and another one may see an uninitialized or a partially initialized object. This is pretty unexpected with immutables. +++ I'm glad you'll look into Lombok (I'm not a contributor, just a big fan of it). The more people use it, the better. +++ You're welcome. \$\endgroup\$ – maaartinus May 17 '15 at 18:49
3
\$\begingroup\$

Your IncrementOperation enum is total overkill. Remember that integers can be negative, so location.adjustX(5, IncrementOperation.DECREASE) is exactly equivalent to location.adjustX(-5) when that method is implemented as

void adjustX(int delta) {
    setX(getX() + delta);
}

Are these classes SOLID? Well, we can look at the list:

  • Single Responsibility Principle: Yes, your classes represent a possibly mutable coordinate.

  • Open–Closed Principle: This is more difficult since two different ideas hide behind that name:

    (1) You can modify a class by subclassing it and inheriting most of its behaviour. Oh God, please not! See the “Fragile Base Class Problem” on why this might be a moronic idea. This might be a consideration for C++ libraries, but it ought to be discouraged everywhere else.

    (2) You can switch out a class in a system by reimplementing its interface. This is possible here since you wrote a Locatable interface. But is following the OCP sensible in your case? My take is that architecture requirements for a published API and for internals code is drastically different: With internal code, you control all dependencies and can therefore safely do arbitrary changes of behaviour. But when you don't control all code that may depend on Location, it might make sense to support the OCP so that other people can write a CustomLocation that suits their needs. In that case, you should also publish a separate interface for the methods of MutableLocation.

  • Liskov Substitution Principle: Your two subclasses are proper subtypes of Location. However, it is possible to create a subclass of the ImmutableLocation that violates the LSP. It would make very much sense to make that class final.

  • Interface Segregation Principle: All your interfaces are sufficiently small.

  • Dependency Inversion Principle: Your code has no dependencies, therefore no dependencies could be inverted. Looks good!

So to summarize, your classes generally pass the SOLID tests, although there are some details where you have to think about what you actually want.

Another point I noticed is that you do not use @Override annotations when implementing interface methods. Consider doing that, it especially makes it easy to see at a glance whether this allows consumers of the class to depend on interfaces rather than on concrete classes.

\$\endgroup\$
  • \$\begingroup\$ I knew it! I was pretty tired while writing this, and I had a feeling I was over-complexing things. Going to look into the Fragile Base Class problem, much props for introducing me :) Always trying to find useful design principles and conventions. I'm wondering how ImmutableLocation violates LSP, since subclasses won't have access to the protected setters. You threw me a fast ball on OCP: "publish a separate interface for the methods of MutableLocation" I should create a separate contract for MutableLocation? Wouldn't this be a victim of "alternative classes with different interfaces"? \$\endgroup\$ – Vince Emigh May 17 '15 at 17:04
1
\$\begingroup\$

This is how you implement quasi-mutable value objects, without generating class hierarchy clutter:

public class Location {
    private final int x;
    private final int y;

    public Location(int x, int y) {
        this.x = x;
        this.y = y;
    }

    public int getX() {
        return x;
    }

    public int getY() {
        return y;
    }

    public Location withX(int newX) {
        return new Location(newX, this.y);
    }

    public Location withY(int newY) {
        return new Location(this.x, newY);
    }
}

Of course replace withFoo methods with more meaningful names from the domain, whenever available. Think of BigDecimal. It is immutable but you can add another BigDecimal to it.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.