Hello everyone I just finished my last program for class this semester and would love to have some feedback from the memebers here on things that I still need to improve on or good coding practices that I may not know about that can help me become better. I have already turned the program in and as far as the testing I have done the program works as intended so hopefully I will do well on it.

The first section will be the main, followed by a Song class, and an MP3_Player class. This is the longest code I have posted to date so I hope I make it readable and easy to understand. I have also tried to add enough comments to make it easier to follow. Thank you for your time and advice!

package cse1301_program3;


public class CSE1301_Program3 {

    /**
     * @param args the command line arguments
     */
    public static void main(String[] args) {
        /* This will create an MP3_Player object called ipod.  It will then give
         * it the initial storageCapacity of 22.
         */        
        MP3_Player ipod = new MP3_Player(22);

        /*This will create 6 Song objects song1-song6.  It will then add each
         * song to the MP3 player if there is enough freeSpace on the MP3 Player.
         */
        Song song1 = new Song("Free Falling", "Tom Petty", 4.54f, 5);
        ipod.addSong(song1);

        Song song2 = new Song("Fearless", "Taylor Swift", 3.6f, 3);
        ipod.addSong(song2);

        Song song3 = new Song("Hotel California", "Eagles", 3.9f, 4);
        ipod.addSong(song3);

        Song song4 = new Song("Paradise City", "Aerosmith", 4.7f, 6);
        ipod.addSong(song4);

        Song song5 = new Song("Chattahoochee", "Alan Jackson", 3.0f, 3);
        ipod.addSong(song5);

        // Song6 will not be added due to the freeSpace being used up.
        Song song6 = new Song("You Belong with Me", "Taylor Swift", 4.2f, 2);
        ipod.addSong(song6);

        // This prints the contents of the MP3 player.
        System.out.println(ipod);

        /* This do loop plays song2 from the beginning of the song until its
         * completion. and then prints that it has finished playing.
         */
        do {
        song2.Play();
        } while (song2.getIsPlaying());

        // Song2 will be removed for the Arraylist of the MP3 Player.
        ipod.removeSong(song2);

        /* This prints the contents of the MP3 player after removing song2.
         * It shows how the freeSpace has been increased.
         */
        System.out.println(ipod);

        /* This do loop tries to play song3 at position 5, but returns that it
         * is past the duration of the song.
         */ 

        do {
            song3.playSong(5);
        } while (song3.getIsPlaying());

        /* This do loop plays song1 at position 2, and moves the start
         * position to 2 and plays the song til the end.
         */ 

        do {
            song1.playSong(2);
        } while (song1.getIsPlaying());
    }
}





package cse1301_program3;

/*
 * To change this template, choose Tools | Templates
 * and open the template in the editor.
 */

public class Song {
    // The following instantiate the attributes given to the Song() class.
    private String title;
    private String artist;
    private float duration;
    private float currentPlayPosition;
    private int startPosition;
    private boolean isPlaying;
    private int fileSize;

    /* These are the default, default(with parameters), and toString() 
     * constructors of the Song() class.
     */
    public Song()
    {
        title = "NO TITLE";
        artist = "NO ARTIST";
        duration = 0.0f;
        currentPlayPosition = 0.0f;
        startPosition = 0;
        isPlaying = false;
        fileSize = 0;
    }

    public Song(String t, String a, float d, int f)
    {
        title = t;
        artist = a;
        duration = d;
        currentPlayPosition = 0.0f;
        isPlaying = false;
        fileSize = f;
    }

    public String toString()
    {
        return "'" + title + "' by " + artist + " lasts for " + duration +
                " and is " + fileSize + "mb large";
    }

    // These are the accessor methods of the Song() class.
    public String getTitle()
    {
        return title;
    }

    public String getArtist()
    {
        return artist;
    }

    public float getDuration()
    {
        return duration;
    }

    public boolean getIsPlaying()
    {
        return isPlaying;
    }

    public int getFileSize()
    {
        return fileSize;
    }

    // These are the mutator methods of the Song() class.
    public void setTitle(String newTitle)
    {
        if (newTitle.compareTo("") != 0)
        {
            title = newTitle;
        }
    }

    public void setArtist(String newArtist)
    {
        if (newArtist.compareTo("") != 0)
        {
            artist = newArtist;
        }
    }

    public void setDuration(float newDuration)
    {
        if (newDuration >= 0)
        {
            duration = newDuration;
        }
    }


    // This is the standard Play() method of the Song class.
    public void Play()
    {
        isPlaying = true;
        currentPlayPosition += 0.1f;

        if (currentPlayPosition < duration)
        {
            System.out.println(title + " is playing at position " +
                    currentPlayPosition);
        }
        else
        {
            isPlaying = false;
            System.out.println(title + " has finished playing.");
            currentPlayPosition = 0.0f;
        }
    }

