* Request for critics
@ 2006-03-22 10:01 Maciej Sobczak
2006-03-22 21:44 ` Gautier
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Maciej Sobczak @ 2006-03-22 10:01 UTC (permalink / raw)
For the purpose of my learning, please criticise the following code:
with Ada.Text_IO;
use Ada.Text_IO;
procedure Automata is
Size : constant := 66;
Max_Steps : constant := 31;
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;
Automata : Row_Of_Cells := (Size / 2 => Active, others => Inactive);
procedure Show(Row : in Row_Of_Cells) is
begin
for I in Row_Of_Cells'Range loop
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
return Active;
else
return Inactive;
end if;
end New_State;
begin
Show(Automata);
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;
Show(Automata);
end loop;
end Automata;
<problem-description>
The above is a short simulation of one particular 1D cellular automata
(the state of the whole automata is printed as a single line and the
time progresses downwards on the printout), where the next state of each
cell is a function of the current states in the immediate neighbourhood
of the given cell. In other words, given ABCDEFGH as a state of the
automata, the next state of cell D is a function of cells C, D and E.
Taking two possible states, there are 256 possible rules. The above
program simulates only one of them.
</problem-description>
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?
Would you drop the Cell_Triplet type and pass the neighbourhood as three
separate parameters instead?
And so on. Pleasy be picky with every aspect you find important.
Any performance considerations for this code (except, of course, that
Put() is called for each cell and might be replaced by whole-line
printouts instaed)?
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)
--
Maciej Sobczak : http://www.msobczak.com/
Programming : http://www.msobczak.com/prog/
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Request for critics
2006-03-22 10:01 Request for critics Maciej Sobczak
@ 2006-03-22 21:44 ` Gautier
2006-03-22 23:17 ` Jeffrey R. Carter
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Gautier @ 2006-03-22 21:44 UTC (permalink / raw)
Just an opinion:
if (Left = Active and Right = Inactive)
or (Left = Inactive and Right = Active) then
seems to me "over-explicit" and indeed less readable than:
if Left /= Right then
_______________________________________________________________
Gautier -- http://www.mysunrise.ch/users/gdm/index.htm
Ada programming -- http://www.mysunrise.ch/users/gdm/gsoft.htm
NB: For a direct answer, e-mail address on the Web site!
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Request for critics
2006-03-22 10:01 Request for critics Maciej Sobczak
2006-03-22 21:44 ` Gautier
@ 2006-03-22 23:17 ` Jeffrey R. Carter
2006-03-23 1:21 ` tmoran
2006-03-23 4:29 ` Steve
2006-03-24 23:30 ` Justin Gombos
3 siblings, 1 reply; 8+ messages in thread
From: Jeffrey R. Carter @ 2006-03-22 23:17 UTC (permalink / raw)
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Request for critics
2006-03-22 23:17 ` Jeffrey R. Carter
@ 2006-03-23 1:21 ` tmoran
0 siblings, 0 replies; 8+ messages in thread
From: tmoran @ 2006-03-23 1:21 UTC (permalink / raw)
A few additional minor suggestions:
Image : constant array(Cell_State) of Character
:= (Active => '*', Inactive => ' ');
then
procedure Show(Row : in Row_Of_Cells) is
use Ada.Text_IO;
begin
for I in Row'range loop
Put(Image(Row(I)));
end loop;
New_Line;
end Show;
and
New_State : constant array(Cell_State, Cell_State) of Cell_State
:= (Active => (Active=>Inactive, Inactive=>Active),
Inactive => (Active=>Active, Inactive=>Inactive),
then
New_Automata(Cell_Index) := New_State(Automata(Cell_Index - 1),
Automata(Cell_Index + 1));
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Request for critics
2006-03-22 10:01 Request for critics Maciej Sobczak
2006-03-22 21:44 ` Gautier
2006-03-22 23:17 ` Jeffrey R. Carter
@ 2006-03-23 4:29 ` Steve
2006-03-24 23:30 ` Justin Gombos
3 siblings, 0 replies; 8+ messages in thread
From: Steve @ 2006-03-23 4:29 UTC (permalink / raw)
I've been writing a lot of time critical code lately so my approach to
coding may be a little out of the norm, but here is some code with the same
functionality. Columns should align nicely if you view them in a fixed
pitch font.:
with Ada.Text_IO;
use Ada.Text_IO;
procedure Automata is
Size : constant := 66;
Max_Steps : constant := 31;
type Cell_State is (Active, Inactive);
type Row_Of_Cells is array (1 .. Size) of Cell_State;
type State_To_Char is array( Cell_State'Range ) of Character;
State_Char : constant State_To_Char := ( Active => '*', Inactive => '
' );
procedure Show(Row : in Row_Of_Cells) is
begin
for I in Row_Of_Cells'Range loop
Put( State_Char( Row(I) ) );
end loop;
New_Line;
end Show;
type State_Map is array( Cell_State'Range, Cell_State'Range,
Cell_State'Range ) of Cell_State;
New_State : constant State_Map :=
( Active => ( Active => ( Active => Inactive, -- Active
Active Active => Inactive
Inactive => Active ), -- Active
Active Inactive => Active
Inactive => ( Active => Inactive, -- Active
Inactive Active => Inactive
Inactive => Active ) ), -- Active
Inactive Inactive => Active
Inactive => ( Active => ( Active => Active, -- Inactive
Active Active => Active
Inactive => Inactive ), -- Inactive
Active Inactive => Inactive
Inactive => ( Active => Active, -- Inactive
Inactive Active => Active
Inactive => Inactive ) ) -- Inactive
Inactive Inactive => Inactive
);
begin
declare
Automata : Row_Of_Cells := (Size / 2 => Active, others => Inactive);
begin
Show(Automata);
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(
Automata(Cell_Index - 1),
Automata(Cell_Index),
Automata(Cell_Index + 1));
end loop;
Automata := New_Automata;
end;
Show(Automata);
end loop;
end;
end Automata;
"Maciej Sobczak" <no.spam@no.spam.com> wrote in message
news:dvr7a5$9o3$1@sunnews.cern.ch...
> For the purpose of my learning, please criticise the following code:
[snip]
> <problem-description>
> The above is a short simulation of one particular 1D cellular automata
> (the state of the whole automata is printed as a single line and the time
> progresses downwards on the printout), where the next state of each cell
> is a function of the current states in the immediate neighbourhood of the
> given cell. In other words, given ABCDEFGH as a state of the automata, the
> next state of cell D is a function of cells C, D and E. Taking two
> possible states, there are 256 possible rules. The above program simulates
> only one of them.
> </problem-description>
>
> 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?
>
In this example of code, IMHO "I" would be enough.
There is only one index involved and the loop is small. Personally I tend
to use a more expressive index name for all cases, and would probably use
Cell_Index, Data_Index, Index, or something similar.
> Would you drop the Cell_Triplet type and pass the neighbourhood as three
> separate parameters instead?
>
In this case I would rop Cell_Triplet since it doesn't add anything to the
readability and introduces a new data type and required conversions. In my
experience, for the simple cases less is better.
> And so on. Pleasy be picky with every aspect you find important.
>
> Any performance considerations for this code (except, of course, that
> Put() is called for each cell and might be replaced by whole-line
> printouts instaed)?
The performance change you'll note in my modified implementation is the use
of a lookup table for New_State instead of a function. I think it is easier
to interpret the mapping that takes place in the lookup table than analyzing
the code.
You'll also note that I moved the definition of Automata into a declare
block in the main procedure. I strive to restrict the scope of variables as
much as possible.
Normally I also avoid using variable names that are the same as package
names. It may be legal, but it may also be confusing.
In your original code the state mapping really only depended on the two
neighbors, so the State_Map really only needs to be 2x2 rather than the 3x3
in my example. I chose to implement 3x3 since it would make it easier to
define other mappings.
>
> 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)
>
You can generate line noise in Ada too. But it is a bit harder to do, and
after all that is not the objective.
I hope this helps,
Steve
(The Duck)
> --
> Maciej Sobczak : http://www.msobczak.com/
> Programming : http://www.msobczak.com/prog/
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Request for critics
2006-03-22 10:01 Request for critics Maciej Sobczak
` (2 preceding siblings ...)
2006-03-23 4:29 ` Steve
@ 2006-03-24 23:30 ` Justin Gombos
2006-03-25 14:00 ` Stephen Leake
3 siblings, 1 reply; 8+ messages in thread
From: Justin Gombos @ 2006-03-24 23:30 UTC (permalink / raw)
On 2006-03-22, Maciej Sobczak <no.spam@no.spam.com> wrote:
>
> Automata : Row_Of_Cells := (Size / 2 => Active, others =>
> Inactive);
One thing that hasn't been caught yet - Automata should be a constant:
Automata : constant Row_Of_Cells := (Size / 2 => Active, others => Inactive);
because it never changes after its declaration.
--
PM instructions: do a C4esar Ciph3r on my address; retain punctuation.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Request for critics
2006-03-24 23:30 ` Justin Gombos
@ 2006-03-25 14:00 ` Stephen Leake
2006-03-26 0:35 ` Jeffrey R. Carter
0 siblings, 1 reply; 8+ messages in thread
From: Stephen Leake @ 2006-03-25 14:00 UTC (permalink / raw)
Justin Gombos <rpbkbq.xax.gld@uluv.kbq> writes:
> On 2006-03-22, Maciej Sobczak <no.spam@no.spam.com> wrote:
>>
>> Automata : Row_Of_Cells := (Size / 2 => Active, others =>
>> Inactive);
>
> One thing that hasn't been caught yet - Automata should be a constant:
>
> Automata : constant Row_Of_Cells := (Size / 2 => Active, others => Inactive);
>
> because it never changes after its declaration.
If you turning on all warnings in GNAT (-gnatwa), it will tell you
this automatically.
--
-- Stephe
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Request for critics
2006-03-25 14:00 ` Stephen Leake
@ 2006-03-26 0:35 ` Jeffrey R. Carter
0 siblings, 0 replies; 8+ messages in thread
From: Jeffrey R. Carter @ 2006-03-26 0:35 UTC (permalink / raw)
Stephen Leake wrote:
> Justin Gombos <rpbkbq.xax.gld@uluv.kbq> writes:
>
>> On 2006-03-22, Maciej Sobczak <no.spam@no.spam.com> wrote:
>>> Automata : Row_Of_Cells := (Size / 2 => Active, others =>
>>> Inactive);
>> One thing that hasn't been caught yet - Automata should be a constant:
>>
>> Automata : constant Row_Of_Cells := (Size / 2 => Active, others => Inactive);
>>
>> because it never changes after its declaration.
>
> If you turning on all warnings in GNAT (-gnatwa), it will tell you
> this automatically.
Except it won't in this case, because there is an assignment to Automata.
--
Jeff Carter
"I spun around, and there I was, face to face with a
six-year-old kid. Well, I just threw my guns down and
walked away. Little bastard shot me in the ass."
Blazing Saddles
40
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2006-03-26 0:35 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-03-22 10:01 Request for critics Maciej Sobczak
2006-03-22 21:44 ` Gautier
2006-03-22 23:17 ` Jeffrey R. Carter
2006-03-23 1:21 ` tmoran
2006-03-23 4:29 ` Steve
2006-03-24 23:30 ` Justin Gombos
2006-03-25 14:00 ` Stephen Leake
2006-03-26 0:35 ` Jeffrey R. Carter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox