Code Review for: Slava Zipp

review.md

bits():
My god. The pure clarity of your bits() code stunned me. My solution involved bit twiddling using band and an ugly hack to get the bit length of the number. I was shocked to see your solution. I just sat there for a few minutes staring at the beauty of the thing.

After getting over my shock, I couldn’t figure out how your solution worked, so I ran it through my tests to make sure it did work, and it passed with flying colors! Next, I ran through a couple of examples in my head, 8, 9, 10, and I’m still not sure how you came up with your solution! But staring at the simplicity of your code has been a balm on my bit frazzled mind.

Critique: What do I do after the bliss disappears, and I’m haunted by doubts that I’ll ever be able to write code like that? What about including some tests with your code? It makes it easier for a reviewer to test changes to your code. I think the problem description mentioned possibly including some tests.

I Forgot to mention this in my submission: Why not use the guard N>0 on fib/1 instead of fib/2, after all fib/2 just immediately returns 0 when fib/1 calls fib/2 with N=0?

perimeter():
The problem description was chock full of errors: there were no example representations of the Shapes, and it mentioned an area() function in one part that didn’t exist in the other part. Sloppy work by the course admins. In any case, a seemingly trivial problem. I didn’t use any coordinates for the shapes--just lengths of sides, so your solution was necessarily more complex than mine, needing to call the segmentLength() function to get the lengths.

area():
No issues.

Critique: You could rewrite both area() and perimeter() like this:

perimeter({triangle, P1, P2, P3}) ->
  L1 = segmentLength(P1, P2),
  L2 = segmentLength(P1, P3),
  L3 = segmentLength(P2, P3),
  L1 + L2 + L3;

and only unpack the points in segmentLength():

segmentLength({X1, Y1}, {X2, Y2}) ->
  DX = X2 - X1,
  DY = Y2 - Y1,
  math:sqrt(DX * DX + DY * DY).

Also, you could avoid unpacking the tuple here:

perimeter({circle, {_, _}, R}) ->
  2 * math:pi() * R;

by simply writing:

perimeter({circle, _, R}) ->
  2 * math:pi() * R;

But maybe you intentionally did both those things for documentation purposes?

enclose():
As you know, the tricky part was the triangle. Forgive me if you already know the following: I played around with drawing some triangles and bounding rectangles, and I discovered that the bounding rectangle has one side that is the height of the triangle drawn from the hypotenuse. The other side of the bounding rectangle is just the hypotenuse of the triangle. If you first calculate the Area of the triangle using the formula that you used in your code, you can get the height of the triangle (and one side of the bounding rectangle) using:

     Area = 1/2 * Base * Height

and subbing in the Area, and the length of the hypotenuse in place of Base. I don’t know about getting the coordinates, I just returned {rectangle, H, W}. Is that what you meant by: “we need to use "Rotating calipers" algorithm”?

Critique: Maybe return the bounding rectangle, as calculated above, with blank coordinates? Or, define a new “type” {bounding_rect, H, W} to return?

I enjoyed reading your code. Thanks!