Updating/modifying elements in ArrayList

[Magnus Kul]

Hi.

I'm making a program that stores information about football players. The program makes it possible to create new players and register match statistics about them, such as goals, minutes played etc.
The program works quite well, except for the part where the program has to check if an element (player) already exists in the arraylist, so that the program doesn't create a duplicate player with the same name, instead of updating the player statistics.
I've tried to write some kind of code that will perform such a check, but the program only notices some of the players in the list, while some of the players who already exist is unnoticed. It really frustrates me, and I hope someone can help me with what I'm doing wrong.

Some of the values etc. are written in Norwegian - I hope it doesn't matter when reading the code.
This code is for creating new players (name only):

  if (e.getSource().equals(window.getRegistrerSpillerButton())) {
			
			kamp Spiller = new kamp(window.getSpillerNavn().getText(), 0, 0, 0, 0, 0, 0, 0);
			
			try {
				windowController.class.getResourceAsStream("statistikk.txt");
				fin = new FileInputStream("statistikk.txt");
				oin = new ObjectInputStream(fin);
				read = (ArrayList<kamp>) oin.readObject();
				spillerListe.removeAll(spillerListe);
				spillerListe.addAll(read);
				
			}
			catch (IOException es) {
				es.printStackTrace();
				JOptionPane.showMessageDialog(null, "En feil oppstod! Prøv igjen. ", "Feil", JOptionPane.ERROR_MESSAGE);
			} catch (ClassNotFoundException es) {
				// TODO Auto-generated catch block
				es.printStackTrace();
			}
			
			if (window.getSpillerNavn().getText().equals("")) {
				JOptionPane.showMessageDialog(null, "Du må skrive inn det fulle navnet til spilleren du vil slette! ", "Feil", JOptionPane.INFORMATION_MESSAGE);
				try {
					e.wait();
				} catch (InterruptedException e1) {
					// TODO Auto-generated catch block
					e1.printStackTrace();
				}}
			
			if (!spillerListe.isEmpty()) {

				CopyOnWriteArrayList<kamp> test = new CopyOnWriteArrayList(); 

				for (Iterator it = test.iterator(); it.hasNext();) {
					
					kamp match = (kamp)it.next();

			if(window.getSpillerNavn().getText().equalsIgnoreCase(match.getSpillerNavn())) {
				JOptionPane.showMessageDialog(null, window.getSpillerNavn().getText() + " finnes allerede i listen!", "Sletting vellykket", JOptionPane.ERROR_MESSAGE);
				kamp Kamp = new kamp(match.getSpillerNavn(), match.getAntallMål(), match.getAntallKamper(), match.getStartetKamper(), match.getStartetBenk(), match.getByttetInn(), match.getAntallMinutter(), match.getAverageMinutter());
				it.remove();
				test.add(Kamp);
				try {
					fout = new FileOutputStream("statistikk.txt");
					oout = new ObjectOutputStream(fout);
					oout.writeObject(test);
					Kamp = null;
					
				} catch (FileNotFoundException e1) {
					// TODO Auto-generated catch block
					JOptionPane.showMessageDialog(null, "Kunne ikke slette spilleren! Prøv igjen. ", "Feil", JOptionPane.ERROR_MESSAGE);
				} catch (IOException e2) {
					// TODO Auto-generated catch block
					JOptionPane.showMessageDialog(null, "Kunne ikke slette spilleren! Prøv igjen. ", "Feil", JOptionPane.ERROR_MESSAGE);
				}
				}
			
			else {
				
				test.add(Spiller);
				try {
					fout = new FileOutputStream("statistikk.txt");
					oout = new ObjectOutputStream(fout);
					oout.writeObject(test);
					
				} catch (FileNotFoundException e1) {
					// TODO Auto-generated catch block
					JOptionPane.showMessageDialog(null, "Kunne ikke slette spilleren! Prøv igjen. ", "Feil", JOptionPane.ERROR_MESSAGE);
				} catch (IOException e2) {
					// TODO Auto-generated catch block
					JOptionPane.showMessageDialog(null, "Kunne ikke slette spilleren! Prøv igjen. ", "Feil", JOptionPane.ERROR_MESSAGE);
				}
				JOptionPane.showMessageDialog(null, window.getSpillerNavn().getText() + " er nå lagt til i listen!", "Sletting vellykket", JOptionPane.INFORMATION_MESSAGE);
				window.getSpillerNavn().setText("");
				test.removeAll(test);
			}
			}}

			if(spillerListe.isEmpty()){
				
				spillerListe.add(Spiller);
				try {
					fout = new FileOutputStream("statistikk.txt");
					oout = new ObjectOutputStream(fout);
					oout.writeObject(spillerListe);
					
				} catch (FileNotFoundException e1) {
					// TODO Auto-generated catch block
					JOptionPane.showMessageDialog(null, "Kunne ikke slette spilleren! Prøv igjen. ", "Feil", JOptionPane.ERROR_MESSAGE);
				} catch (IOException e2) {
					// TODO Auto-generated catch block
					JOptionPane.showMessageDialog(null, "Kunne ikke slette spilleren! Prøv igjen. ", "Feil", JOptionPane.ERROR_MESSAGE);
				}
				JOptionPane.showMessageDialog(null, window.getSpillerNavn().getText() + " er nå lagt til i listen!", "Sletting vellykket", JOptionPane.INFORMATION_MESSAGE);
				window.getSpillerNavn().setText("");
			}  

It's probably really terrible written atm, cause I've tried so many differents loops and lists etc., but it doesn't work whatever I try.
If someone knows what I'm doing wrong I'd really appreciate any help, cause I'm really bummed out now.

Peace.

[Brian Legg]

Instead of doing all that extra checking to make sure you haven't created a Player yet or not I would use a collection that accepts only unique entries. The Set collection is great for this. Each time you add a Player to the Set it will only allow a unique Player. How does the Set collection know if the Player is unique or not? You override the equals method for the Player class and define what makes one Player different from another. There are many others as well that I think would work much better for what you are trying to do. You should look up the Java Collection framework and read the descriptions of each one.... I'm sure it will give you several different ideas for doing this that would be much more efficient.

Let me know if you are absolutely stuck on using an ArrayList....

[Magnus Kul]

Thanks for the reply, but the thing is the list is also meant to handle non-unique players, such as when match statistics are updated for a player who already exists. So I think ArrayList is the best way to store these elements.
So, how can solve this problem?

[Steve Luke]

Your best bet is to implement a good equals() method that using those fields that identify a player. You could then use:

if (list.contains(player))
{
    Player oldPlayer = list.get(list.indexOf(player));
    //copy stats from player to oldPlayer
    //no need to remove/re-insert if you can update
}
else
{
    list.add(player);
}
//save list

[David Newton]

As a previous poster suggested a list really isn't the optimal collection for this--it may not matter for your particular app, but consider keeping the statistics for a million players. You wouldn't want to iterate through (potentially) the entire list to find a single player. That's what hash-like structures are good at.

[Magnus Kul]

I agree with that, but this app is only meant to be used by a small club to keep statistics about their players. I need some good way of iterating through the list of elements and find out if the name of the player the user wants to create already exists in the list. Like player.getName().equals() etc. It's not good enough to check for equality amongst the objects, cause number of goals, minutes played etc. may differ even though they try to add a player with a name that already exists. So I have to check if the playername exists in the list, not the object. If someone understands the issue here, all help is still very much appreciated, cause I'm really out of clues right now.

Thanks for all replies so far.

[David Newton]

It's kind of hard to read your code, since the code posted appears to be doing several different things. It might be easier to understand what the actual problem is if it was broken out into separate methods, each doing only one small thing, and posting only the code that's causing a problem.

If all you need to do is iterate over a list of objects and check for equal names the pseudo-code would look something like this:

for (Player p : players) {
    if (p.name.equalsIgnoreCase(aName)) {
        updateTheStats(p, stats);
        foundPlayer = true;
        break;
    }
}
// If no player was found it's a new player, so add them.
if (!foundPlayer) {
    addPlayerWithStats(player info, stats, and so on)
    savePlayers();
}

There are a several ways this could actually be written, some better than this, some worse, but this is the basic idea. Plus I'm not entirely clear on the requirements.

[Steve Luke]

Magnus Kul wrote:I agree with that [David Newton's suggestion to use a Hashing Collection], but this app is only meant to be used by a small club to keep statistics about their players.

Regardless of that, you may find it easy to use a HashMap<String, Player> where the key is the player's name. That would all but eliminate the need to iterate and compare...

Magnus Kul wrote: I need some good way of iterating through the list of elements and find out if the name of the player the user wants to create already exists in the list. Like player.getName().equals() etc. It's not good enough to check for equality amongst the objects, cause number of goals, minutes played etc. may differ even though they try to add a player with a name that already exists. So I have to check if the playername exists in the list, not the object.

To implement my last suggestion, you would write an equals method that just compares those parts of the Player data that would determine equality. If it is just name, than your equals would simply call getName().equals()... Example:

public class Player
{
    private final String name;
    private final String team;
    private int[] stats;

    ....

    public boolean equals(Object other)
    {
        if (other == null)
        {
            return false;
        }

        if (!(other instanceof Player.class)
        {
            return false;
        }

        Player otherPlayer = (Player)other;
        return this.getName().equalsIgnoreCase(otherPlayer.getName());
    }

The goal here is to take as much work out of your hands as possible, let the List iterate to search for equality, while you define what equality is.

That being said - David Newton's second post should also solve the problem for you.

[Brian Legg]

I think Steve answered your question.... but I would still recommend using a different collection.

I need some good way of iterating through the list of elements and find out if the name of the player the user wants to create already exists in the list.

Sounds like a perfect job for a HashMap. The HashMap takes a unique key (your player name), and a value (your player Object). This way you can just code like this (pseudo):

Get Player from HashMap with name="My name"
if Player is null then create new Player and add to HashMap
else if Player is not null then update Player stats

That's a condensed version but changing your code from an ArrayList to a HashMap is not hard at all and will save you lots of time/energy. You should always use the most correct collection for what your trying to do.. that's why they were created, to make coding easier.

[fred rosenberger]

Magnus Kul wrote:It's not good enough to check for equality amongst the objects, cause number of goals, minutes played etc. may differ even though they try to add a player with a name that already exists.

I think you're missing a key point. if you define the object, YOU get to decide what 'equals' means. You can write your own equals() method that ONLY checks to see if the name are the same, if that is what makes sense. OR you could define it to be 'name' and 'jersey_number' are both the same, in case you really do get two players with the same name. OR you could define it in any other way that makes sense for YOUR application

[David Newton]

I'd add only that it's not ridiculously uncommon for two people to have the same name, which always makes things more entertaining.

[Magnus Kul]

Thanks alot for all your replies! You guys are really great. I will try implementing a HashMap tomorrow and see how it turns out. I will also look into the different code-suggestions that are posted here. If I'm still blank (hopefully not though) I'll post further questions tomorrow.

Once again, thanks alot! I really appreciate all the replies.

[Brian Legg]

You're welcome, and if you have problems implementing the HashMap just let us know.

[Christophe Verré]

And don't forget what David suggested :

David wrote:
It's kind of hard to read your code, since the code posted appears to be doing several different things. It might be easier to understand what the actual problem is if it was broken out into separate methods, each doing only one small thing, and posting only the code that's causing a problem.

[Magnus Kul]

I just want to inform you guys that the HashMap did the trick! It solved all my problems, and the program behaves just the way I want!

Once again, a great thanks to everyone who replied to this post. You made my week!

- Magnus