Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

How can I make this code shorter? The code I am using is so long. Just making one simple level is taking about 100 lines of code.

public class Sister_age_guesser {

    public static void main(String[] args) {


         Scanner input = new Scanner(System.in);

         System.out.println("Sherlock holmes - The game");

          System.out.println(" This is a mystery game where you have to find clues after solving math sums");

          System.out.println("Write your name ");
              String name = input.nextLine(); 
             System.out.println("------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------");

           System.out.println("Instructions");

            System.out.println("1.This game has 1 level ");

            System.out.println("2.Clues will be given before you write down the answer.Writing the correct answer will unlock the next clue ");

            System.out.println("------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------   ");

           System.out.println("  FLORENTINES MURDER "); 
          System.out.println("   ");

           int SisterBirthday = 1997;

           System.out.println("FIND THE MURDER WEAPON");
           System.out.println(" ");
           System.out.println("Clues :-");
           System.out.println("  ");
           System.out.println(" 1. This number is 1000 added to itself. 2. cut 6 into 2 pieces and take half of it and subtract it from the result from the previous clue ");


           int number = input.nextInt();
            if(number < SisterBirthday)

            {  
                 System.out.println(name + " , you need more practice" + name);
                 System.exit(0); 
            } 
            else if(number > SisterBirthday)
            {
                System.out.println(" cant u solve this simple clue " + name + " ?");
               System.exit(0);  
            }
            else

                 System.out.println("Good thing you found out the murder weapon.We found some fingerprints on the weapon . Keep it up " + name + " !");
           System.out.println("------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ ");    
             System.out.println("TRACE THE CRIMINAL WITH THE FINGERPRINTS");
             System.out.println("Clues:-");
             System.out.println("This added with itself gives nothing.Neither a positive nor a negative");
            int clue2 = 0;

            int number2 =  input.nextInt();
              if(number2 < clue2)

              { 
                  System.out.println("You need to get your eyes checked !!" + name   +" Can't you just match the fingerprint found on the weapon with the fingerprints we have got.We have got many other capabale officers who can do this job" + name +  "!!!!" );
                  System.exit(0);
              }
              else if (number2 > clue2)

              { 
                  System.out.println("wonder why the case was assigned to you when you are not even capable of tracing a fingerprint" + name);
                  System.exit(0);

              }
              else

                  System.out.println(" Ah ! very Good !" + name + " .This is the type of work I expect from  you!" );
              System.out.println("-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------");

              System.out.println("CAPTURE THE CRIMINAL");

              System.out.println("Clues:-");
              System.out.println("Subtract 1999 from 2000 and you get your answer");
              int clue3 = 1;

             int number3 =  input.nextInt();
              if(number3 < clue3)
              {
                  System.out.println("Oh no"+  name +  "!! The criminal escaped !!! ");
                System.exit(0);  
              }
              else if (number3 > clue3)
              { 
                  System.out.println("Oh no"+  name +  "!! The criminal escaped !!! ");
                 System.exit(0); 
              }
              else

                  System.out.println(" Oh yes ! We managed to catch the criminal and all the credit goes to you " + name + " !");

                  System.out.println(" Level 2");







    }

}
share|improve this question
7  
I do understand that you are trying to code & that the purpose of this site is to review it,. However, coding a text-adventure is such massive undertaking that there are (mature, well supported) programming languages devoted to it (TADS, Inform, et al). See brasslantern.org and en.wikipedia.org/wiki/Interactive_fiction – Mawg Jan 6 at 13:39
1  
I am not even a beginner with Java, but instead of recursion (which could cause a [StackOverflow](stackoverflow.com)), I would use a system where each question returns true or false, and you run the code if (question1() && question2() && etc...) you_completed_ the_game() else you_lost_the_game. This works because Java will run the first question and, if it returns true, will try the next one. If it returns false... false && anything is false, so it doesn't run the next thing (what's the point?). Using this prevents a Stack Overflow. Please, somebody, fix this and make it an answer. – wizzwizz4 Jan 6 at 19:51
2  
if (question1() && question2() ...) is not recursion. It is short-circuit evaluation of conditionals. – Jonathon Reinhart Jan 7 at 15:42
    
Using less verbose languages will make your program shorter, at least in line count. ;) – ThorSummoner Jan 11 at 20:21

Separate your data from your code

What you are looking for is called a Game Engine. The idea is to abstract the code away from the game specifics.

So you could write your level is almost plain English (with some keywords thrown in).

As a very basic example, you might have a level file like