    /*  This is the added playSong() method to start a song at a certain position
     *  in the song.  If the start position is past the duration it will give an
     * error message.
     */
    public void playSong(int i)
    {
        isPlaying = true;
        startPosition = i;
        currentPlayPosition += 0.1f;

        if (currentPlayPosition <= startPosition)
        {
            currentPlayPosition = startPosition;
        }

        else if (currentPlayPosition <= duration)
        {
            System.out.println(title + " is playing at position " +
                    currentPlayPosition);
        }

        else if (startPosition > duration)
        {
            isPlaying = false;
            System.out.println("Cannot play song at this position, it is past "
                    + "the duration.");
        }

        else
        {
            isPlaying = false;
            System.out.println(title + " has finished playing.");
            currentPlayPosition = 0.0f;
        }

    }
}





package cse1301_program3;

import java.util.ArrayList;

/*
 * To change this template, choose Tools | Templates
 * and open the template in the editor.
 */
public class MP3_Player {
    // This creates the ArrayList that will hold the songs in the MP3 Player.
    ArrayList<Song> songList = new ArrayList<Song>();

    //This creates the two attributes associated with the MP3 player.
    private int storageCapacity;
    private int freeSpace;

    // This is the default constructor of the MP3 player
    MP3_Player()
    {
        storageCapacity = 0;
        freeSpace = storageCapacity;
    }

    // This is the constructor with parameters for the Mp3 player.
    MP3_Player(int t)
    {
        storageCapacity = t;
        freeSpace = storageCapacity;
    }

    /* This returns a toString() for the Mp3 player with formatting to display
     information about storageCapacity, freeSpace, and the list of songs in 
       the MP3 player.
    */
    public String toString()
    {       
        return "MP3 player with capacity " + storageCapacity + ", " + freeSpace 
         + " left, contains:\n" + songList.toString().replace("[","").replace("]", "").replace(",", "\n");
    }

    //  These are three accessor methods for the MP3 player.
    public int getStorageCapacity()
    {
        return storageCapacity;
    }

    public int getFreeSpace()
    {
        return freeSpace;
    }

    public ArrayList<Song> getSongList()
    {
        return songList;
    }

    /*  This is the method for adding a song to the MP3 player, and it checks if
     * there is enough space before adding the song and if there is it is added.
     *  If there is not enough space it returns the error.
     */
    public void addSong(Song s)
    {
            if (s.getFileSize() <= freeSpace) 
            {
                songList.add(s);
                System.out.println(s.getTitle() + " added to the MP3 player.");
                freeSpace -= s.getFileSize();
            }
            else
            {
                System.out.println("Cannot add " + s.getTitle() + 
                        "; not enough free space.");
            }
    }

    /*  This is the method for removing a song from the MP3 player.  It searches
     *  the ArrayList for the song in question and if a match is found it removes
     * it from the ArrayList.  It will also adjust the amount of freeSpace that 
     * is added back to the MP3 player.
     */
    public void removeSong(Song s)
    {
        boolean songWillBeRemoved = false;
        for (Song i : songList)
          {
            if (s.equals(i))
            {
            freeSpace += s.getFileSize();
            songWillBeRemoved = true;
            System.out.println(s.getTitle() + " removed from the MP3 Player.");
            }
            else if (s.equals(i))
            {
                System.out.println("This song is not stored in the MP3 Player.");
            }
          }
        if (songWillBeRemoved)
        {
            songList.remove(s);
        }
    }

}
Comments
Good post!

The Play method goes against Java naming conventions that almost everyone follows. See: Code Conventions for Java

The startPosition field is being used like a local variable, not a field.

Your else if on line 328 is never going to happen.

Returning an ArrayList<Song> from getSongList means that it would be impossible to update your MP3_Player to return a different kind of List<Song> later. Also, returning songList to the client means that your client can break the rules by putting more songs into the list than are allowed.

Your Song constructor uses one-letter parameter names. Remember that you can name your private fields and local variables however you like, but your parameter names are part of the documentation for your class that people will see and you should take care in providing clear and meaningful names.

Edited 3 Years Ago by bguild

Comments
Thank you for your help

Thank you for the quick response bguild. That was carelessness on my part about the Play method naming.

Now that I have had time to step back and come back to it, the startposition isn't really even necessary. I could get rid of it all together and just use the variable i to do what needs to be done.

I will remove the getSonglist, it isn't necessary to the program and now I am not even sure why I put it in there. I will have to do a lot more reading and studying on this topic as well.

