1

Does anyone why when the search value matches a value stored in array it doesn't remove that item?

String titles = "";
String lengths = "";

for (int i = 0; i < numOfSongs; i++) {
    titles += songTitles[i] + " ";
    lengths += songLengths[i] + " ";
}

String search = JOptionPane.showInputDialog("Enter a song title to remove it or -1 to end:");

while (!search.equals("-1")) {  
    for (int i = 0; i < numOfSongs; i++) {
        if (search.equalsIgnoreCase(songTitles[i])) {
            songTitles[i] = songTitles[i + 1];
        }
    }
    numOfSongs--;

    JOptionPane.showMessageDialog(null, "**Current Playlist**" + "\nSong titles: " + titles + "\nSong lengths: " + lengths);

    search = JOptionPane.showInputDialog("Enter a song title to remove it or -1 to end:");
} 
dguay
  • 1,635
  • 15
  • 24
ush
  • 49
  • 7
  • 4
    dont use arrays if you want to remove an item at a given index, instead use an `ArrayList` – user1231232141214124 Oct 28 '15 at 14:50
  • Java Provides `LinkedList` which gives much better performance in delete operation in comparison to Array or ArrayList(which is backed by Array). or you can follow this [link](http://stackoverflow.com/questions/642897/removing-an-element-from-an-array-java) – Subhrajyoti Majumder Oct 28 '15 at 14:56
  • Yes, that would be the easiest way, but how would you do it without using ArrayList – ush Oct 28 '15 at 14:57
  • You cant remove an item from the array (if you dont want empty indexes that is). Arrays are fixed sized. If you want to 'remove an item' you need to create a new array with and upper bound of n-1, where n is the upper bound of the previous array, then copy the remaining values from the original array over. – Mark W Oct 28 '15 at 15:11

2 Answers2

1

Many things are wrong with this code:

  1. You never update titles and lengths inside your while loop, so whatever happens inside has no effect on what's printed in the dialog
  2. When you find song title to remove, you copy the next song title to the current one, but don't copy anything else, so [a, b, c, d] will after removing b change to [a, c, c, d] - you need to shift everything behind the deleted element left by one position
  3. When you find song title to remove, you assume the i+1th position is valid - this isn't true if you remove the last song on the list, that would either fail with ArrayIndexOutOfBounds exception or copy some garbage from behind the currently valid playlist
  4. You're never updating songLengths array
  5. Concatenating strings in a loop using += is very ineffective - use StringBuilder instead
Jiri Tousek
  • 12,211
  • 5
  • 29
  • 43
  • For your second point, how would I shift everything behind the deleted element by one position to the left? – ush Oct 28 '15 at 15:29
  • You need another for loop that iterates from the deleted position to the end of (valid part) of the array and copies every element to the left. Not that this is very time consuming, that's why others suggested using linked list instead for real-life applications. – Jiri Tousek Oct 28 '15 at 15:31
1

Sorry this took a while, but hopefully it's pretty comprehensive.

I am assuming that song title and song length are supposed to correspond with one another, so that if you remove the title you also remove the length? It may be good to create a class, e.g. Song, which has a field for both title and length. There are more methods you can add, e.g. setters, default constructor, etc. You can also include more fields like Song Artist, year, etc. I'm just including those required for your program to run.

I'll use red's suggestion of an ArrayList, so you can see what they meant (in case you haven't learned what that is)

public class Song {
    String title; //these are known as fields, or instance variables
    String length;

    public Song(String title, String length) {
        this.title = title;
        this.length = length;
    }

    public String getTitle() {
        return title;
    }

    public String getLength() {
        return length;
    }

    //you can format this differently. Just keeping it simple though. If you don't include toString() method in this class, you will run into some problems if you try to print the object itself. 
    public String toString() {
        return "title = " + title + " length = " + length + "\n";
    }

From here, in your main method you can do...

ArrayList<Song> playlist = new ArrayList<>();

//here, inside a do-while loop, get input for each song, then store into strings, let's call them songTitle and songLength. I'm not showing this step since I don't know where you want the input to come from, but I'm sure you can figure this bit out. ;)

Then we create objects and add them to your list like so:

Song song = new Song(songTitle, songLength); //creates a new object with arguments songTitle and songLength
playlist.add(song); //adds object to array list.

Once you have your playlist set up, we return to your question regarding song removal, and here is where Lists(there are different ones you can use)/Objects really make things far simpler.

Iterator<Song> songIt = playlist.iterator();
while (!search.equals("-1") && songIt.hasNext()) {
    if (search.equalsIgnoreCase(songIt.next().getTitle())) {
        songIt.remove();
    }
}

And printing is simple too.

for (int i = 0; i < playlist.size(); i++) {
    System.out.println(playlist.get(i);
}

-EDIT-

To put into perspective, here is what you would have to do for removal in your program using array and without objects.

int removeCount = 0;
while (!search.equals("-1")) {
    for (int i = 0; i < songTitles.length; i++) {
        if (search.equalsIgnoreCase(songTitles[i])) {
             for (int j = i; j < songTitles.length - 1; j++) {
                songTitles[j] = songTitles[j + 1];
                songLengths[j] = songLengths[j + 1];
                removeCount ++;
            }
        }
    }
}

String remainingTitles[] = new String[songTitles.length - removeCount];
String remainingLengths[] = new String[songTitles.length - removeCount];
for (int i = 0; i < temp.length; i++) {
    remainingTitles[i] = songTitles[i];
    remainingLengths[i] = songLengths[i];
}

Suffice it to say, this is much more ugly, and there's many more places where you can make a stupid mistake that may or may not throw an exception.

  • 2
    I don't think you want to remove in the middle of a for loop. Probably better to use an Iterator. – JustinKSU Oct 28 '15 at 17:28
  • Alright. I've replaced what I had before with an iterator. Thanks a lot for the feedback. Still learning as well, so will take any and all pointers I can get. :) – Monkeygrinder Oct 28 '15 at 18:35