Re: [qs-dev] for review: client interface and sample

[Fergus H. <fj...@cs...>]

On 08-Feb-2001, Ina Cheng <in...@st...> wrote:
> +++ options.m	2001/02/08 04:19:22
> +	% General client options
> +option_default(uri,		string("\"\"")).

Why isn't that just

   option_default(uri,		string("")).

?
The default of using "\"\"" (i.e. "") rather than
"" (i.e. the empty string) is odd.
I think at least you should put a comment explaining it.

> +% XXX if the default value is not defined, the option cannot
> +%     be recognised, but this has no default value 
> +option_default(method, 		string("Hello")).

Use

   option_default(method, 		string("")).

and then have the code which uses that option check whether the value
of the option is the empty string.

> +:- module soap_interface.
> +:- interface.
> +:- import_module io, list, std_util, string.
> +:- import_module tcp.
> +
> +:- pred soap_call_mercury_type(host::in, port::in, string::in, 
> +	string::in, list(univ)::in, list(univ)::out, 
> +	io__state::di, io__state::uo) is det.

You should document *in the interface section*
what that predicate does and what it's arguments mean.

> +/*  Outline:
> +soap_call_mercury_type(uri, method_name, list_of_univ_in, 
> +  list_of_univ_out) :-
> +        generate SOAP message in XML format and SOAP protocol,
> +        tcp__connect to the server,
> +	service_connection,
> +		( which will handle:
> +		   send the request to the server using HTTP,
> +		   wait for the response,
> +	  	   read response in XML format,
> +		)
> +        decode the response to mercury types, 
> +	return list_of_univ as response.
> +
> +soap_call_xml_type:
> +        tcp__connect to the server,
> +	service_connection,
> +		( which will handle:
> +		   send the request to the server using HTTP,
> +		   wait for the response,
> +	  	   read response in XML format,
> +		)
> +	return response.
> +
> +*/

Shouldn't you disconnect from the server at some point?

The tcp__disconnect should be done at the same level as the
tcp__connect.  If the disconnect is done from within
service_connection, then I think the tcp__connect should 
also be done from within service_connection.

> +decode_SP_response(XMLResponse, Responses) :- 
> +	( 
> +		string__sub_string_search(XMLResponse, "<price>", Start),
> +		string__sub_string_search(XMLResponse, "</price>", End) 
> +	->
> +		string__left(XMLResponse, End, Response0),
> +		TagLength = 7,		% <price>
> +		string__right(Response0, End-Start-TagLength, Response1),

Put whitespace around operators, i.e.  s/-/ - /g

> +%------------------------------------------------------------------------%
> +	
> +	% Connects to the server using TCP/IP, sends request to server
> +	% using HTTP, receives response from server and terminates
> +	% the connection. 
> +:- pred service_connection(tcp::in, string::in, string::out, 
> +	% io__state::di, io__state::uo) is cc_multi.
> +	io__state::di, io__state::uo) is det.
> +
> +service_connection(TCP, Request, Response) -->
> +	send_request(TCP, Request),
> +	read_response(TCP, Response),
> +	tcp__shutdown(TCP).

The documentation says this connects to the server,
but the code doesn't do the connect.


In the long term of course the hard-coding of the method names
("Hello", etc.) needs to be fixed.  But otherwise this looks good.

-- 
Fergus Henderson <fj...@cs...>  |  "I have always known that the pursuit
                                    |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.