Simple Playlist Editor/Player

Question

I am starting with Java and at the beginning I would like someone to catch what mistakes I make. I would like my code to look its best and I want to learn good Java writing habits. Could anyone explain to me where I am making mistakes or what I should do differently for this purpose?

My program presents a music playlist. To add song to a playlist, we must first add the song to the album. All 3 classes are in separate files in my program. I set up getters and setters only when I use them - I don't know if it's a good habit. calledNext flag checks whether iterator.next() was called in the previous run.

public class Main {
 private static LinkedList<Song> playlist = new LinkedList<>();
 private static LinkedList<Album> albums = new LinkedList<>();
 public static void main(String[] args) {
 Album a1 = new Album();
 a1.addToAlbum("song1", 1.57);
 a1.addToAlbum("song2", 2.00);
 albums.add(a1);
 albums.get(0).addToPlaylist(playlist, "song1");
 albums.get(0).addToPlaylist(playlist, "song2");
 System.out.println();
 runPlaylist();
 }
 public static void runPlaylist() {
 ListIterator<Song> iterator = playlist.listIterator();
 Scanner scanner = new Scanner(System.in);
 boolean quit = false;
 boolean calledNext = true;
 System.out.println("Now playing " + iterator.next().toString());
 printMenu();
 while (!quit) {
 if (playlist.isEmpty()) {
 System.out.println("Playlist is empty");
 break;
 }
 System.out.println("Enter your choice ");
 int x = scanner.nextInt();
 scanner.nextLine();
 switch (x) {
 case 1:
 quit = true;
 System.out.println("bye bye");
 break;
 case 2:
 if (!calledNext)
 if (iterator.hasNext())
 iterator.next();
 calledNext = true;
 if (iterator.hasNext())
 System.out.println("Now playing: " + iterator.next().toString());
 else
 System.out.println("No more songs in the playlist");
 break;
 case 3:
 if (calledNext)
 if (iterator.hasPrevious())
 iterator.previous();
 calledNext = false;
 if (iterator.hasPrevious())
 System.out.println("Now playing: " + iterator.previous().toString());
 else
 System.out.println("You are at the beginning");
 break;
 case 4:
 if (calledNext) {
 if (iterator.hasPrevious())
 System.out.println("Replaying: " + iterator.previous().toString());
 else
 System.out.println("Replaying: " + playlist.getFirst().toString());
 calledNext = false;
 } else {
 if (iterator.hasNext())
 System.out.println("Replaying: " + iterator.next().toString());
 else
 System.out.println("Replaying: " + playlist.getLast().toString());
 calledNext = true;
 }
 break;
 case 5:
 printMenu();
 break;
 case 6:
 String titleToAdd = scanner.nextLine();
 for (int i = 0; i < albums.size(); i++) {
 Album currentAlbum = albums.get(i);
 if (!currentAlbum.addToPlaylist(playlist, titleToAdd))
 System.out.println("Song not found in album");
 }
 break;
 case 7:
 String titleToRemove = scanner.nextLine();
 for (int i = 0; i < albums.size(); i++) {
 Album currentAlbum = albums.get(i);
 if (!currentAlbum.removeFromPlaylist(playlist, titleToRemove))
 System.out.println("Song not found in playlist");
 }
 if (!playlist.isEmpty())
 System.out.println("Now playing: " + playlist.getFirst().toString());
 break;
 case 8:
 System.out.println(playlist);
 break;
 default:
 System.out.println("Invalid input");
 break;
 }
 }
 }
 public static void printMenu() {
 System.out.println("Instructions : \nPress" +
 " 1 to exit\n" +
 "2 to play next song\n" +
 "3 to play previous song\n" +
 "4 replay\n" +
 "5 print menu \n" +
 "6 add song to playlist\n" +
 "7 remove song from playlist\n" +
 "8 print playlist\n");
 }
public class Album {
 private ArrayList<Song> songs;
 public Album() {
 this.songs = new ArrayList<>();
 }
 