Sherlock Holmes - The game
This is a mystery game where you have to find clues after solving math sum 
Write your name
<input:name>
FIND THE MURDER WEAPON
Clues :- 1. This number is 1000 added to itself. 2. cut 6 into 2 pieces and take half of it and subtract it from the result from the previous clue
<input="1997":win>

In this example, the <> serves to designate a keyword text. In this style of level data all you need to do it create a program that reads in the file and prints it out to the console. Stop printing when a keyword is detected, and
in the case of <input:name> store the next value entered under the key "name"[1].
And in the case of <input="1997":win>, compare the next input to "1997", and if equal skip to line "win", or file "win". However you want to set it up.

You can get more technical with xml, or use any number of different file or data formats to store your level data. The idea is just to separate the code from the data, which will greatly reduce your number of lines and the time it takes to write.

[1]: These are called associative arrays, or Maps in Java I believe.

share|improve this answer
3  
While is it perfectly fine to create your own format, most times it is a better idea to use a standard (CSV, JSON, XML,...). Mainly because they take care of special characters, encoding etc, and also because they then to be key-value in nature, making changes in the format easier – RobAu Jan 6 at 14:54
4  
Another reason to separate code/data is that you can have a data.en file as you show, then data.es in Spanish, data.fr in French... the translators who help you out don't need to see your code. – Monty Harder Jan 6 at 18:35
    
I recommend using YAML for the data files. – Zenadix Jan 7 at 15:40
1  
Well I am not going to argue with everyone. I personally feel that a custom, lets say simplified (non-nesting) HTML like format, as shown, is by far the easiest to use for the project as described. And that any other data format suggested would needlessly complicate the programming and writing for no benefit. And while normally you would want to abstract away the data even farther and use an existing library to deal with it that is both unnecessary, inefficient, more complicated, and probably not a good idea from a learning perspective. – Jonathon Wisnoski Jan 7 at 21:18
    
I recommend Not using yaml for data files, or any files. Check out Toml for a yaml-but-sane format, or something else well encapsulated – ThorSummoner Jan 11 at 20:19

I'm pretty new at Java myself, and actually made something pretty similar to this recently, hopefully I can teach you some hints which I have learned myself.


Formatting

The way your code is formatted makes it difficult to read. To make it more compact, avoid adding too many blank lines between statements. Also, make sure you have consistent indentation to the left, it seems like a lot of your statements are +/- 2 spaces from where they should be, at random.


Formatted print statements

This will reduce the amount of typing commands to System.out for every line. Java has a handy System.out.format() (or System.out.printf if you prefer) method you can use to make nicely formatted, multi-line output and include formatted variables without having to break your strings to + var + your values. And for new lines, just add %n into the string.

    System.out.format(
        "Sherlock holmes - The game%n" +
        "This is a mystery game where you have to find clues after solving math sums%n" +
        "Write your name: "
    );

Organize into methods

First, you should try to keep your main method to be the entry point of your program only, and move the rest of the code outside of it. Many times in larger programs, Main will be a class of its own in a separate file, so it's best to get into good habits early.

As you start breaking down the code into smaller, more focused chunks, it will become much easier to create more content without the code becoming a huge mass of code.

I added a questionManager() method using do while so that it executes each question at least once, and will repeat the question until the answer is correct, then move on to the next one. Each question method can return a boolean value to indicate whether the answer was correct.

To get you started... Partial demo on repl.it

Just keep applying this kind of technique to organize better:

PS: I added a few more hints for you into the code comments, which should be removed from your improved code.

class Main {
    public static void main(String[] args) {
        SisterAgeGuesser game = new SisterAgeGuesser();
        game.start();
    }
}

class SisterAgeGuesser {
    Scanner input = new Scanner(System.in);
    String name = "";

    void start() {
        System.out.format(
            "Sherlock holmes - The game%n" + //etc.
            "This is a mystery game where you have to find clues after solving math sums%n"
        );
        askForName();
        displayInstructions();
        questionManager();
    }

    void askForName() {
        System.out.format("Write your name: ");
        name = input.nextLine();
    }

    void displayInstructions() {
        System.out.format(
              "-- Instructions --%n" +
              "Clues will be given before you write down the answer. Writing the correct answer will unlock the next clue"
        );
    }

    void questionManager() {
        do {
            level1Question1();
        } while (!level1Question1());
        // Add more do-while statements when you add more question methods
    }

