4
\$\begingroup\$

Let's consider we are a company who sells our products for other companies. In the programme the actors are an Agency (agencies of our company, we have a few in the country, all are listed in AGENCIES in TestMain) and Client.

  • Every Agency has an Address
  • Every Address contents postalCode
  • Every Client has a postal code

Now, in class Distance, the first method takes a postal code of the agency and the postal code of the client, calculates the longitude and latitude for these postal codes (by using DICTIONARY_POSTAL_CODES created in TestMain) and computes the distance. The second method calculates the distance from the client to the furthest agency.

It works, but is it written in a professional way?

import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class Address {

    private String postalCode;
    private String pattern = "[0-9][0-9]-[0-9][0-9][0-9]"; //postal codes for Poland
    Pattern p = Pattern.compile(pattern);

    private void setPostalCode(String pc) {
        Matcher m = p.matcher(pc);
        if (m.find()) {
            postalCode = m.group();
        } else {
            postalCode = "-----";
        }
    }



    public Address(String pc) {
        setPostalCode(pc);
    }

    public String getPostalCode() {
        return postalCode;
    }

}
public class Agency {

    private Address address;

    public Agency(String postalCode) {
        address = new Address(postalCode);

    }


    public Address getAddress(){
        return address;
    }
}
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

public class TestMain {

    // ****************************************************************************************
    // The dictionary of postal codes and corresponding longitude/latitude
    public static Map<String, PairLongLat> createDictionaryPostalCodes() {
        Map<String, PairLongLat> dictionary = new HashMap<>();
        dictionary.put("00-000", new PairLongLat(21.04383, 52.2315136));
        dictionary.put("00-001", new PairLongLat(21.010353, 52.235382));
        dictionary.put("00-002", new PairLongLat(21.0131092, 52.2363414));
        dictionary.put("02-092", new PairLongLat(20.9790658, 52.183422));
        dictionary.put("53-605", new PairLongLat(17.0199851, 51.1088447));
        dictionary.put("61-696", new PairLongLat(16.9488458, 52.4316596));
        return dictionary;
    }

    public final static Map<String, PairLongLat> DICTIONARY_POSTAL_CODES = createDictionaryPostalCodes();

    // ****************************************************************************************
    // List of our agencies
    public static List<Agency> createAgenciesList() {
        List<Agency> agencies = new ArrayList<>();
        agencies.add(new Agency("02-092"));
        agencies.add(new Agency("53-605"));
        agencies.add(new Agency("61-696"));
        return agencies;
    }

    public final static List<Agency> AGENCIES = createAgenciesList();

    public static void main(String[] args) {

    }
}
public class Distance {

    /**
     * 
     * @param pc1
     *            - postal code first
     * @param pc2
     *            - postal code second
     * @return the euclidean distance between 2 postal codes
     */
    public double getDistance(String pc1, String pc2) {

        PairLongLat coordinates1, coordinates2;
        double long1, long2, lat1, lat2;

        coordinates1 = TestMain.DICTIONARY_POSTAL_CODES.get(pc1);
        coordinates2 = TestMain.DICTIONARY_POSTAL_CODES.get(pc2);

        long1 = coordinates1.getLongitude();
        long2 = coordinates2.getLongitude();
        lat1 = coordinates1.getLatitude();
        lat2 = coordinates2.getLatitude();

        return Math.sqrt((long1 - long2) * (long1 - long2) + (lat1 - lat2) * (lat1 - lat2));
    }

/**
     * 
     * @param pcRegCli
     *            - postal code of client
     * @return the maximum distance between (all distances between agency and
     *         client's postal code) and (client's postal code).
     */
public double getDistanceMax(String pcRegCli) {

    double distanceMax = 0;
    double tmp = 0;

    for (Agency a : TestMain.AGENCIES) {

        String pcAgency = a.getAddress().getPostalCode(); // Postal Code of
                                                            // EFL's agency
        tmp = getDistance(pcRegCli, pcAgency);

        if (tmp > distanceMax)
            distanceMax = tmp;
    }

    return distanceMax;
}
}
/**
/* this class PairLongLat is used only in creating DICTIONARY_POSTAL_CODES
/* pairs are the values of HashMap
*/

