1
\$\begingroup\$

Looking for critiques on my implementation of a list of countries that offers many ways to sort it in a useful and easy way for a client. The main concern is about the design of the enum that holds the comparators and the way they are provided to the client (in this case, AlliancesExample class). More on this in the comments of the code.

I am not allowed to use Java 8 so although I know all of my comparators admit a lambda expression, I can't use them.

Any comments or suggestions on my approach, coding style, conventions, etc would be greatly appreciated! Thanks in advance.

public enum Continent {
    ASIA("Asia"),
    AFRICA("Africa"),
    NORTH_AMERICA("North America"),
    SOUTH_AMERICA("South America"),
    ANTARCTICA("Antarctica"),
    EUROPE("Europe"),
    AUSTRALIA("Australia");

    private final String name;

    Continent(String name) {
        this.name = name;
    }

    @Override
    public String toString() {
        return this.name;
    }
}

import java.util.Comparator;

// Country implements Comparable just in case we want an ordered set of countries
// The comparison is made on ccTLD

public class Country implements Comparable<Country> {
    private final String ccTLD; //UNIQUE, like a Primary Key
    private final String name;
    private final Continent continent;
    private int population;
    private int sizeSquareKiloMeters;

    public Country(String ccTLD, String name, Continent continent, int population, int sizeSquareKiloMeters) {
        this.ccTLD = ccTLD;
        this.name = name;
        this.continent = continent;
        this.population = population;
        this.sizeSquareKiloMeters = sizeSquareKiloMeters;
    }

    //Here is one issue, I offer a dummy constructor to search by ccTLD a list of countries
    public Country(String ccTLD) {
        this.ccTLD = ccTLD;
        this.name = null;
        this.continent = null;
        this.population = 0;
        this.sizeSquareKiloMeters = 0;
    }

    //Its own public enum or inner enum?
    public enum SortingOrder {
        CCTLD(new Comparator<Country>() {
            @Override
            public int compare(Country o1, Country o2) {
                return o1.getCcTLD().compareTo(o2.getCcTLD());
            }
        }),
        NAME(new Comparator<Country>() {
            @Override
            public int compare(Country o1, Country o2) {
                return o1.name.compareTo(o2.getName());
            }
        }),
        CONTINENT(new Comparator<Country>() {
            @Override
            public int compare(Country o1, Country o2) {
                if (o1.getContinent().equals(o2.getContinent())) {
                    return o1.getCcTLD().compareTo(o2.getCcTLD());
                }
                return o1.getContinent().compareTo(o2.getContinent());
            }
        }),
        POPULATION(new Comparator<Country>() {
            @Override
            public int compare(Country o1, Country o2) {
                return Integer.compare(o1.getPopulation(), o2.getPopulation());
            }
        });

        //More comparators

        Comparator<Country> comparator;

        SortingOrder(Comparator<Country> comparator) {

            this.comparator = comparator;

        }

        public Comparator<Country> getComparatorDescending() {
            return comparator.reversed();
        }

        public Comparator<Country> getComparatorAscending() {
            return comparator;
        }
    }

    //Equals only looks ccTLD because is the PK
    //IntelliJ IDEA template
    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;

        Country country = (Country) o;

        return ccTLD.equals(country.ccTLD);
    }

    @Override
    public int hashCode() {
        return ccTLD.hashCode();
    }

    @Override
    public int compareTo(Country o) {
        return this.ccTLD.compareTo(o.ccTLD);
    }

    @Override
    public String toString() {
        return "Country{" +
                "ccTLD='" + ccTLD + '\'' +
                ", name='" + name + '\'' +
                ", continent=" + continent +
                ", population=" + population +
                ", sizeSquareKiloMeters=" + sizeSquareKiloMeters +
                '}';
    }

    public String getStringFullData() {
        return getCcTLD() +
                " - " + getName() +
                " - " + getContinent().toString().toUpperCase() +
                "  " + getPopulation() + " pop. " +
                "  " + getSizeSquareKiloMeters() + " km\u00B2";
    }

    public String getCcTLD() {
        return ccTLD;
    }

    public String getName() {
        return name;
    }

    public Continent getContinent() {
        return continent;
    }

    public int getPopulation() {
        return population;
    }

    public void setPopulation(int population) {
        this.population = population;
    }

    public int getSizeSquareKiloMeters() {
        return sizeSquareKiloMeters;
    }

    public void setSizeSquareKiloMeters(int sizeSquareKiloMeters) {
        this.sizeSquareKiloMeters = sizeSquareKiloMeters;
    }
}

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

