Protected Variables aren't allowed by default in Checkstyle, but what about in an enum?

Question

I'm using Java in eclipse, and I read a thread

I really liked home's answer, because I never really thought about it before. The whole idea that subclasses should really not have access to members that is. However, I have an interesting edge case if someone wants to give it a go.

Suppose I have an interface Buildable and an Enum CatanPiece:

public interface Buildable
{
    HashMap<Resource, Integer> getCost();
    Buildable build( final PlayerHand payment );
}

public enum CatanPiece implements Buildable
{
    ROAD
    {
        @Override
        public HashMap<Resource, Integer> getCost()
        {
            if ( cost.isEmpty() )
            {
                cost.put( BRICK, 1 );
                cost.put( LUMBER, 1 );
            }
            return cost;
        }
    },

    SETTLEMENT
    {
        @Override
        public HashMap<Resource, Integer> getCost()
        {
            if ( cost.isEmpty() )
            {
                cost.put( BRICK, 1 );
                cost.put( LUMBER, 1 );
                cost.put( SHEEP, 1 );
                cost.put( WHEAT, 1 );
            }
            return cost;
        }
    },

    CITY
    {
        @Override
        public HashMap<Resource, Integer> getCost()
        {
            if ( cost.isEmpty() )
            {
                cost.put( WHEAT, 2 );
                cost.put( ORE, 3 );
            }
            return cost;
        }
    };

    protected final HashMap<Resource, Integer> cost;

    private CatanPiece()
    {
        cost = getCost();
    }

    @Override
    public abstract HashMap<Resource, Integer> getCost();

    @Override
    public Buildable build( final PlayerHand payment )
    {
        return ( payment.remove( cost ) ? null : this );
    }
}

So Checkstyle is giving me crap about the protected HashMap I'm using, but for those looking at this question that understand it, you can see I don't have the problem of someone misusing that variable. AND I would in fact make it private and use a protected get method, however the return on that method is different per instance.

Potential answers: Ignore the checkstyle warning, or perhaps include an abstract init() method to initialize the HashMap then simply implement the getCost() method generically for all memebers of the enum.

What do you think?

Answer 1

Checkstyle's understanding of Enums is sometimes incomplete. I would guess that the author of the VisibilityModifier check did not think of Enums. Enums are a corner case that would require some additional properties of the check.

However, accidentally, in your case, the warning is still correct. I believe that there is no situation where you should use protected fields in an Enum. If you need instance specific state in your Enum constants, initialize it via the constructor or via static initialization.

Try the following approach. This should gain you several benefits:

• The Checkstyle warning goes away because the Map is now private.
• The CatanPiece enum constants are now really constants because the cost map is immutable.
• No overhead for lazy initialization. Users of the CatanPiece enum constants can be sure that the costs map is initialized properly.

The only downside is that whenever the Enum is extended (e.g. with SHIP), the developer must not forget to update buildCostMap(). Such a bug would show up very quickly though.

public enum CatanPiece implements Buildable
{
    ROAD, SETTLEMENT, CITY;

    private static final Map<CatanPiece, Map<Resource, Integer>> allCosts =
            buildCostMap();

    private static Map<CatanPiece, Map<Resource, Integer>> buildCostMap()
    {
        Map<CatanPiece, Map<Resource, Integer>> result =
            new HashMap<CatanPiece, Map<Resource, Integer>>();

        Map<Resource, Integer> cost = new EnumMap<Resource, Integer>(Resource.class);
        cost.put(Resource.WHEAT, 2);
        cost.put(Resource.ORE, 3);
        result.put(CITY, Collections.unmodifiableMap(cost));

        cost = new EnumMap<Resource, Integer>(Resource.class);
        cost.put(Resource.BRICK, 1);
        cost.put(Resource.LUMBER, 1);
        cost.put(Resource.SHEEP, 1);
        cost.put(Resource.WHEAT, 1);
        result.put(SETTLEMENT, Collections.unmodifiableMap(cost));

        cost = new EnumMap<Resource, Integer>(Resource.class);
        cost.put(Resource.BRICK, 1);
        cost.put(Resource.LUMBER, 1);
        result.put(ROAD, Collections.unmodifiableMap(cost));

        return Collections.unmodifiableMap(result);
    }

    @Override
    public Map<Resource, Integer> getCost() {
        return allCosts.get(this);
    }

    @Override
    public Buildable build(final PlayerHand payment) {
        return payment.remove(cost) ? null : this;
    }
}