Am 17.05.2004 um 11:33 schrieb Colin Curtin:

> Hi again.
>
> I must first apologize for the oversights in my code - this is my 
> second
> large-ish project in Squeak; I'm still getting used to things around 
> here.
>
> I fixed the things Bert mentioned in his brief code review. Most 
> notably,
> HCFacade is now a proper 'Facade', all of the methods work and take 
> either Urls
> or Strings. Also, I have tried to minimize my habit of stream to string
> concatenation to stream which concatenates strings. Most everything is 
> now
> stream based, and looks a lot cleaner.

Yep. Looks a bit better now :)

However, it's not quite there yet. For a change, I tried using a 
multipart post (file upload) that I had working in the old HTTPCLient. 
However:

- HCFacade>>httpPostMultipart:args: is missing
- arguments in a multipart post are expected to be MIMEDocuments, the 
old implementation allowed plain strings, too
- multipart post fails with a MIMEDocument containing binary data 
(ByteArray)
- After I fixed all of these, my server (Apache2, mod_cgi, simple 
CGI.pm script) still reports a "Malformed multipart POST". The request 
had these headers:

	POST /xxx/xxx.xxx HTTP/1.1
	Host: xxx.xxx.xxx
	User-Agent: HTTPClient (Squeak)-v.02
	ACCEPT: application/octet-stream text/html
	Content-Type: multipart/form-data; 
boundary=c46ff15084ef270e997f21c1be1ed9c0
	Content-Length: 4698

	--c46ff15084ef270e997f21c1be1ed9c0
	Content-disposition: multipart/form-data; name=image filename=xxx.xxx
	Content-Type: image/png

Looks to me like some quotes and punctuation is missing?

Some more findings :

- You're using class Bag quite often where you actually want to use a 
Set.
- stream in HCMethod is undeclared
- 'http://host/bla/bla/bla/bla' asUrl pathstring returns an empty path. 
Actually, this method is so confusing and wrong, a complete rewrite 
would be in order. How about this?
	^ String streamContents: [:s |
		s nextPut: $/.
		1 to: path size - 1 do: [:i |
			s nextPutAll: (path at: i); nextPut: $/]]
- Number class>>toDecimal:fromBase: seems unused

Apart from bugs and nits like the above (I'm pretty sure there are 
more, although I did only test the methods I needed), I'd expect a new 
HTTP client to be more efficient then the old one. For example, the old 
one buffered all file uploads (multipart posts) in memory. This is 
really bad if you want to upload large files. AFAICT, your client does 
the same. It would be nice if this was handled in a streaming manner.

Maybe after a few more iteration this really becomes THE client :-)

- Bert -