    private boolean level1Question1() {
        int SisterBirthday = 1997;

        System.out.format(
            "  FLORENTINES MURDER  %n%n" +
            "FIND THE MURDER WEAPON%n%n" +
            "Clues :%n%n" +
            "1. This number is 1000 added to itself. 2. cut 6 into 2 pieces and take half of it and subtract it from the result from the previous clue."
        );
        int number = input.nextInt();
        if(number < SisterBirthday) {
            //Here, %s holds the place of your String variable `name`.
            System.out.format("You need mor199e practice, %s. %nTry again!%n%n", name);
            return false;
        } 
        else if (number > SisterBirthday) {
            System.out.format("Can't you solve this simple clue, %s? %nTry again!%n%n", name);
            return false;
        }
        else {
            //Here, %d (for decimal) holds the place of your int variable
            System.out.format(
                "Correct!, the answer is %d!%n" +
                "Good thing you found out the murder weapon. We found some fingerprints on the weapon." +
                "Keep it up %s!%n%n",
                SisterBirthday, name
            );
            return true;
        }
    }
share|improve this answer
9  
Recursion is useful when the solution to a problem depends on solutions to smaller instances of the same problem. This isn't the case here and if this solution is scaled to many questions or someone is really bad at guessing this will cause a StackOverflowError. – Johnbot Jan 6 at 10:13
1  
nothing justifies recursion here, nor to call level1Question2 from level1Question1 – njzk2 Jan 6 at 22:20
1  
@Phrancis A better way would be to make question1 return false if the player got the question wrong and return true if they got it right so that the other code can decide whether to move on to the next question. question1 should have no knowledge that question2 even exists. As far as it's concerned it should think itself to be the only question and that it has the job of asking a question, getting input, verifying that, and reporting back if the answer was correct or not. The other code (effectively question1's manager) then decides whether question1 should ask again or not. – Pharap Jan 7 at 1:16
1  
@Phrancis Quick caveat to my suggestion, you may still want to give the question the responsibility of telling the player if they were wrong or not if you want each question to have a customised response. But whatever you do the important bit is that the questions don't decide what happens next, their manager/controller does. – Pharap Jan 7 at 2:44
1  
@AlexisKing I don't think anyone is debating whether or not recursion works or the mechanics of it, rather that there's no reason to use it here. It's like trying to use a teaspoon to eat a large tub of icecream. It works, but it's not designed for that job. – Dan Pantry Jan 7 at 9:31

First of all, I'd say that lines of code (LOC) isn't a useful metric in most cases - some things just take more code, and having it on multiple lines isn't a bad thing. That being said, here are some general tips.

The name Sister_age_guesser isn't a normal class name in Java - they are typically named like SisterAgeGuesser (called either PascalCase or TitleCase).

Next, you have more newlines than you need - in general you should very rarely need more than one blank line between any given line of code.

While on the topic of whitespace, you have very inconsistent indentation - this makes the code much harder to read.

Much of your code is taken up printing stuff - make them variables (well, constants) and you can print it out much easier.

You have a ton of repeated code - the whole guessing process is identical every time, so you should put that into a function.

The answers are constants, so they should be marked as such.

