hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Oleg Kalnichevski <o.kalnichev...@dplanet.ch>
Subject Re: [PATCH] code page problem fix (Review)
Date Thu, 09 Jan 2003 09:54:35 GMT
Ortwin



On Thu, 2003-01-09 at 09:27, Ortwin Gl├╝ck wrote: 
> I reviewed your patch and noticed the following:
> 
> - The HTTP charset is ISO-8859-1 and *not* US-ASCII !
> 


My understanding is that ISO-8859-1 is default content encoding, however
protocol elements such as headers are supposed to use US-ASCII. Do you
see this differently?


> - Does not cover the NTLM class: the NTLM class makes excessive use of 
> String.getBytes(String) but all with hard-coded and repeated "ASCII" 
> encoding identifiers. These should define a class-local constant for it. 
> Some of the methods throw UnsupportedEncodingException. This exception 
> should be caught internally because ASCII has to be supported by *all* 
> JVM implementations.
> 

Got it. I'll clean it up


> - I noticed there are some really strange methods (toUsingCharset) in 
> URIUtil. What the heck are they for? They do basically nothing but 
> replace missing characters with question marks.... I don't think this is 
> really useful for anything.
> 


I have little idea of what is going on in the URIUtil class. Maybe time
has come for me to get my self introduced


> Following line numbers refer lines AFTER applying your patch.
> 
> there are still getBytes() with default encoding in:
> - PostMethod lines 361, 809
> - PutMethod line 189
> - Base64 line 136
> - SimpleHttpConnection line 139
> - TestAuthenticator: all lines like String expected = "Basic " + new 
> String(Base64.encode("username:password".getBytes()));
> - TestBase64
> - TestStreams
> - TestWebappMethods
> - TestWebappRedirect
> 
> there are still calls to new String(byte[]) with default encoding in:
> - PostXML sample
> - Authenticator lines 288, 833
> - HttpConnection line 666; line 635 has a static reference to "ISO-8859-1"
> - HttpMethodBase line 692
> - TestStreams lines 36, 51, 99, 119
> 
> - I did not run any tests for now
> 

Hmmm, I have overlooked quite a few. However, I felt that in some cases
the use of getBytes() was legitimate. I'll review all these cases one
more time

What about better class and methods names. Any ideas?

Cheers

Oleg


Mime
View raw message