Getting list from an Oracle database and storing it in an object

I have two admin groups - VCS, and VPN. For each group, I need to get the list of all the people in that group, along with their IDs.

What is the best way to do this? Could you please suggest if it is good enough?

Also, should I use a map to store the data instead of creating a new object GetAdminInfo?

public class GetAdminInfo
{
private List persons;
private List personIds; 

public List getPersonIds() {
    return personIds;
}
public void setPersonIds(List personIds) {
    this.personIds = personIds;
}

public List getPersons() {
    return persons;
}
public void setPersons(List persons) {
    this.persons = persons;
}
}

This is the method I have written in my DAO class.

public static GetAdminInfo fetchPersonIdInfo(String adminGroup) throws SQLException
{
    GetAdminInfo personInfo =null;
    Connection connection = null;
    PreparedStatement statement = null;
    ResultSet rs = null;
    List personId = new ArrayList();
    List persons = new ArrayList();     
    try {
        connection = createConnection();
        statement = connection.prepareStatement("SELECT PERSON_ID,PERSONS FROM ADMIN_INFO WHERE ADMINGROUP = ?");
        statement.setString(1, adminGroup);
        rs = statement.executeQuery();
        if(rs.next())
        {
            personInfo = new GetAdminInfo();
            adminGroup = rs.getString(COLUMN_ADMINGROUP)!=null?rs.getString(COLUMN_ADMINGROUP):"";
            personId.add(rs.getString(1));
            persons.add(rs.getString(1));
            personInfo.setDeviceIds(deviceId);
            personInfo.setPersonIds(personId);          
        }           

    } finally{
        rs.close();
        conn.close();
        statement.close();
    }

    return personInfo;
}

Well my first gut reaction is that I don't like what I see. Here is why though and hopefully that will help you understand why I feel this way. First is with the naming convention. GetAdminInfo is a verb. Classes are nouns. This is true in all of mainstream OO languages. Therefore a more appropriate name would be AdministratorInformation. But even that name is misleading because of the contents. Your class has 2 arrays which of type String. Since you are dealing with a database it would be ok to make a class called AdministratorGroup which has 2 properties Name and Id with appropriate Getters and setters. Then your AdministratorInformation class would be renamed to AdministratorGroupCollection and have only 1 Array of type AdministratorGroup. Now after you put this program down for some time and come back to it you will find it easier to remember what these classes do. A name should always indicate its intent. You would be very angry at me if i had a class called CartesianPoint and instantiated it like this CartesianPoint point = new CartesianPoint("5Mhz"); You'd be angry because I lied to you. I normally don't do this as I leave it up to you to make changes, but I want to show some sort of code. SO here is what your code would look like after I got my hands on it.

AdministratorInfo

public class AdministratorInfo
{
    private String ID;
    private String name;

    public String getID()
    {
        return ID;
    }
    public String getName()
    {
        return name;
    }

    public void setID(String ID)
    {
        this.ID = ID;
    }
    public void setName(String name)
    {
        this.name = name;
    }
}

AdministratorGroupCollection

public class AdministratorGroupCollection
{
    private List persons;

    public List getPersons()
    {
        return persons;
    }

    public void setPersons(List newList)
    {
        persons = newList;
    }
}

With this code you will not have to add more and more Lists to your collection class to add or remove properties. You'll only have to change the AdministratorGroup class to adjust those properties.

Last point I want to point out is the confusing basis for your SQL method. By the name of it I get the impression that you are returning a single user and not a collection of users. but your code says that you are returning a collection of users. Don't let your names and methods lie about their intent. If you only intend to return a single user then return a class that is not a collection. If your intent is to get a group or a collection of things then you will return a list. Otherwise you are just making it hard for others (others can mean you in the future) to know how to use your code. Although I would break this method up some more to make it a little bit easier to read it serves its purpose. This is how I interpreted your code, and how I would have fixed it.

public static AdministratorInfo fetchPersonIdInfo(String adminGroup) throws SQLException
{
    AdministratorInfo personInfo = new AdministratorInfo();
    Connection connection = null;
    PreparedStatement statement = null;
    ResultSet rs = null;
    try
    {
        connection = createConnection();
        statement = connection.prepareStatement("SELECT PERSON_ID,PERSONS FROM ADMIN_INFO WHERE ADMINGROUP = ?");
        statement.setString(1, adminGroup);
        rs = statement.executeQuery();
        if (rs.next())
        {
            //what purpose does it serve to change adminGroup? 
            adminGroup = rs.getString(COLUMN_ADMINGROUP) != null ? rs.getString(COLUMN_ADMINGROUP) : "";
            personInfo.setID(rs.getString(1));
            personInfo.setName(rs.getString(2));
            personInfo.setDeviceIds(deviceId); //I don't like not knowing what this is
        }

    }
    finally
    {
        rs.close();
        connection.close();
        statement.close();
    }

    return personInfo;
}

StackExchange Code Review Q#32953, answer score: 2