Any defect in the attached code

[Arun Chidam]

HI GUys,

Can you spot any issues on the below code apart from the one i spotted(in bold)...

import java.util.*;

public class Sort {

    public static ArrayList<Name> sort(ArrayList<Name> names) {

        ArrayList<Name> results;

        results = new ArrayList<Name>();

        do {

            Name firstName = new Name("zzz", "zzz");
            for (int i = 0; i < names.size(); i++)
//For better readability open and close with curly braces, also change the For loop to Java1.5 enhanced For loop as shown below
            {
                if (names.get(i).getFirstName().compareTo(firstName.getFirstName()) < 0
                        || names.get(i).getFirstName() == firstName.getFirstName()
//Change it to .equals inorder to compare the content of the value rather than reference of object,
//say for new Name(new String("zzz"),new String("zzz")) the condition always remain false   
                    && names.get(i).getSurName().compareTo(firstName.getSurName()) < 0)
//For better readability open and close with curly braces
                {
                    firstName = new Name(names.get(i).getFirstName(), names.get(i).getSurName());
                }
            }

            results.add(firstName);

            names.remove(firstName);

        } while (names.size() > 0);
//Do-While will work for Name("azz","zzz") but will run into infinite loop for 
// Name("zzz1","zzz2") because lexicographically comparison returns value > 0
// due to length of 2nd string, so change the do-while condition as per the below code which initialize
// the count to array size and later checks and decrements for each completed loop before terminating at zero

        return results;
    }

    //Reviewed and replaced as per the above comments
    public static ArrayList<Name> sort1(ArrayList<Name> names) {

        ArrayList<Name> results;

        results = new ArrayList<Name>();

        int count = names.size();
        do {

            Name firstName = new Name("zzz", "zzz");
            for (Name name : names)
            {
                if (name.getFirstName().compareTo(firstName.getFirstName()) < 0
                        || name.getFirstName().equals(firstName.getFirstName())
                        && name.getSurName().compareTo(firstName.getSurName()) < 0)
                {
                    firstName = new Name(name.getFirstName(), name.getSurName());
                }
            }

            results.add(firstName);

            names.remove(firstName);

            count--;

        } while (count > 0);

        return results;
    }
}

class Name {

    String firstName;
    String surName;

    public Name() {
    }

    public Name(String firstName, String surName) {

        this.firstName = firstName;
        this.surName = surName;
    }

    public String getFirstName() {
        return firstName;
    }

    public void setFirstName(String firstName) {
        this.firstName = firstName;
    }

    public String getSurName() {
        return surName;
    }

    public void setSurName(String surName) {
        this.surName = surName;
    }

    public String toString() {
        return firstName + " " + surName;
    }

    public boolean equals(Object other) {
        return firstName == ((Name) other).firstName && surName == ((Name) other).surName;
//== operator compare's the reference of the object rather than the contents of the string value,
//below commented code is the better option
        // String fname = ((Name)other).firstName;
        // String sname = ((Name)other).surName;
        // if(firstName.equals(fname) && surName.equals(sname)){
            // return true;
        // }else{
            // return false;
        // }
    }
}

[Stephan van Hulst]

It looks like someone is already reviewing this code for you. Why do you want us to look at it?

[Arun Chidam]

Stephan van Hulst wrote:It looks like someone is already reviewing this code for you. Why do you want us to look at it?

Yes it was me who reviewed the code, I want to know if anything i missed in my review???

[Ulf Dittmer]

What kind of issues?

[Stephan van Hulst]

And please just post the fixed code. Not much point it looking for mistakes in code that is obviously mistaken.

[Arun Chidam]

Stephan van Hulst wrote:And please just post the fixed code. Not much point it looking for mistakes in code that is obviously mistaken.

Updated the fixed code....

import java.util.*;

public class Sort {


    public static ArrayList<Name> sort(ArrayList<Name> names) {

        ArrayList<Name> results;

        results = new ArrayList<Name>();

        int count = names.size();
        do {

            Name firstName = new Name("zzz", "zzz");
            for (Name name : names)
            {
                if (name.getFirstName().compareTo(firstName.getFirstName()) < 0
                        || name.getFirstName().equals(firstName.getFirstName())
                        && name.getSurName().compareTo(firstName.getSurName()) < 0)
                {
                    firstName = new Name(name.getFirstName(), name.getSurName());
                }
            }

            results.add(firstName);

            names.remove(firstName);

            count--;

        } while (count > 0);

        return results;
    }
}

class Name {

    String firstName;
    String surName;

    public Name() {
    }

    public Name(String firstName, String surName) {

        this.firstName = firstName;
        this.surName = surName;
    }

    public String getFirstName() {
        return firstName;
    }

    public void setFirstName(String firstName) {
        this.firstName = firstName;
    }

    public String getSurName() {
        return surName;
    }

    public void setSurName(String surName) {
        this.surName = surName;
    }

    public String toString() {
        return firstName + " " + surName;
    }

    public boolean equals(Object other) {
         String fname = ((Name)other).firstName;
         String sname = ((Name)other).surName;
         if(firstName.equals(fname) && surName.equals(sname)){
             return true;
         }else{
             return false;
         }
    }
}

[Arun Chidam]

Ulf Dittmer wrote:What kind of issues?

i am looking for runtime error(if any)?
Also is there any better way of coding the sort method?

[Stephan van Hulst]

Well, you can just make your Name class implement Comparable, and then use the Collections.sort() method.

I assume you're implementing your own sort method as an exercise though. Anyway, there are several things you could consider wrong. Your method

1) is destructive, it destroys the list you pass into it. This is not considered very wanted behavior.
2) discards names that are 'greater' than "zzz, zzz", and inserts this name instead.
3) creates new instances for each name in the list, and adds them to the new list. Why not add the names directly?
4) uses a do-while loop. Don't use do-while loops. Use while loops instead.

Your Name class implements equals() wrongly, and on top of that, it doesn't implement hashCode(). Read the contract for Object.equals() and Object.hashCode().

[Arun Chidam]

HI stephan, thanks a lot for your comments....your reviews are very helpful