Java iteration...how?

[squibbles]

Right, once again, I've hit a wall in my code, I know what the problem is, and I know in need an Iterator, but I have no idea how to implement it... This is my code:

Code: Select all

public class Club
{
    public ArrayList<Membership> Member;
    public ArrayList<Membership> PurgedMembers;
    
    /**
     * Constructor for objects of class Club
     */
    public Club()
    {
        Member = new ArrayList<Membership>();
        PurgedMembers = new ArrayList<Membership>();
    }

    public ArrayList<Membership> purge(int month, int year)
    {
        if(month >= 1 || month >= 12)
        {
            for(Membership member : Member)
            {
                if(member.month == month && member.year == year)
                {
                    PurgedMembers.add(member);
                    Member.remove(member);
                }
            }
        }
        else
        {
            System.out.println("Month must be between 1 and 12. Your entry of " + month + " does not comply with this condition.");   
        }
        return PurgedMembers;
    }
}

There is more, but, well, it doesn't affect this part of code...just more functions.

Any help would be much appreciated.

[LittleViking]

Problem:

public ArrayList<Membership> purge(int month, int year)
{
if(month >= 1 || month >= 12)
{

What do you need an iterator for here, by the way?

[Tanner]

You also need to make certain that all your variables have the same capitalization and change if(member.month == month && member.year == year) to if((member.month == month) && (member.year == year)).

[squibbles]

The error i get is:

cannot remove from an array list whilst iterating over it...My teacher said that using a separate iterator would solve this problem.

I feel i should explain the names as well:

Member is an arraylist which contains three fields per membership:

member
year
month

Code: Select all

if(month >= 1 || month >= 12)

This is simply in there to return an error if month (the variable input by the user) is not a valid month. What's wrong with it?

[LittleViking]

Code: Select all

if(month >= 1 || month >= 12)

This is simply in there to return an error if month (the variable input by the user) is not a valid month. What's wrong with it?

You're checking if the month is greater than 1 or greater than 12. You probably want to check if the month is less than 1 or greater than 12.

[squibbles]

*facepalm*

I can't believe I was that dumb. :(

I still however get the same error as earlier with the for-each loop, saying I cannot remove an entry from a collection while iterating over it.

[scythe]

Code: Select all

if(month >= 1 || month >= 12)

This is simply in there to return an error if month (the variable input by the user) is not a valid month. What's wrong with it?

You're checking if the month is greater than 1 or greater than 12. You probably want to check if the month is less than 1 or greater than 12.

Also, he might want to remove the "or equal to". Last I checked, both January and December were months.

If I may comment on your style, PurgedMembers should probably not be a class variable; just declare it in your function and return it. Unless you plan to make other functions read and write to that same variable; in which case, I'd say make your function return "void" (PurgedMembers is already public).

[t̷s͢uk̕a͡t͜ư]

I don't actually know any Java, but if it's anything like C++ in the way it handles more advanced data structures, it's often a very good idea / necessary to use an iterator when you're using data structure templates (and your declaration of ArrayList<Membership> looks an awful lot like that).
In C++, every STL data structure has an iterator class that you treat something like a pointer.
Quick example:

Code: Select all

vector<int> numbers;
// push_back() crap to numbers
vector<int>::iterator num_iter;
for (num_iter = numbers.begin(); num_iter != numbers.end(); num_iter++)
    cout << (*num_iter) << endl;

Sounds like there's something similar in Java?

[Qix]

Your problem is that using a for-each loop limits the functional of your iterator. If you attempt to remove something from a collection while iterating, the iterator loses track of the spot in the ArrayList. Instead you need to create an Iterator seperately:

Code: Select all

Iterator iter = Member.iterator()
 while (iter.hasNext()) {
Membership m = iter.Next();
PurgedMembers.add(m); //I assume there is no specific order to the list?
iter.remove();
}

Using iterator's remove method allows the iterator to keep track of the changes within the arraylist. Hope this helps.

[PsychoSnail]

I still however get the same error as earlier with the for-each loop, saying I cannot remove an entry from a collection while iterating over it.

I actually ran into nearly the exact same problem just a few weeks ago.
The way I solved it (since I know nearly nothing about Iterators) is I used a regular for loop, and if I removed an entry, I set the index value back one at the end of the loop body (so when 1 is added to the index value it ends up that nothing is changed), because the elements in an ArrayList move over to fill up the space that used to be filled by the element you just removed, so counting resumes from the place you were already at.
I'm talking about something like this:

Code: Select all

for(int i = 0; i < Member.size(); i++)
{
     Membership member = Member.get(i);
     if(member.month == month && member.year == year)
     {
           PurgedMembers.add(member);
           Member.remove(i);
           i--;
     }
}

I have no idea if it's better to use Iterators, as I've never used one before.

Edit: Looking at Qix's code, it looks to me that using an Iterator would be easier.

Edit 2:

Also, he might want to remove the "or equal to". Last I checked, both January and December were months.

If I may comment on your style, PurgedMembers should probably not be a class variable; just declare it in your function and return it. Unless you plan to make other functions read and write to that same variable; in which case, I'd say make your function return "void" (PurgedMembers is already public).

QFE.

[squibbles]

Yeah, thanks for the help guys, the problem has been resolved. :D