public class PairLongLat {

    private double longitude, latitude;

    public PairLongLat(double lon, double lat) {
        longitude = lon;
        latitude = lat;
    }

    public double getLongitude() {
        return longitude;
    }


    public double getLatitude() {
        return latitude;
    }


}
import static org.junit.Assert.assertEquals;

import org.junit.Test;

public class TestUnit {

    @Test
    public void testGetDistance() {

        Distance da = new Distance();

        assertEquals(0.033699763316084305, da.getDistance("00-000", "00-001"), 0);
        assertEquals(da.getDistance("00-000", "00-001"), da.getDistance("00-001", "00-000"), 0);
        assertEquals(4.09987243942609, da.getDistance("00-000", "61-696"), 0);

    }

@Test
    public void testGetDistanceMax() {

    DistanceAfs da = new DistanceAfs();

    assertEquals(4.177524774107656, da.getDistanceMax("00-000"), 0);
    assertEquals(4.146338416680638, da.getDistanceMax("00-001"), 0);
    assertEquals(4.149251605592471, da.getDistanceMax("00-002"), 0);

}
}
\$\endgroup\$
4
\$\begingroup\$

What's the point of writing "professional looking" code if the algorithm is flawed to begin with?

One degree of latitude is not equal to one degree of longitude, distance-wise, in Poland. The algorithm would be fine around the equator. Otherwise, multiply long by a factor of cos(lat). (Think about the extreme case: what distance is one degree of longitude on the north pole?)

Also, when only looking at a ranking of distances, you might drop the sqrt and sort by distance squared - the ranking will be the same.

As for the structure of the whole thing, I would put the distance calculation function in the coordinate class. The Distance class as it stands now doesn't actually represent anything. Looking at the function signatures the Distance class or its functions don't have much use anywhere else. Change it so you would be able to do: coord1.distanceTo(coord2) (which could return a value in km) and/or coord1.angleDistanceTo(coord2) (which could return a value in deg) and/or coord1.squaredAngleDistanceTo(coord2) (which could return the raw dlat*dlat+dlon*dlon*cos(lat)*cos(lat) you need).

In a perfect world I would make all three and let them depend on eachother: distanceTo() uses the value calculated by angleDistanceTo() which uses the value calculated by squaredAngleDistanceTo(). This way, any improvement to part of the algorithm, applies to all functions at once. (E.g. when you would calculate the arc length considering the actual shape of the earth, or the fact that cos(lat) may be different for the start and end of the line). Also, you would get optimal performance as you can pick the function which adds the least amount of overhead to your algorithm.

Regarding the coordinate table: I would make a separate class which then could use static values, values read from a file, database or web service without touching other code. It could even combine various sources. All that is seen by your other code would be a "CoordinateTable". The logical next step would be to add functions to this class to get a list, an iterator, a search function. In the end your code could look like: CoordinateTable.loadStatic();CoordinateTable.loadFromStream(....); CoordinateTable.findPlacesNear(coord); You don't want to actually do a lot now to allow for other sources (because YAGNI) but you can make some choices that would make such a change later on a bit less painful. Looking at the broader perspective: the Distance as you had it, did not actually represent anything, but the table actually does, so it deserves its own class that can be easily extended in the future.

\$\endgroup\$
  • \$\begingroup\$ With regard to point 3, I can't think of any reason to prefer comparing or using smaller scalar doubles in a language like Java where everything is the same width anyways -- is there one? \$\endgroup\$ – cat Jun 1 '17 at 1:20
  • \$\begingroup\$ I think you're misunderstanding what I mean here; the reason to drop the sqrt step, is that it doesn't add anything except a performance penalty. \$\endgroup\$ – mvds Jun 1 '17 at 1:24
  • 1
    \$\begingroup\$ (and if you would keep the sqrt in to get an actual distance measure, I would calculate an estimate in km by mulitplying the angle with the radius of the earth) \$\endgroup\$ – mvds Jun 1 '17 at 1:25
  • \$\begingroup\$ Right, and I'm saying that there can't be any noticeable gain from working with smaller scalars in Java -- maybe in C, you care about memory usage and you are going to (float) the result of sqrting a long double or something to save 48 bits per scalar. \$\endgroup\$ – cat Jun 1 '17 at 1:28
  • \$\begingroup\$ Still don't understand what you mean. Performing a square root costs time and energy. I'm not thinking about float, double, bits, or whatever, it's just a useless step in the algorithm as it's posted here. I've done a lot of these kind of algorithms and most of the times you don't actually need the sqrt. \$\endgroup\$ – mvds Jun 1 '17 at 1:32