I will make the Song constructor names more meaningful and I understand that it does make it easier to follow for everyone else who see's the program.

Thank you again for you help and advice.

I wrote the same statement as the previous if statement, I just saw your comment about that. That is the first time I have used the.equals method, but would a else if !(s.equals(i)) be the way to fix that. Thank you again for your help and if you see anything else please let me know.

Thank you for the quick response bguild. That was carelessness on my part about the Play method naming.

Personally, dont listen to bguild; In most cases, noone sees your code except you. You dont need to write how a set of code conventions state. You write the code however it is clearest/makes sense to you. First, write the code how it is clearest to you, the developer. THEN do things to better the code, optimize, etc.

I think a precomplied jar/exe would have been nice to test your program out...Was your program free choosing (you choose to do this) or assigned?

Edited 3 Years Ago by riahc3

It was assigned to do it in this way. I have never compiled a program into a jar/exe, but I have been wondering how would you do that? And does that make an icon that I can click to run the program?

Edited 3 Years Ago by nova4005: wrong spelling

I ment that if you want to call it Play, call it Play. If you want to call itkljsdgkjhdskjg, call it that. Whatever is clearest to you...

And about the jar/exe thing.........I THOUGHT a jar was made when you ran but you have to change some setting. Ill look it up.

... ou dont need to write how a set of code conventions state ...

Possibly the worst advice of 2013 so far in this forum. Even if you never write a single program for real, even your teacher will read and mark your code. Coding standards and conventions are there to help everybody, and the sooner you engrain all the right habits the better.

Re: comnments
Your commenting varies widely, some is excellent and some of it is redundant.
You should write your code with meaningful names so that someone who knows Java can read it easily. Comments like line 37 or 85 just clutter the code to no benefit.
On the other hand, you have some very good commenting, eg lines 312-315. Comments like that explain the overall content and strategy of what will follow, and any Java programmer will be able to read what follows and understand it perfectly.

Possibly the worst advice of 2013 so far in this forum. Even if you never write a single program for real, even your teacher will read and mark your code. Coding standards and conventions are there to help everybody, and the sooner you engrain all the right habits the better.

I disagree.

If noone is going to read your code, name things however you feel like that makes sense to you and you remember it. This of course if only YOU are going to read the code.

I shouldnt prepare my code for others.

One of the most important things when coding is commenting; Commenting is more for you than others as you maybe one day see your code again and say "Why did I do that?". It makes life so much easier.

But other than that? If you want to name two int variables HiIAmBerry and another one klshldgfkjhsgfdj and add them up to a ohrlylolo varaible, be my guest! As long as you comment them and later remember why/what they do

Thank you everyone for the feedback.

@Jamescherrill About the commenting that is redundant is it ok to not comment simple things like that. Things like simple print statements and variable assignments? The main reason i made so many comments was to make sure i didn't lose points of my grade, but I will probably lose them anyway now because of bad commenting. I will remember from now on to make comments meaningful. I like all of the help and feedback I receive from members on this site when I post my code.

Commenting standards do vary, and for a school assignment you do do what the teacher wants! I don't think your comments were "bad", and some were excellent. I would not deduct any points for that.

Just ask yourself - would a Java programmer understand that line or block of code? If so then it doesn't need a comment, if not then either improve the names you use, or add a comment.

Personally I think your code is pretty good. Well done.

A few more if you are game:

  • Use camel casing when naming methods/classes/variables etc. as opposed to snake case (e.g. MP3Player as opposed to MP3_Player)
  • Don't blindly provide getters and setters for every field in your class. For e.g. you already have addSong and removeSong methods in your code; why provide a getter for songList. This is made worse by the fact that since songList is mutable, I can add/remove songs without triggering your checks which is not a good thing. If you still have to provide a list of all the songs, wrap up your list in an immutable wrapper which the user can iterate but not mutate.
  • Think twice before adding methods; for e.g. why do you need a default constructor for MP3_Player?
  • Favor constructor initialization over setters
  • Be extra careful when validating user input. Currently you check for blank titles i.e. "". What happens if I pass in " "? Is that a valid title? If not, does your code catch that?
  • Try out testing frameworks like JUnit for testing/validating your code.

Thank you for the response sos, I can't look up some of that stuff on my phone at work, but when I get home tonight I am going to look them up.

I will say that the only reason I put a default constructor in the mp3 player class was because I thought it had to have one. I based that off of the examples the teacher gave on classes and constructors.

I will respond to more when I get home tonight. Thank you again for your time.

@s.o.s. You are definitely right about the validating user input. My checks fails if a " " is used for the song title or artist name. It leads to a pretty meaningless line of text. I do think that some sort of validation would be good to have, but I can't think of how to change it yet. I am going to do some reading and thinking and see if I can come up with a solution.

