317 Perfect Rune Pouch

Dexter Morgan

Banned
Dexter Morgan
 
 
Nov 11, 2008
4,428
477
0
Server side:
Code: 
package com.elvarg.game.content.item;

import java.lang.reflect.Type;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;

import com.elvarg.game.World;
import com.elvarg.game.entity.impl.player.Player;
import com.elvarg.game.model.Item;
import com.elvarg.game.model.JsonIO;
import com.elvarg.game.model.ui.UserContainerInterface;
import com.elvarg.net.packet.interaction.PacketInteraction;
import com.elvarg.util.Misc;
import com.google.gson.Gson;
import com.google.gson.JsonElement;
import com.google.gson.JsonObject;
import com.google.gson.reflect.TypeToken;

/**
 * 
 * @author Dexter Morgan <http://www.rune-server.org/members/dexter+morgan/>
 *
 */
public class RunePouch extends PacketInteraction {

	private static final int INTERFACE_ID = 24_200;

	private static final int POUCH_CONTAINER_ID = 24206;

	private static final int INVENTORY_CONTAINER_ID = 24209;

	private static final int MAX_POUCH_SIZE = 3;

	public static final int RUNE_POUCH = 12791;

	private enum Rune {
		FIRE_RUNE(554),

		WATER_RUNE(555),

		AIR_RUNE(556),

		EARTH_RUNE(557),

		MIND_RUNE(558),

		BODY_RUNE(559),

		DEATH_RUNE(560),

		NATURE_RUNE(561),

		CHAOS_RUNE(562),

		LAW_RUNE(563),

		COSMIC_RUNE(564),

		BLOOD_RUNE(565),

		SOUL_RUNE(566),

		ASTRAL(9075),

		;

		private int id;

		Rune(int id) {
			this.id = id;
		}

		private static final HashMap<Integer, Rune> FOR_ID = new HashMap<>();

		static {
			for (Rune r : values()) {
				FOR_ID.put(r.id, r);
			}
		}
	}

	private static final Type REFERENCE = new TypeToken<HashMap<Rune, Integer>>() {
	}.getType();

	public static final JsonIO IO = new JsonIO("./data/saves/rune_pouch/") {

	  [MENTION=381607]Override[/MENTION]
		public void init(String name, Gson builder, JsonObject reader) {
			JsonElement element = reader.get("pouch");

			HashMap<Rune, Integer> result = GSON.fromJson(element.toString(), REFERENCE);

			Optional<Player> p = World.getPlayerByName(name);

			if (!p.isPresent()) {
				return;
			}

			p.get().runePouch.pouch.putAll(result);
		}

	  [MENTION=381607]Override[/MENTION]
		public JsonObject save(String name, Gson builder, JsonObject object) {

			Optional<Player> p = World.getPlayerByName(name);

			if (!p.isPresent()) {
				return object;
			}

			object.add("pouch", builder.toJsonTree(p.get().runePouch.pouch, REFERENCE));
			return object;
		}
	};

	static {
		new UserContainerInterface(POUCH_CONTAINER_ID) {

		  [MENTION=381607]Override[/MENTION]
			public boolean handleOption(Player player, int id, int slot, int option, int amount) {
				if (player.getInterfaceId() != INTERFACE_ID) {
					return false;
				}

				takeOut(player, slot);
				return true;
			}
		};

		new UserContainerInterface(INVENTORY_CONTAINER_ID) {

		  [MENTION=381607]Override[/MENTION]
			public boolean handleOption(Player player, int id, int slot, int option, int amount) {
				if (player.getInterfaceId() != INTERFACE_ID) {
					return false;
				}

				int existing = player.getInventory().getAmount(id);

				if (existing == 0) {
					return true;
				}

				store(player, new Item(id, existing));

				open(player);
				return true;
			}
		};
	}

	private HashMap<Rune, Integer> pouch = new HashMap<>();

	private static void open(Player p) {
		ArrayList<Item> pouch = new ArrayList<>();

		for (Map.Entry<Rune, Integer> e : p.runePouch.pouch.entrySet()) {
			pouch.add(new Item(e.getKey().id, e.getValue()));
		}

		p.getPacketSender().sendItemContainerList(pouch, POUCH_CONTAINER_ID);

		p.getPacketSender().sendItemContainer(p.getInventory(), INVENTORY_CONTAINER_ID);

		p.getPacketSender().sendInterface(INTERFACE_ID);
	}

