0
\$\begingroup\$

A list of objects needs to be grouped.

The list is iterated and checked with other objects in the same list except itself. The method People.isAttributeEqual() checks if two objects can be grouped.

The list returns a list of two unique objects with the same attributes.

I am trying to optimize the method People.groupSimilar(). All other details given here are just snippets.

The code below works but looks clunky and non optimized. Is there a way to optimize and use lambda instead?

public class People
{

  private String name;
  private String age;
  private String address;

  public People(final String name, final String age, final String address)
  {
    this.name = name;
    this.age = age;
    this.address = address;
  }

  public boolean isAttributeEqual(final People dupPeop)
  {
    return this.address.equals(dupPeop.address) && this.age.equals(dupPeop.age) && this.name.equals(dupPeop.name);
  }

  public static void main(final String[] args)
  {

    List<People> asList = Arrays.asList(new People("Sri", "28", "TN"), new People("Sri", "28", "TN"), new People("Sri", "28", "TN"),
        new People("Pri", "28", "TN"));

    List<People> groupSimilar = groupSimilar(asList);

  }

  public static List<People> groupSimilar(final List<People> people)
  {
    List<People> duplicatePeople = new ArrayList<>(people);
    for (Iterator<People> iterator = duplicatePeople.iterator(); iterator.hasNext();)
    {
      People people2 = iterator.next();
      for (People orignalPeople : people)
      {
        if (!orignalPeople.equals(people2) && orignalPeople.isAttributeEqual(people2))
        {
          iterator.remove();
          people2 = iterator.next();
        }
      }
    }
    return duplicatePeople;
  }
}
\$\endgroup\$
2
\$\begingroup\$
public class People

The class is named People, but it should probably be called Person. Every instance of the class is another Person, not another People.


private String name;
private String age;
private String address;

It might be worth to consider splitting up the name attribute into a firstName and lastName, because otherwise it could be difficult to compare two peoples' last names, for example.

Representing age as a String is not the best fit. Age is usually a whole number, i.e. an integer, so it would be better to use int instead. Then you could also more easily compare ages by e.g. calculating the age difference between two people. Strings should be used for things that are actual text or at least sequences of characters.

Even better than having an age attribute at all could be to have a dateOfBirth instead, represented by the Java type Date. Age is just an implicit attribute of a person that could change any day, and so instead of saving it in a variable you can calculate it from the date of birth and the current date whenever you need.

I'd say that representing address as a String is fine, but could be worth getting its own class with street, zipCode etc., then saving a reference to an Address inside the Person.


public boolean isAttributeEqual(final People dupPeop)

isAttributeEqual does not tell you which attribute it checks. The singular name implies that one attribute is being checked, but what the method actually does is compare all attributes. There is a convention for that in Java, which is to override the equals method which the class inherits from Object.


List<People> asList = Arrays.asList(new People("Sri", "28", "TN"), new People("Sri", "28", "TN"), new People("Sri", "28", "TN"),
    new People("Pri", "28", "TN"));

List<People> groupSimilar = groupSimilar(asList);

It can be more readable if a variable name states what it contains, independently from how it came to be. asList is the operation that creates the list, but that will not be relevant anymore if you use the variable later on in the code. So, assuming the rename of the class that I recommended earlier, you could have something like this:

List<Person> persons

or

List<Person> people

Similarly, groupSimilar groups similar people, but the result is a list of similar people:

List<Person> similarPeople = groupSimilar(people);

or

List<Person> similarlyGroupedPeople = groupSimilar(people);

Also, it seems to me that what groupSimilar does is to find the people that are duplicated in the same list, rather than actually grouping similar people, so I would rename the variable and the method to this:

List<Person> duplicates = findDuplicates(people);

  public static List<People> groupSimilar(final List<People> people)
  {
    List<People> duplicatePeople = new ArrayList<>(people);
    for (Iterator<People> iterator = duplicatePeople.iterator(); iterator.hasNext();)
    {
      People people2 = iterator.next();
      for (People orignalPeople : people)
      {
        if (!orignalPeople.equals(people2) && orignalPeople.isAttributeEqual(people2))
        {
          iterator.remove();
          people2 = iterator.next();
        }
      }
    }
    return duplicatePeople;
  }

Rather than manually controlling the iterator, i.e. checking hasNext() and advancing it with next(), I would use a simple for-each loop for the outer loop, just like your inner loop already is.

Since you are using the default implementation for equals which is inherited from Object, it is equivalent to a reference equality check, which can be done on reference types using the == operator (!= for reference inequality).

So, assuming that you replaced your isAttributeEqual with an overriding implementation of equals, and use ==/!= instead of the default equals, I would rewrite the method like this:

public static List<People> findDuplicates(final List<People> people) {
    List<People> duplicates = new ArrayList<>();

    for (Person person : people) {
        for (Person person2 : people) {
            if (person != person2 && person.equals(person2) {
                duplicates.add(person);
            }
        }
    }

    return duplicates;
}

The code also becomes, in my view, more easily understandable by only adding the people that are verified as duplicates, rather than declaring all as duplicates and removing those that are not.

\$\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.