Thread: Prize drawing class (redone)

Not really a fan of the design of it, kinda limits you straight off the bat. Instead of pointing out the problems/issues, I decided to write my own version, using your code and your design of it(somewhat). Perhaps it'll help you better understand.. something; if nothing, at least it shows java format(it was automatically all formatted by eclipse IDE)

Code:

/**
 * Enum for a prize
 */
public enum Prize {

	/**
	 * Example blood money
	 */
	LOW_BM(50, 150, GameConstants.BLOOD_MONEY);

	private static final Prize[] VALUES = values();
	private static final Map<PrizeRarity, List<Prize>> MAPPED_VALUES = new HashMap<PrizeRarity, List<Prize>>(PrizeRarity.VALUES.length);

	/**
	 * Initialises a new instance of the prize enum Allows for rarity, lower quantity bound, upper quantity bound
	 */
	private Prize(final PrizeRarity prizeRarity, final Integer lowerQuantityBound, final int upperQuantityBound, final int... prizeItemIds) {
		this.prizeRarity = prizeRarity;
		this.randomQuantity = lowerQuantityBound != null;
		this.lowerQuantityBound = lowerQuantityBound;
		this.upperQuantityBound = upperQuantityBound;
		this.prizeItemIds = prizeItemIds;
	}

	static {
		// Fill the map with empty lists to keep it basically null-safe.
		for (final PrizeRarity rarity : PrizeRarity.VALUES) {
			MAPPED_VALUES.put(rarity, new ArrayList<Prize>());
		}

		// Populate the respective lists with prizes.
		for (final Prize prize : VALUES) {
			final PrizeRarity rarity = prize.getRarity();
			final List<Prize> list = MAPPED_VALUES.get(rarity);
			list.add(prize);
		}
	}

	/**
	 * Initialises a new instance of the prize enum Allows for a rarity and set quantity
	 */
	private Prize(final PrizeRarity rarity, final int quantity, final int... prizeIds) {
		this(rarity, null, quantity, prizeIds);
	}

	/**
	 * Initialises a new instance of the prize enum Allows for a lower bound of quantity and upper bound of quantity Defaults to low rarity
	 */
	private Prize(final Integer lowerQuantityBound, final Integer upperQuantityBound, final int... prizeIds) {
		this(PrizeRarity.LOW, lowerQuantityBound, upperQuantityBound, prizeIds);
	}

	/**
	 * Initialises a new instance of the prize enum Allows for a set quantity Array due to ambiguous constructors Defaults to low rarity
	 */
	private Prize(final int quantity, final int[] prizeIds) {
		this(PrizeRarity.LOW, null, quantity, prizeIds);
	}

	/**
	 * Method used to get the prize item ids.
	 */
	public int[] getPrizes() {
		return prizeItemIds;
	}

	/**
	 * Gets the prize rarity
	 */
	public PrizeRarity getRarity() {
		return prizeRarity;
	}

	/**
	 * Method used to get the prize quantity
	 */
	public int getQuantity() {
		if (randomQuantity) {
			return Misc.inclusive(lowerQuantityBound, upperQuantityBound);
		}
		return upperQuantityBound;
	}

	/**
	 * The prize rarity
	 */
	private final PrizeRarity prizeRarity;

	/**
	 * The prizes
	 */
	private final int[] prizeItemIds;

	/**
	 * Determines if the quantity is to be random
	 */
	private final boolean randomQuantity;

	/**
	 * The lower quantity bound
	 */
	private final int lowerQuantityBound;

	/**
	 * The upper quantity bound
	 */
	private final int upperQuantityBound;

	/**
	 * Gets a random prize from the requested rarity group, or null if there are none.
	 * 
	 * @param rarity
	 *            the rarity for the prize.
	 * @return a prize object, or null if none could be found.
	 */
	public static final Prize getRandomPrize(final PrizeRarity rarity) {
		final List<Prize> list = MAPPED_VALUES.get(rarity);
		if (list == null || list.isEmpty()) {
			return null;
		}
		return list.get(Misc.getRandom(list.size()));// Make sure the getRandom() method returns a value between 0 & (length - 1), or you'll
														// get an array out of bounds error.
	}

}

/**
 * Enum for a prize's rarity
 */
enum PrizeRarity {
	LOW,
	MEDIUM,
	HIGH,
	RARE,
	VERY_RARE;

	/**
	 * A constant array of the rarities, because values() creates a new array every time it's referenced.
	 */
	public static final PrizeRarity[] VALUES = values();

}

Originally Posted by Kris

Not really a fan of the design of it, kinda limits you straight off the bat. Instead of pointing out the problems/issues, I decided to write my own version, using your code and your design of it(somewhat). Perhaps it'll help you better understand.. something; if nothing, at least it shows java format(it was automatically all formatted by eclipse IDE)

Code:

/**
 * Enum for a prize
 */
public enum Prize {

	/**
	 * Example blood money
	 */
	LOW_BM(50, 150, GameConstants.BLOOD_MONEY);

	private static final Prize[] VALUES = values();
	private static final Map<PrizeRarity, List<Prize>> MAPPED_VALUES = new HashMap<PrizeRarity, List<Prize>>(PrizeRarity.VALUES.length);

	/**
	 * Initialises a new instance of the prize enum Allows for rarity, lower quantity bound, upper quantity bound
	 */
	private Prize(final PrizeRarity prizeRarity, final Integer lowerQuantityBound, final int upperQuantityBound, final int... prizeItemIds) {
		this.prizeRarity = prizeRarity;
		this.randomQuantity = lowerQuantityBound != null;
		this.lowerQuantityBound = lowerQuantityBound;
		this.upperQuantityBound = upperQuantityBound;
		this.prizeItemIds = prizeItemIds;
	}

