Awright, here's the source code for a (badly formed) setDate() method. Write up what you think about it and append a better version of it.

(This is actually a method from a JavaBean, so the String argument is quite okay - it gets set in the preamble: <jsp:setProperty name="myBean" property="date" />)

    public void setDate( String d ) throws Exception {
        if (d ==  null || !(d.length() > 0 )) return;
            if( anotherCheck(d) ) {
                this.date = d;
                return;
            }
            throw new Exception("Illegal date");
    }

Janne

I can see at least the following things wrong with this method:

• Indentation is terrible
• You should never throw a java.lang.Exception
• Don't use this (WhyUsingThisIsBad)
• Is that really the best way to write the if() logic?
• Exception message is not very clear
• If this is a bean setter, it probably even shouldn't throw an Exception in the first place.
• Multiple return points.

My suggestion for a better replacement (I ignore Java coding conventions, and use the Avalon coding standards :).

    public void setDate( String date ) 
    {
        if( (date != null) && (date.length() > 0) && anotherCheck(date) )
        {
            m_date = date;
        }
    }

    public void setDate( String date )
         throws IllegalArgumentException
    {
        if( (date != null) && (date.length() > 0) )
        {
            if( anotherCheck(date) )
            {
                m_date = date;
            }
            else
            {
                throw new IllegalArgumentException("Bad date format: "+date);
            }
        }
    }

Though, you can compress it a lot by leaving out the braces around the single-line statements in the then and else -branches. However, I am not certain if that's a good practice. Thus you would get (by inverting a condition)

    public void setDate( String date )
         throws IllegalArgumentException
    { 
        if( (date != null) && (date.length() > 0) )
        {
            if( !anotherCheck(date) )
                throw new IllegalArgumentException("Bad date format: "+date);

            m_date = date;
        }
    }

ebu: I would tend to go with the last version. Some say that dropping the curly brackets is bad, but... well, the aim is legibility, and in a short method like this, without is neater. (If you have a long method with several nested if/elses, I'd go with brackets for overall consistency.)

Notice, especially, the block openings (BSD/Allman style, bracket on its own line) and indented throws-clause. Tidy, structured, neat. (If you want to start a war, go advocate one coding style over another in a newsgroup, or take a look at this JavaRanch discussion. My opinion? Brackets on their own lines delineate blocks clearly. If you need to worry about seeing enough code on your screen, your methods are too long.)

For more propaganda, take a look at the AmbySoft Coding Standards.

Janne: Propaganda indeed. I'd take those instructions with a grain of salt - especially the testing bit. Also, the Ambysoft standards are not really standards, they just seem like a collection of "you could do this, but then again, you could do it like this, too".

ebu: Don't know why he calls it 'standards', but it is pretty much a rational approach to neat coding, and rather similar to the Avalon guidelines (if you interpret the then agains your way;).