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.

This is a long one, I'll edit as instructed.

I humbly submit the following for critique. This was my final project for an Intro To Java Programming class ("handed-in" already, I can't make changes).

The abundant commenting is required by the professor.

The basic instructions:

Design and implement a stringed musical instrument class using the following guidelines:

a. Data fields for your instrument should include number of strings, an array of string names representing string names (e.g. E,A,D,G), and boolean fields to determine if the instrument is tuned, and if the instrument is currently playing. You are welcome to add additional data fields if you like.

b. A constructor method that set the tuned and currently playing fields to false.

c. Other methods 1) to tune the instrument, 2) to start the instrument playing, and 3) to stop the instrument from playing.

d. Other methods as you see fit (Add at least one unique method).

Write the output from your Instrument class methods to a text file that a user entered from the command line arguments (e.g. java Mynamep3tst myfilename.txt). This allows your program to accept filenames from the user via a command line argument.

The Guitar class:

//data fields include all required by assignment plus numberOfGuitars and numberOfTwelveStringGuitars
private int numberOfStrings = 6;
private static int numberOfGuitars = 0, numberOfTwelveStringGuitars = 0;
StringPackage stringPackage = new StringPackage("guitar", 6);
private boolean tuned = false;
private boolean playing = false;

//simple constructor, built with tuned and playing false
public Guitar(){    
    numberOfGuitars++;
}

//constructor for different number of strings
//same basic build re: tuning and playing
public Guitar(int numberOfStrings) throws IOException{
        this.numberOfStrings = numberOfStrings;
        stringPackage = new StringPackage("guitar", numberOfStrings);
        if(numberOfStrings == 12) numberOfTwelveStringGuitars++;
}

//displays string notes using methods in StringPackage class
public void viewStringPackage(Guitar thisGuitar) throws IOException {
    String notes = thisGuitar.stringPackage.Notes();
    StringPackage.comboPrint("The strings on this guitar are: " + notes + "\n");
    }

//method to find out how many strings a particular guitar has
public int getNumberOfStrings() {
    return numberOfStrings;
}

//this is my unique method per assignment instructions
public static int getNumberOfGuitars(){
    return numberOfGuitars;
}

//this too
public static int getNumberOfTwelveStringGuitars() {
    return numberOfTwelveStringGuitars;
}

//method to determine if Guitar is tuned
public boolean isTuned() {
    return tuned;
}

//method to tune Guitar
public void tune() throws IOException {
    StringPackage.comboPrint("The Guitar is tuned." + "\n");
    tuned = true;
}

//method to see if Guitar is playing
public boolean isPlaying() {
    return playing;
}

//method to start Guitar playing
public void play() throws IOException {
    StringPackage.comboPrint("The Guitar is now playing." + "\n");
    playing = true;
}

//method to stop Guitar playing
public void stopPlay() throws IOException{
    StringPackage.comboPrint("The Guitar is no longer playing." + "\n");
    playing = false;
}

}//class

The StringPackage class:

public class StringPackage {

//data fields include a String[] to hold string names as per the assignment
private static final String GUITAR = "guitar", VIOLIN = "violin";
String[] stringArray;

//builds the StringPackage for each instrument
//can be amended to include new instruments as they're built
//don't know how to throw exceptions yet so this will have to do as a substitute
public StringPackage(String typeOfInstrument, int numberOfStrings) {
    if(numberOfStrings == 6 && typeOfInstrument.equalsIgnoreCase(GUITAR)){
        String temp = "E A D G B E";
        stringArray = temp.split(" ");
    }//6 string
    else if(numberOfStrings == 12 && typeOfInstrument.equalsIgnoreCase(GUITAR)){
        String temp ="E E A A D D G G B B E E";
        stringArray = temp.split(" ");
    }//12 string
    else if (numberOfStrings == 4 && typeOfInstrument.equalsIgnoreCase(VIOLIN)){
        String temp = "G D A E";
        stringArray = temp.split(" ");
    }//violin
    else
        System.out.println("***Exception! " + numberOfStrings + " string "+ typeOfInstrument + " has not been created yet!***");

}//StringPackage

//.length method to for use in loops, etc.
public int length(){
    int length = stringArray.length;
    return length;
}

//builds string for notes display
//used by viewStringPackage in Guitar and Violin
public String Notes(){
    StringBuilder notes = new StringBuilder();
    for(int i = 0; i < stringArray.length; i++){
        notes.append(stringArray[i] + " ");
    }
    String notes1 = notes.toString();
    return notes1;
}

public static void comboPrint(String myString) throws IOException {
    boolean append = true;
  FileWriter myOutFile = new FileWriter("GuitarViolin.txt", append);  
  PrintWriter fileOut = new PrintWriter(myOutFile); 
  System.out.println(myString);
  fileOut.println(myString);
  fileOut.close();
}

}//StringPackage

