Thread: Blast Furnace

Page 2 of 2 FirstFirst 12
Results 11 to 15 of 15
  1. #11  
    Software Developer

    Tyrant's Avatar
    Join Date
    Jul 2013
    Age
    24
    Posts
    1,562
    Thanks given
    678
    Thanks received
    423
    Rep Power
    1060
    Quote Originally Posted by Andys1814 View Post
    There's nothing wrong with that really. I actually usually prefer that over static methods
    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.
    Reply With Quote  
     

  2. Thankful user:


  3. #12  
    Registered Member

    Join Date
    Jan 2011
    Posts
    1,940
    Thanks given
    1,217
    Thanks received
    547
    Rep Power
    607
    Quote Originally Posted by Kaleem View Post
    It also shouldn't be instanced per player..too much code to critique bro
    Any critique is welcome and encouraged. I'll post an updated version soon that should be a bit cleaner.
    Reply With Quote  
     

  4. Thankful user:


  5. #13  
    (Official) Thanksgiver

    Arham's Avatar
    Join Date
    Jan 2013
    Age
    23
    Posts
    3,415
    Thanks given
    7,254
    Thanks received
    1,938
    Rep Power
    3905
    • 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:
    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;
            }
        }
    }
    Player:
    Code:
        private List<Integer> oreDeposit; //store ores being added to conveyor belt here
    
        public List<Integer> getOreDeposit() {
            return oreDeposit;
        }
    Wherever you initiate variables saved in Player:
    Code:
    oreDeposit = new ArrayList<>();
    Take away question: Do you now see how powerful enums are to your code?


    Quote Originally Posted by Lumiere View Post
    I honestly don't even understand how that #depositOre method is working properly. Must be buggy as hell at the least.

    It would take far too long to list all the things wrong with this, and how it should be done properly..
    But something I think you should know is that;

    Code:
    			for (int id : ores) { // id == id's in ores array
    				if (ores != null && ore != null)
    					if (ore.getId() == id) { // if item id in inventory matches id in array, deposit will be success
    						if (!SkillHandler.hasRequiredLevel(player, Skill.SMITHING, getLevelReq(id), "smelt this")) {
    							return;
    						}
    						if (player.getInventory().playerHasItem(id))
    							oreDeposit.add(id);
    						player.getInventory().removeItem(new Item(id));
    						continue;
    					}
    			}
    The lines colored in blue, are single line statement blocks.
    For example;

    Code:
    			if (player.getRights().isStaff())
    				player.forceChat("I am staff!");
    It isn't enclosed in brackets, so the block ends with the first line after the if statement. A single line statement block.
    So, if you placed code after that, no matter what it is, if they are staff, or aren't staff doesn't matter either, it will continue on whatever is next.

    Code:
    			if (player.getRights().isStaff())
    				player.forceChat("I am staff!");
    
    			player.forceChat("I am not staff!");
    If you were to initiated that ^ in a process, or through a command, doesn't really matter..
    If the player was staff, and only if they were staff, they would be forced to say "I am staff!"
    Then, it would continue, and force the player to say "I am not staff!"..
    You'll notice, if the player is staff, they are forced to say both, "I am staff!" and "I am not staff!";
    because you've only created a single line statement block with the condition that, if the player is staff, force a chat, and nothing else.

    To make that work properly, you would need to add a return, which means, you would need to enclose that statement block in brackets to use multiple lines.
    Code:
    			if (player.getRights().isStaff()) {
    				player.forceChat("I am staff!");
    				return;
    			}
    			player.forceChat("I am not staff!");
    The point of this information is that.. You more than likely have bugs with this you don't even realize, and I can point one out on the topic of this information;
    Code:
    						if (player.getInventory().playerHasItem(id))
    							oreDeposit.add(id);
    						player.getInventory().removeItem(new Item(id));
    						continue;
    With that bit of code, if the player has the ore id in their inventory, deposit the ore. That's all that if statement does.
    So, if they don't have the ore id in their inventory, it wont add the id to the oreDeposit list, but it will continue to whatever is next.
    Which would then delete the ore from the players inventory, and the loop continue will be completely pointless without enclosing in that statement
    The oreId wont be added to the list, but the player will have the ore removed from their inventory, resulting in basically trashing the item for nothing.

    Now, this bit;
    Code:
    				if (ores != null && ore != null)
    					if (ore.getId() == id) { // if item id in inventory matches id in array, deposit will be success
    This might work, because it's reading the next if statement on the next line, but I am honestly unsure of how that would work.
    Let's be honest though, it'd probably just be easier combine those two if statements, rather than worry about enclosing the first if statement.
    Code:
    				if (ores != null && ore != null && ore.getId() == id) {
    This is the only statement you should NOT have enclosed, to save a couple lines and for in my opinion, easier readability in some cases.
    Code:
    			for (int id : ores) { // id == id's in ores array
    				if (ores != null && ore != null && ore.getId() == id) {
    						
    					if (!SkillHandler.hasRequiredLevel(player, Skill.SMITHING, getLevelReq(id), "smelt this")) 
    						return;
    					
    					if (player.getInventory().playerHasItem(id)) {
    						oreDeposit.add(id);
    						player.getInventory().removeItem(new Item(id));
    						continue;
    					}
    				}
    			}
    I even enclosed the other statements correctly for ya there
    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:
    Code:
    if(one)
        if(two)
            foo();
        else
            bar();
    From this:
    Code:
    if(one)
        if(two)
            foo();
    else
        bar();
    But in practice, it still works. Just discouraged due to maintanence.
    Attached image
    Attached image
    Quote Originally Posted by MrClassic View Post
    Arham is the official thanker!
    List of my work here!
    Reply With Quote  
     

  6. Thankful users:


  7. #14  
    What's a sundial in the shade?

    Lumiere's Avatar
    Join Date
    May 2013
    Age
    27
    Posts
    543
    Thanks given
    224
    Thanks received
    100
    Rep Power
    113
    Spoiler for Quote:
    Quote Originally Posted by arham 4 View Post
    • 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:
    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(new Integer(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
                for (Ore ore : Ore.VALUES) {
                    if (inventoryItem != null && ore != null) {
                        if (inventoryItem.getId() == ore.getOreId()) { // if item id in inventory matches id in array, deposit will be success
                            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),
            COAL(453, 30),
            GOLD(444, 40),
            SILVER(442, 20),
            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 int getOreId() {
                return oreId;
            }
    
            public int getLevelRequirement() {
                return levelRequirement;
            }
        }
    
        private enum Bar {
            RUNITE(2363, Ore.RUNITE, new Item(Ore.COAL.getOreId(), 4), 12, 50),
            ADAMANTITE(2361, Ore.ADAMANTITE, new Item(Ore.COAL.getOreId(), 3), 8, 38),
            MITHRIL(2359, Ore.MITHRIL, new Item(Ore.COAL.getOreId(), 2), 5, 30),
            STEEL(2353, Ore.IRON, new Item(Ore.COAL.getOreId(), 1), 4, 18),
            IRON(2351, Ore.IRON, null, 2, 13),
            COPPER(2349, Ore.COPPER, new Item(Ore.TIN.getOreId(), 1), 1, 6),
            GOLD(2357, Ore.GOLD, null, 3, 23),
            SILVER(2355, Ore.SILVER, null, 2, 14);
    
            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;
            }
        }
    }
    Player:
    Code:
        private List<Integer> oreDeposit; //store ores being added to conveyor belt here
    
        public List<Integer> getOreDeposit() {
            return oreDeposit;
        }
    Wherever you initiate variables saved in Player:
    Code:
    oreDeposit = new ArrayList<>();
    Take away question: Do you now see how powerful enums are to your code?




    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:
    Code:
    if(one)
        if(two)
            foo();
        else
            bar();
    From this:
    Code:
    if(one)
        if(two)
            foo();
    else
        bar();
    But in practice, it still works. Just discouraged due to maintanence.


    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

    Spoiler for Revy is perfect:
    Reply With Quote  
     

  8. #15  
    (Official) Thanksgiver

    Arham's Avatar
    Join Date
    Jan 2013
    Age
    23
    Posts
    3,415
    Thanks given
    7,254
    Thanks received
    1,938
    Rep Power
    3905
    Quote Originally Posted by Lumiere View Post

    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
    Yes, the comment is right. However, if a statement is made, like a while loop or an if-statement or something that has something happen after it's stated, then the if statement will follow along it to the end. (:
    Attached image
    Attached image
    Quote Originally Posted by MrClassic View Post
    Arham is the official thanker!
    List of my work here!
    Reply With Quote  
     

  9. Thankful user:


Page 2 of 2 FirstFirst 12

Thread Information
Users Browsing this Thread

There are currently 1 users browsing this thread. (0 members and 1 guests)


User Tag List

Similar Threads

  1. Blast Furnace
    By Jaegon in forum Selling
    Replies: 9
    Last Post: 11-16-2012, 06:47 AM
  2. Interbellum; Blast Furnace
    By brkownz in forum Show-off
    Replies: 19
    Last Post: 07-21-2012, 10:16 AM
  3. BLast Furnace Xfer
    By Zahhak in forum Guides
    Replies: 6
    Last Post: 01-22-2010, 09:54 PM
Posting Permissions
  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •