|
You're actually completely wrong and it's a bad habit. Static references are made for such things.
Why would you even think about storing the Player instance? do you ever access the variable as if it was not a local variable? it has no impact that requires to be instanced, just a waste of memory.
On topic, code is poorly written, you shouldn't iterate using while to create each orb, you do want it to pretend like the while effect, but you rather do it the way the server parses it correctly, using a task.
- When writing code, one should always have the DRY principle in mind. Not only is maintenance easier (not like it's a major thing to maintain in this case), it saves you time.
- Consider reading up naming conventions.
- You should name your methods, variables, and such to be as clear as possible. For example, checkOres() sounds like a boolean, when it really just sends a game message. Try something like sendMachineAmountMessage() (this might not be the best).
- Use access modifiers better. Everything is either package-private or public. Make access levels only for what's needed. Here's something to help with that.
- I don't see why you were even using static, but now it is required because you shouldn't instance each Player for this. Use your formal parameters to get Player.
I don't know how to explain why not to initiate player in this case (somebody else, please do so), I just feel it doesn't make logical sense for a class that handles a minigame.I see, the thread talked about it.- I'd make two enums. One handles the ores, another for the bars. Too much random info and lack of structure if it's a singular enum in my opinion. Not to mention making ores separate allows us to address singular ore like copper, whereas there is no copper bar for example.
- Making an Ore enum completely kills the getLevelRequirement() integer.
- The bars ArrayList is never used.
- oreDeposit should be something stored in Player, because this is player-dependent.
- Side note: removeSpecifiedAmount() really confused me, but I finally got it. I don't like the concept really.
Another side note specifically to your source: Doesn't RuneSource have an Action system or something? Looks like it. I CBA to make an action because I don't have a RuneSource source, but you should consider changing that while loop to an Action.Just realized it isn't an original RuneSource feature. Regardless, I feel like the while loop could be improved, just don't really remember what RuneSource has. Surely there is an Event system or something similar?
Keep in mind, I used Notepad++ to edit this code because I don't have a source to stick this into:
Player:Code:package com.rs2.model.content; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; import com.rs2.model.content.skills.Skill; import com.rs2.model.content.skills.SkillHandler; import com.rs2.model.players.Player; import com.rs2.model.players.item.Item; /** * @author Lucid * @author Arham 4 */ public final class BlastFurnace { private static final int NUGGET = 680; public static void createBar(Player player) { for (Bar bar : Bar.VALUES) { boolean containsSecondaryOre = bar.getSecondaryOre() == null || player.getOreDeposit().stream().filter(x -> x == bar.getSecondaryOre().getId()).count() >= bar.getSecondaryOre().getAmount(); /* * Should consider making the following loop into an Action */ while (player.getOreDeposit().contains(bar.getOre().getOreId()) && containsSecondaryOre && player.getInventory().getItemContainer().freeSlots() > 0) { player.getInventory().addItem(new Item(NUGGET, bar.getNuggets())); player.getOreDeposit().remove(bar.getOre().getOreId()); for (int i = 0; i < bar.getSecondaryOre().getAmount(); i++) { player.getOreDeposit().remove(bar.getSecondaryOre().getId()); } player.getInventory().addItem(new Item(bar.getBarId(), 1)); player.getSkill().addExp(Skill.SMITHING, bar.getExperience()); } } if (player.getInventory().getItemContainer().freeSlots() <= 0) { player.getActionSender().sendMessage("Your inventory is too full to hold anymore bars."); } else { player.getActionSender().sendMessage("There's no ore to be smelted."); } } public static void depositOre(Player player) { if (player.getOreDeposit().size() > 200) { player.getActionSender().sendMessage("Please collect your bars before entering more ore in the machine."); return; } for (Item inventoryItem : player.getInventory().getItemContainer().getItems()) { //go through inventory items if (inventoryItem != null) { Ore ore = Ore.getOre(inventoryItem.getId()); if (ore != null) { if (!SkillHandler.hasRequiredLevel(player, Skill.SMITHING, ore.getLevelRequirement(), "smelt this")) { return; } if (player.getInventory().playerHasItem(ore.getOreId())) { player.getOreDeposit().add(ore.getOreId()); } player.getInventory().removeItem(new Item(ore.getOreId())); } } } } public static void sendMachineAmountMessage(Player player) { player.getActionSender().sendMessage("You have a total of " + player.getOreDeposit().size() + " ore stored in the machine."); } private enum Ore { COPPER(436, 1), TIN(438, 1), IRON(440, 15), SILVER(442, 20), COAL(453, 30), GOLD(444, 40), MITHRIL(447, 50), ADAMANTITE(449, 70), RUNITE(451, 85); private static final List<Ore> VALUES = Collections.unmodifiableList(Arrays.asList(values())); private final int oreId; private final int levelRequirement; Ore(int oreId, int levelRequirement) { this.oreId = oreId; this.levelRequirement = levelRequirement; } public static Ore getOre(int oreId) { for (Ore ore : VALUES) { if (ore.getOreId() == oreId) { return ore; } } return null; } public int getOreId() { return oreId; } public int getLevelRequirement() { return levelRequirement; } } private enum Bar { COPPER(2349, Ore.COPPER, new Item(Ore.TIN.getOreId(), 1), 1, 6), IRON(2351, Ore.IRON, null, 2, 13), SILVER(2355, Ore.SILVER, null, 2, 14), STEEL(2353, Ore.IRON, new Item(Ore.COAL.getOreId(), 1), 4, 18), GOLD(2357, Ore.GOLD, null, 3, 23), MITHRIL(2359, Ore.MITHRIL, new Item(Ore.COAL.getOreId(), 2), 5, 30), ADAMANTITE(2361, Ore.ADAMANTITE, new Item(Ore.COAL.getOreId(), 3), 8, 38), RUNITE(2363, Ore.RUNITE, new Item(Ore.COAL.getOreId(), 4), 12, 50); private static final List<Bar> VALUES = Collections.unmodifiableList(Arrays.asList(values())); private final int barId; private final Ore ore; private final Item secondaryOre; private final int nuggets; private final int experience; Bar(int barId, Ore ore, Item secondaryOre, int nuggets, int experience) { this.barId = barId; this.ore = ore; this.secondaryOre = secondaryOre; this.nuggets = nuggets; this.experience = experience; } public int getBarId() { return barId; } public Ore getOre() { return ore; } public Item getSecondaryOre() { return secondaryOre; } public int getNuggets() { return nuggets; } public int getExperience() { return experience; } } }
Wherever you initiate variables saved in Player:Code:private List<Integer> oreDeposit; //store ores being added to conveyor belt here public List<Integer> getOreDeposit() { return oreDeposit; }
Take away question: Do you now see how powerful enums are to your code?Code:oreDeposit = new ArrayList<>();
It works perfectly fine. They just keep nesting the statements together. But it's bad practice, yes. As explained in StackOverFlow, there's no way of telling apart:
From this:Code:if(one) if(two) foo(); else bar();
But in practice, it still works. Just discouraged due to maintanence.Code:if(one) if(two) foo(); else bar();
Spoiler for Quote:
Good to know, thanks but, if actions are performed after that, are they also only performed under the condition of the statement?
Code:if(one) foo(); bar(); //I was told whether that statement was true or false, the #bar method would still be called
« Previous Thread | Next Thread » |
Thread Information |
Users Browsing this ThreadThere are currently 1 users browsing this thread. (0 members and 1 guests) |