The Test:

public class test {


public static void main(String[] args) throws IOException {

    //allows user to enter file to write from command line
    //satifies section 4 requirement
    if(args.length > 0)
        section4(args);         

    //create arrays to hold new instruments
    Guitar[] sixStrings = new Guitar[4];
    Guitar[] twelveStrings = new Guitar[4];
    Violin[] violins = new Violin[4];

    //create 4 new 6-String Guitars, 12-String Guitars and Violins
    //for a total of 12 new instruments (assignment called for 10)
    for (int i = 0; i < 4; i++){
        sixStrings[i] = new Guitar();   
        twelveStrings[i] = new Guitar(12);
        violins[i] = new Violin();
    }

    howMany(); //displays number of instruments created     

    //Display status of the instruments
    statusAll(sixStrings, twelveStrings, violins);

    //Tune the instruments and start them playing
    for(int i = 0; i < violins.length; i++){
        sixStrings[i].tune();
        sixStrings[i].play();
        twelveStrings[i].tune();
        twelveStrings[i].play();
        violins[i].tune();
        violins[i].play();
    }//tune and start play

    statusAll(sixStrings, twelveStrings, violins);

    //stop instrument from playing
    for(int i = 0; i < violins.length; i++){
        sixStrings[i].stopPlay();
        twelveStrings[i].stopPlay();
        violins[i].stopPlay();
    }//stop playing

    statusAll(sixStrings, twelveStrings, violins);      

    //created this 3 string guitar to test what happens with my "exception" that isn't an exception
    System.out.println("......................");
    Guitar weirdo = new Guitar(3);
    weirdo.isPlaying();
    weirdo.isTuned();

    //To show that weirdo doesn't increase numberOfTwelveStringGuitars
    howMany(); 

}//main

//getNumberOf... is my unique method
//it displays how many instruments of each type were created
public static void howMany() throws IOException{
    StringPackage.comboPrint("Number of 6-String Guitars is: " + Guitar.getNumberOfGuitars() + 
            "\nNumber of 12-String Guitars is: " + Guitar.getNumberOfTwelveStringGuitars() +
            "\nNumber of Violins is: " + Violin.getNumberOfViolins() + "\n");
}//howMany

//displays all status of ALL instruments
//I created this to avoid repeating code in the main
public static void statusAll(Guitar[] sixStrings, Guitar[] twelveStrings, Violin[] violins) throws IOException{
    instrumentStatus(sixStrings);
    instrumentStatus(twelveStrings);
    instrumentStatus(violins);
}//statusAll

//displays current status of all guitars
//uses getNumberofStrings method to determine type of guitar
public static void instrumentStatus(Guitar[] guitars) throws IOException{
    for (int i = 0; i < guitars.length; i++){
        if (guitars[i].getNumberOfStrings() == 12)
            StringPackage.comboPrint("12-String Guitar " + (i +1) + ": ");
        else 
            StringPackage.comboPrint("Six String Guitar " + (i + 1) + ":");
        if (guitars[i].isPlaying()) 
            StringPackage.comboPrint("Is currently playing.");
        else
            StringPackage.comboPrint("Is currently not playing.");
        if (guitars[i].isTuned())
            StringPackage.comboPrint("Is currently tuned.");
        else
            StringPackage.comboPrint("Is currently not tuned.");
        guitars[i].viewStringPackage(guitars[i]);
    }//for
}//instrumentStatus(Guitar[])

//displays current status of all violins
public static void instrumentStatus(Violin[] violins) throws IOException{
    for (int i = 0; i < violins.length; i++){
        StringPackage.comboPrint("Violin " + (i +1) + ": ");
        if (violins[i].isPlaying()) 
            StringPackage.comboPrint("Is currently playing.");
        else
            StringPackage.comboPrint("Is currently not playing.");
        if (violins[i].isTuned())
            StringPackage.comboPrint("Is currently tuned.");
        else
            StringPackage.comboPrint("Is currently not tuned.");
        violins[i].viewStringPackage(violins[i]);
    }//for
}//instrumentStatus(Violin[])

public static void section4(String[] args){
    String fileToOpen = "";
    fileToOpen = args[0];
    System.out.println("This program will read from and write to this file: " + fileToOpen + "\n");
    try{    
        File myOutFile = new File(fileToOpen);
        Scanner input = new Scanner(myOutFile);
        input.useDelimiter("\n");
        while (input.hasNext()){
    String text = input.next();
    System.out.println("The text is: " + text + ".\n");
        }
      PrintWriter fileOut = new PrintWriter(myOutFile); 
      fileOut.println("After much thought and many hours of work I have come to the conclusion\n" +
            "that Section 4 of Project 3 is not possible as written.\n\nIt says to \"Write the output " +
            "from your Instrument class to a text file that a user entered from the command line " +
            "arguments\"\n\nTo accomplish this, a method in the instrument class would need to " +
            "access a String variable local to the main method.  That is beyond my current skill level " +
            "and, I think, beyond that of any 'Intro to Java Programming' student.\n\n" +
            "I've created this file to show that I'm able to allow a user to enter a filename " +
            "from the command line and have the program write info to it.");
      fileOut.close();
    }//try

    catch (IOException io){
  System.out.println("Sorry that file is not found " + io);
    }//catch
}//section4



}//class

