Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

How to shorten up this code? I know there's a way that this kind of code could be shortened, but I don't know how by myself yet. Who can help me?

            //CHOOSE LEVEL: FROZEN
case 1213:
    sendOption5("Level 1 " + (c.achievedFloor <= 1 ? "@gre@(Achieved)" : "@red@(Not achieved)") + "",
            "Level 2 " + (c.achievedFloor <= 2 ? "@gre@(Achieved)" : "@red@(Not achieved)") + "",
            "Level 3 " + (c.achievedFloor <= 3 ? "@gre@(Achieved)" : "@red@(Not achieved)") + "",
            "Level 4 " + (c.achievedFloor <= 4 ? "@gre@(Achieved)" : "@red@(Not achieved)") + "",
            "More");
    c.dialogueAction = 1213;
    break;
case 1214:
    sendOption5("Level 5 " + (c.achievedFloor <= 5 ? "@gre@(Achieved)" : "@red@(Not achieved)") + "",
            "Level 6 " + (c.achievedFloor <= 6 ? "@gre@(Achieved)" : "@red@(Not achieved)") + "",
            "Level 7 " + (c.achievedFloor <= 7 ? "@gre@(Achieved)" : "@red@(Not achieved)") + "",
            "Level 8 " + (c.achievedFloor <= 8 ? "@gre@(Achieved)" : "@red@(Not achieved)") + "",
            "More");
    c.dialogueAction = 1214;
    break;
case 1215:
    sendOption5("Level 9 " + (c.achievedFloor <= 9 ? "@gre@(Achieved)" : "@red@(Not achieved)") + "",
            "Level 10 " + (c.achievedFloor <= 10 ? "@gre@(Achieved)" : "@red@(Not achieved)") + "",
            "Level 11 " + (c.achievedFloor <= 11 ? "@gre@(Achieved)" : "@red@(Not achieved)") + "",
            "",
            "Choose Floor");
    c.dialogueAction = 1215;
    break;
share|improve this question
4  
Welcome to Code Review! Can you explain a bit about the context of the code? What is it used for? What is it doing? Providing some context will likely give you better reviews. –  Simon André Forsberg Mar 8 at 11:36

5 Answers 5

In a first step I would extract a method to generate the string.

changed code:

            //CHOOSE LEVEL: FROZEN
case 1213:
    sendOption5(
            levelString( 1, c),
            levelString( 2, c),
            levelString( 3, c),
            levelString( 4, c),
            "More");
    c.dialogueAction = 1213; 
    break;
case 1214:
    sendOption5(
            levelString( 5, c),
            levelString( 6, c),
            levelString( 7, c),
            levelString( 8, c),
            "More");
    c.dialogueAction = 1214;
    break;
case 1215:
    sendOption5(
            levelString( 9, c),
            levelString(10, c),
            levelString(11, c),
            "",
            "Choose Floor");
    c.dialogueAction = 1215;
    break;

extracted method:

private static String levelString(int i, TypeOfC c){
   return "Level " + i + (c.achievedFloor <= i ? " @gre@(Achieved)" : " @red@(Not achieved)");
}

You should also think about changing the parameters of your sendOption5 to get only c and two integers from to which define the inteval of values.

And you can move the c.dialogueAction = xxxx; line behind the switch and assign the value you use for the switch.

share|improve this answer