public class Alliance {
    private final List<Country> countries = new ArrayList<Country>();
    private final String name;

    public Alliance(String name) {
        this.name = name;
    }

    //It could be a set but I want to sort the list many times and in many ways
    public boolean add(Country c) {
        if (this.contains(c)) {
            return false;
        }
        return this.countries.add(c);
    }

    public boolean remove(Country c) {
        return this.countries.remove(c);
    }

    public boolean contains(Country c) {
        return this.countries.contains(c);
    }

    public Country getByCcTLD(String cctld) {
        for (Country c :
                this.countries) {
            if (c.getCcTLD().equals(cctld)) {
                return c;
            }
        }
        return null;
    }


    //Should I offer the client a generic sort where they pass me the comparator
    //they want to use or encapsulate sort options like this?
    public void sortByName() {
        Collections.sort(this.countries, Country.SortingOrder.NAME.getComparatorAscending());
    }

    public void sortByPopulationDESC() {
        Collections.sort(this.countries, Country.SortingOrder.POPULATION.getComparatorDescending());
    }

    public String getListOfCountries() {
        String output = this.name + "\n";
        for (Country c :
                this.countries) {
            output += c.getStringFullData() + "\n";
        }
        return output;
    }

    public String getName() {
        return name;
    }
}

public class AlliancesExample {
    public static void main(String[] args) {
        Alliance nafta = new Alliance("North American Free Trade Agreement (NAFTA)");
        Country usa = new Country("us",
                "United States of America",
                Continent.NORTH_AMERICA,
                325_719_170,
                9_833_543);
        Country canada = new Country("ca",
                "Canada",
                Continent.NORTH_AMERICA,
                35_151_728,
                9_984_670);
        Country mexico = new Country("mx",
                "Mexico",
                Continent.NORTH_AMERICA,
                123_675_325,
                1_972_550);
        nafta.add(usa);
        nafta.add(canada);
        nafta.add(mexico);
        System.out.println(nafta.getListOfCountries());
        nafta.sortByName();
        System.out.println(nafta.getListOfCountries());
        nafta.sortByPopulationDESC();
        System.out.println(nafta.getListOfCountries());

        Country c = nafta.getByCcTLD("mx");
        if (c != null) {
            System.out.println(c.getName() + " belongs to " + nafta.getName());
            System.out.println("And its data are: " + c.getStringFullData());
        }

    }
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ You might make the field comparator public final. And reversed() only exists since java 8. That and maybe thenComparing you could backport to a utility class as functions. \$\endgroup\$
    – Joop Eggen
    Commented Feb 2, 2018 at 10:48

1 Answer 1

2
\$\begingroup\$

One tip I can give is that enums can implement interfaces. So you can let SortingOrder implement Comparator<Country> like this:

public enum SortingOrder implements Comparator<Country> {
    CCTLD {
        @Override
        public int compare(Country o1, Country o2) {
            return o1.getCcTLD().compareTo(o2.getCcTLD());
        }
    },
    NAME {
        @Override
        public int compare(Country o1, Country o2) {
            return o1.name.compareTo(o2.getName());
        }
    },
    CONTINENT {
        @Override
        public int compare(Country o1, Country o2) {
            if (o1.getContinent().equals(o2.getContinent())) {
                return o1.getCcTLD().compareTo(o2.getCcTLD());
            }
            return o1.getContinent().compareTo(o2.getContinent());
        }
    },
    POPULATION {
        @Override
        public int compare(Country o1, Country o2) {
            return Integer.compare(o1.getPopulation(), o2.getPopulation());
        }
    };

    public Comparator<Country> reversed() {
        return Collections.reverseOrder(this);
    }
}

That saves a few characters in the implementation, and also when using it:

Collections.sort(this.countries, Country.SortingOrder.NAME);

Collections.sort(this.countries, Country.SortingOrder.POPULATION.reversed());

I think it's also good to have SortingOrder as a nested enum. If you were to put it separately, you would probably have to rename it CountrySortingOrder any ways to avoid confusion.


I'm not a fan, however, of having Country implement Comparable, since there is no inherent ordering for Countrys, as evidenced by the multiple different sorting orders. Your comment says you have it to be able to use ordered sets, but those often allow a separate comparator to be used any ways (for instance TreeSet).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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