Jump to content
  • Advertisement
RidiculousName

Java Is There any way to Make this Method More Elegant?

Recommended Posts

I have a method that's gotten rather bloated and unwieldy. This is mostly because I ended embedding a switch-statement inside another switch-statement. The outer switch-statement could be swapped with an if-statement, but that doesn't seem like it would help much. I selected this method in particular because all of the switch-cases do very very similar things, and it seems like I may be able to simplify the method.

To explain the method a bit:

I have a Bandit class that holds various things like the bandit's physical stats, and includes custom Item class objects that represent what sort of weapons and armor the bandit is equipped with.

Each type of Item can only be equipped once, since each bandit can only wield one weapon, wear one helmet, use one shield, or wear one leather-jerkin of course.

So, if I want to equip an Item and an Item of that same type is already equipped, I have to take it off the bandit and put it in storage before I can equip the new Item.

This method just handles equipping bandits. I have another that removes equipment.

My Class Order:

Weapon and Armor are both subclasses of Item. I use the Armor class attribute String armorType to determine what type of armor is being utilized.

My Method:

   /**
    * Equips a bandit with the given Item parameter
    * @param item The Item to equip
    */
   public void equipBandit(Item item) {
      // get the item's simple class name.
      String equipmentType = item.getTypeName();
      switch(equipmentType) {
      case "Weapon":
         if(this.weapon == null) {
            this.weapon = item;
         } else {
            // increment the amount owned of the un-equipped weapon
            Main.organ.addAmount(this.weapon, 1);
            // decrement the amount owned of the equipped weapon
            Main.organ.addAmount(item, -1);
            // equip the weapon
            this.weapon = item;
         }
         break;
      case "Armor":
         Armor armor = (Armor) item;
         String armorType = armor.getArmorType();
         switch(armorType) {
         case "torso":
            if(this.torsoArmor == null) {
               this.torsoArmor = item;
            } else {
               // Does the same things as the else statement above
               Main.organ.addAmount(this.torsoArmor, 1);
               Main.organ.addAmount(item, -1);
               this.torsoArmor = item;
            }
            break;
         case "head":
            if(this.headArmor == null) {
               this.headArmor = item;
            } else {
               Main.organ.addAmount(this.headArmor, 1);
               Main.organ.addAmount(item, -1);
               this.headArmor = item;
            }
            break;
         case "shield":
            if(this.shieldArmor == null) {
               this.shieldArmor = item;
            } else {
               Main.organ.addAmount(this.shieldArmor, 1);
               Main.organ.addAmount(item, -1);
               this.shieldArmor = item;
            }
            break;
         default:
            throw new IllegalArgumentException("Armor type not found");
         }
         break;
      default:
         throw new IllegalArgumentException("Invalid Item");
      }
   }

 

Share this post


Link to post
Share on other sites
Advertisement

I think this is in Java, so if you wanted to make this method neater you could replace the torsoArmor, headArmor and shieldArmor with a single HashMap, so to add an item you would have

main.organ.addAmount(this.armor.get(armorType), 1);
main.organ.addAmount(item, -1);
this.armor.put(armorType, item);

where armor is defined as follows somewhere in the Bandit class

private HashMap<String, Item> armor;

In this case you would still need to check that the Armor items have a valid type

Share this post


Link to post
Share on other sites

This:

      case "Weapon":
         if(this.weapon == null) {
            this.weapon = item; // <-- Notice how this line is identical to...
         } else {
            Main.organ.addAmount(this.weapon, 1);
            Main.organ.addAmount(item, -1);
            this.weapon = item; // <-- This other line
         }
         break;

can be turned into this:

      case "Weapon":
         if(this.weapon != null) {
            Main.organ.addAmount(this.weapon, 1);
            Main.organ.addAmount(item, -1);
         }
         this.weapon = item;
         break;

I am not sure why the decrement doesn't happen if there was no weapon equipped...

Anyway, the repetitiveness of the code is an obvious code smell, even after this minor cleanup.  Perhaps Weapon, HeadArmor, ShieldArmor... can derive from Equipment, which provides a method to do the update?

Anyway, I am not a Java programmer. I can think of some options in C++, but I'll let someone else suggest an idiomatic way to do this in Java.

The other code smell is the "Main.organ", which looks a lot like global state. If at all possible, make sure your code doesn't use global state for anything. Also, why is it called "organ"?

 

Share this post


Link to post
Share on other sites
45 minutes ago, alvaro said:

I am not sure why the decrement doesn't happen if there was no weapon equipped...

Heh, I am not sure either. That doesn't make sense.

Quote

Perhaps Weapon, HeadArmor, ShieldArmor... can derive from Equipment, which provides a method to do the update?

There is no Equipment class, you mean Item, their super-class?

Quote

The other code smell is the "Main.organ", which looks a lot like global state. If at all possible, make sure your code doesn't use global state for anything.

I posted a while back about not being sure how to manage a bunch of objects across multiple files. It's an instance of my Organizer class that, well, helps me organize everything. I'm not sure what other option I really have. Organizer itself is a mess because it deals with multiple custom types and, well, lots of different things that need to be handled in different ways.

Quote

Also, why is it called "organ"?

Because Organizer is it's class type, and I couldn't think of anything.. 😚

Edited by RidiculousName

Share this post


Link to post
Share on other sites

How's this?

      case "Weapon":
         Main.organ.swap(this.weapon, item); // This function can check if this.weapon is null if it needs to
         this.weapon = item;
         break;

EDIT: Depending on the semantics of function calls in Java, you might be able to do the assignment inside that function as well. In C++ one could do that by passing the first argument as a non-const reference.

Edited by alvaro

Share this post


Link to post
Share on other sites

Not change amount of items inside. Just get bool for checking equipment.

interface Item {
    unsigned long getGUID() const;
}

bool tryEquip(Item item) {
    if(cache.get(item.getGUID())) return false; // equiped
    int type = item.getGUID() & ITEM_TYPE_MASK; /// 0xff .. 0xffff
    
    if(handles.get(type))
        return handles[type](item); // hmmm... logic
    
    return false; // handle not found
}

Share this post


Link to post
Share on other sites

You have a lot of individual variables that would work better as a container of some kind.

public void equipBandit(Item item) {
  int slot = item.getEquipmentSlot(); // Or make it a string, I don't care.
  if (slot == -1) { throw new IllegalArgumentException("Invalid Item"); }
  if (this.equipment[slot] != null) { Main.organ.addAmount(this.equipment[slot], 1); }
  Main.organ.addAmount(item, -1);
  this.equipment[slot] = item;
}

 

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!