[erlang-questions] is this a well written function

[Richard A. O'Keefe]

Time to get onto some of the fine detail.

(1) The main comment sometimes says "integer" and sometimes says "number".
    This is at best confusing.  All integers are numbers, but not all
    numbers are integers.

(2) "The Erlang Programming book" is not a helpful reference.
    Which book?  Is the book called "Erlang Programming"?  Then
    a better comment would be
	This is exercise <which> from page <what> of
	"Programming Erlang" by <authors>

    For example, your b_and/2 &c exercise should have been documented as
	%%% This module is my solution to exercise 2-3 on page 44 of
	%%% "Programming Erlang" by Cesarini and Thomson.


    By the way, that is a really fine book.  The wording of some of
    the exercises could be improved.  For example, exercise 3-2 says
    "format" when it means "form".  The first time I saw this book,
    I was so impressed by the rest of the text that I paid no attention
    to the exercises.

(3) So let's start with a master comment for the module.

    %%% This module is my solution to exercise 3-3 "Side Effects" on
    %%% page 83 of "Programming Erlang" by Cesarini and Thomson.

(4) Let's note the wording of the exercise.

    "Write a function that PRINTS out the INTEGERS between 1 and N."
    "Write a function that PRINTS out the EVEN INTEGERS between 1 and N."

    It is wise to stick with the words you are given.  Someone who knows
    that you are supposed to "print integers" is going to find
    'print_integers' *instantly* understandable, while
    'display_numbers' is going to take some decoding: what is the
    difference between displaying and printing, what is the difference
    between integers and numbers (is your function meant to handle
    floats as well?)

    So we should continue the module like this:

    -module(exercise_3_2).
    -export([
        print_even_integers/1,
        print_integers/1
    ]).

    %% "a function that prints out the integers between 1 and N."

    print_integers(N) when is_integer(N), N >= 0 ->
        ...

    %% "a function that prints out the even integers between 1 and N."

    print_even_integers(N) when is_integer(N), N >= 0 ->
	...

    In the spirit of sticking with the words you are given, the text of
    the exercise consistently calls the upper bound N.  You could make
    a case for using a *MORE* informative name, such as Upper_Bound.
    But using Number is not a good idea.  It is *different* from the
    name used in the exercise, so the reader constantly has to keep in
    mind "Number really means N" instead of the much simpler "N means N",
    and it does not *help* in any way that N does not.  It is in fact
    *less* helpful than N, because there has been a convention since
    before Fortran (that since, since before the late 1950s) that N is
    to be used only for integers.

(5) Consistency again.
> 
> display_numbers(Number) when is_integer(Number), Number > 0 ->
>  display_numbers_loop(0, Number ).

    You have no space before the right parenthesis in the head,
    and that is GOOD.
    You one one space before the right parenthesis in the body,
    which I find ugly, but some people think it's a good idea.
    What nobody thinks is a good idea is mixing BOTH styles in
    the same function!

    Like everything I've mentioned in this message so far, this has
    nothing to do with Erlang.  Some people consistently write
    f(X).  Some people consistently write f( X ).  Some people
    consistently write f(X ) or consistently write f( X), both of
    which look horrible to me.  Feel free to choose a style
    without paying any heed to what I like or dislike, but have
    *reasons* for your choice and be consistent.

    Cesarini and Thomson's style is consistently
    - no space after ( in a function call
    - no space before ) in a function call.

> 
> %%  When the control argument)(second argument)

(6) This is a classic "incomplete edit" problem.  I found one of those in
    my own code two days ago.  I was ashamed of it.  A major refactoring
    had drastically simplified a large file, but some old code that
    wasn't needed any more was still being called, and in the new context,
    doing bad things.  The control argument is in fact the second.

    It would be even better to write

    %% print_integers(N0, N) writes the rest of the integers 1 .. N
    %% after the integers 1 .. N0 have already been printed.


> display_numbers_loop(Current, Number) ->
>  io:format("Number:~p~n",[Current + 1]),
>  display_numbers_loop(Current +1, Number ).

(7) Consistency again.  There are three argument-separating commas.
    Two of them are followed by spaces but one is not.
    There are two addition operators.  One of them has spaces on
    both sides, but the other only on one side.

    Consistency in a layout style doesn't get noticed.
    Inconsistency in a layout style slows your readers down while
    they try to figure out whether a difference *means* something
    or is an accident.


By the way, exercise 3-3 includes

	Hint: use guards.

You are using guards to check the argument N, which is fine,
but that's not what the hint is pointing you to, and if you had
started with print_even_integers/1 first, you would have noticed.

Let's be clever.

To print all the integers between 1 and N is to
print all the integers L, L+1, L+2, ... U
where L = 1 and U = N.

To print the even integers between 1 and N is to
print all the integers L, L+2, L+4, ... U
where L = 2 and U = N.

If we had a function

    print_integers(Lower_Bound, Upper_Bound, Increment)

that did the obvious thing, then we could write

    print_integers(1, N) when is_integer(N), N >= 0 ->
        print_integers(1, N, 1).

    print_even_integers(1, N) when is_integer(N), N >= 0 ->
        print_even_integers(2, N, 2).

So all we need is print_integers/3.  And _that_ is basically
display_numbers_loop/2, except that the constant 1 is replaced
by the Increment argument, and the test for having printed all
the numbers uses a guard Lower_Bound > Upper_Bound instead of
a pattern match.

Why a guard?  Because print_even_integers(5) is going to stop
when the next number to print is 6; it will never be 5.

(8) AUTHORITIES MAY DISAGREE.

    In exercise 3-4 on page 84 of Cesarini and Thomson, they
    tell you to write flatten/1 and hint that you should
    use concatenate/1 in your answer.

    It is indeed *possible* to use concatenate/1 in the answer,
    but it is much simpler and an order of magnitude more
    efficient not to.  I suspect that Cesarini and Thomson
    are trying to get you to
    (a) do something with a tree structure, not just a sequence
    (b) give you some more practice in using building blocks
        you've just written
    (c) give you some practice in seeing [_|_] as a binary tree
        constructor, not specifically a sequence constructor.
    I can't at the moment see a better exercise they could have
    written to serve those purposes, but in actual programming
    practice, using concatenate/1 in flatten/1 is as bad as
    and for much the same reason as doing string concatenation
    in a loop in Java.