	private static void store(Player p, Item item) {
		Rune r = Rune.FOR_ID.getOrDefault(item.getId(), null);

		if (r == null) {
			p.getPacketSender().sendMessage("You can only store runes into the rune pouch.");
			return;
		}

		if (!p.runePouch.pouch.containsKey(r) && p.runePouch.pouch.size() == MAX_POUCH_SIZE) {
			p.getPacketSender().sendMessage("Your rune pouch is full.");
			return;
		}

		if (!p.getInventory().contains(item)) {
			return;
		}

		String name = item.getName();

		int amount = item.getAmount();

		p.getInventory().delete(item);

		p.runePouch.pouch.merge(r, amount, Integer::sum);

		p.getPacketSender().sendMessage("You store " + Misc.format(amount) + " " + name + " into the rune pouch.");
	}

	private static void takeOut(Player p, int slot) {
		ArrayList<Rune> list = new ArrayList<>(p.runePouch.pouch.keySet());

		Rune r = list.get(slot);

		if (p.getInventory().getFreeSlots() > 0 || p.getInventory().contains(r.id)) {
			int existing = p.runePouch.pouch.getOrDefault(r, 0);

			if (existing == 0) {
				return;
			}

			p.runePouch.pouch.remove(r);

			p.getInventory().add(r.id, existing);

			open(p);
		} else {
			p.getInventory().full();
		}
	}

	private static void empty(Player p) {
		HashMap<Rune, Integer> pouch = new HashMap<>(p.runePouch.pouch);

		for (Map.Entry<Rune, Integer> e : pouch.entrySet()) {
			Rune r = e.getKey();
			int amount = e.getValue();

			if (p.getInventory().getFreeSlots() > 0 || p.getInventory().contains(r.id)) {
				p.runePouch.pouch.remove(r);
				p.getInventory().add(r.id, amount);
			}
		}
	}

	public static boolean useRune(Player p, Item rune) {
		Rune r = Rune.FOR_ID.getOrDefault(rune.getId(), null);

		if (r == null) {
			return false;
		}

		if (!p.runePouch.pouch.containsKey(r)) {
			return false;
		}

		int existing = p.runePouch.pouch.getOrDefault(r, 0);

		if (rune.getAmount() > existing) {
			return false;
		}

		p.runePouch.pouch.put(r, existing - rune.getAmount());
		return true;
	}

  [MENTION=381607]Override[/MENTION]
	public boolean handleItemInteraction(Player player, Item item, int type) {
		switch (item.getId()) {
		case RUNE_POUCH:
			if (type == 1) {
				open(player);
			} else if (type == 2) {
				empty(player);
			}
			return true;
		}
		return false;
	}

  [MENTION=381607]Override[/MENTION]
	public boolean handleItemOnItemInteraction(Player player, Item use, Item usedWith) {
		if (usedWith.getId() == RUNE_POUCH) {
			store(player, use);
			return true;
		}
		return false;
	}
}

Client side:
Code: 
package com.runescape.cache.graphics.widget.impl.osrs.impl;

import com.runescape.cache.graphics.widget.impl.osrs.ClearOSRSBackground;

/**
 * 
 * @author Dexter Morgan <http://www.rune-server.org/members/dexter+morgan/>
 *
 */
public class RunePouchWidget extends ClearOSRSBackground {

	public RunePouchWidget() {
		super(24_200, "Rune Pouch", 400, 310);
		super.closeX = 370;
	}

  [MENTION=381607]Override[/MENTION]
	public void init() {
		super.init();

		add(addCenteredText("Pouch:", 1), 256, 50);
		add(addBox(200, 65, false), 156, 65);
		add(addItemContainer(3, 1, 0, 0, new String[] { "Take out" }, "pouch"), 208, 82);

		add(addCenteredText("Inventory:", 1), 256, 135);
		add(addBox(350, 140, false), 256 - 175, 150);
		add(addItemContainer(7, 4, 10, 0, new String[] { "Add" }, "inventory"), 256 - (21 * 7), 158);
	}

}


In class Client in

