This is the mail archive of the mailing list for the Java project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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.


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. ;)

dalibor topic

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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]