 public void addToAlbum(String title, double duration) {
 if (checkAlbum(title) == null) {
 songs.add(new Song(title, duration));
 } else {
 System.out.println("Song already in album");
 }
 }
 public void removeFromAlbum(String title) {
 Iterator<Song> iterator = songs.iterator();
 if (checkAlbum(title) != null) {
 while (iterator.hasNext()) {
 if (iterator.next().getTitle().equals(title)) {
 iterator.remove();
 }
 }
 }
 }
 public boolean addToPlaylist(LinkedList<Song> playlist, String title) {
 Song checkedSong = checkAlbum(title);
 if (checkedSong != null) {
 playlist.add(checkedSong);
 System.out.println("Song " + title + " has been added to playlist");
 return true;
 }
 return false;
 }
 public boolean removeFromPlaylist(LinkedList<Song> playlist, String title) {
 for (int i = 0; i < playlist.size(); i++) {
 Song currentSong = playlist.get(i);
 if (currentSong.getTitle().equals(title)) {
 playlist.remove(currentSong);
 System.out.println("Song " + title + " has been removed from playlist");
 return true;
 }
 }
 return false;
 }
 public Song checkAlbum(String title) {
 for (int i = 0; i < songs.size(); i++) {
 Song currentSong = songs.get(i);
 if (currentSong.getTitle().equals(title)) {
 return currentSong;
 }
 }
 return null;
 }
}
public class Song {
 private String title;
 private double duration;
 public Song(String title, double duration) {
 this.title = title;
 this.duration = duration;
 }
 public String getTitle() {
 return title;
 }
 @Override
 public String toString() {
 return this.title + ": " + this.duration;
 }
}
```

Answer 1

I suggest avoiding static state. It makes your code more rigid. It works for this very simple app, but the limitations become more apparent in more complex apps. You should remove the static keywords from your two lists and the runPlaylist method. You can instantiate an instance of the class to use in main() and call public methods on it.

Some of your functionality doesn't employ separation of concerns. For example, your Album class has an addToPlaylist method. The functionality of the method is to check if the given Song is a member of the Album and only add it to the provided list if it is. The only part of that having anything to do with the Album is seeing if it owns the Song. Having the Album responsible for all that other behavior is tangling up your classes. On a large-scale, complicated project, this kind of code would become very difficult to maintain.

So, I would remove addToPlaylist and removeFromPlaylist from the Album class. I would change the method checkAlbum to containsSong (names are important for code legibility!) and return a boolean. Those two removed methods should be moved up to the Main class, since it is the owner of the Album and the Playlist and so it is the one that knows what it wants to do with a Song.

You are currently comparing Songs by their title field. This requires other classes (in this case Album) to dig into its internals to be able to compare Songs. Instead, you should have Album override equals() and provide the behavior you want. Since you are currently ignoring the duration, you can do that in its equals method. Now your code will be more maintainable, because you can change this behavior in a single location, and it's the most logical location. Also, it will help you replace some of these verbose for loops with simple contains() calls. (Note, if you override equals, you should always override hashcode as well. The IDE can help you generate these together.)

I would also change the parameters for Album.addSong() and removeSong() to take a Song instance instead of making Album responsible for instantiating Songs.

And a tip. Use final when declaring fields that don't need to change, especially collections. This reduces the number of mutable things you have to keep track of. When you have a non-final List field, it is mutable in two different ways: you could change the contents of the list or replace the list entirely. Keep it simpler by restricting that to one way of modifying it.

• user13630431: do you suggest me to create a Main class object in the main function and call runPlaylist with this object? like this Main play = new Main(); play.runPlaylist()
• Tenfour04: Yes. And probably rename the class Playlist.
• user13630431: so there is no rule that the program must contain the main class? the main class is where psvm method is right?
• Tenfour04: It doesn't matter what the name of the class with the main method is. Personally, in a case like this, I would have a Playlist class and a separate Main class that has nothing in it except a main method that instantiates your Playlist class and uses it.