I also created a Violin class, basically a copy of the Guitar class.

share|improve this question
    
I haven't looked at it in depth but something I can see straight away is that you didn't really document the methods: public methods are exposed for other classes to use. These classes don't need to know the implementation (including local comments). They only need to know the exposed interface along with the javadoc, which is what you are missing. See the difference in purpose between local comments and javadoc for the class' clients? Accordingly javadoc shouldn't make any references to any details of the internal implementation, only to what the client needs to care about. –  Jubbat Dec 17 '12 at 17:36
    
@Jubbat I'm embarrassed to ask: When you say "client" do you mean customer, user, or another programming using the class I wrote? We haven't done anything with comments other than "Here are the types of comments, we'll discuss them in Chapter 24 (next semester)". –  MayNotBe Dec 17 '12 at 18:28
    
@Jubbat If I understand, you're saying javadoc each method to document how it interacts with other classes. That way a client will know how to call the method and what it will do for them. –  MayNotBe Dec 17 '12 at 18:29
1  
No need to be embarrased! Your guess in the second comment is right. "Client" here is any other class who calls any of your methods. Imagine some other person created this class as part of a library and you need to use it. How do you know what does a method do? Documentation, which is created from javadoc. That's all. –  Jubbat Dec 18 '12 at 1:04
    
Thanks @Jubbat ! –  MayNotBe Dec 23 '12 at 1:54
add comment

2 Answers 2

C++ guy here so maybe I am wrong about this but it looks like you've got a bug. Does java automatically call no-argument constructors from constructors that take arguments? If not then Guitar(int numberOfStrings) doesn't increment "numberOfGuitars".

Also it would make more design sense to have "StringPackage" be ignorant of any particular instrument type and instead just have one constructor that takes the number of strings and the "tuning" String and maybe another String which is the instrument name. This leaves the the class open to being a container for any type of stringed instrument without ever having to modify the class to support new instrument types.

share|improve this answer
    
I explained to my professor in the test plan that that constructr was a nod toward the theoretical finished project - a complete mapping of the stringed instrument class. That constructor increments numberOfTwelveStringGuitars if the int is 12. If not, nothing happens. In the finished project there would be one for every string configuration. If I understand you correctly, the parameters for the StringPackage constructor would receive int noOfStrings and String "Open E" (or somesuch?). The idea being that there are less tunings than stringed instruments? Thanks for the input! –  MayNotBe Dec 17 '12 at 18:12
1  
Why use English when code will do? StringPackage(String StringDescription, String Instrument) { stringArray = StringDescription.split(" "); instrumentName = Instrument; } –  jayinbmore Dec 17 '12 at 19:37
    
The above enables you to add new instrument types without having to change StringPacakage for each type. In a commercial application this is important in several ways. For example, suppose you had contracted out someone to add new instruments to your application but your contract with them stipulates that you can't give them source, just the interface documentation (so they could link with your .jar file but not see the source). –  jayinbmore Dec 17 '12 at 19:46
    
