Code review for: Joel Potischman

review.md

perimeter():
area():

Well done. Straight forward. Thanks for including tests! It makes it easier to review code.

Critique: The future learn web site is not equipped to handle computer programmers. To preserve the formatting of your code and make it easier to read, consider posting your code someplace off site and include a link. As the next video mentions, you can post to a place called hastebin, which will provide a url, and you don't even need to log in. Or, you can open an account here at github or somewhere similar.

Another suggestion: I thought that writing a test when a float was the return value from one of the functions was a little tricky. I see that you used some floats in your tests. I used the following trick, I don't know if it's the best way, but maybe you will find it useful:

    Result = round(6.2831 * 1000),  %Convert float to an integer, preserving a few 
                                    %decimal places => 6283
    %Now the test:
    Result = round(
        perimeter({circle, 1}) * 1000  %Convert perimeter result to an integer,
    ),                                 %preserving a few decimal places => 6283

The problem with comparing floats is that you can get small discrepencies like .0000001. round() takes care of that--after you multiply by something like 1000 to preserve the number of decimal places of accurracy that you want to compare.

enclose():

Nice, simple solution for right triangles.

Critique: If you are interested in a general solution:

I ended up drawing all kinds of triangles, and then drawing a bounding box around them. That led me to a discovery: the height of the triangle drawn from the hypotenuse--is the same length as one side of the bounding box; and the hypotenuse is the other side of the bounding box. Try drawing a few triangles to see what I mean. Using an obtuse triangle, where the angle opposite the hypontenuse is greater than 90 degrees, will prove enlightening: think slender triangles with two sides like this:

\ 
 \
  \ 
   \_ _ _

If you draw a height line (from the hypotenuse), then slide a height line out to each endpoint of the hypotenuse, then connect the ends of the height lines (which will be a line parallel to the hypotenuse), that is your smallest bounding box.

I defined a triangle like this:

{triangle, A, B, Hyp}

That way, I didn’t have to go through the drudgery of determining which side was the longest of the three sides. You also need to provide some guards so that the three sides can actually form a triangle in order for this to work: any pair of sides needs to be bigger than the other side, so: A+B > Hyp, B+Hyp > A, Hyp+A > B. Therefore, (1, 1, 2) is not a legal triangle because the first two sides are not long enough to form an arch over the hypotenuse of length 2: they would have to be laid along the hypotenuse to connect to each other, forming two lines on top of each other--not a triangle.

Okay, for the bounding box: one side is the hypotenuse of the triangle, which you have in the variable Hyp, and the other side is equal to the height of the triangle drawn from the hypotenuse. You can calculate the height using this triangle formula:

  Area = 1/2 * Base * Height

Rearranging you get:

  Area*2/Base = Height

Again, the Base will be Hyp and you can calculate the area of any triangle with this formula:

S = (A+B+Hyp)/2,  
Area = math:sqrt( S*(S-A)*(S-B)*(S-Hyp) )

Back to the Height formula: if you plug in the value for the Area that you calculated and replace Base with Hyp you can calculate the Height. Again, the Height has the same length as one side of the bounding box. Then you can return a rectangle tuple containing Hyp and Height.

bits():

Wow! Clap, clap, clap! Standing ovation! A+.

I've reviewed two other people's solutions, and they both used only div and rem, which was extremely elegant, and a stunning revelation to me because I also used band. But, the logic of their very short, compact code was hard for me to reason about. When I looked at the div/rem solutions, it didn't seem like the code could possibly work, and I had to mentally try a few numbers to confirm that the solution did work. Your solution, on the other hand, was so easy to read and understand it made me smile. :)

My own solution using band was really tortured and I wasn't happy with it. I started off trying to figure out how to slide a bit off the right hand side of N, and for some reason I tried to subtract 2, and I couldn't get it to work. As a result, I took the approach of moving a 1 bit to the left, one step step at a time, and band'ing with N, which involved raising to powers of 2 to move the 1 bit to the left. Now, you would think that if I could move the 1 bit to the left by raising to powers of 2, I would have realized that I could slide a bit off the right side of N by dividing by 2, but no! So seeing your solution do exactly that caused a light bulb to go off in my head.

Critique: The exercise asked for both a non tail recursive solution and a tail recursive solution for bits(). I don't see a non-tail recursive solution. Also, the problem description said to define bits() to handle positive integers. I tested your solution with a negative number, and it didn't provide the correct answer (-17 => 2), so you need to add a guard to your definition of bits().

In your tests for bits(), you could have used binary syntax for the N's:

2#10001

In one of the videos, Simon introduced the syntax, so it was covered in class. In your tests, you obviously picked specific numbers to test the bits in various positions, so you could have made it patently obvious what you were doing, like this:

    0 = bits(2#0), 
    1 = bits(2#100000), 
    8 = bits(2#11111111), 
    1 = bits(2#100000000), 

(In ruby, you can use the binary syntax 0b1000_0000 for an integer, which is even easier to read. I wish erlang allowed you to use an underscore separator!)

And, you could have included one test with alternating 1's and 0's, say

3 = bits(2#10101)

just to mix things up and more rigoriously test your code.

The binary syntax has an added benefit: anyone can look at the right hand side of a pattern match in your tests and know exactly what the answer should be by summing the 1's in their head.

You are writing erlang for Great Good! I'm honored to get to review such nice code. See you in class!

Thanks for the feedback! Very much appreciated!

I definitely cut some corners due to my time constraints this week, hence no non-tail call version of bits, assumptions about triangle geometry, and being sloppy and failing to notice I hadn't guarded against negative integers. I will definitely go back and incorporate most of your changes for my own edification tonight. Thanks again!