2
\$\begingroup\$

I am making an Address Book like application .
I have a contactType enum.

public enum ContactType
{
  //just a cut down version
  MOBILE,PHONE,FAX,EMAIL;

}

Then I have a Contact class

public class Contact
{
   private ContactType contactType;
   private String value;//e.g phone number or email
   private String areaCode;//does not apply to email or mobile
   //....getters and setters
}

Then I have a Person class

public class Person
{
    private List<Contact>contacts;
    //----Other attributes and getters and setters

}

I thought I have nailed very well :) but as it turns out probably not:( Because

I now needs to display a list of people(list of person) and in the table along with some other things (name etc)I have these columns:

Name|......|Phone|Mobile|Fax  |Email

I now realised that I can not just loop through the list of people and display contact numbers since these contacts are in list inside every object in list of people.
In simple words as you might know by now already if I have a list of people e.g

List<Person>people ;

Then this is not a valid option

for(Person person: people)
{
  //can not get phone number for example by going
  person.getPhone();
}

but this is

for(Person person: people)
    {
      //but will have to get it from the list of contacts e.g
      List<Contact>contacts = person.getContacts();
      for(Contact contact:Contacts)
      {
         if (contact.getContactType.isPhone())
         {
            contact.getValue();
         }
      }
    }

So I have now introduced a Rowitem Class

  public class Rowitem {

        private String phone;
        private String fax;
        private String mobile;
        private String email;

        ....

     }

and populating is like this

private void populateForDisplay(Rowitem item,List<Contact> contacts) 
 {
        for (Contact contact : contacts) 
        {
            if (contact.getType().isEmail()) {
                item.setEmail(contact.getValue());
            }
            if (contact.getType().isFax()) {
                item.setFax(contact.getValue());
            }
            if (contact.getType().isMobile()) {
                item.setMobile(contact.getValue());
            }
            if (contact.getType().isPhone()) {
                item.setPhone(contact.getValue());
            }
        }

    }

While this approach works I just do not think it is right. Firstly I do not like the fact that I have to introduce this RowItem class and then all these if statements do not look right. How can I improve this?
The front end framework that I am using is JSF2 but probably not relevant here.

I need someone to review my approach rather than line by line code.
I have read https://codereview.stackexchange.com/faq and I think my question is with the rules if not then please guide me and I will move this.
Because not code problem I did not think will go to StackOverFlow

\$\endgroup\$
3
\$\begingroup\$

At first I would suggest to get rid of the contact type enumeration and instead derive concrete contacts from your contact class. This will increase cohesion dramatically as this way you do not need to have fields like area code in your base contact class.

Regarding your problem: You could use something like a visitor pattern to navigate the "tree" of data (Persons -> Person -> Contacts) with your Person and Contact acting as elements accepting the abstract visitor. This will give you the flexibility to walk the tree for different purposes. For your scenario you could write something like a TableVisitor that creates the table structure you want to generate.

\$\endgroup\$
  • \$\begingroup\$ +1 excellent answer. Depending on the context the visitor pattern might be overkill, though. \$\endgroup\$ – Landei Sep 14 '11 at 11:56
2
\$\begingroup\$

If your data structure for storing a contact's value is simply a String, this code should help :-

public class Person {

    private Map<ContactType, String> contactMap;

    public Person() {
        contactMap = new HashMap<ContactType, String>();
    }

    public void addContact(ContactType type, String value) {
        contactMap.put(type, value);
    }

    public String getContact(ContactType type) {
        return contactMap.get(type);
    }
}

Adding a new ContactType won't change the above code. Simply add the new type in enum ContactType.

Iterating part is cleaner now. You can easily iterate over a Person's contact using enums as the key.

ContactType[] contactTypes=ContactType.values();
for (Person p : personList) {
    // get other Person fields.
    for(ContactType t:contactTypes){
        p.getContact(t);
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.