I think the 2nd and 3rd points you made are great and I think with more preperation and planning I can avoid things like that in the future and just have what I need in the program.

I do have one thing to ask though just for clarification on point 4. I think I understand but you mean that I should lean more towards using the constructor than using the mutator methods?

I will look into the testing frameworks like Junit, because I have no idea what those are yet. Does that come with the netbeans installation or is it a plugin that I can install seperately?

I do have one thing to ask though just for clarification on point 4. I think I understand but you mean that I should lean more towards using the constructor than using the mutator methods?

Yes. So instead of:

Person p = new Person();
p.setName("sos");
p.setAge(666);

prefer

Person p = new Person("sos", 666);

To add to this, if you don't plan on changing the fields after the object is created, you can also mark them as final and be rest assured that there won't be any accidental assignment or mutation to these fields. It doesn't make a lot of difference in a single threaded program but become really important when you start using threads. Shared mutable state (i.e. sharing a Person object between two threads wherein the fields are mutate/can be set) can lead to headaches as the system grows in size. So for e.g. if you are asked to add a new song to the playlist and remove an existing one, instead of "mutating" or modifying the object you plan on removing, it's better to actually remove a given song object and add a new one. In short, stay away from unnecessary mutation! If you want to know more or need a detailed snippet to showcase the problem, just let me know and I'll write it out for you.

Does that come with the netbeans installation or is it a plugin that I can install seperately?

Some IDE's like Eclipse come pre-installed with JUnit 4 framework (make sure you use the latest JUnit version, the one which supports annotations). I think that's also the case with Netbeans. Here is a document which might help you out.

Edited 3 Years Ago by ~s.o.s~

so in a multithreaded program if i had my song class that was shared between two or more classes then they could possibly both have the ability to change song objects. then this could lead to the program not functioning correctly. If you have time and don't mind making an example I would love to see it.

I well look into junit tonight when I get home.

so in a multithreaded program if i had my song class that was shared between two or more classes then they could possibly both have the ability to change song objects. then this could lead to the program not functioning correctly. If you have time and don't mind making an example I would love to see it.

It need not be a multi-threaded program. Mutation related bugs can slip in even for a single-threaded program though they are normally more painful and hard to find in case of a multi-threaded one.

As an example, let's say that you plan on providing a "replace" functionality to your users of the playlist class. So basically, they can pick an existing song from the playlist and replace it with a new one. If you resort to mutation (to save memory as they all say) and instead of creating a new song, try modifying the existing song, this is what happens:

MP3_Player ipod = null; // initialize with songs; assume
Song toRemoveSong = ipod.getSongByName("Hotel California");

// we need to maintain a list of removed songs
List<Song> removedSongs = new ArrayList<Song>();
removedSongs.add(exisingSong);

// In some other method; try replacing the song
Song oldSong = ipod.getSongByName("Hotel California");
oldSong.setTitle("Baby");
oldSong.setArtist("Justin Bieber");

// Show the songs user has removed from the ipod
showToUser(removedSongs);

In case you haven't noticed yet, we are hosed. Since Song is mutable, the list of removed songs now contains 'Baby' instead of 'Hotel California' and you have lost the song which you actually removed. This might sound like a trivial or maybe even a poorly fabricated example but this does happen in real code. For a small code base, it might not take a lot of effort but as code-base grows, you are in for some sleepless nights.

On a related note, shared static state is your worst enemy when it comes to multi-threaded code. As an example:

// parse Person objects from a text file; one line per person
class PersonParser {
  public static SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd");

  public Person parsePerson(String line) {
    // parse date of birth using the static SimpleDateFormat above
    // and other details. Return after creating a Person object
  }
}

Now let's say I need to speed up the parsing and so decide to use multiple threads when parsing the text file or array of strings. Again, in case you haven't noticed, we are hosed. Why? Because SimpleDateFormat has its own internal mutable state. If we use it across threads, state trampling might occur which in turn would give wrong results or if you are lucky an exception. These kinds of bugs are really hard to find because your program might seem to be working but it really isn't.

Hopefully those are good enough examples to give you a brief overview of the kind of havoc uncontrolled mutation and state sharing can cause.

Edited 3 Years Ago by ~s.o.s~

Comments
Thank you for the great examples

@ sos, Thank you. You have given me a lot to think about and the examples really make it easier to understand. As I have been working on this small program it has made me think about what actually goes into an actual music player to make it function and play music through speakers. There is so much that I still don't understand but I am learning something new everyday and it makes me want to learn even more.

This question has already been answered. Start a new discussion instead.