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.

Below is some code I have written today to test out some bits I have learnt. It isn't much nor is it spectacular. Please critique and let me know what I could/should have done differently or anything I could do to improve it so far.

package easy8;

import java.util.Scanner;

public class song99bottles {

public static void main(String[] args) {

// Declare a reference variable of type song99bottles - new object  
song99bottles go = new song99bottles();
// calls the method "queston" for object "go".
go.question();

}

public void question(){

    song99bottles start = new song99bottles();

    String answer;

    System.out.println("Would you like to hear a nursery rhyme?\nPlease enter yes or no:");
    Scanner input = new Scanner(System.in);
    answer = input.next();
    if(answer.equalsIgnoreCase("yes")){
        start.lyrics();
    } else{
        System.out.println("Bye");
    }

}



public void lyrics() {

    int peeps  = 10;
    String intro = "There were ";
    String intro2 = " in the bed and the little one said rollover";
    String fall = "\nSo they all rolled over and 1 fell out.";
    String end = "And the little said 'I'm lonely'.";

    while (peeps > 0){
        System.out.println( intro + peeps + intro2 + fall);
        peeps--;
        if (peeps == 1){
            System.out.println(end);
            return;
                        }
    }


}

}
share|improve this question
    
Missing "one". "And the little one said 'I'm lonely'." –  user1008646 Mar 26 at 9:31
1  
According to this video the last line is "There was one in the bed and the little one said 'I'm lonely'" youtube.com/v/TdDypyS_5zE%26start=130%26end=138%26version=3 –  user1008646 Mar 26 at 9:41

5 Answers 5

  1. Class names should be PascalCase (song99bottles -> Song99Bottles)
  2. You should indent your code or you will end up with a long code and you don't understand the various { } pairs (and an horrible code!)

    public class song99bottles {
    public static void main(String[] args) {
    // Declare a reference variable of type song99bottles - new object  
    song99bottles go = new song99bottles();
    // calls the method "queston" for object "go".
    go.question();   
    }
    // [...]
    

    if you want to follow Code conventions, you code should look like this

    public class Song99Bottles {
    
        public static void main(String[] args) {
            // Declare a reference variable of type song99bottles - new object
            Song99Bottles go = new Song99Bottles();
            // calls the method "queston" for object "go".
            go.question();
        }
    
        // [...]
    

    Personally i follow the new-line style, but you will see with time you will get "your style".

    i hope you understand the concept. Read wikipedia page.

  3. You should give to your variables a more descriptive name, you should understand the scope of the variable by just reading the name. (song99bottles go = new song99bottles(); go? What is? a countdown? I could call it song (example))


About your code:

  1. You create two song99bottles objects, why? One inside main and one inside question(). You should move question code inside main because song99bottles class should just play the song, the main will take care if the user want or not to play the song. Then in your main, just call go.lyrics();

  2. About lyrics() code, see Roger answer.

share|improve this answer
    
Java Code Style Conventions require that the opening brace for a compound statement goes at the end of the line, not on a new line. Your should be example code is wrong.... –  rolfl Mar 25 at 21:54
1  
I will edit with that. But i should admit i always follow the "new line" style, i thought the code more clear. –  Marco Acierno Mar 25 at 21:57
    
I understand, and I have worked with both. Two comments though... if you are producing 'example code' it should follow conventions.... and, with the { on the next line it looks like C#. Also, by the way, empty-line before method declarations ;-). Your answer is good, it already has my +1. –  rolfl Mar 25 at 22:02
    
About empty-line, yes.. i think i deleted the line by error... Fixed thanks! –  Marco Acierno Mar 25 at 22:08
    
Might as well update my answer too... –  ArthurChamz Mar 25 at 22:08

Taking Roger's answer as a starting point, you can also only use one string in the lyrics() function, because you always use intro2 before fall. If you use one string, you save an allocation and concatenation.

String intro = "There were ";
String fall  = " in the bed and the little one said rollover"
               + System.lineSeparator()
               + "So they all rolled over and 1 fell out.";
String end   = "And the little said 'I'm lonely'.";

for(int peeps = 10; peeps > 0; peeps--){
    System.out.println(intro + peeps + fall);
}
System.out.println(end);
share|improve this answer
    
... I don't even know what to say... Thanks. –  ArthurChamz Mar 25 at 21:55
    
You might want to get the line break sequence from the System object instead of hard coding a newline in the middle of the 'fall' string. –  atk Mar 26 at 1:56
    
@atk Like this? It's kinda verbose, but it might save a cross platform headache or two. –  ArthurChamz Mar 26 at 2:46
    
Yep :-) ........ –  atk Mar 26 at 11:51

There are a number of improvements you can make, but removing the declaration for peeps in lyrics() and changing the while loop to a for loop would be a good start:

for(int peeps=10;peeps>0;peeps--) {
    System.out.println( intro + peeps + intro2 + fall);            
}
System.out.println(end);
share|improve this answer

Suggestion to code

  1. Do not create Song99bottles twice, from question() you can call lyrics()
  2. Do not + strings, it creates an unbelievable overhead of strings in the stack (memory). Use a string formatter String.format("format %d", integer)
  3. Do not use comments unless the really matter. Comments are a sign of bad names and too few funtions, see this http://ptgmedia.pearsoncmg.com/images/9780132350884/samplepages/0132350882_Sample.pdf
share|improve this answer

In addition to the other good advice I see here, I would rename your lyrics method to imply exactly what it does. For example printLyrics().

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.