LatLon.java doesn't handle malformed strings or check array length before assigning values

[kennethmcfarland]

Consider the simplistic test code I have provided for you. You can remove these errors by throwing your code in a try catch block.

public class TestLatLon {
    public static void main(String[] args) {
        final String TEST_STR1 = "breakyou";
        final String TEST_STR2 = "1";
        //NumberFormatException
        LatLon testObj = new LatLon(TEST_STR1);
        //ArrayIndexOutOfBoundsException
        LatLon testObj2 = new LatLon(TEST_STR2);
    }
}

[timothyasp]

Thanks for the report! Can you provide more context to this? A crash report, stack-trace, line numbers, etc.

[kennethmcfarland]

The constructor simply doesn't check for exceptions. Lines 10 and 11 specifically. It isn't necessarily wrong, its just fragile .

Here is what I meant about refactoring it to use a try catch block. This code is able to handle any string you throw at it and gracefully use defaults if the string is messed up.

public class LatLon {
   private double latitude;
   private double longitude;

   public LatLon(String latlon) {
      if (latlon.length() == 0 || latlon == null) {
          this.latitude = 0.0;
          this.longitude = 0.0;
      } else {
          try {
             String[] values = latlon.split(",");
                 this.latitude = Double.valueOf(values[0]);
                 this.longitude = Double.valueOf(values[1]);
          } catch(NumberFormatException | ArrayIndexOutOfBoundsException e) {
              this.latitude = 0.0;
              this.longitude = 0.0;
          }
      }
   }

   public LatLon(double latitude, double longitude) {
      this.latitude = latitude;
      this.longitude = longitude;
   }

   public Double getLatitude() {
      return latitude;
   }

   public Double getLongitude() {
      return longitude;
   }
}

I've ran this on it and I can't break it. Tell Kyle I say hello :)

public class TestLatLon {
    public static void main(String[] args) {
        LatLon[] testCases = new LatLon[5];
        testCases[0] = new LatLon("garbage");
        testCases[1] = new LatLon(",");
        testCases[2] = new LatLon("fds789fds");
        testCases[3] = new LatLon("");
            testCases[4] = new LatLon("3.1415,2.71828"); //works as expected

        for(LatLon ll : testCases) {
            System.out.println("Latitude: " + ll.getLatitude() 
                             + " Longitude: " + ll.getLongitude());
        }
    }
}

Console Output:

Latitude: 0.0 Longitude: 0.0
Latitude: 0.0 Longitude: 0.0
Latitude: 0.0 Longitude: 0.0
Latitude: 0.0 Longitude: 0.0
Latitude: 3.1415 Longitude: 2.71828

Once again, not necessarily better, but it handles odd cases so the app wont tank with an exception. At the very least, it handles the possible exceptions in the offending class instead of percolating them back up the call stack.

[timothyasp]

That makes sense - thanks for the detailed explanation! I'll add this code to the new build releasing soon.

If you don't mind me asking, how did you run across this code and this problem?

[kennethmcfarland]

I submitted a job application for the tester position, and ran across this repository after someone else was hired. This was not a use case problem, I just know the method throws exceptions if it doesn't parse correctly.

[kennethmcfarland]

I had some extra time tonight so I've opened a PR to make your life easier. Here is a pr that closes https://github.com/iFixit/iFixitAndroid/pull/280