Solution for swapping ArrayList data members with another

Question

Two possible solutions but which one? Or is there a Better?

I have a class MP3Track with members: Track number (int), track name (String), artist (String), in faourites (boolean) etc. My catalogue class contains two ArrayLists, one containing all tracks (vectorMain) and the other containing favourites (vectorFav) which are copies of the tracks in the main catalogue.

I want the user to have the option of swapping two track numbers (which are unique in both arrays).

Sample printout of the main catalogue below:

Both samples get the desired result but:

• (Solution 1) deleted as didn't work in reverse.
• (Solution 2) Below Seems rather cumbersome!
• (Solution 3) Below Seems more elegant but very long winded!

getMainIndex() - returns the index position in the ArrayList for a given track number or -1.

Here is Solution 2 (overcame problem of solution 1 - but ugly)

Here is start of method:

public void swapTrack(){ int t1 = -1, t2 = -1; System.out.println(face.getSwapTrackMenu()); System.out.print("1) Please enter first track number to swap: "); t1 = scan.readInt(); System.out.print("2) Please enter second track number to swap: "); t2 = scan.readInt(); MP3Track mp31 = null; MP3Track mp32 = null; 

Now solution 2

if((getMainIndex(t1)!=-1)&&(getMainIndex(t2)!=-1)){ String s1 = null; String s2 = null; for(MP3Track track : vectorMain){ if(track.getTrackNo() == t1){ s1 = track.getTitle(); } if(track.getTrackNo() == t2){ s2 = track.getTitle(); } } for(MP3Track track : vectorMain){ if(track.getTitle().equals(s1)){ track.setTrackNo(t2); } if(track.getTitle().equals(s2)){ track.setTrackNo(t1); } } for(MP3Track track : vectorFav){ if(track.getTitle().equals(s1)){ track.setTrackNo(t2); } if(track.getTitle().equals(s2)){ track.setTrackNo(t1); } } } 

Here is Solution 3

 //Loop thorugh main (ArrayList) looking for both track numbers for(MP3Track track : vectorMain){ if(track.getTrackNo() == t1){ //If track 1 found assign it to temp var mp31 = vectorMain.get(getMainIndex(t1)); } if(track.getTrackNo() == t2){ //If track 2 found assign it to temp var mp32 = vectorMain.get(getMainIndex(t2)); } } //Check if we found track 1 if(vectorMain.contains(mp31)){ //Set temp var new track number mp31.setTrackNo(t2); } //Check if we found track 2 if(vectorMain.contains(mp32)){ //Set temp var new track number mp32.setTrackNo(t1); } mp31 = null; //Remove pointer reference mp32 = null; //Remove pointer reference //loop thorugh Fav (ArrayList) looking for track numbers for(MP3Track track : vectorFav){ if(track.getTrackNo() == t1){ //Assign it to temp var mp31 = vectorFav.get(getFavIndex(t1)); } if(track.getTrackNo() == t2){ //Assign it to temp var mp32 = vectorFav.get(getFavIndex(t2)); } } //Check if we found it first if(vectorFav.contains(mp31)){ mp31.setTrackNo(t2); } //Check if we found it first if(vectorFav.contains(mp32)){ mp32.setTrackNo(t1); } 

Is there something I'm missing here? I've hit a wall as to a better solution!!

I'm new to Java so any suggestions, improvements & comments appreciated.

Many thanks

Answer 1

In Solution 3, assuming you initialize mp31 and mp32 to null, you can replace this

//Check if we found track 1 if(vectorMain.contains(mp31)){ //Set temp var new track number mp31.setTrackNo(t2); } 

With:

//Check if we found track 1 if(mp31 != null) { //Set temp var new track number mp31.setTrackNo(t2); } 

There is really no need to check if the arraylist contains the object since you used the get(...) method on the arraylist to get it in the first place. Therefore, you know that if the object has been initialized (is not null), the arraylist contains the object.

Also, I would change the method signature for getMainIndex to also take the list to search in as an argument to the method. In that case, you don't need to have two methods (with probably a lot of duplicated code) getMainIndex and getFavIndex can be the same method, getIndexInList(int trackNumber, List<MP3Track> list).

---

I am not sure how your getMainIndex method works at the moment, but I assume the code mp31 = vectorMain.get(getMainIndex(t1)); within the loop is the same thing as mp31 = track;. Therefore, you can replace this entire loop:

for(MP3Track track : vectorMain){ if(track.getTrackNo() == t1){ //If track 1 found assign it to temp var mp31 = vectorMain.get(getMainIndex(t1)); } if(track.getTrackNo() == t2){ //If track 2 found assign it to temp var mp32 = vectorMain.get(getMainIndex(t2)); } } 

With either:

for(MP3Track track : vectorMain){ if(track.getTrackNo() == t1){ //If track 1 found assign it to temp var mp31 = track; } if(track.getTrackNo() == t2){ //If track 2 found assign it to temp var mp32 = track; } } 

Or:

int index = getMainIndex(t1); if (index != -1) mp31 = vectorMain.get(index); index = getMainIndex(t2); if (index != -1) mp32 = vectorMain.get(index); 

---

As for Solution 2, this code here does not guarantee uniqueness of trackNo if two songs have the same title:

for(MP3Track track : vectorMain){ if(track.getTrackNo() == t1){ s1 = track.getTitle(); } if(track.getTrackNo() == t2){ s2 = track.getTitle(); } } for(MP3Track track : vectorMain){ if(track.getTitle().equals(s1)){ track.setTrackNo(t2); } if(track.getTitle().equals(s2)){ track.setTrackNo(t1); } } 

What you are essentially doing here is that you first scan through the list to get the title of the matching songs, and then you search for their title to change their track number. Why not change their track number within the first loop? You don't need all the three loops you have in Solution 2. Instead, search through vectorMain once and change the track numbers there, then search through vectorFav and change it there.

---

Some other comments:

I am not sure about how exactly you are using these two vectors and your MP3Track objects, or your trackNumber values. It seems to me as if whenever you swap track numbers, you do it in both of the lists. Therefore, perhaps you should use the same object references in both of the list. Don't create new MP3Track objects to place in vectorFav, you can re-use the same objects. That way, it will be enough to search only the vectorMain for the tracks, because it's the same object it will also update in vectorFav. As I said, I don't know exactly how you are using the trackNumber values so this might fit for you at the moment, but if it doesn't fit then you might want to change your design of the project.

As far as I have seen of the usage of getMainIndex and getFavIndex, you only use those methods to first get the index in the list, and then get the MP3Track object itself. Consider returning an MP3Track (the object itself) instead of an int (the index of the object in the list). Then you should also rename the method to reflect this change in behavior.