[mercury-users] Request for code review

[David Overton]

On Mon, May 26, 2003 at 11:40:28PM +0100, Keith Braithwaite wrote:
> Attached are the modules I've got to. What I'd appreciate is some 
> pointers as to:
> 1) where what I've written isn't idiomatic Mercury

There are two main points I can see where what you've written isn't
``idiomatic'' Mercury.  The first is that you might like to use a
semidet predicate for valid_position instead of returning a bool.

The second is that your calls to append in do_test are unnecessary.
The calls where you are appending to [] can be removed --- the state
variable syntax transformation will take care of adding the unification 
`!:Failures = !.Failures' for that case.
For the case of appending to a singleton list, you should use
the list construction syntax, i.e. `Xs = [X | Xs0]' is equivalent to
`append([X], Xs0, Xs)' but more ``idiomatic'' (and more efficient too).

I've attached a modified version of your program which makes the changes
above and also some minor syntactic changes to conform to the coding
standards that we use within the Mercury project (in case you're
interested in how the Mercury developers themselves write Mercury).

> 2) how this code could be improved, particularly
> 	a: is there any reflection like mechanism available to avoid having 
> 	to give the name of the test function?

I don't think any such mechanism is available for predicates.

> 	b: what's to choose between the io__print and and io__write 
> facilities? Or rather, how should I choose between them?

io__print is for when the output is intended to be human readable.
io__write is for when you need to be able to read the output back in
using io__read.

> 3) how could I use modes (or whatever) to strengthen the contract 
> between module mtest and particular test modules, as with gotests. I've 
> begun with type mtest__tests, but feel that there's much more to do 
> there.

What aspect(s) of the contract would you like to express?

> Let me also say that I'm very impressed with Mercury, both the language 
> and the tool set! My first thought when I saw Mercury code was that it 
> was ugly and verbose, but having learned more about what the syntax is 
> for (and a little of how it go that way), I'm rather taken with it. 
> Certainly, Mercury is very strongly in Brooks' program-system-product 
> arena, and the syntax (and semantics) reflect that, so maybe it's 
> inevitable that "hello world" is going to look silly.

Yes, it's certainly not the ideal language for writing "hello world".  :-)



David
-- 
David Overton                  Uni of Melbourne     +61 3 8344 1354
dmo at cs.mu.oz.au                Monash Uni (Clayton) +61 3 9905 5779
http://www.cs.mu.oz.au/~dmo    Mobile Phone         +61 4 0337 4393
-------------- next part --------------
% $Id: go.m,v 1.2 2003/05/26 22:19:53 keithb Exp $
:- module go.

:- interface.

:- import_module int. 
:- import_module bool.

:- type player ---> black ; white.

:- type line == int.
:- type hline == line.
:- type vline == line.

:- type point ---> at(hline, vline). 

:- type stone ---> play(player, point).

:- type board ---> goban(int).

:- pred valid_position(board::in, point::in) is semidet.

:- implementation.

valid_position(goban(BoardSize), at(Hline,Vline)) :-  
	Hline > 0,
	Hline =< BoardSize,
	Vline > 0,
	Vline =< BoardSize.
-------------- next part --------------
% $Id: gotests.m,v 1.7 2003/05/26 22:18:07 keithb Exp $
:- module gotests.

:- interface.

:- import_module mtest.

:- func tests = (tests::out(tests)) is det.

:- implementation.

:- import_module bool.
:- import_module go.

:- pred test_valid_position11 is semidet.

test_valid_position11 :-
	valid_position(goban(9), at(1, 1)).

:- pred test_valid_position99 is semidet.

test_valid_position99 :-
	valid_position(goban(9), at(9, 9)).

:- pred test_valid_positionXX is semidet.

test_valid_positionXX  :-
	valid_position(goban(9), at(-1, 10)).

:- import_module list.

tests = [
		test(test_valid_positionXX, "test_validPositionXX"), 
		test(test_valid_position11, "test_validPosition11"), 
		test(test_valid_position99, "test_valid_position99")
	].
-------------- next part --------------
% $Id: mtest.m,v 1.5 2003/05/26 22:19:53 keithb Exp $
:- module mtest.
:- interface.

:- import_module io.
:- import_module list.

:- type test
	--->	test(
			case :: (pred), 
			name :: string
		).

:- inst test
	--->	test(
			(pred) is semidet,
			ground
		).

:- type tests == list(test).
:- inst tests == list_skel(test).

:- type suite == string.

:- pred run_tests(suite::in, tests::in(tests), io::di, io::uo) is det.

:- implementation.

:- import_module list.
:- import_module bool.

:- pred do_test(tests::in(tests), list(string)::in, list(string)::out, 
			io::di, io::uo) is det.

do_test([Test | RemainingTests], !Failures, !IO) :-
	( call(Test ^ case) ->
		write_string(".", !IO)
	;
		write_string("F", !IO),
		!:Failures = [Test ^ name | !.Failures]
	), 
	do_test(RemainingTests, !Failures, !IO).

do_test([], !Failures, !IO) :-
	write_string("\n",!IO).

run_tests(Suite, Tests, !IO) :-
	write_string("Executing suite", !IO),
	write_string(Suite, !IO),
	write_string("\n", !IO),
	do_test(Tests, [], Failures, !IO),
	write_string("failures: ", !IO),
	print(Failures, !IO),
	write_string("\n", !IO).

-------------- next part --------------
% $Id: test.m,v 1.7 2003/05/26 22:19:53 keithb Exp $
:- module test.

:- interface.
:- import_module io.

:- pred main(io::di, io::uo) is det.

:- implementation.

:- import_module mtest.
:- import_module gotests.

main(!IO) :- 
	run_tests("gotests__tests", gotests__tests, !IO).