School project critique

Question

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.

Answer 1

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.

Answer 2

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.