You shouldn't be using System.Exit (thank you Simon Forsberg McFeely for pointing this out in the comments) - this generally indicates that either your program has reached its conclusion (it hasn't, there is probably cleanup or something) or an unrecoverable error has occurred (and this isn't unrecoverable - you could pretty easily give them an option to restart or try that question again). Instead you should return from that function with some informative return value (this is either a success or failure, so boolean should be fine, but it could also be an Enum or something if you have more cases) and then return from main if that return value indicated game over.

Lastly, remember to close your Scanner.

Here is a fully rewritten version of your code (I've left out the declaration of string constants, because that is straightforward).

public class SisterAgeGuesser {

    private static final int sistersBirthday = 1997;
    private static final int secondClue = 0;
    private static final int thirdClue = 1;

    private static boolean guessNumber(Scanner s, int correct, String successMessage, String failureMessage) {
        boolean guessWasCorrect = s.nextInt() == correct;
        System.out.println(guessWasCorrect ? successMessage : failureMessage);
        return guessWasCorrect;
    }

    public static void main(String[] args) {

        Scanner input = new Scanner(System.in);

        System.out.println(intro);
        String name = input.nextLine();

        System.out.println(instructions);

        System.out.println(findMurderWeapon);
        if (!guessNumber(input, sistersBirthday, foundMurderWeapon, cantDoAnythingRight))
        {
            System.out.println(youLoseMessage);
            return;
        }

        System.out.println(traceFingerprints);
        if (!guessNumber(input, secondClue, goodWork, getEyesChecked))
        {
            System.out.println(youLoseMessage);
            return;
        }

        System.out.println(catchCriminal);
        if (!guessNumber(input, thirdClue, creditToYou, criminalEscaped))
        {
            System.out.println(youLoseMessage);
            input.close();
            return;
        }

        input.close();
    }
}

You could make this even simpler, however. Because this is the same thing over and over again, put the values in an iterable and loop

public class SisterAgeGuesser {

    private static List<String> prompts = new ArrayList<String>();
    private static List<int> answers = new ArrayList<String>();
    private static List<String> goodAnswers = new ArrayList<String>();
    private static List<String> badAnswers = new ArrayList<String>();

    static {
      // Add the prompts, answers, and responses here
    }

    private static boolean guessNumber(Scanner s, int correct, String successMessage, String failureMessage) {
        boolean guessWasCorrect = s.nextInt() == correct;
        System.out.println(guessWasCorrect ? successMessage : failureMessage);
        return guessWasCorrect;
    }

    public static void main(String[] args) {
        Scanner input = new Scanner(System.in);

        System.out.println(intro);
        String name = input.nextLine();    
        System.out.println(instructions);

        for (int i = 0; i < prompts.size(); ++i) {
          System.out.println(prompts[i]);
          if (!guessNumber(input, answers[i], goodAnswers[i], badAnswers[i]))
          {
              System.out.println(youLoseMessage);
              input.close();
              return;
          }
        }

        input.close();
    }
}

Again, I've left out the mundane details of filling up your ArrayLists.

share|improve this answer
7  
I would argue that System.exit shouldn't be called at all, I'd use return; instead. And "Game over" is not the same as "Program failure", and as such the exit code should definitely be zero. – Simon Forsberg Jan 5 at 23:55

1. Coding style

To give you a short pointer to a guide considered good coding style, please refer to original Java Code Conventions document.

2. Formatting

Use a decent IDE (like NetBeans, Eclipse or IntelliJ) and use automatic code formatting. This will take care of some rules from above document too, regarding whitespace in source code.

Alternatively, learn to use Checkstyle and/or share its settings.

3. Clean code

Likewise the @Phrancis wrote in his example refactored code, try to write short and well-named methods, that split program logic into parts that are easy to understand.

In my opinion the best source of knowledge about this matter can be found in respected book, "Clean code" by Robert C. Martin.

share|improve this answer
2  
@SirPython this answer is too general and does not give actual precise improvements. – Caridorc Jan 6 at 0:42
2  
@Caridorc Right, I'm sorry. I should've read the post more carefully. – SirPython Jan 6 at 0:42

You could consider moving the generic text which precedes the body of your level to an external text document - that way you can output this rather than doing so line by line.

Outputting text from a file can be achieved using something similar to this:

PrintStream out = new PrintStream(new FileOutputStream("output.txt"));
System.setOut(out);
share|improve this answer

You have 32 print statements. You could combine multiple print statements into one line by using print("line1\nline2\n") instead of println("line1"); println("line2"); but this makes it harder to see what the output looks like while looking at the code. Some people here suggested the even more extreme idea of moving most of text into a separate file. This is a good idea when your game has many levels, but I think it's too soon for a game this small, because as your game evolves you may need to generalize the file format to handle everything you want. By the time you have finished the first few levels you should look into that idea. So let's not do those things and see what else we can do. The thing to remember is we're not going to be able to get below 32 lines here, because we need that many just to list all the strings you need.

As one or two others have already mentioned: many of these lines are blank. Some others are consist of nothing but opening and closing braces. As an extreme example (some might say I'm going too far), we can rewrite your first if-else chain like this:

int number = input.nextInt();
if(number < SisterBirthday) {
    System.out.println(name + " , you need more practice" + name);
    System.exit(0); 
}else if(number > SisterBirthday){
    System.out.println(" cant u solve this simple clue " + name + " ?");
    System.exit(0);
}else System.out.println("Good thing you found out the murder weapon.  We ...);

After going through this compaction process, your code is 57 lines. To bring it down further, let's look at where you are spending your lines. We already discussed the print statements. But even among your print statements, one thing jumps out: you like to print dashes. It makes sense to spend a line to specify how many dashes you want, so if you ever want to change it, you can do it in one place instead of adjusting throughout the whole file. (In fact I already suggest you change it: it's way too many to fit on most people's screen.) This will make your code one line longer, but the size in bytes will go down. If you ask me, this is the important number, since it's easy to have a one line program just by deleting all the newlines, but that line will be thousands of characters long, which is crazy.

String scene_break = // I think this many dashes is enough :)
    "------------------------------------------------------";
System.out.println(scene_break);

Ok, that was minor, we're at 58 lines now. What can we do that really helps? We must look for patterns in the code. If you're doing something very similar many times, see if you can write a function for it (or method in Java-speak), and then just call the method. As some one else already said: your guessing code is the same each time. Let's write a method to handle it. How? First, we write an empty method, then we copy/paste one instance of the pattern we want to generalize, and then we replace the constants with variables. Let's do it painfully slowly:

static void guess(){}

Now let's copy/paste:

static void guess(){
    System.out.println(" 1. This number is 1000 added to itself. 2. cut...");
    int number = input.nextInt();
    if(number < SisterBirthday) {
        System.out.println(name + " , you need more practice" + name);
        System.exit(0);
    }else if(number > SisterBirthday){
        System.out.println(" cant u solve this simple clue ...");
        System.exit(0);
    } else System.out.println("Good thing you found out the murder ...");
}

And now we generalize by introducing variables for the things that change each time:

static void guess(String clue, int answer, String lo, String hi, String good){
    System.out.println(clue);
    int number = input.nextInt();
    if(number < answer) {
        System.out.println(lo);
        System.exit(0);
    }else if(number > answer){
        System.out.println(hi);
        System.exit(0);
    } else System.out.println(good);
}

Note that we could have made this method do slightly more, like print "Clues :-". The reason I didn't is that I want my code to have the same output as yours, and you never print "clues" the same way twice. Sometimes you have a blank line after, sometimes you have a space before the ":-", and sometimes not. If these differences are on purpose, then keep things as they are. If they are by accident, then bring that code into the method as well to prevent such accidents in the future. Plus, your code shrinks even more :) And here is the final code.

public class Sister_age_guesser {
    public static void main(String[] args) {
        java.util.Scanner input = new java.util.Scanner(System.in);
        System.out.println("Sherlock holmes - The game");
        System.out.println(" This is a mystery game where you have...");
        System.out.println("Write your name ");
        String name = input.nextLine(); 
        String scene_break =
            "------------------------------------------------------";
        System.out.println(scene_break);
        System.out.println("Instructions");
        System.out.println("1.This game has 1 level ");
        System.out.println("2.Clues will be given before you write...");
        System.out.println(scene_break);
        System.out.println("  FLORENTINES MURDER "); 
        System.out.println();
        System.out.println("FIND THE MURDER WEAPON");
        System.out.println();
        System.out.println("Clues :-");
        System.out.println();
        guess(
            " 1. This number is 1000 added to itself. 2. cut 6...",
            1997, // sister birthday
            name + " , you need more practice" + name,
            " cant u solve this simple clue " + name + " ?",
            "Good thing you found out the murder weapon.  We..."
        );
        System.out.println(scene_break);
        System.out.println("TRACE THE CRIMINAL WITH THE FINGERPRINTS");
        System.out.println("Clues:-");
        System.out.println();
        guess(
            "This added with itself gives nothing.   Neither a...",
            0,
            "You need to get your eyes checked !!" + name   +"...",
            "wonder why the case was assigned to you when you...",
            " Ah ! very Good !" + name + " .This is the type of..."
        );
        System.out.println(scene_break);
        System.out.println("CAPTURE THE CRIMINAL");
        System.out.println("Clues:-");
        guess(
            "Subtract 1999 from 2000 and you get your answer",
            1,
            "Oh no"+  name +  "!! The criminal escaped !!! ",
            "Oh no"+  name +  "!! The criminal escaped !!! ",
            " Oh yes ! We managed to catch the criminal and all..."
        );
        System.out.println(" Level 2");
    }
    static void guess(String klu, int ans, String lo, String hi, String yes){
        System.out.println(klu);
        int number = input.nextInt();
        if(number < ans) {
            System.out.println(lo);
            System.exit(0);
        }else if(number > ans){
            System.out.println(hi);
            System.exit(0);
        } else System.out.println(yes);
    }
}

Creating the guess method was an investment in line count, but eventually it will pay off. As it is, we're up around 61 or 62 lines now, so it didn't pay off yet, but at least it didn't cost us much. Our main method is less than 50 lines long now, which is pretty close to the lower bound of 32 we discussed earlier. Hope this helps. (Leave a comment if not. Or if it does. I'd like to know.) Cheers and good luck with the game!

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.