Code Review for: Reiner Rodriguez

review.md

perimeter():
area():

I think you should include tests with your code so that reviewers can make proposed changes and run the tests to make sure everything still works. I apologize if you already know how to create tests.

You can create a test by figuring out in your head what, say, the perimeter of a square with side 3 is, i.e. 12, and then seeing if your code produces the same result:

perimeter_tests() ->
    12 = perimeter({square, 3}),
    all_tests_passed.

Then you can insert more tests for the other shapes before the last line. It’s really easy. Then in the shell, you just run:

1> modname:perimeter_tests().
all_tests_passed

Or, you may get an error on one of the lines in the perimeter_tests() function, and then you can check that line to see what calculation failed, then try and track down the error.

The only tricky part is when you get a float for an answer. Here is how you can deal with that:

    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

I put a test function right above each function:

perimeter_tests() ->
   ...
 
enclose_tests() ->
   ...
   
bits_tests() ->
   ...

I run the test function while I'm working on the function. Then, when I'm all done, and I'm just making final tweaks here and there, I write the following at the top of my program:

all_tests() ->
    perimeter_tests(),
    enclose_tests(),
    bits_tests().

Then in the shell, I can write:

3> modname:all_tests().

I think the problem description suggested that you include tests.

I noticed you had this clause:

perimeter(Other) -> 
    {error, invalid_object}.

A couple of times, Simon has talked about the "Let it fail" principle: you program for the correct case, and let if fail for other cases. That doesn't quite square with using guards (why not leave the guards out, and let it fail for input that doesn't meet the guard conditions?), nevertheless I think returning "error tuples" is what Simon was directing his "Let it fail" comments at. If you return an error tuple from a function, then the caller has to create code that tries to discern between a successful result and the error tuple. As a result, I don't think you should return an error tuple--that's also quite different than raising an error yourself when you receive bad input, something we haven't covered (yet?).

enclose():

I ended up drawing all kinds of triangles, and then drawing a bounding box around them. That led me to the 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 and see what I mean.

I defined a triangle to be 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 3 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 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():
You were pretty close. I couldn’t come up with a solution as clean as the one you are attempting. Rather, I examined each bit of the number (using band--"binary and"), what we call bit twiddling in other languages that I’ve studied, and I did all kinds of crazy things to get my solution. It worked, but it was much more mathematically complex. I reviewed someone else’s solutions before yours, and they had a gorgeous solution that looked almost exactly like yours.

First, you have a syntax error here:

bits((N div 2), (N rem 2).

You are missing a closing parenthesis. You probably shouldn’t propose a solution that won’t even compile due to a syntax error. It’s one thing if your code doesn’t work--entirely understandable--but it has to compile. Maybe you couldn’t figure out the syntax error? In that case, I think you should have included a comment saying that was the case.

Second, as I’m sure you are aware, your code doesn’t work. Pointing that out with a comment would have been helpful, as well. One thing you are missing is a guard. The problem description said that the solution was for positive integers, i.e. N>0.

For this portion:

bits(0, P) -> P; 
bits(N, P) -> 
bits((N div 2), (N rem 2) ).

the beautiful solution that I reviewed had a case for when N=1, like your N=0 clause, so maybe try adding a clause for 1. I really couldn’t figure out how the person came up with the solution, so I think you may have to keep tweaking things as you try numbers that don't work. Now that I’ve seen the solution, I’m going to try to reproduce it and thereby learn more about how it works.

Next, you are trying to sum the bits, so I don’t know if P is a very descriptive variable name, how about S or Sum? And, because you are trying to sum the bits, this line:

bits((N div 2), (N rem 2) ).

needs to add something to P, try:

bits( (N div 2), P + (N rem 2) ) 

You were on the right track. I wasn’t even close to being on the right track!

Personally, I think the recursion exercises are too hard, and the enclose() exercise was a geometry puzzle--I'm not sure what that has to do with learning erlang.

Nice work on the area() and perimeter() solutions! Keep on plugging away. See you in class!