2
\$\begingroup\$

I have some suggestions:

In Address you should move the constructor before the setPostalCode method.

The pattern is a constant, it should be static and final. Also I'd rename it to POSTAL_CODE_PATTERN.

Pattern p should be private. p is a meaningless name, a good name could be compiledPostalCodePattern.

Please remove decorations from the code (I mean the // ******* lines): a single empty line is already a good separator.

PairLongLat is really a bad name: what represent the pair? Coordinates, so you should name it Coordinates.

I'd extract the euclidean distance cal in a method that works with two Coordinates object:

public double getDistance(String postalCode1, String postalCode2) {
    final Coordinates c1 = TestMain.DICTIONARY_POSTAL_CODES.get(postalCode1);
    final Coordinates c2 = TestMain.DICTIONARY_POSTAL_CODES.get(postalCode2);

    return calculateEuclideanDistance(c1, c2); 
}

private double calculateEuclideanDistance(Coordinates c1, Coordinates c2) {
    ...
}

I don't like the use of TestMain.DICTIONARY_POSTAL_CODES, you should inject your mapping in Distance class (I'd rename it to DistanceCalculator) using the constructor.

\$\endgroup\$
  • \$\begingroup\$ thank you! Are ya sure it is better to put DICTIONARY_POSTAL_CODES in constructor of DistanceCalculator ? What if there will be more classes which want to use DICTIONARY_POSTAL_CODES ? Then I need to put that in all constructors in each class. \$\endgroup\$ – wBacz May 31 '17 at 21:05
  • \$\begingroup\$ maybe make an interface with predefined DICTIONARY_POSTAL_CODES method? \$\endgroup\$ – wBacz May 31 '17 at 21:06
  • \$\begingroup\$ @wBacz In that case you should pass the DICTIONARY_POSTAL_CODES (a dependency) to all objects that need it. See [What is dependency injection? ](stackoverflow.com/questions/130794/what-is-dependency-injection) The goal is to have objects easier to test and easier to extend. \$\endgroup\$ – user131519 Jun 2 '17 at 7:18
  • \$\begingroup\$ Thank you! So i guess I should make a singleton and inject it in classes which need it... \$\endgroup\$ – wBacz Jun 2 '17 at 19:05
2
\$\begingroup\$

My two cents:

  • String pattern: I think you can do something like "[0-9]{5}".
  • "-----": I think you should throw an Exception, because the app will fail otherwise.
  • Agency constructor: Better pass the Address directly, the Agency should not care about the creation of an Address
  • As Alessandro mentioned: Coordinates would be a more suitable name, instead of PairLongLat
  • Distance.getDistance(): There's a few points. 1): This method can be static - you don't need an instance of Distance to calculate. 2) You're actually passing postal codes. I would have expected to pass the coordinates directly. I don't think the Distance class should have a dependency to Coordinate. 3) calculateDistance fits better.
\$\endgroup\$
  • \$\begingroup\$ the pattern is like:: 54-789 there is a sign - :) Thank you. \$\endgroup\$ – wBacz May 31 '17 at 21:33
  • \$\begingroup\$ Errr, didn't see that. So I'd go with something like "[0-9]{2}-[0-9]{3}". So the next reader actually sees the dash better than me ;-) \$\endgroup\$ – slowy May 31 '17 at 21:36
  • \$\begingroup\$ If I do like ya advice in the last point at 2) then where the programme should convert from postal codes to coordinates? \$\endgroup\$ – wBacz May 31 '17 at 21:37

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.