Wow. So simple! My first thought was "what happens when the wrong strings are entered?" My second was: fix it with a setter - that could change tuning, etc too. Also, there could be an 'int noOfStrings = stringArray.length()' in there in case that's needed. Thanks @jayinbmore! –  MayNotBe Dec 17 '12 at 21:38
    
Glad the idea came across. You could also write some format checking code in the constructor since the format is well defined (and you can throw an exception when someone tries to construct StringPackage with the wrong format). –  jayinbmore Dec 17 '12 at 22:26
add comment

The instructions do not make that much sense in this way. Did you translate them? I think I do not understand the term "string" correctly. From your code, it looks connected to "notes" but I do not know which string is a "Software-String" and which one is a "Guitar-String"

If I follow them exactly, I will get this:

  • a) 4 fields
  • b) Set the initial value to false, then the default constructor fulfills the requirements.
  • c) Ok, 3 methods
  • d) Ok, a method which detunes looks like the easiest approach
  • Text) The class methods do not have any output, they do not need to return anything. So the file will be empty. We will take the argument and create an empty file to fulfill the requirements.

Here we go:

import java.io.File;
import java.io.FileWriter;
import java.io.IOException;

public class SomeThing {
    private int numberOfStrings;
    private String[] stringNames;
    private boolean isTuned = false;
    private boolean isPlaying = false;

    public void tune() {
        isTuned = true;
    }

    public void startPlaying() {
        isPlaying = true;
    }

    public void stopPlaying() {
        isPlaying = true;
    }

    public void throwItAgainstAWallToDetuneIt() {
        isTuned = false;
    }

    public static void main(final String[] args) {
        File file = null;
        try {
            file = new File(args[0]);
        } catch (final Exception e) {
            System.err.println("First argument should be a path to a file.");
            System.exit(1);
        }
        appendStringToFile("", file);
    }

    private static void appendStringToFile(final String string, final File file) {
        FileWriter fileWriter = null;
        try {
            fileWriter = new FileWriter(file);
            fileWriter.write(string);
        } catch (final IOException e) {
            System.err.println("Filewriting failed: " + e);
            System.exit(1);
        } finally {
            if (fileWriter != null)
                try {
                    fileWriter.close();
                } catch (final IOException e) {
                    System.err.println("fileWriter.close() failed: " + e);
                    System.exit(1);
                }
        }
    }
}

(Obviously, I do not think that this is the way it should be, but this is the way from the requirements)

Some points about your code:

StringPackage stringPackage = new StringPackage("guitar", 6);

Make it private. And I do not see the point of this extra class (could be because of the string thing)

private boolean tuned = false;
private boolean playing = false;

better names: isTuned and isPlaying

It was already pointed out the the two constructors do not look consistent.

public Guitar(final int numberOfStrings) throws IOException {

there is no need to throw this exception.

The StringPackage class is too confusing for me.

public int length() {
    final int length = stringArray.length;
    return length;
}

You do not need to have this intermediate variable, a return stringArray.length; is fine in this case.

public static void comboPrint(final String myString) throws IOException {
    final boolean append = true;
    final FileWriter myOutFile = new FileWriter("GuitarViolin.txt", append);
    final PrintWriter fileOut = new PrintWriter(myOutFile);
    System.out.println(myString);
    fileOut.println(myString);
    fileOut.close();
}

It is a better way to not throw the IOException. Instead handle it and properly close the resource. See my ode above.

access a String variable local to the main method

You can solve this in several ways. If you want to do it like you describe it, your main class could have a public static String path variable. You can access this variable from all other classes then.

Final discussion point: Talk about the comments. I know a lot of people too, which have the opinion that a lot of code is good or high quality code. Even with some suggestions like more code then source code lines. The answer to this theory is just no. Even more, a lot of code lines is most often a good metric for bad code. As long as you are not coding in Fortran77 (where you probably have only 8 char for a variable name), highly-optimized code or something like this, always prefer easy and good readable code. Add comments only if you can not express it clearly in source code.

share|improve this answer
    
+1 for isTuned and isPlaying... a lot easier for others to understand if not familiar with the code. –  d-_-b Dec 19 '12 at 1:00
    
I'll keep the isPlaying, etc in mind when naming. That's awesome about the length() method!! I haven't learned exceptions yet - the book just said to put that there to make the method work. The instructions were as I printed them - I agree it's poorly worded and confusing. Thanks for your help! @tb- –  MayNotBe Dec 23 '12 at 1:58
add comment

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.