	static {
		// Fill the map with empty lists to keep it basically null-safe.
		for (final PrizeRarity rarity : PrizeRarity.VALUES) {
			MAPPED_VALUES.put(rarity, new ArrayList<Prize>());
		}

		// Populate the respective lists with prizes.
		for (final Prize prize : VALUES) {
			final PrizeRarity rarity = prize.getRarity();
			final List<Prize> list = MAPPED_VALUES.get(rarity);
			list.add(prize);
		}
	}

	/**
	 * Initialises a new instance of the prize enum Allows for a rarity and set quantity
	 */
	private Prize(final PrizeRarity rarity, final int quantity, final int... prizeIds) {
		this(rarity, null, quantity, prizeIds);
	}

	/**
	 * Initialises a new instance of the prize enum Allows for a lower bound of quantity and upper bound of quantity Defaults to low rarity
	 */
	private Prize(final Integer lowerQuantityBound, final Integer upperQuantityBound, final int... prizeIds) {
		this(PrizeRarity.LOW, lowerQuantityBound, upperQuantityBound, prizeIds);
	}

	/**
	 * Initialises a new instance of the prize enum Allows for a set quantity Array due to ambiguous constructors Defaults to low rarity
	 */
	private Prize(final int quantity, final int[] prizeIds) {
		this(PrizeRarity.LOW, null, quantity, prizeIds);
	}

	/**
	 * Method used to get the prize item ids.
	 */
	public int[] getPrizes() {
		return prizeItemIds;
	}

	/**
	 * Gets the prize rarity
	 */
	public PrizeRarity getRarity() {
		return prizeRarity;
	}

	/**
	 * Method used to get the prize quantity
	 */
	public int getQuantity() {
		if (randomQuantity) {
			return Misc.inclusive(lowerQuantityBound, upperQuantityBound);
		}
		return upperQuantityBound;
	}

	/**
	 * The prize rarity
	 */
	private final PrizeRarity prizeRarity;

	/**
	 * The prizes
	 */
	private final int[] prizeItemIds;

	/**
	 * Determines if the quantity is to be random
	 */
	private final boolean randomQuantity;

	/**
	 * The lower quantity bound
	 */
	private final int lowerQuantityBound;

	/**
	 * The upper quantity bound
	 */
	private final int upperQuantityBound;

	/**
	 * Gets a random prize from the requested rarity group, or null if there are none.
	 * 
	 * @param rarity
	 *            the rarity for the prize.
	 * @return a prize object, or null if none could be found.
	 */
	public static final Prize getRandomPrize(final PrizeRarity rarity) {
		final List<Prize> list = MAPPED_VALUES.get(rarity);
		if (list == null || list.isEmpty()) {
			return null;
		}
		return list.get(Misc.getRandom(list.size()));// Make sure the getRandom() method returns a value between 0 & (length - 1), or you'll
														// get an array out of bounds error.
	}

}

/**
 * Enum for a prize's rarity
 */
enum PrizeRarity {
	LOW,
	MEDIUM,
	HIGH,
	RARE,
	VERY_RARE;

	/**
	 * A constant array of the rarities, because values() creates a new array every time it's referenced.
	 */
	public static final PrizeRarity[] VALUES = values();

}

Thanks for the rewrite, it's quite helpful to see how you would've done it! Can I ask how my original was limited? Thanks!

Originally Posted by CodeForScape

Thanks for the rewrite, it's quite helpful to see how you would've done it! Can I ask how my original was limited? Thanks!

Rarity groups. I know this is heavily used in 317s for some reason, but it's a pretty bad design IMO. You're forced to use one of the five rates for all items. A much better option is a weighted system. A weighted system would eliminate the need for all those nasty lists as well.

Also um this isn't how I would've done it, as I would've definitely gone with a weighted system. Besides, due to lack of information I can't really tell how I would've gone. Would have to know all the details about the system beforehand, evaluate my choices and only then start writing it.

Originally Posted by Kris

Rarity groups. I know this is heavily used in 317s for some reason, but it's a pretty bad design IMO. You're forced to use one of the five rates for all items. A much better option is a weighted system. A weighted system would eliminate the need for all those nasty lists as well.

Also um this isn't how I would've done it, as I would've definitely gone with a weighted system. Besides, due to lack of information I can't really tell how I would've gone. Would have to know all the details about the system beforehand, evaluate my choices and only then start writing it.

For every day developments, do you tend to properly flesh things like this out? Thanks for all the info, really helpful!

Originally Posted by CodeForScape

For every day developments, do you tend to properly flesh things like this out? Thanks for all the info, really helpful!

Usually yes. I try to avoid having to rewrite content, got a bit of an OCD so whenever something is wrong.. I have to fix it the right way and that's usually through rewriting (not quick-patching or whatever).
Although in some cases, if I ain't sure what I need.. I just randomly start writing and see where it takes me. Usually end up rewriting that piece of content multiple times before I'm satisfied though.

Originally Posted by Kris

Usually yes. I try to avoid having to rewrite content, got a bit of an OCD so whenever something is wrong.. I have to fix it the right way and that's usually through rewriting (not quick-patching or whatever).
Although in some cases, if I ain't sure what I need.. I just randomly start writing and see where it takes me. Usually end up rewriting that piece of content multiple times before I'm satisfied though.

Very sound advice. Thanks very much.