0

Say I have a class which stores a weapon Arsenal

public class Arsenal{

  List<RocketLauncher> rocketLaunchers;
  List<HandGrenade> handGrenades;
  List<LandMine> landMines;
  //Followed by a few more weapons

}

The weapons are part of an enum, Weapon

Now, I am trying to display a summary screen or report elsewhere and I am preparing a map. See the snippet below. I have shown two approaches

public Map<Weapon,Integer> generateReport(Arsenal arsenal){

 Map<Weapon,Integer> weaponCountMap = //suitable map impl

 //Technique 1
 //Create and use a larger num of getters from the Arsenal Class
 weaponCountMap.put(Weapon.HAND_GRENADE, arsenal.getHandGrenades);
 //.. and so on

 //Technique 2
 //Create a single method in Arsenal which returns a Map
 weaponCountMap = arsenal.getSummary();

 return weaponCountMap;
}

Question : Is it just me or does everyone feel 'not right' to use a large number of getters. Suppose Arsenal stores around 50 weapons, it's like 50 methods in the class. Double with setters.

Also. I feel less flexible using the 2nd method, with no accessor methods.

Can you guys please critically evaluate both approaches and possibly suggest new ones?

1
  • what information regarding weapons do you want to show in summary ?
    – M Alok
    May 5, 2015 at 7:19

5 Answers 5

3

How about not hard-coding types of weapon inside of your Arsenal? The following is simple implementation of heterogeneous container for your specific case. However, as I don't quite familiar with Generics in enum, this implementation is when you have Weapon and their subclasses, e.g. HardGrenade extends Weapon, etc.

public class Arsenal{
    private Map<Class<?>, Collection<?>> weaponArsenal;

    public <T extends Weapon> Collection<T> get(Class<T> weaponClass) {
        if (weaponArsenal.containsKey(weaponClass) {
            return (Collection<T>) weaponArsenal.get(weaponClass);
        }
        return new ArrayList<T>(); // to avoid checking null everytime in client code
    }

    public <T extends Weapon> void put(T weapon) {
        if (!weaponArsenal.containsKey(weapon.class)) {
            Collection<T> weaponList = // initialize correct collection here
            weaponArsenal.put(weapon.class, weaponList);
        }

        weaponArsenal.get(weapon.class).add(weapon);
    }
}

and in the client code

Arsenal arsenal = new Arsenal();

arsenal.put(new HandGrenade());
arsenal.put(new RocketLauncher());

Collection<HandGrenade> handGrenades = arsenal.get(HandGrenade.class);
Collection<RocketLauncher> rocketLaunchers = arsenal.get(RocketLauncher.class);
2
  • Thank you!. What do yo think is the advantage of this instead of using a map with Weapon enum as the key? May 5, 2015 at 8:46
  • One distinct advantage is type-safety and less casts in client code because the type information is not lost, e.g. you don't need to cast for (Sword sword : arsenal.get(Sword.class)), compared to for (Weapon weapon : arsenal.get(Weapon.SWORD)). However, as can be seen from the code, it comes with cost on complexity in Arsenal class.
    – tkokasih
    May 5, 2015 at 9:09
2

In the arsenal you can duplicate the map instead of using lists. Then in the generateReport method you can iterate over the enum and use the enum value to get the suitable list from the map.

Something like

Arsenal:

 Map<Weapon,List<Weapon>> weaponsMap;
 arsenalMap.put(Weapon.HAND_GRENADE,handGrenades);

generate report:

for (Weapon weapon: Weapon.values()) {
 weaponCountMap.put(Weapon.HAND_GRENADE, arsenal.weaponsMap.get(weapon));
}

Might not be the best solution but you will remove some of the getters.

2
  • Seems good. Using a single map entry instead of all those lists can do the trick May 5, 2015 at 8:45
  • @NikhilKuriakose Instead of implementing that map yourself, try to use an existing one like Guava Multimap.
    – Tom
    May 5, 2015 at 8:51
1

If you make Arsenal immutable and construct it using a builder (to avoid having a bunch of constructors), you can make the instance variables public.

This approach allows you to use technique 1 from your question but without getters, and still keep state management of the object internal to the object.

public class Arsenal {

  public final List<RocketLauncher> rocketLaunchers;
  public final List<HandGrenade> handGrenades;
  public final List<LandMine> landMines;
  //Followed by a few more weapons

  private Arsenal(final Arsenal.Builder builder) {
      this.rocketLaunchers = Collections.unmodifiableList(builder.rocketLaunchers);
      this.handGrenades = Collections.unmodifiableList(builder.handGrenades );
      this.landMines= Collections.unmodifiableList(builder.landMines);
      // and so on
  }

  public static class Builder {
      private final List<RocketLauncher> rocketLaunchers = new ArrayList<>();
      private final List<HandGrenade> handGrenades = new ArrayList<>();
      private final List<LandMine> landMines = new ArrayList<>();

      public Builder rocketLaunchers(List<RocketLauncher> rocketLaunchers) {
          this.rocketLaunchers.addAll(rocketLaunchers);
          return this;
      }

      public Builder handGrenades(List<HandGrenade> handGrenades) {
          this.handGrenades.addAll(handGrenades);
          return this;
      }

      public Builder landMines (List<LandMines> landMines ) {
          this.landMines .addAll(landMines );
          return this;
      }

      public Arsenal build() {
          return new Arsenal(this);
      }
  }
}

You can now use this in the following way.

List<RocketLauncher> rocketLaunchers = //whatever
Arsenal arsenal = new Arsenal.Builder().rocketLaunchers(rocketLaunchers).build();
....
weaponCountMap.put(Weapon.HAND_GRENADE, arsenal.handGrenades);
//.. and so on

All fields of the arsenal are non-null, and can't be modified. If you define the builder in an external class (i.e. not a static class within Arsenal) your Arsenal class will be very small - just fields and the constructor, plus logic methods.

2
  • Thanks Steve. How do you suggest exposing the elements in the Arsenal class. I don't want unncessary compilation and memory overhead for a huge number of methods. May 5, 2015 at 8:44
  • They're already exposed, because they're public. It also sounds like you're trying to micro-optimise. May 5, 2015 at 8:46
1

Please have a look at other posts like this one...Java Conventions: use getters/setters WITHIN the class? to obtain an idea on when you could use getters instead of direct access.

Its kind of a design question depending upon what you would like to encapsulate within your arsenal class and what you would like to expose to the outside world for viewing your arsenal.

Its like the UN trying to inspect your arsenal and you tell them hey I wont tell you what weapons I am dealing with in my arsenal but I can give you a report and that is the report you externalize to the outside world. Now it depends on you what report you want to give out and which weapons will land in that map of yours.

Now looking at your second technique, are you planning to shift the map creation logic into your Arsenal class.

Another design question to answer .... is this logic just to obtain reports then my suggestion is to keep it out of the arsenal class and keep it light. Otherwise you might end up putting in all kind of report logics into the arsenal class and it will get heavier and might explode.

1
  • Thanks. I am not planning anything heavyweight with the report method. Just a plain Map. May 5, 2015 at 8:08
0

See https://projectlombok.org/ or https://github.com/google/auto/tree/master/value. Add separate field for each weapon, Lombok or AutoValue will create getters/setters/hashcode/equals/toString methods for you.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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