Request for review of Polyline.java

[GoogleCodeExporter]

Purpose of code changes on this branch:
Added decoding of encoded polyline and storing of decoded coordinates.

When reviewing my code changes, please focus on:
Overall style and code structure.
Method decodeLine() is a translation from a Google Utility written in 
javascript, so it may be a point of concern.

Original issue reported on code.google.com by mne...@gmail.com on 23 Feb 2010 at 7:19

[GoogleCodeExporter]

CODE REVIEW - Bus Stop Alarm
Date: Feb 23, 2010
File: Polyline.java
Revision: 117
Author: Michael Eng
Reviewer: Derek Cheng

Summary:

This is a first review of Polyline.java. Since last change, the author has 
added 
decoding of encoded line and storing of decoded coordinates. The entire file is 
about 
150 lines long and contains implementation of polyline encoding as well as 
decoding. 
This review will focus on code formatting, adherence to Java/Android coding 
guidelines, and object-oriented design principles. Author has requested focus 
be 
placed on overall style and code structure, as well as correctness of the 
Javascript 
to Java translation of the decodeLine() method.

Comments:

CODING STANDARDS

I can understand what decodeLine() is doing, but it would be helpful to briefly 
document some of the things it's doing there. For example, why are we 
subtracting 63 
when we read a character from the string each time? How is the encoded line 
actually 
formatted to ensure we don't get index out of bounds error or not get overflows 
in 
the coordinates from shifting too many times?

COMMENTS

It seems there are adequate comments for your file. The only small concern I 
have, 
which should only matter if we are generating a Javadocs page from our 
comments, is 
that some methods have "@return" which some other have "@returns". Whichever 
one is 
incorrect may not be parsed correctly, so it might be worthwhile to fix this 
for sake 
of consistency. ("@return" is the correct one, so only the next() method in 
PolylineIterator needs to be fixed)

LOGIC

decodeLine() should be idempotent - multiple calls to it should give the same 
result. 
(We might not actually do this, but just in case we might, and the Javascript 
version 
is idempotent anyways) To do this, coordinates should be cleared of its content 
as 
the first step of the method.

ERROR HANDLING

If encodedLevels only contains "B"s, then it might be worthwhile to check for 
the 
validity of the given String encodedLevel argument just so we won't be able to 
pass 
in any arbitrary Strings that could possibly be invalid there.

CODE DECISIONS

There are two constructors for Polyline, but only the first one initializes 
coordinates, the ArrayList of Geopoints, and fills it using decodeLine(). 
Polyline 
objects created using the second constructor will have null for coordinates, 
which 
might cause problems if we try to get its PolylineIterator.

Original comment by chen...@gmail.com on 24 Feb 2010 at 6:44

[GoogleCodeExporter]

Verified Derek's code review.

Original comment by mne...@gmail.com on 2 Mar 2010 at 6:09

• Changed state: Done