Code clarity [possibly a bikeshed thread] (Was: Re: FYI: Patch: java.net:socket stuff)

[Dalibor Topic <robilad at kaffe dot org>]

Hi Michael,

Michael Koch wrote:
> On Tue, Nov 25, 2003 at 02:57:23PM +0100, Dalibor Topic wrote:
>
> > Michael Koch wrote:
> >
> > > Hi list,
> > >
> > >
> > > I commited the attached big patch to fix several issues in the socket 
> > > stuff. This mainly adds checks to many methods to see if the socket is 
> > > closed already.
> >
> > I like the patch, though I'm not a gcj developer. I have one small nit 
> > to pick, though. Instead of copying
> >
> >    if (isClosed())
> >      throw new SocketException("socket is closed");
> >
> > so many times, you could have used a private method to do the checking 
> > and throwing.
> >
> > 	checkIfIsClosed();
> >
> > private void checkIfIsClosed() throws SocketException {
> >   if (isClosed())
> >      throw new SocketException("socket is closed");
> > }
> >
> > Code duplication is evil ;)
>
>
> Introducing a new method for this really unnecessary. It doenst make the
> code more clear.

It may make the code more clear if the method name is chosen better and 
the method is documented.

The check* prefix is used widely (see SecurityManager) for methods that 
return nothing, and test a condition, throwing an exception if the test 
fails. Look at checkRead, checkWrite, etc. Which is the type of utility 
method we have here. Of course you could call checkPermission directly 
with the appropriate permission by hand, but the helper methods help the 
understanding in this case, in my opinion.

I agree that the method name is not great, and I'm of course open to 
suggestions on naming such helper methods. checkIfClosed, 
assertThatSocketIsStillOpen, or whatever someone can come up that's 
nicer to understand.

It may also help making the code a tiny bit more maintainable. Instead 
of someone later having to spend time searching and replacing all the 
inlined occurences of the test and the exception being thrown, when a 
change becomes necessary, you can help them by putting redundant code 
into small, functional methods.

So, for example, when some GNU Classpath policy on having exception 
messages start with a capital letter arrives, or a policy stating that 
all exception messages should be properly punctuated strings, or a 
policy to internationalize exception messages [1] the person fixing the 
code has an easier job doing it.

That person may be you, then all is well, you get to eat your own soup, 
but it may be Mohan or me, or someone else in case you lose your 
interest in searching and replacing Strings one day. ;)

cheers,
dalibor topic

[1] de_DE: "Der Socket ist geschlossen." ? ;)