Edit: After reading what people have said, I did go way too far with the amount of enums I used. It could've just been simplified into one enum and handled the same. Here are the updates.
I appreciate all the constructive criticism and feedback. I do disagree taking a more "OOP" approach by creating a new instance of GodswordHandler for simple use. The use of static is a better approach than creating a new instance of the class.
Credits and a big thanks to Metorrite for a way to handle it differently.
Spoiler for GodswordHandler.java:
Code:
package com.rs.game.player.content.godsword;
import static com.rs.game.player.content.godsword.Godsword.ARMADYL_GODSWORD;
import java.util.Arrays;
import java.util.EnumSet;
import java.util.List;
import java.util.Optional;
import com.rs.game.Animation;
import com.rs.game.item.Item;
import com.rs.game.player.Player;
import com.rs.game.player.Skills;
/**
*
* @author Config / Mikey
*
*/
public class GodswordHandler {
private static final EnumSet<Godsword> godswordSet = EnumSet.allOf(Godsword.class);
private static final List<Item> SHARDS = Arrays.asList(new Item(11710), new Item(11712), new Item(11714));
public static final int GODSWORD_BLADE = 11690;
public static Godsword getComponent(int itemId) {
return godswordSet.stream().filter(x -> x.getGodswordId() == itemId).findFirst().orElse(null);
}
public static boolean isShard(int itemId) {
return SHARDS.contains(new Item(itemId));
}
public static void createGodswordBlade(Player player) {
if (player.getSkills().getLevel(Skills.SMITHING) < 80 && player.getInventory().containsItem(2347, 1)) {
player.getPackets()
.sendGameMessage("You must have 80 Smithing and all three shards to create a Godsword blade");
return;
}
player.lock(2);
player.setNextAnimation(new Animation(898));
player.getInventory().removeItems(SHARDS);
player.getInventory().addItem(new Item(GODSWORD_BLADE));
player.getSkills().addXp(Skills.SMITHING, 200);
player.getPackets().sendGameMessage("You have created a Godsword blade!");
}
public static void createGodsword(Player player, int itemId) {
Optional<Godsword> component = Optional.ofNullable(godswordSet.stream()
.filter(x -> x.getGodswordId() == itemId || x.getHiltId() == itemId).findAny().orElse(null));
player.getInventory().deleteItem(new Item(GODSWORD_BLADE));
player.getInventory().deleteItem(new Item(component.get().getHiltId()));
player.getInventory().addItem(new Item(component.get().getGodswordId()));
player.getDialogueManager().startDialogue(
"ItemMessage", "Congratulations, you have created "
+ (component.get().equals(ARMADYL_GODSWORD) ? "an " : "a ") + component.get() + ".",
component.get().getGodswordId());
}
public static void dismantleGodsword(Player player, Godsword godsword, int slot) {
if (player.getInventory().getFreeSlots() < 1) {
player.getPackets().sendGameMessage("You do not have enough inventory space.");
return;
}
player.getInventory().deleteItem(new Item(godsword.getGodswordId()));
player.getInventory().addItem(new Item(godsword.getHiltId()));
player.getInventory().addItem(new Item(GODSWORD_BLADE));
player.getInventory().refresh(slot);
}
}
Spoiler for Godsword.java:
Code:
package com.rs.game.player.content.godsword;
import com.rs.game.item.Item;
/**
*
* @author Config
*
*/
public enum Godsword {
ARMADYL_GODSWORD(11694, 11702),
BANDOS_GODSWORD(11696, 11704),
SARADOMIN_GODSWORD(11698, 11706),
ZAMORAK_GODSWORD(11700, 11708);
private int godswordId;
private int hiltId;
private Godsword(int godswordId, int hiltId) {
this.godswordId = godswordId;
this.hiltId = hiltId;
}
public int getGodswordId() {
return this.godswordId;
}
public int getHiltId() {
return this.hiltId;
}
@Override
public String toString() {
String godswordName = this.name().replace("_", " ");
return godswordName.substring(0, 1) + godswordName.substring(1).toLowerCase();
}
}
You might want to use some for each loops, they tend to be more readable in cases like this. Moreover I think you make a little bit too much enums. I think there are alternatives that fit a use case like this way better. Making everything static isn't a great example of OOP too.
I agree that it does look odd to have enumerations in this case, but they do hold a higher advantage over constants in many cases
Originally Posted by Curiousity
Making everything static isn't a great example of OOP too.
Static methods seem fitting assuming the GodswordHandler class will never need to be instantiated and the fields are defined without taking (unnecessary) additional memory
Static methods seem fitting assuming the GodswordHandler class will never need to be instantiated and the fields are defined without taking (unnecessary) additional memory
Basically you are explaining what static means right here. Though I really do think this should be made OOP, the argument you give here can always be given to write statically.
Basically you are explaining what static means right here. Though I really do think this should be made OOP, the argument you give here can always be given to write statically.
I defined static because I was clarifying my point, but I would agree that there is a case to be made against the object-oriented nature of static fields and methods as they represent a global state and violate data encapsulation principles in an object. However, the variables and methods included in OP's post are instance-independent and will never need to be changed, which I believe proves acceptable use of the static modifier