Tech:Reviewing Code/Example2
From Cyclopath
up to: Tech:Reviewing Code
Date: Wed, 23 Jan 2008 17:41:55 -0600
From: Reid Priedhorsky <reid@umn.edu>
To: xxx
Subject: detailed feedback on turn-by-turn directions
Hi xxx,
Here's some more feedback on the turn-by-turn direction stuff. As
always, let me know if you have questions or disagree.
> Index: flashclient/Conf.as
> ===================================================================
> --- flashclient/Conf.as (revision 6022)
> +++ flashclient/Conf.as (revision 6023)
> @@ -175,6 +175,53 @@
> 3: 'Good',
> 4: 'Excellent' };
>
> + // Turn-by-turn direction stuff
> + public static const min:int = 0;
> + public static const max:int = 1;
What do these constants mean? Please rename them to be more clear.
> + // (min, max) angles of the directions in degrees
> + public static const angles_east:Array = new Array(350, 22.5);
> + public static const angles_north_east:Array = new Array(22.5, 85);
> + public static const angles_north:Array = new Array(85, 95);
Regarding this part, I have a few notes:
1. Let's use the terms "compass bearing" (cbearing for short?) and
"relative bearing", to be more clear. This also eliminates the concept
of e.g. "relative" north which is a bit strange.
2. I think you can reformulate these constants to be more concise, for
example:
public static const cbearing_pretty = [['N', 22.5],
['NW', 67.5],
...
['NE', 337.5]
['N', 360 ]];
The number is the maximum bearing with that name, and you'd do another
one for the relative bearing names. Note that the bearing crossing 0 has
to be in the table twice. (Numbers aren't correct, but double-check
yours too.) Don't take this particular example as gospel, but I think
something along these lines would be better.
This yields a simple loop over the table rather than a messy if/else tree.
> + // This is the minimum distance in meters that a byway of the same name
> + // as the previous traveling in a different direction must be to avoid
> + // being compressed into the last one.
> + public static const direction_compress_min:Number = 50;
This should be as small as possible; I know you had a specific example
which motivated this -- how small can the above be and still work with
your example?
> Index: flashclient/G.as
> ===================================================================
> --- flashclient/G.as (revision 6022)
> +++ flashclient/G.as (revision 6023)
> + if (m.width > width) {
> + // the word is longer than the allowed width, so start adding
> + // the letters one at a time.
> + letters = word.split('');
I like to prefer putting the smaller (i.e., less code) cases first so
that the average distance from each line of code to the beginning of the
if/else is less. This needs to be balanced against other clarity issues,
and I don't know what the choice should be here, but something to think
about.
I like how each case is concisely explained by comments, but the wording
could be improved a little. Suggestions below.
> + } else if (m.width + curr_width > width) {
> + // End of the line and we can't fit, so move to the next one
Who is "we"? How about "No room for word on this line, so place it on
the next"?
> + // On the line, so adjust the width by the word size and a space
What's on the line? I think "word fits on line" might be sufficient.
> + curr_width += m.width;
> + if (curr_width + space_width < width)
> + curr_width += space_width;
Is this conditional necessary? What happens if you always increment by
space_width?
> Index: flashclient/Route.as
> ===================================================================
> + public static function length_to_pretty_string(length:Number,
> + decimal:int = 1,
> + units:Boolean = true)
> + :String
> + {
> + if (units)
> + return (length / 1000.0).toFixed(decimal) + ' km';
> + else
> + return (length / 1000.0).toFixed(decimal);
> + }
Let's make this convert to miles (people were asking for it in the
study), though the input (length) should still be meters. My instinct is
to do the conversion once and then append the units, even though this
requires an extra variable.
(Hint: if you're not familiar with the units program, it's excellent.
Say "man units". I find the output much improved with --verbose.)
> + public static function ang_abs(x:Number, y:Number) :Number
Each of these can be trivially implemented in terms of the other, so
pick one to be the "real" implementation (the other should be a line or
two).
> + {
> + var a:Number;
> +
> + if (x == 0 && y >= 0)
> + return 90.0;
> + if (x == 0 && y < 0)
> + return 270.0;
> +
> + a = Math.atan(y / x) * 180 / 3.1415927;
1. What are the constants doing here? Don't put "magic numbers" in code.
(In this case, write a rad-to-degrees function and put it in G.as.)
2. Look into Math.atan2(), it will reduce the complexity of this code.
> +
> + if (x > 0 && y >= 0)
> + return a;
> + if (x < 0 && y >= 0)
> + return a + 180;
> + if (x < 0 && y < 0)
> + return a + 180;
> + if (x > 0 && y < 0)
> + return a + 360;
> + return 0;
> + }
> +
> + // Calculates the angle between <x2,y2> and the x axis of the ortho basis
> + // that has <x1, y1> as its y axis
> + // Return an angle from 0 to 360
You'll want to operate on angles in the interval [0,360), i.e. 0 is
included in the interval but 360 is _not_, because as it is now you have
two valid inputs that have the same meaning (0 and 360). If you think
you might have angles outside this range, write a normalization function
and use that on input (see the Python code for some examples, I believe).
> + public static function ang_rel(x1:Number, y1:Number,
> + x2:Number, y2:Number) :Number
> + {
> + // Calculate the length to normalize <x1, y1> and <y1, -x1>
> + var l:Number = Math.sqrt(x1 * x1 + y1 * y1);
Try to abstract away generic calculations. In this case there is already
a distance function in G.as, but if there wasn't you should write one.
> +
> + // to prevent divide-by-zero
> + if (y1 != 0) {
> + // Returns the absolute angle of the transformed vector <x2, y2>'
> + // which is [[y1/l, x1/l][-x1/l, y1/l]]^-1 * <x2, y2>
> + return ang_abs(l * x2 / y1 - x1 * (x1 * x2 + y1 * y2) / (l * y1),
> + (x1 * x2 + y1 * y2) / l);
> + } else if (x1 > 0)
> + // The appropriate transform of <x2, y2> if y1 == 0 and x1 > 0
> + return ang_abs(-y2, x2);
> + else if (x1 < 0)
> + // The appropriate transform of <x2, y2> if y1 == 0 and x1 < 0
> + return ang_abs(y2, -x2);
> + else
> + // Should never happen, hopefully
> + G.assert(false);
Good assert! I think you can omit the comment, though (the meaning is
clear without it). To be even better, just omit the final else and put
the assert before the return.
> + return 0;
Put a comment that the return is there just to satisfy the compiler,
because it can't be reached.
I think that's all I have for now. Thanks for all your hard work!
Reid
