Is this good coding practice?

[brookey86]

I would like to keep an arraylist of all the Member object references created in my program. Thus, I have placed a public static variable inside the Member class itself that gets added to in the constructor every time a new object is created. I have also placed an instance method called "deleteMember()" that will deleted the object reference if needed.

Is it good Java practice to keep this array inside of the Member class itself, and have additions made in the constructor and deletions through an instance method?

The other option would be to put the arraylist inside the main() function and use the arraylist class functions to add and delete when needed.


public class Member {

private String fullName;
private int birthYear;
private double weight;
private boolean isAdmin;

public static ArrayList<Member> memberList = new ArrayList<Member>();

public Member(String fullName, int birthYear, double weight, boolean isAdmin) {
//super();
this.fullName = fullName;
this.birthYear = birthYear;
this.weight = weight;
this.isAdmin = isAdmin;

//add the newest object to the memberList arraylist
memberList.add(this);
}

public void deleteMember()
{
//delete current object from the memberList arraylist
memberList.remove(this);
}
}

[Fou-Lu]

Its not wrong practice if thats what you mean. You're only other real option would be to create a collection yourself which has significant overhead compared to using what you do have.
I'd stick with an arraylist within the class you've created. It makes sense for it to be static as well since its not actually a member but a collection of members. What I would change is the usage of anything altering what you have. First, make it protected or private, then use public static mutators for it: public static void addMember(Member member);public static void deleteMember().... Although you certainly don't require it to be, it just seems wrong having something like:

Member mem1 = new Member(...);
mem1.addMember(new Member(...));
// Something like this would be better:
Member mem1 = new Member(...);
Member.addMember(mem1);
Member.addMember(new Member(...));


Hope that helps you out!

[ess]

Agree with Fou-Lu comment....but I would abstract the member class where it does not hold collections of members....and define another class which would handle how members are stored etc.

Personally, I would go further and implement the Singleton design pattern on the MemberCollection class so that there is only instance that exists...holding all members etc. and ultimately, improving the overall performance of your application

The following are examples of how I would implement this:
Member.java file
public class Member {

private String fullName;
private int birthYear;
private double weight;
private boolean isAdmin;

//public static ArrayList<Member> memberList = new ArrayList<Member>();

public Member(String fullName, int birthYear, double weight, boolean isAdmin) {
//super();
this.fullName = fullName;
this.birthYear = birthYear;
this.weight = weight;
this.isAdmin = isAdmin;

//add the newest object to the memberList arraylist
//memberList.add(this);
}

/*
public void deleteMember()
{
//delete current object from the memberList arraylist
memberList.remove(this);
}
*/
@Override
public String toString() {
return "Name: " + this.fullName + "\tD.O.B: " + this.birthYear + "\tIs Admin: " + this.isAdmin;
} //-- ends toString
}

MemberCollection.java
import java.util.*;

public class MemberCollection {

private ArrayList<Member> memberList = new ArrayList<Member>();
private static MemberCollection instance = null;

private MemberCollection() {

} //-- ends constructor

// you can only have one instance of this class
// since we've implemented the Singleton design pattern
public static MemberCollection getInstance() {
if(MemberCollection.instance==null) {
MemberCollection.instance = new MemberCollection();
}
return MemberCollection.instance;
} //-- ends MemberCollection

public void addMember(final Member m) {
this.memberList.add(m);
} //-- ends addMember

public void delMember(final Member m) {
this.memberList.remove(m);
} //-- ends delMember

@Override
public String toString() {
StringBuffer buf = new StringBuffer();
for(int i=0; i < this.memberList.size();i++) {
buf.append(this.memberList.get(i).toString()+"\n");
} //-- ends for loop
return buf.toString();
} //-- ends toString

} //-- ends class definition

and finally MemberRunner.java to test the application
public class MemberRunner {
public static void main(String [] args) {
Member m1 = new Member("John Smith", 1977, 12.25, false);
Member m2 = new Member("Marco Smith", 1970, 10.25, false);
// get instance an instance of the MemberCollection class
MemberCollection coll = MemberCollection.getInstance();
coll.addMember(m1);
coll.addMember(m2);
System.out.println("+++ Collection of Members +++");
System.out.println(coll);
// to prove that you can only have one instance
// and therefore improving performance
// let's create another pointer to the already
// existing instance of the MemberCollection class
MemberCollection second = MemberCollection.getInstance();
// print it out and examin the results
System.out.println("--- Declaring another pointer ---");
System.out.println(second);
} //-- ends class method main
} //-- ends class definition

If you wish to read about the Singleton design pattern, please visit this page http://www.javabeginner.com/singleton.htm

Cheers
~E