Re: Request for critics

["Jeffrey R. Carter" <spam.not.jrcarter@acm.not.spam.org>]

Maciej Sobczak wrote:
> For the purpose of my learning, please criticise the following code:

Sure. I love to tear apart others' work :)

> with Ada.Text_IO;
> use Ada.Text_IO;

Given 2 calls to Put and 1 to New_Line, all in 1 procedure, a global use of 
Ada.Text_IO seems inappropriate;

> procedure Automata is
> 
>    Size : constant := 66;
>    Max_Steps : constant := 31;

This appears not to be a maximum (which implies it may not be reached), but a 
number, so I'd probably call it Num_Steps.

> 
>    type Cell_State is (Active, Inactive);
>    type Row_Of_Cells is array (1 .. Size) of Cell_State;
> 
>    type Cell_Triplet is array (-1 .. 1) of Cell_State;

Cell_Triplet, with its attendant type conversion, seems inappropriate. How about 
something like

type Cell_List is array (Positive range <>) of Cell_State;
subtype Row_Of_Cells is Cell_List (1 .. Size); -- Cell_Row?
subtype Cell_Triplet is Cell_List (1 .. 3);

You can then call New_State with a slice of a Row_Of_Cell of length 3.

>    Automata : Row_Of_Cells := (Size / 2 => Active, others => Inactive);

Personally, I'd put this after the subprograms, simply to avoid accidental use 
of it from within them. I also dislike the potential confusion of reusing the 
identifier Automata like this.

>    procedure Show(Row : in Row_Of_Cells) is

You can put your use clause here, or fully qualify the 3 calls.

>    begin
>       for I in Row_Of_Cells'Range loop

I prefer to base indices on objects rather than types:

for I in Row'range loop

I also like to name all loops, but I seem to be in the minority about that.

>          case Row(I) is
>             when Active   => Put('*');
>             when Inactive => Put(' ');
>          end case;
>       end loop;
>       New_Line;
>    end Show;
> 
>    function New_State(Neighbourhood : in Cell_Triplet)
>                       return Cell_State is
>       Left  : Cell_State := Neighbourhood(-1);
>       Right : Cell_State := Neighbourhood( 1);
>    begin
>       if (Left = Active and Right = Inactive)
>         or (Left = Inactive and Right = Active) then

This seems to be

if Left /= Right then

which also seems clearer.

>          return Active;
>       else
>          return Inactive;
>       end if;
>    end New_State;

Neighbourhood (0) is not referenced, so perhaps it shouldn't be passed, either.

> begin
>    Show(Automata);

I'd like to see a blank line here.

>    for Step in 1 .. Max_Steps loop
>       declare
>          New_Automata : Row_Of_Cells := (others => Inactive);
>       begin
>          for Cell_Index in
>            Row_Of_Cells'First + 1 .. Row_Of_Cells'Last - 1 loop
> 
>             New_Automata(Cell_Index) := New_State(Cell_Triplet(
>                Automata(Cell_Index - 1 .. Cell_Index + 1)));
> 
>          end loop;
>          Automata := New_Automata;
>       end;

And here.

>       Show(Automata);
>    end loop;
> end Automata;

> The goal is the readability and proper expression of the problem.
> 
> How do you name loop control variables for array traversals? Is 
> "Cell_Index" overly verbose and would be "I" just enough?

I don't have any problem with either, and would probably use Index.

> Would you drop the Cell_Triplet type and pass the neighbourhood as three 
> separate parameters instead?

I'd probably just pass Left and Right. But, consider making this a generic, with 
New_State as a generic parameter (and Size and Max_Steps as parameters), and 
think about what the parameter list for New_State should be then.

> BTW: The C version is just a bit shorter:
> 
> #include <stdio.h>
> int main(){char s[]=
> "                                *                                 "
> ;char p,n;int st,i;puts(s);for(st=0;st!=31;++st){p=' 
> ';for(i=1;i!=65;++i){n=(s[i-1]^s[i+1])|0x20;s[i-1]=p;p=n;}s[i-1]=p;puts(s);}return 
> 0;}
> 
> ;-)  (the last three characters are not part of the program)

Indeed, and as C lovers love to point out, shorter things are more readable (see 
my comment on your condition above), so this must be clearer, as well :)

Seriously, part of the reduction is the use of a string to represent the state; 
this could be incorporated in Ada as well, eliminating Show. That would make the 
Ada shorter, too. For this little example, it doesn't much matter, but in 
general modeling the problem directly by using appropriate user-defined types is 
better. Another part is incorporating the update algorithm in line, but using a 
function is clearer and allows for parameterization.

The formatting also helps reduce the size, but I would not recommend this 
formatting style (except for C :).

By the way, automata is the plural of automaton, but you seem to use it as a 
singular.

-- 
Jeff Carter
"Son of a window-dresser."
Monty Python & the Holy Grail
12