There are two sets of repeating code here. The standard solution to DRY (Don't repeat yourself) is Function Extraction.

Consider two functions:

private static String levelAchievedOption(int level, int achieved) {
    return String.format("Level %d @%s", level,
         achieved >= level ? "gre@(Achieved)" : "red@(Not Achieved)");
}

Then, the second function for setting the options:

private static void setLevelOptions(int levela, int levelb, int levelc, int leveld, String postfix, int levelAchieved) {
    sendOption5(
        levela >= 0 ? levelAchievedOption(levela, levelAchieved) : "",
        levelb >= 0 ? levelAchievedOption(levelb, levelAchieved) : "",
        levelc >= 0 ? levelAchievedOption(levelc, levelAchieved) : "",
        leveld >= 0 ? levelAchievedOption(leveld, levelAchieved) : "",
        postfix);
}

Then, your case statement can become:

case 1213:
    setLevelOptions(1, 2, 3, 4, "More", c.achievedFloor);
    c.dialogueAction = 1213;
    break;
case 1214:
    setLevelOptions(5, 6, 7, 8, "More", c.achievedFloor);
    c.dialogueAction = 1214;
    break;
case 1215:
    setLevelOptions(9, 10, 11, -1, "Choose Floor", c.achievedFloor);
    c.dialogueAction = 1215;
    break;
share|improve this answer
1  
I think there is an error or maybe I am not understanding clearly. But, in levelAchievedOption(levela >= 0 ? levelAchievedOption(levela, levelAchieved) : "") levelAchievedOption takes one int and another String parameter. So howcome outer levelAchievedOption will be invoked with only one String? –  tintinmj Mar 8 at 14:01

There's a good talk about it by a Google's employee (https://www.youtube.com/watch?v=4F72VULWFvc). In fact these kind of conditions can be avoided using inheritance. You could create 3 different implementations of an abstract class which all have a method sendOption5() and you could choose one of it using a case. A better way to do it, but it depends on your code is to create an abstract class (or interface) like

interface Option
{
  public void sendOptions();
  public boolean haveToBeCalled(Integer number);
}

where haveToBeCalled returns true if number if a certain value, false otherwise. And so if you have a list of "Option" you can use it as

for (Option o : options)
{
  if (o.haveToBeCalled(number))
  {
    o.sendOptions();
  }
}

Anyway, it really depends on the rest of your code. The kind of scheme you're can often be improved by refactoring a part of the code.

share|improve this answer
    
I know this, but I was more talking about lines like these: "Level 6 " + (c.achievedFloor <= 6 ? "@gre@(Achieved)" : "@red@(Not achieved)") + "", –  user3395769 Mar 8 at 10:31
1  
why don't you create a method that generate your string depending on "c"? –  Thüzhen Mar 8 at 10:35

My best guess will be create a method to build your Strings and make sendOption5 a method that takes List of String. You can also use var-args.

List<String> levelDescriptions;
Case 1213:
    levelDescriptions = generateStringUponLevelNumber(c,1,2,3,4);
    levelDescriptions.add("More");
    sendOption5(levelDescriptions);

    c.dialogueAction = caseNumber;
// same goes for others

------

public static List<String> generateStringUponLevelNumber(TypeOf c, int... levels) {
    List<String> levelDescription = new ArrayList<>();
    for(int level : levels) {
        String description = "Level " + level + " ";
        description += c.achievedFloor <= level ? "@green@(Achieved)" : "@red@(Not achieved)";
        levelDescription.add(description);
    }
    return levelDescription;
}

After OP's comment

Change your method accordingly

public void sendOption5(List<String> levelDescriptions) {
    c.getPA().sendFrame126("Select an Option", 2493);
    for(int level = 0, from = 2494; level <= 4; level++, from++) {
        c.getPA().sendFrame126(levelDescriptions.get(level), from);
    }
    c.getPA().sendFrame164(2492);
}

Some suggestion upon naming

  1. The method name sendOption5 is misleading. Cause if someone is seeing the code and see the name he/she will think there are N options and you are sending the 5th option as parameter, which is not true. You are sending 5 Strings or options. So the apt. name would be sendFiveOptions.
share|improve this answer
    
This was pretty helpful but my method wont work anymore which is: 'code'( public void sendOption5(String s, String s1, String s2, String s3, String s4) { c.getPA().sendFrame126("Select an Option", 2493); c.getPA().sendFrame126(s, 2494); c.getPA().sendFrame126(s1, 2495); c.getPA().sendFrame126(s2, 2496); c.getPA().sendFrame126(s3, 2497); c.getPA().sendFrame126(s4, 2498); c.getPA().sendFrame164(2492); }) –  user3395769 Mar 8 at 11:00
1  
@user3395769 see the changes. Though I highly recommend you to further refactoring. –  tintinmj Mar 8 at 11:12

This seems to me like it's probably easiest to pre-build all the strings, then select four to use. First, I think you probably want to re-check your logic on one minor point. Did you really mean <= when you did the comparisons to c.achievedFloor? Typically you'd expect floors to be achieved in order, so if (for example) the player has achieved floor 7, that means they've also achieved floors 1 through 6. The way you've written seems to imply the reverse--that achieving (for example) level 4 means they've also achieved levels 5 though 10.

Anyway, for the moment, let's assume the logic is correct and roll with it.

In that case, I'd start by building a table of strings:

int i;
floors[12] = "";
for (i=11; i>=c.achievedFloor; i--)
   floors[i] = ""@gre@(Achieved)";
for ( ; i>0; i--)
   floors[i] = "@red@(Not achieved)";

Since you haven't shown its name, I'm going to assume the value you're using in the case statements is named input.

The values you're using (1214, 1215, ...) should probably be given some sort of meaningful name. Right now, they're pretty much the essence of magic numbers. For the moment, I'm going to assign the name first to 1213 and last to 1215.

Using that, we'd have something like:

int base = (input - first) / 4;
SendOption5(floors[base], floors[base+1], floors[base+2], floors[base+3], 
    input == last ? "" : "More");
c.DialogAction = input;
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.