Thread: How can I improve this code?

Page 1 of 2 12 LastLast
Results 1 to 10 of 11
  1. #1 How can I improve this code? 
    Registered Member
    Join Date
    May 2015
    Posts
    158
    Thanks given
    52
    Thanks received
    5
    Rep Power
    11
    Some code of mine is looking a bit off and me being such a perfectionist, want a cleaner way of writing this out.

    Don't like the way this looks, having written out the same text twice looks ugly. Isn't there a cleaner way to do this?
    Code:
    player.getPackets().sendIComponentText(788, 18, burnedLogs ? "<col=008000>Burn 100 yew logs" : "<col=DF0101>Burn 100 yew logs");
    Seems a bit much for just a simple message, can this be shorten?
    Code:
    public void completionMessage(int difficulty) {
    		switch (difficulty) {
    		case 1: {
    			player.getPackets().sendGameMessage("<col=FF0000>Well done! You have completed a easy task. Your Achievement Diary has been updated.");
    			return;
    		}
    		case 2: {
    			player.getPackets().sendGameMessage("<col=FF0000>Well done! You have completed a medium task. Your Achievement Diary has been updated.");
    			return;
    		}
    		case 3: {
    			player.getPackets().sendGameMessage("<col=FF0000>Well done! You have completed a hard task. Your Achievement Diary has been updated.");
    			return;
    		}
    		case 4: {
    			player.getPackets().sendGameMessage("<col=FF0000>Well done! You have completed a elite task. Your Achievement Diary has been updated.");
    			return;
    		}
    		}
    	}
    Reply With Quote  
     

  2. #2  
    Registered Member Zirnek's Avatar
    Join Date
    May 2016
    Posts
    34
    Thanks given
    4
    Thanks received
    1
    Rep Power
    11
    Code:
    player.getPackets().sendGameMessage("<col=FF0000>@@@@  task has been Completed.");
    or just
    Code:
    player.getPackets().sendGameMessage("<col=FF0000>@@@@  Achievement Diary has been updated.");
    If you want the message shortened, I don't see how you're getting the two in one message without it sounding off.

    And regarding the first bit of code, if you're trying to make an accomplishment diary/list, you can try it the way I do that, if you want,
    Add the red text with brackets containing the int of how many yew logs have already been burnt, and replace that with green when it's done.
    Like so:
    Burn 100 yew logs (58);
    Burn 100 yew logs;
    Reply With Quote  
     

  3. #3  
    Registered Member
    Stugger's Avatar
    Join Date
    Apr 2016
    Posts
    208
    Thanks given
    119
    Thanks received
    294
    Rep Power
    358
    I think the first one is generally fine but you should just use the shorthand if/else on what is actually affected by it, like so:
    Code:
    player.getPackets().sendIComponentText(788, 18, (burnedLogs ? "<col=008000>" : <col=DF0101>") + "Burn 100 yew logs"); //only color is within scope of if/else
    As for the rest, I don't know how all your achievement stuff is done, but you can establish a utility object/enum to attach to content that uses difficulty as a defining factor. It can help with organizing, readability and expression (messages)
    like so:
    Code:
    public enum Difficulty {
    	EASY,
    	MEDIUM,
    	HARD,
    	ELITE
    	;
    	@Override
    	public String toString() {
    		return name().substring(0, 1) + name().substring(1).toLowerCase(); //such as "Easy" as opposed to "EASY"
    	}
    }
    Then you can attach a reference to a Difficulty to whichever objects would benefit from it, such as your task object, a basic example would be:

    Code:
    public class AchievementTask {
    	public int progress;
    	public Difficulty difficulty; //may want to store ordinal or something instead (for serialization sake), up to you, just an example
    }
    Then you can reduce that switch statement to one line, and use other switch statements based on difficulties in a more readable and finalized fashion.. like determining points rewarded for example.

    Code:
    public class AchievementTask {
    	public int progress;
    	public Difficulty difficulty;
    	
    	public void sendCompletionMessage() {
    		player.getPackets().sendGameMessage("<col=FF0000>Well done! You have completed a " + difficulty.toString() + " task. Your Achievement Diary has been updated.");
    	}
    	
    	public int getCompletionPoints() {
    		switch (difficulty) {
    			case EASY: //a lot more readable and debugable than using integers as keys
    				return 5;
    			case MEDIUM:
    				return 10;
    			case HARD:
    				return 15;
    			case ELITE:
    				return 20;
    			default:
    				return 0;
    		}
    	}
    Reply With Quote  
     

  4. Thankful user:


  5. #4  
    Registered Member

    Join Date
    Jul 2014
    Posts
    85
    Thanks given
    19
    Thanks received
    49
    Rep Power
    79
    Hi,

    I'm not sure I would approve of having a difficulty as an int, however ignoring this fact & keeping code change to a minimum - you could do the following:
    Code:
    private static final String[] DIFFICULTY = { "easy", "medium", "hard", "elite" };
    
    public void completionMessage(int difficulty) {
        player.getPackets().sendGameMessage(
            "<col=FF0000>Well done! You have completed a " + DIFFICULTY[difficulty - 1] + " task. Your Achievement Diary has been updated."
        );
    }
    If (difficulty - 1) can be outside the array range you'd have to add checks to prevent array index out of bounds, otherwise this is fine.

    You could also use string interpolation if you preferred:
    Code:
    player.getPackets().sendGameMessage(
        String.format("<col=FF0000>Well done! You have completed a %s task. Your Achievement Diary has been updated.", DIFFICULTY[difficulty - 1])
    );
    I would personally encourage not having strings within code in the first place and placing them in a properties file or elsewhere.

    In regards to the first snippet provided, what Stugger has provided is sufficient.
    Reply With Quote  
     

  6. Thankful user:


  7. #5  
    nice


    Join Date
    Jul 2014
    Posts
    740
    Thanks given
    382
    Thanks received
    562
    Rep Power
    4239
    NOTE: this is just a few tips on how u can make ur current code shorter and not how to design what ur trying to do in a better way (some of the answers above do explain that already)
    The first can be shortened to:
    Code:
    player.getPackets().sendIComponentText(788, 18, burnedLogs ? "<col=008000>" : "<col=DF0101>" + "Burn 100 yew logs");
    For the other, you can use switch expressions (instead of switch statements), introduced in java 12 as a preview feature and actually added in 13 iirc:
    Code:
        public void completionMessage(int difficulty) {
            String message = switch (difficulty) {
                case 1 -> "<col=FF0000>Well done! You have completed a easy task. Your Achievement Diary has been updated.";
                case 2 -> "<col=FF0000>Well done! You have completed a medium task. Your Achievement Diary has been updated.";
                case 3 -> "<col=FF0000>Well done! You have completed a hard task. Your Achievement Diary has been updated.";
                case 4 -> "<col=FF0000>Well done! You have completed a elite task. Your Achievement Diary has been updated.";
                default -> throw new IllegalStateException("Unexpected value: " + difficulty);
            };
            player.getPackets().sendGameMessage(message);
        }
    We can do better tho, the 4 messages are identical except for the 'difficulty' string in the middle:
    Code:
        public void completionMessage(int difficulty) {
            String taskDifficulty = switch (difficulty) {
                case 1 -> "easy";
                case 2 -> "medium";
                case 3 -> "hard";
                case 4 -> "elite";
                default -> throw new IllegalStateException("Unexpected value: " + difficulty);
            };
            player.getPackets().sendGameMessage("<col=FF0000>Well done! You have completed a " + taskDifficulty + " task. Your Achievement Diary has been updated.");
        }
    Attached image
    Reply With Quote  
     

  8. Thankful user:


  9. #6  
    Registered Member

    Join Date
    Jul 2014
    Posts
    85
    Thanks given
    19
    Thanks received
    49
    Rep Power
    79
    Quote Originally Posted by Suic View Post
    NOTE: this is just a few tips on how u can make ur current code shorter and not how to design what ur trying to do in a better way (some of the answers above do explain that already)
    The first can be shortened to:
    Code:
    player.getPackets().sendIComponentText(788, 18, burnedLogs ? "<col=008000>" : "<col=DF0101>" + "Burn 100 yew logs");
    For the other, you can use switch expressions (instead of switch statements), introduced in java 12 as a preview feature and actually added in 13 iirc:
    Code:
        public void completionMessage(int difficulty) {
            String message = switch (difficulty) {
                case 1 -> "<col=FF0000>Well done! You have completed a easy task. Your Achievement Diary has been updated.";
                case 2 -> "<col=FF0000>Well done! You have completed a medium task. Your Achievement Diary has been updated.";
                case 3 -> "<col=FF0000>Well done! You have completed a hard task. Your Achievement Diary has been updated.";
                case 4 -> "<col=FF0000>Well done! You have completed a elite task. Your Achievement Diary has been updated.";
                default -> throw new IllegalStateException("Unexpected value: " + difficulty);
            };
            player.getPackets().sendGameMessage(message);
        }
    We can do better tho, the 4 messages are identical except for the 'difficulty' string in the middle:
    Code:
        public void completionMessage(int difficulty) {
            String taskDifficulty = switch (difficulty) {
                case 1 -> "easy";
                case 2 -> "medium";
                case 3 -> "hard";
                case 4 -> "elite";
                default -> throw new IllegalStateException("Unexpected value: " + difficulty);
            };
            player.getPackets().sendGameMessage("<col=FF0000>Well done! You have completed a " + taskDifficulty + " task. Your Achievement Diary has been updated.");
        }
    Following on from this, if you are interested about these "Switch Expressions" you can read more about them here: https://openjdk.java.net/jeps/361
    Reply With Quote  
     

  10. #7  
    Registered Member
    Join Date
    May 2015
    Posts
    158
    Thanks given
    52
    Thanks received
    5
    Rep Power
    11
    Quote Originally Posted by Suic View Post
    NOTE: this is just a few tips on how u can make ur current code shorter and not how to design what ur trying to do in a better way (some of the answers above do explain that already)
    The first can be shortened to:
    Code:
    player.getPackets().sendIComponentText(788, 18, burnedLogs ? "<col=008000>" : "<col=DF0101>" + "Burn 100 yew logs");
    For the other, you can use switch expressions (instead of switch statements), introduced in java 12 as a preview feature and actually added in 13 iirc:
    Code:
        public void completionMessage(int difficulty) {
            String message = switch (difficulty) {
                case 1 -> "<col=FF0000>Well done! You have completed a easy task. Your Achievement Diary has been updated.";
                case 2 -> "<col=FF0000>Well done! You have completed a medium task. Your Achievement Diary has been updated.";
                case 3 -> "<col=FF0000>Well done! You have completed a hard task. Your Achievement Diary has been updated.";
                case 4 -> "<col=FF0000>Well done! You have completed a elite task. Your Achievement Diary has been updated.";
                default -> throw new IllegalStateException("Unexpected value: " + difficulty);
            };
            player.getPackets().sendGameMessage(message);
        }
    We can do better tho, the 4 messages are identical except for the 'difficulty' string in the middle:
    Code:
        public void completionMessage(int difficulty) {
            String taskDifficulty = switch (difficulty) {
                case 1 -> "easy";
                case 2 -> "medium";
                case 3 -> "hard";
                case 4 -> "elite";
                default -> throw new IllegalStateException("Unexpected value: " + difficulty);
            };
            player.getPackets().sendGameMessage("<col=FF0000>Well done! You have completed a " + taskDifficulty + " task. Your Achievement Diary has been updated.");
        }
    This looks super clean.

    Also thanks to everyone else for giving me some ideas.
    Reply With Quote  
     

  11. #8  
    nice


    Join Date
    Jul 2014
    Posts
    740
    Thanks given
    382
    Thanks received
    562
    Rep Power
    4239
    Quote Originally Posted by __super View Post
    Following on from this, if you are interested about these "Switch Expressions" you can read more about them here: https://openjdk.java.net/jeps/361
    They've been improving switch expressions pretty much every version, so that jep is a bit outdated
    For example in 17 they added pattern matching for switch expressions: https://www.baeldung.com/java-switch-pattern-matching
    So definitely read that too if u want to know everything about it, altho the jep covers most of what u would use 95% of the time(i think i've only used pattern matching for switch once in the last few months)
    Attached image
    Reply With Quote  
     

  12. #9  
    Extreme Donator

    JayArrowz's Avatar
    Join Date
    Sep 2008
    Posts
    104
    Thanks given
    99
    Thanks received
    107
    Rep Power
    810
    Quote Originally Posted by Levels View Post
    Some code of mine is looking a bit off and me being such a perfectionist, want a cleaner way of writing this out.

    Don't like the way this looks, having written out the same text twice looks ugly. Isn't there a cleaner way to do this?
    Code:
    player.getPackets().sendIComponentText(788, 18, burnedLogs ? "<col=008000>Burn 100 yew logs" : "<col=DF0101>Burn 100 yew logs");
    Seems a bit much for just a simple message, can this be shorten?
    Code:
    public void completionMessage(int difficulty) {
    		switch (difficulty) {
    		case 1: {
    			player.getPackets().sendGameMessage("<col=FF0000>Well done! You have completed a easy task. Your Achievement Diary has been updated.");
    			return;
    		}
    		case 2: {
    			player.getPackets().sendGameMessage("<col=FF0000>Well done! You have completed a medium task. Your Achievement Diary has been updated.");
    			return;
    		}
    		case 3: {
    			player.getPackets().sendGameMessage("<col=FF0000>Well done! You have completed a hard task. Your Achievement Diary has been updated.");
    			return;
    		}
    		case 4: {
    			player.getPackets().sendGameMessage("<col=FF0000>Well done! You have completed a elite task. Your Achievement Diary has been updated.");
    			return;
    		}
    		}
    	}


    You can do something like this:


    Code:
    var colour = burnedLogs ? "<col=008000>" : "<col=DF0101>";
    player.getPackets().sendIComponentText(788, 18, String.format("%sBurn 100 yew logs", colour));

    Also regarding this you can do something like this:

    Code:
    final Map<Integer, String> difficultyMap = new HashMap<Integer, String>() {{
        put(1, "easy");
        put(2, "medium");
        put(3, "hard");
        put(4, "elite");
    }};
    
    public void completionMessage(int difficulty) {
        var difficultyStr = difficultyMap.get(difficulty);
        player.getPackets().sendGameMessage(String.format("<col=FF0000>Well done! You have completed a %s task. Your Achievement Diary has been updated.", difficultyStr));
    }
    Reply With Quote  
     

  13. #10  
    nice


    Join Date
    Jul 2014
    Posts
    740
    Thanks given
    382
    Thanks received
    562
    Rep Power
    4239
    Quote Originally Posted by JayArrowz View Post
    You can do something like this:


    Code:
    var colour = burnedLogs ? "<col=008000>" : "<col=DF0101>";
    player.getPackets().sendIComponentText(788, 18, String.format("%sBurn 100 yew logs", colour));

    Also regarding this you can do something like this:

    Code:
    final Map<Integer, String> difficultyMap = new HashMap<Integer, String>() {{
        put(1, "easy");
        put(2, "medium");
        put(3, "hard");
        put(4, "elite");
    }};
    
    public void completionMessage(int difficulty) {
        var difficultyStr = difficultyMap.get(difficulty);
        player.getPackets().sendGameMessage(String.format("<col=FF0000>Well done! You have completed a %s task. Your Achievement Diary has been updated.", difficultyStr);
    }
    Code:
    Map<Integer, String> difficultyMap = new HashMap<Integer, String>() {{
        put(1, "easy");
        put(2, "medium");
        put(3, "hard");
        put(4, "elite");
    }};
    please don't initialize a hashmap like that, also not taking adventage of the diamond operator? hes definitely not using java 6 or lower
    To initialize a map easily we can use the
    Code:
    Map.of(...)
    method:
    Code:
            Map<Integer, String> difficultyMap = Map.of(
                    1, "easy",
                    2, "medium",
                    3, "hard",
                    4, "elite"
            );
    Which will create an unmodifiable map with the specified entries
    Attached image
    Reply With Quote  
     

  14. Thankful user:


Page 1 of 2 12 LastLast

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. How can I improve this code?
    By Levels in forum Help
    Replies: 4
    Last Post: 12-17-2021, 04:23 AM
  2. How can I improve this code?
    By Skrew in forum Help
    Replies: 4
    Last Post: 06-09-2019, 04:04 PM
  3. How can I improve this code?
    By Levels in forum Help
    Replies: 10
    Last Post: 10-14-2017, 12:43 PM
  4. How can I improve this piece of code?
    By Arham in forum Help
    Replies: 2
    Last Post: 08-05-2015, 07:44 PM
  5. How can I improve this?
    By Colby in forum Application Development
    Replies: 15
    Last Post: 01-26-2010, 12:22 AM
Posting Permissions
  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •