Java class for creating HeadPhone class and Test class

Question

Here is the Assignment instructions:

Create a Java class named HeadPhone to represent a headphone set. The class contains:

• Three constants named LOW, MEDIUM and HIGH with values of 1, 2 and 3 to denote the headphone volume.
• A private int data field named volume that specifies the volume of the headphone. The default volume is MEDIUM.
• A private boolean data field named pluggedIn that specifies if the headphone is plugged in. The default value if false.
• A private String data field named manufacturer that specifies the name of the manufacturer of the headphones.
• A private Color data field named headPhoneColor that specifies the color of the headphones.
• getter and setter methods for all data fields.
• A no argument constructor that creates a default headphone.
• A method named toString() that returns a string describing the current field values of the headphones.
• A method named changeVolume(value) that changes the volume of the headphone to the value passed into the method
• Create a TestHeadPhone class that constructs at least 3 HeadPhone objects. For each of the objects constructed, demonstrate the use of each of the methods.

Here is the code I have written so far that does work. I need your help in reviewing and improving the code:

// HEADPHONE CLASS
public class HeadPhone {
    public static final int LOW = 1;
    public static final int MEDIUM = 2;
    public static final int HIGH = 3;

    private int volume;
    private boolean pluggedIn;
    private String manufacturer;
    private String headPhoneColor;

    String currentVolume;
    String statusPluggedIn;
    String playlist;

    // Constructor
    public HeadPhone(int volume, boolean pluggedIn, String manufacturer, String headPhoneColor) {
        this.volume = volume;
        this.pluggedIn = pluggedIn;
        this.manufacturer = manufacturer;
        this.headPhoneColor = headPhoneColor;
    }

    // Default Constructor
    public HeadPhone() {
        volume = MEDIUM;
        pluggedIn = false;
        manufacturer = "DEFAULT";
        headPhoneColor = "DEFAULT";
    }

    // Setter methods
    // setVolume
    public void setVolume(int volume) {
        this.volume = volume;
    }

    // setPluggedIn
    public void setPluggedIn(boolean pluggedIn) {
        this.pluggedIn = pluggedIn;   
    }

    // setManufacturer 
    public void setManufacturer(String manufacturer) {
        this.manufacturer = manufacturer;
    }

    // setHeadPhoneColor
    public void setHeadPhoneColor(String headPhoneColor) {
        this.headPhoneColor = headPhoneColor;
    }

    // getVolume
    public int getVolume() {
        if (volume == 1) {
            currentVolume = "LOW";
        }
        else if (volume == 2) {
            currentVolume = "MEDIUM";
        }
        else {
            currentVolume = "HIGH";
        }
        return volume;
    }

    // getPluggedIn
    public boolean getPluggedIn() {
        if(pluggedIn == true) {
            statusPluggedIn = "plugged in";
        }
        else {
            statusPluggedIn = "disconnected";
        }

        return pluggedIn;
    }

    // getManufacturer
    public String getManufacturer() {
        return manufacturer;
    }

    // getHeadPhoneColor
    public String getHeadPhoneColor() {
        return headPhoneColor;
    }

    // getPlaylist
    public String getPlaylist() {

        if(pluggedIn == false) {
            playlist = "Please plug the Head Phones into a device.";
        }
        else if(pluggedIn == true && volume == 1) {
            playlist = "Currently playing classical music playlist";
        }
        else if(pluggedIn == true && volume == 2) {
            playlist = "Currently playing country music playlist";
        }
        else {
            playlist = "Currently playing rock music playlist";
        }
        return playlist;
    }

    // changeVolume
    public void changeVolume(int volume) {
        setVolume(volume);
    }

    // toString
    public String toString() {
        int volume = this.getVolume();
        boolean pluggedIn = this.getPluggedIn();
        String manufacturer = this.getManufacturer();
        String headphoneColor = this.getHeadPhoneColor();
        String currentVolume = this.currentVolume;
        String playlist = this.getPlaylist();

        StringBuilder sb = new StringBuilder();
        sb.append(String.format("Manufacturer: %s\n", manufacturer));
        sb.append(String.format("Color: %s\n", headPhoneColor));
        sb.append(String.format("Currently: %s\n", statusPluggedIn));
        sb.append(String.format("Volume is set to: %s\n", currentVolume));
        sb.append(String.format("%s\n", playlist));

        return sb.toString();
    }    
}



// TEST CLASS

public class TestHeadPhone {

    /**
     * @param args the command line arguments
     */
    public static void main(String[] args) {

        HeadPhone HeadPhone1 = new HeadPhone();
        HeadPhone HeadPhone2 = new HeadPhone(1, true, "JVC", "Green");
        HeadPhone HeadPhone3 = new HeadPhone(3, true, "Beats", "Red");

        int HeadPhone1Volume = HeadPhone1.getVolume();
        boolean HeadPhone1PluggedIn = HeadPhone1.getPluggedIn();
        String HeadPhone1Manufacturer = HeadPhone1.getManufacturer();
        String HeadPhone1HeadPhoneColor = HeadPhone1.getHeadPhoneColor();
        String HeadPhone1CurrentVolume = HeadPhone1.currentVolume;
        String HeadPhone1StatusPluggedIn = HeadPhone1.statusPluggedIn;
        String HeadPhone1Playlist = HeadPhone1.playlist;

        int HeadPhone2Volume = HeadPhone2.getVolume();
        boolean HeadPhone2PluggedIn = HeadPhone2.getPluggedIn();
        String HeadPhone2Manufacturer = HeadPhone2.getManufacturer();
        String HeadPhone2HeadPhoneColor = HeadPhone2.getHeadPhoneColor();
        String HeadPhone2CurrentVolume = HeadPhone2.currentVolume;
        String HeadPhone2StatusPluggedIn = HeadPhone2.statusPluggedIn;

        int HeadPhone3Volume = HeadPhone3.getVolume();
        boolean HeadPhone3PluggedIn = HeadPhone3.getPluggedIn();
        String HeadPhone3Manufacturer = HeadPhone3.getManufacturer();
        String HeadPhone3HeadPhoneColor = HeadPhone3.getHeadPhoneColor();
        String HeadPhone3CurrentVolume = HeadPhone3.currentVolume;
        String HeadPhone3StatusPluggedIn = HeadPhone3.statusPluggedIn;

        System.out.println("Head Phone 1 has the following parameters:");
        System.out.println(HeadPhone1.toString());
        System.out.println("Head Phone 2 has the following parameters:");
        System.out.println(HeadPhone2.toString());
        System.out.println("Head Phone 3 has the following parameters:");
        System.out.println(HeadPhone3.toString());  
    }  
}

The questions that I have directly are:

• What exactly is the intended result of the changeVolume(change) method in the descriptions?
• Should String currentVolume / statusPluggedIn / playlist be private?
• Any other details or help that could improve both the usability and functionality of this code?

Answer 1

I think your changeVolume() is correct. But it does actually the same thing like setVolume(), which may be sort of redundant.

It is always a good idea to make your members private unless you have a reason for giving them another access level.

As for other ideas, your specification specificies very detailed what your code should do, so there is not so much possibility to change anything. But the constants could be replaced with enums (of course this does not conform to your spec).

Apart from that, you can replace if(pluggedIn==true) with if(pluggedIn) and if(pluggedIn!=true) with if(!pluggedIn)

After if(!pluggedIn) you don't need if(pluggedIn == true && volume == 1) because it can only be true. So if(volume == 1) is enough. If/else statements in getVolume() can be somewhat simplified by using a switch statement.

Answer 2

What exactly is the intended result of the changeVolume(change) method in the descriptions?

I don't get it either. Since the spec requires a setVolume method, the distinction is really not clear. Perhaps the idea is that the setters are for low-level manipulations with no intelligence, simply setting values, and changeVolume might be a less technical, slightly higher level term, and do something more intelligent. But I can't think of anything to distinguish two kinds of volume changing operations.

Should String currentVolume / statusPluggedIn / playlist be private?

It would be best to delete these fields completely. You don't need them. They duplicate information you already have. This is a maintenance overhead, and ugly. For example, you set these strings when the getters are called, so your getters have side effects. It's really ugly to have side effects.

Also, what is a playlist doing in a head phone?

Any other details or help that could improve both the usability and functionality of this code?

Avoid pointless comments like these:

// Constructor
public HeadPhone(int volume, boolean pluggedIn, String manufacturer, String headPhoneColor) {

// Default Constructor
public HeadPhone() {

// Setter methods
// setVolume
public void setVolume(int volume) {

// setPluggedIn
public void setPluggedIn(boolean pluggedIn) {

In your demo code, you named variables starting with capital letters. The convention is to use camelCase for variable names. Also, using numbers in variable names is a bad practice. It would be better to find names that are more descriptive. For example:

HeadPhone defaultHeadPhone = new HeadPhone();
HeadPhone greenHeadPhone = new HeadPhone(1, true, "JVC", "Green");
HeadPhone redHeadPhone = new HeadPhone(3, true, "Beats", "Red");

Although the requirements were probably imposed on you, I have to point out some problems there too:

• Constants named "LOW", "MEDIUM", "HIGH" are not exactly obvious in a head phone class. "VOLUME_LOW", "VOLUME_MEDIUM", "VOLUME_HIGH" would be better.

  • Even so, volume is usually controlled with a dial, not with discrete values, so I find this an unrealistic setup

• The fragment "headPhone" in the name of the headPhoneColor field in a HeadPhone class is redundant, as it duplicates what is already implied by the class name

• I don't think it's a good idea to add getters and setters willy-nilly without a good reason. Code shouldn't exist without a good reason.

• A no-argument constructor with no default color doesn't make any sense. What would such a head phone look like?

• The purpose of the changeVolume method is unclear.

• When writing test code, the convention is to name the class with a "Test" suffix after the name of the class under the test, rather than a prefix.

Answer 3

    String currentVolume;
    String statusPluggedIn;
    String playlist;

You do not need fields by these names, so I'd just remove them entirely. You can either make them local variables in the relevant methods or rewrite the methods so as not to need them. E.g. in

    public String getPlaylist() {

        if(pluggedIn == false) {
            playlist = "Please plug the Head Phones into a device.";
        }
        else if(pluggedIn == true && volume == 1) {
            playlist = "Currently playing classical music playlist";
        }
        else if(pluggedIn == true && volume == 2) {
            playlist = "Currently playing country music playlist";
        }
        else {
            playlist = "Currently playing rock music playlist";
        }
        return playlist;
    }

You could say instead

    public String getPlaylist() {    
        if (!pluggedIn) {
            return "Please plug the Head Phones into a device.";
        }

        switch (volume) {
            case 1:
                return "Currently playing classical music playlist";
            case 2:
                return "Currently playing country music playlist";
            case 3:
            default:
                return "Currently playing rock music playlist";
        }
    }

Note that I simplified the expression in the if. In my opinion, this reads more naturally than the the other version.

Since we are returning, the if does not need an else. Since volume can take more than two values, a switch is a better fit than an if/else construct.

As others said, an enum would make more sense for the volume than an integer and constants. However, that wasn't what the assignment said. Perhaps the next assignment will be to rewrite it with an enum with a toString method.

You don't need

    // getPlaylist

That doesn't tell you anything that the method signature does not. It would be different if it were Javadoc, but it isn't. I would rather have an explanation of why the playlist is based on the volume. That would be a helpful comment.

In general, comments should explain why code does things the way that it does, not just restate what the code does.

    public int getVolume() {
        if (volume == 1) {
            currentVolume = "LOW";
        }
        else if (volume == 2) {
            currentVolume = "MEDIUM";
        }
        else {
            currentVolume = "HIGH";
        }
        return volume;
    }

This is confusing. You have a getter method with a side effect. It would be better to split this into two methods.

    public int getVolume() {
        return volume;
    }

    public String getVolumeString() {
        switch (volume) {
            case 1:
                return "LOW";
            default:
            case 2:
                return "MEDIUM";
            case 3:
                return "HIGH";
        }
    }

Note that I also changed the behavior to default to MEDIUM rather than HIGH.

    public boolean getPluggedIn() {
        if(pluggedIn == true) {
            statusPluggedIn = "plugged in";
        }
        else {
            statusPluggedIn = "disconnected";
        }

        return pluggedIn;
    }

Same thing here

    public boolean getPluggedIn() {
        return pluggedIn;
    }

    public String getPluggedInStatus() {
        if (pluggedIn) {
            return "plugged in";
        }

        return "disconnected";
    }

Then you can change your toString to use these methods directly.

    public String toString() {
        int volume = this.getVolume();
        boolean pluggedIn = this.getPluggedIn();
        String manufacturer = this.getManufacturer();
        String headphoneColor = this.getHeadPhoneColor();
        String currentVolume = this.currentVolume;
        String playlist = this.getPlaylist();

        StringBuilder sb = new StringBuilder();
        sb.append(String.format("Manufacturer: %s\n", manufacturer));
        sb.append(String.format("Color: %s\n", headPhoneColor));
        sb.append(String.format("Currently: %s\n", statusPluggedIn));
        sb.append(String.format("Volume is set to: %s\n", currentVolume));
        sb.append(String.format("%s\n", playlist));

        return sb.toString();
    }

becomes

    public String toString() {
        StringBuilder sb = new StringBuilder();

        sb.append(String.format("Manufacturer: %s\n", getManufacturer()));
        sb.append(String.format("Color: %s\n", getHeadPhoneColor()));
        sb.append(String.format("Currently: %s\n", getPluggedInStatus()));
        sb.append(String.format("Volume is set to: %s\n", getVolumeString()));
        sb.append(String.format("%s\n", getPlaylist()));

        return sb.toString();
    }

Also, it doesn't hurt anything, but you don't need to use this to access methods from the class inside the class.

Your test class does not call the setters, but the assignment says all methods. I would suggest calling toString before and after using the setters.