Code: 
public int executeScript(Widget widget, int id) {
replace if statement
Code: 
				if (instruction == 4) {
					Widget other = Widget.interfaceCache[script[counter++]];
					int item = script[counter++];

					if (item >= 0 && item < ItemDefinition.TOTAL_ITEMS
							&& (!ItemDefinition.lookup(item).is_members_only || isMembers)) {
						label: for (int slot = 0; slot < other.inventoryItemId.length; slot++) {

							boolean hasPouch = false;

							if (other.id == 3214) {
								for (int inventory = 0; inventory < other.inventoryItemId.length; inventory++) {
									if (other.inventoryItemId[inventory] == 12792) {
										hasPouch = true;
									}
								}
							}

							if (other.id == 3214 && hasPouch) {
								Widget runePouch = Widget.interfaceCache[24206];

								for (int runePouchSlot = 0; runePouchSlot < runePouch.inventoryItemId.length; runePouchSlot++) {
									if (runePouch.inventoryItemId[runePouchSlot] == item + 1) {
										value += runePouch.inventoryAmounts[runePouchSlot];
										break label;
									}
								}
							}
							if (other.inventoryItemId[slot] == item + 1) {
								value += other.inventoryAmounts[slot];
							}
						}
					}
				}


Code: 
package com.runescape.model.content.rune;

import java.util.ArrayList;

/**
 * 
 * @author Dexter Morgan <http://www.rune-server.org/members/dexter+morgan/>
 *
 */
public enum Rune {
	FIRE_RUNE(554),

	WATER_RUNE(555),

	AIR_RUNE(556),

	EARTH_RUNE(557),

	MIND_RUNE(558),

	BODY_RUNE(559),

	DEATH_RUNE(560),

	NATURE_RUNE(561),

	CHAOS_RUNE(562),

	LAW_RUNE(563),

	COSMIC_RUNE(564),

	BLOOD_RUNE(565),

	SOUL_RUNE(566),

	ASTRAL(9075),

	;

	private int id;

	Rune(int id) {
		this.id = id;
	}

	private static final ArrayList<Integer> IDS = new ArrayList<>();

	static {
		for (Rune r : values()) {
			IDS.add(r.id);
		}
	}

	public static boolean isRune(int id) {
		return IDS.contains(id);
	}
}

Search for

Code: 
if (childInterface.inventoryAmounts[item] == 0) {

Below add

Code: 
									if (childInterface.id == 24209) {
										if (!Rune.isRune(itemId)) {
											itemOpacity = 95;
										}
									}
 
Overriding classes during construction time is a nice gimmick but doesn't need to be overused. You'd likely prefer a package and group 3-4 classes together to make things cleaner. (JsonIO, enum, packet interaction)

Also you're passing player into every method, would benefit from making it a field in the class. Weird/too much spacing. Can leverage java streams to condense some simple things
 
  • Like
Reactions: Dexter Morgan and Hc747
Intrice Joe said:
Overriding classes during construction time is a nice gimmick but doesn't need to be overused. You'd likely prefer a package and group 3-4 classes together to make things cleaner. (JsonIO, enum, packet interaction)

Also you're passing player into every method, would benefit from making it a field in the class. Weird/too much spacing. Can leverage java streams to condense some simple things
Click to expand...

Thank you for the constructive criticism!
 
Why do you have a enum which is storing the rune data, and on static initialization placing this data inside a hashmap. The only thing his hashmap is doing is telling ur code if the rune is usable with the pouch. Use a hashset


HashSet<int> runes = new HashSet<int>() [rune1, rune2, rune3];


Then shit like this
Rune r = Rune.FOR_ID.getOrDefault(item.getId(), null);

Can be turned into:

int id = item.getId();
boolean containsRune = runes.Contains(id);
if(!containsRune) return;



....

This message was ruder but i had to tone it down. Please stop writing code like this, its a consistent style of yours to make a shitty enum and shove all of it into a hashmap


Edit:
What is this a switch case with 1 case? Are you still using notepad to write code, the indentations are wrong?:
4YiK8eX.png
 
Last edited:
How can a community veteran be writing code of this nature? Lmao
 
Just use EnumMap instead of HashMap when using enums as keys,

inside your enum instead of using private static final HashMap<Integer, Rune> FOR_ID = new HashMap<>();

you would use private static final Map<Integer, Rune> FOR_ID =
EnumSet.allOf(Rune.class).stream().collect(Collectors.toMap(r -> r.id, r -> r));

than you would be able to call the rune like Rune rune = Rune.FOR_ID.get(item.getId());

this allows for O(1) time when looking up a rune by its id

for store, you would be able to use
Code: 
private void store(Item item) {
    Rune rune = Rune.FOR_ID.get(item.getId());
    if (rune == null) {
        player.getPacketSender().sendMessage("You can only store runes into the rune pouch.");
        return;
    }

    if (!pouch.containsKey(rune) && pouch.size() == MAX_POUCH_SIZE) {
        player.getPacketSender().sendMessage("Your rune pouch is full.");
        return;
    }

for take out you can streamline the removable of runes

Code: 
private void takeOut(int slot) {
    Rune rune = new ArrayList<>(pouch.keySet()).get(slot);
    if (player.getInventory().hasFreeSlots() || player.getInventory().contains(rune.id)) {
        int amount = pouch.remove(rune);
        if (amount > 0) {
            player.getInventory().add(rune.id, amount);
            open();
        }

for handleItemInteraction, you dont need a switch as its only one item id

just use an if statement if its RUNE_POUCH

Code: 
public boolean handleItemInteraction(Player player, Item item, int type) {
    if (item.getId() == RUNE_POUCH) {
        if (type == 1) {
            open();
        } else if (type == 2) {
            empty();
        }
        return true;
    }
 
Remi said:
Just use EnumMap instead of HashMap when using enums as keys,

inside your enum instead of using private static final HashMap<Integer, Rune> FOR_ID = new HashMap<>();

you would use private static final Map<Integer, Rune> FOR_ID =
EnumSet.allOf(Rune.class).stream().collect(Collectors.toMap(r -> r.id, r -> r));

than you would be able to call the rune like Rune rune = Rune.FOR_ID.get(item.getId());

this allows for O(1) time when looking up a rune by its id

for store, you would be able to use
Code: 
private void store(Item item) {
    Rune rune = Rune.FOR_ID.get(item.getId());
    if (rune == null) {
        player.getPacketSender().sendMessage("You can only store runes into the rune pouch.");
        return;
    }

    if (!pouch.containsKey(rune) && pouch.size() == MAX_POUCH_SIZE) {
        player.getPacketSender().sendMessage("Your rune pouch is full.");
        return;
    }

for take out you can streamline the removable of runes

Code: 
private void takeOut(int slot) {
    Rune rune = new ArrayList<>(pouch.keySet()).get(slot);
    if (player.getInventory().hasFreeSlots() || player.getInventory().contains(rune.id)) {
        int amount = pouch.remove(rune);
        if (amount > 0) {
            player.getInventory().add(rune.id, amount);
            open();
        }

for handleItemInteraction, you dont need a switch as its only one item id

just use an if statement if its RUNE_POUCH

Code: 
public boolean handleItemInteraction(Player player, Item item, int type) {
    if (item.getId() == RUNE_POUCH) {
        if (type == 1) {
            open();
        } else if (type == 2) {
            empty();
        }
        return true;
    }
Click to expand...

Thank you, appreciated!
 
  • Like
Reactions: Sub
Remi said:
Just use EnumMap instead of HashMap when using enums as keys,

inside your enum instead of using private static final HashMap<Integer, Rune> FOR_ID = new HashMap<>();

you would use private static final Map<Integer, Rune> FOR_ID =
EnumSet.allOf(Rune.class).stream().collect(Collectors.toMap(r -> r.id, r -> r));

than you would be able to call the rune like Rune rune = Rune.FOR_ID.get(item.getId());

this allows for O(1) time when looking up a rune by its id

for store, you would be able to use
Code: 
private void store(Item item) {
    Rune rune = Rune.FOR_ID.get(item.getId());
    if (rune == null) {
        player.getPacketSender().sendMessage("You can only store runes into the rune pouch.");
        return;
    }

    if (!pouch.containsKey(rune) && pouch.size() == MAX_POUCH_SIZE) {
        player.getPacketSender().sendMessage("Your rune pouch is full.");
        return;
    }

for take out you can streamline the removable of runes

Code: 
private void takeOut(int slot) {
    Rune rune = new ArrayList<>(pouch.keySet()).get(slot);
    if (player.getInventory().hasFreeSlots() || player.getInventory().contains(rune.id)) {
        int amount = pouch.remove(rune);
        if (amount > 0) {
            player.getInventory().add(rune.id, amount);
            open();
        }

for handleItemInteraction, you dont need a switch as its only one item id

just use an if statement if its RUNE_POUCH

Code: 
public boolean handleItemInteraction(Player player, Item item, int type) {
    if (item.getId() == RUNE_POUCH) {
        if (type == 1) {
            open();
        } else if (type == 2) {
            empty();
        }
        return true;
    }
Click to expand...

Why use a enum at all?

Code: 
package com.elvarg.game.content.item;

import java.lang.reflect.Type;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.Map;
import java.util.Optional;
import java.util.Set;

import com.elvarg.game.World;
import com.elvarg.game.entity.impl.player.Player;
import com.elvarg.game.model.Item;
import com.elvarg.game.model.JsonIO;
import com.elvarg.game.model.ui.UserContainerInterface;
import com.elvarg.net.packet.interaction.PacketInteraction;
import com.elvarg.util.Misc;
import com.google.gson.Gson;
import com.google.gson.JsonElement;
import com.google.gson.JsonObject;
import com.google.gson.reflect.TypeToken;

/**
 * 
 * @author Dexter Morgan <http://www.rune-server.org/members/dexter+morgan/>
 *
 */
public class RunePouch extends PacketInteraction {

	private static final int INTERFACE_ID = 24_200;

	private static final int POUCH_CONTAINER_ID = 24206;

	private static final int INVENTORY_CONTAINER_ID = 24209;

	private static final int MAX_POUCH_SIZE = 3;

	public static final int RUNE_POUCH = 12791;

	private static final Set<Integer> RUNE_IDS = new HashSet<>(Set.of(
		554,  // FIRE_RUNE
		555,  // WATER_RUNE
		556,  // AIR_RUNE
		557,  // EARTH_RUNE
		558,  // MIND_RUNE
		559,  // BODY_RUNE
		560,  // DEATH_RUNE
		561,  // NATURE_RUNE
		562,  // CHAOS_RUNE
		563,  // LAW_RUNE
		564,  // COSMIC_RUNE
		565,  // BLOOD_RUNE
		566,  // SOUL_RUNE
		9075  // ASTRAL
	));

	private static final Type REFERENCE = new TypeToken<Map<Integer, Integer>>() {}.getType();

	public static final JsonIO IO = new JsonIO("./data/saves/rune_pouch/") {

	  [MENTION=381607]Override[/MENTION]
		public void init(String name, Gson builder, JsonObject reader) {
			JsonElement element = reader.get("pouch");

			Map<Integer, Integer> result = GSON.fromJson(element.toString(), REFERENCE);

			Optional<Player> p = World.getPlayerByName(name);

			if (!p.isPresent()) {
				return;
			}

			p.get().runePouch.pouch.putAll(result);
		}

	  [MENTION=381607]Override[/MENTION]
		public JsonObject save(String name, Gson builder, JsonObject object) {

			Optional<Player> p = World.getPlayerByName(name);

			if (!p.isPresent()) {
				return object;
			}

			object.add("pouch", builder.toJsonTree(p.get().runePouch.pouch, REFERENCE));
			return object;
		}
	};

	static {
		new UserContainerInterface(POUCH_CONTAINER_ID) {

		  [MENTION=381607]Override[/MENTION]
			public boolean handleOption(Player player, int id, int slot, int option, int amount) {
				if (player.getInterfaceId() != INTERFACE_ID) {
					return false;
				}

				takeOut(player, slot);
				return true;
			}
		};

		new UserContainerInterface(INVENTORY_CONTAINER_ID) {

		  [MENTION=381607]Override[/MENTION]
			public boolean handleOption(Player player, int id, int slot, int option, int amount) {
				if (player.getInterfaceId() != INTERFACE_ID) {
					return false;
				}

				int existing = player.getInventory().getAmount(id);

				if (existing == 0) {
					return true;
				}

				store(player, new Item(id, existing));

				open(player);
				return true;
			}
		};
	}

	private Map<Integer, Integer> pouch = new HashMap<>();

	private static void open(Player p) {
		ArrayList<Item> pouchItems = new ArrayList<>();

		for (Map.Entry<Integer, Integer> e : p.runePouch.pouch.entrySet()) {
			pouchItems.add(new Item(e.getKey(), e.getValue()));
		}

		p.getPacketSender().sendItemContainerList(pouchItems, POUCH_CONTAINER_ID);

		p.getPacketSender().sendItemContainer(p.getInventory(), INVENTORY_CONTAINER_ID);

		p.getPacketSender().sendInterface(INTERFACE_ID);
	}

	private static void store(Player p, Item item) {
		int id = item.getId();

		if (!RUNE_IDS.contains(id)) {
			p.getPacketSender().sendMessage("You can only store runes into the rune pouch.");
			return;
		}

		if (!p.runePouch.pouch.containsKey(id) && p.runePouch.pouch.size() == MAX_POUCH_SIZE) {
			p.getPacketSender().sendMessage("Your rune pouch is full.");
			return;
		}

		if (!p.getInventory().contains(item)) {
			return;
		}

		String name = item.getName();
		int amount = item.getAmount();

		p.getInventory().delete(item);

		p.runePouch.pouch.merge(id, amount, Integer::sum);

		p.getPacketSender().sendMessage("You store " + Misc.format(amount) + " " + name + " into the rune pouch.");
	}

	private static void takeOut(Player p, int slot) {
		ArrayList<Integer> list = new ArrayList<>(p.runePouch.pouch.keySet());

		int id = list.get(slot);

		if (p.getInventory().getFreeSlots() > 0 || p.getInventory().contains(id)) {
			int existing = p.runePouch.pouch.getOrDefault(id, 0);

			if (existing == 0) {
				return;
			}

			p.runePouch.pouch.remove(id);

			p.getInventory().add(id, existing);

			open(p);
		} else {
			p.getInventory().full();
		}
	}

	private static void empty(Player p) {
		Map<Integer, Integer> pouchCopy = new HashMap<>(p.runePouch.pouch);

		for (Map.Entry<Integer, Integer> e : pouchCopy.entrySet()) {
			int id = e.getKey();
			int amount = e.getValue();

			if (p.getInventory().getFreeSlots() > 0 || p.getInventory().contains(id)) {
				p.runePouch.pouch.remove(id);
				p.getInventory().add(id, amount);
			}
		}
	}

	public static boolean useRune(Player p, Item rune) {
		int id = rune.getId();

		if (!RUNE_IDS.contains(id)) {
			return false;
		}

		if (!p.runePouch.pouch.containsKey(id)) {
			return false;
		}

		int existing = p.runePouch.pouch.getOrDefault(id, 0);

		if (rune.getAmount() > existing) {
			return false;
		}

		p.runePouch.pouch.put(id, existing - rune.getAmount());
		return true;
	}

  [MENTION=381607]Override[/MENTION]
	public boolean handleItemInteraction(Player player, Item item, int type) {
		if (item.getId() == RUNE_POUCH) {
			if (type == 1) {
				open(player);
			} else if (type == 2) {
				empty(player);
			}
			return true;
		}
		return false;
	}

  [MENTION=381607]Override[/MENTION]
	public boolean handleItemOnItemInteraction(Player player, Item use, Item usedWith) {
		if (usedWith.getId() == RUNE_POUCH) {
			store(player, use);
			return true;
		}
		return false;
	}
}
 
Remi said:
Just use EnumMap instead of HashMap when using enums as keys,

inside your enum instead of using private static final HashMap<Integer, Rune> FOR_ID = new HashMap<>();

you would use private static final Map<Integer, Rune> FOR_ID =
EnumSet.allOf(Rune.class).stream().collect(Collectors.toMap(r -> r.id, r -> r));
Click to expand...

Thats not an enummap. Thats just a hashmap same as he was using. Is there a reason to use an enum set over just directly calling the stream off of the enum values array? If so, id like to know because i usually just call the stream directly off the values array.
 
Advocatus said:
Thats not an enummap. Thats just a hashmap same as he was using. Is there a reason to use an enum set over just directly calling the stream off of the enum values array? If so, id like to know because i usually just call the stream directly off the values array.
Click to expand...

You are right that using EnumSet.allOf(Rune.class).stream().collect(Collectors.toMap(r -> r.id, r -> r)) creates a HashMap rather than an EnumMap. The main idea was to initialize the FOR_ID map efficiently.

However, still converting the pouch itself to an EnumMap is highly optimized for enums when the keys are enums;

private EnumMap<Rune, Integer> pouch = new EnumMap<>(Rune.class);

Streaming and using an EnumSet are both suitable for creating the map, but streaming is slightly simpler. I used EnumSet because it makes the intent of working with all enum constants clear and provides better performance and maintainability.

JayArrowz said:
Why use a enum at all?
[/code]
Click to expand...

Limited functionality :penguin:
 
  • Like
Reactions: Dexter Morgan
Remi said:
You are right that using EnumSet.allOf(Rune.class).stream().collect(Collectors.toMap(r -> r.id, r -> r)) creates a HashMap rather than an EnumMap. The main idea was to initialize the FOR_ID map efficiently.

However, still converting the pouch itself to an EnumMap is highly optimized for enums when the keys are enums;

private EnumMap<Rune, Integer> pouch = new EnumMap<>(Rune.class);

Streaming and using an EnumSet are both suitable for creating the map, but streaming is slightly simpler. I used EnumSet because it makes the intent of working with all enum constants clear and provides better performance and maintainability.



Limited functionality :penguin:
Click to expand...

What are you talking about? I dont understand what you mean by limited functionality. There is no need to use a enum here
 
  • Like
Reactions: Dexter Morgan
Advocatus said:
Thats not an enummap. Thats just a hashmap same as he was using. Is there a reason to use an enum set over just directly calling the stream off of the enum values array? If so, id like to know because i usually just call the stream directly off the values array.
Click to expand...

I think its fine, either way, streams just make this pretty common operation into a 1-liner. But even using the static block to initialize is okay.

JayArrowz said:
What are you talking about? I dont understand what you mean by limited functionality. There is no need to use a enum here
Click to expand...

Its definitely useful. Like in the second enum, you can have something like Rune#isRune which hides away implementation details. You could also have a Rune#forItemId which can either throw exception or return Optional. You can use a hashset, enumset, hashmap, whatever, it can change without it impacting your code.

But beyond that, a rune enum could be useful for multiple reasons. we can use it to define runes for spells. We could have a system entire "CastSpell" code could directly interact via Runes without ever having to convert to item ids - at least when interacting with the rune pouch and not inventory.

And its nice to have organizationally. There is literally no reason to prefer a hashset with a bunch of random numbers (but we added comments). Idk why we'd prefer that even if we do literally nothing else with this enum lol.
 
Intrice Joe said:
I think its fine, either way, streams just make this pretty common operation into a 1-liner. But even using the static block to initialize is okay.



Its definitely useful. Like in the second enum, you can have something like Rune#isRune which hides away implementation details. You could also have a Rune#forItemId which can either throw exception or return Optional. You can use a hashset, enumset, hashmap, whatever, it can change without it impacting your code.

But beyond that, a rune enum could be useful for multiple reasons. we can use it to define runes for spells. We could have a system entire "CastSpell" code could directly interact via Runes without ever having to convert to item ids - at least when interacting with the rune pouch and not inventory.

And its nice to have organizationally. There is literally no reason to prefer a hashset with a bunch of random numbers (but we added comments). Idk why we'd prefer that even if we do literally nothing else with this enum lol.
Click to expand...

The enums main purpose is to store the rune ID's, and ensure the subsequent logic checks that the rune can be used with the pouch.
You do not need a enum to store a simple "contains" functionality. You could even use a List, array (with a helper function) or a set.
There is no point of using a enum, you do not need to strongly reference any of the enum items, just declare them as what they are an array of IDs.

Infact in the enum the OP uses he even uses a type which he does not need and uses more than needed: HashMap<Integer, Rune>
Why? Why doesn't the OP just use an array, list or set INSTEAD of transfering all the enum items to a HashMap to be able to use "contains"?


Yes and "contains" is all he needs, because even when OP gets the rune for the HashMap he's only using the "contains" function AND the "Rune" object to save in json when he can use the ID directly and completely remove the useless rune enum and hashmap which makes the code overly complex

Some chatgpt if my explanation is hard to understand:
ENk0GmZ.png
 
Last edited:
JayArrowz said:
The enums main purpose is to store the rune ID's, and ensure the subsequent logic checks that the rune can be used with the pouch.
You do not need a enum to store a simple "contains" functionality. You could even use a List, array (with a helper function) or a set.
There is no point of using a enum, you do not need to strongly reference any of the enum items, just declare them as what they are an array of IDs.

Infact in the enum the OP uses he even uses a type which he does not need and uses more than needed: HashMap<Integer, Rune>
Why? Why doesn't the OP just use an array, list or set INSTEAD of transfering all the enum items to a HashMap to be able to use "contains"?


Yes and "contains" is all he needs, because even when OP gets the rune for the HashMap he's only using the "contains" function AND the "Rune" object to save in json when he can use the ID directly and completely remove the useless rune enum and hashmap which makes the code overly complex

Some chatgpt if my explanation is hard to understand:
ENk0GmZ.png
Click to expand...

I have to agree with you. A Set of the rune ids would of been better than an whole enum.

Thanks for your criticism!
 
Remi said:
Just use EnumMap instead of HashMap when using enums as keys,

inside your enum instead of using private static final HashMap<Integer, Rune> FOR_ID = new HashMap<>();

you would use private static final Map<Integer, Rune> FOR_ID =
EnumSet.allOf(Rune.class).stream().collect(Collectors.toMap(r -> r.id, r -> r));

than you would be able to call the rune like Rune rune = Rune.FOR_ID.get(item.getId());

this allows for O(1) time when looking up a rune by its id

for store, you would be able to use
Code: 
private void store(Item item) {
    Rune rune = Rune.FOR_ID.get(item.getId());
    if (rune == null) {
        player.getPacketSender().sendMessage("You can only store runes into the rune pouch.");
        return;
    }

    if (!pouch.containsKey(rune) && pouch.size() == MAX_POUCH_SIZE) {
        player.getPacketSender().sendMessage("Your rune pouch is full.");
        return;
    }

for take out you can streamline the removable of runes

Code: 
private void takeOut(int slot) {
    Rune rune = new ArrayList<>(pouch.keySet()).get(slot);
    if (player.getInventory().hasFreeSlots() || player.getInventory().contains(rune.id)) {
        int amount = pouch.remove(rune);
        if (amount > 0) {
            player.getInventory().add(rune.id, amount);
            open();
        }

for handleItemInteraction, you dont need a switch as its only one item id

just use an if statement if its RUNE_POUCH

Code: 
public boolean handleItemInteraction(Player player, Item item, int type) {
    if (item.getId() == RUNE_POUCH) {
        if (type == 1) {
            open();
        } else if (type == 2) {
            empty();
        }
        return true;
    }
Click to expand...

Code: 
        private static final Map<Integer, Rune> FOR_ID = new EnumMap<>(Rune.class);

        static {
            for (Rune r : values()) {
                FOR_ID.put(r.id, r);
            }
        }

        public static Rune getById(int id) {
            return FOR_ID.get(id);
        }
    }
 
JayArrowz said:
The enums main purpose is to store the rune ID's, and ensure the subsequent logic checks that the rune can be used with the pouch.
You do not need a enum to store a simple "contains" functionality. You could even use a List, array (with a helper function) or a set.
There is no point of using a enum, you do not need to strongly reference any of the enum items, just declare them as what they are an array of IDs.

Infact in the enum the OP uses he even uses a type which he does not need and uses more than needed: HashMap<Integer, Rune>
Why? Why doesn't the OP just use an array, list or set INSTEAD of transfering all the enum items to a HashMap to be able to use "contains"?


Yes and "contains" is all he needs, because even when OP gets the rune for the HashMap he's only using the "contains" function AND the "Rune" object to save in json when he can use the ID directly and completely remove the useless rune enum and hashmap which makes the code overly complex

Some chatgpt if my explanation is hard to understand:
ENk0GmZ.png
Click to expand...

true, we should design things only for the immediate use case with no consideration of how it may or may not be useful otherwise

SomeSpell(cost=Rune.FIRE, Rune....)

Or also runes can contain additional data within them for combination runes
LAVA_RUNE(combination=FIRE, EARTH)

But lets ignore that. Practically speaking, creating an enum here doesn't take any more actual dev time, and the enum is not what makes the code complex here lol. Things like strict typing and the code being more readable are widely more valuable than the least complex data structure. Much more preferrable to a bunch of lists/maps of integers hanging around only being known by comments and variable names.
 
You must log in or register to reply here.

Similar threads

Dexter Morgan
317 All Skill Menus
Replies
1
Views
1K
Snippets
socklol
socklol
Dexter Morgan
Offline Item Collection System
Replies
6
Views
2K
Snippets
Farchest
F
Corrupt
317 A better server sided interface system
Replies
1
Views
559
Snippets
.css
.css
Dexter Morgan
Best In Slot
Replies
4
Views
1K
Snippets
Intrice Joe
I
Dexter Morgan
Basic Achievement System
Replies
0
Views
738
Snippets
Dexter Morgan
Dexter Morgan

Users who are viewing this thread (total: 1, members: 0, guests: 1)

Who read this thread (total members: 29)

Top Bottom