harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexei Fedotov <alexei.fedo...@gmail.com>
Subject Re: Idiomatic Java: return-at-method-end in Harmony
Date Tue, 27 Oct 2009 10:23:41 GMT
I like literate threads from Jesse just because they recall
understanding and participation feelings. :-)



On Tue, Oct 27, 2009 at 12:06 PM, Tim Ellison <t.p.ellison@gmail.com> wrote:
> As I general rule (which is, of course, made to be broken as you sit in
> front of the editor where it suddenly doesn't make sense), I'll try to:
>
>  - keep the methods short, factoring out to private helpers, so that
> each method is understandable, and details pushed down to collections of
> other simple methods,
>
>  - perform the argument verification and precondition checks first,
> returning early if they do not hold (so in this case, I agree with you
> readConfiguration() checks that return early),
>
>  - try to have the method return at the end, rather than having multiple
> return points throughout (of course, there is always that case where you
> need to return from inside a loop, which to me always feels like a jail
> break<g>)
>
> I suspect that we are all not too far from adopting the same style of
> programming.  I'd hate for us to get into the practice of rewriting code
> that works just to fix the style, when there are some real interesting
> problems to solve.  And as a rule breaker, yes, I do go in and fix the
> style of working code!
>
> Regards,
> Tim
>
> On 26/Oct/2009 21:34, Jesse Wilson wrote:
>> Harmony Team,
>>
>> Some of the Java code in Harmony suffers from being written in a non-Java
>> style. In particular, our Java code often attempts to limit the number of
>> exit points from a method. This approach is common in C/C++ programs because
>> of the need to manually collect garbage. But the approach has no advantages
>> in Java. I dislike it because it requires the reader to carry more in their
>> mental stack.
>>
>> Consider LogManager.readConfiguration(), the bulk of which lives inside an
>> if() block:
>>
>>     public void readConfiguration() throws IOException {
>>
>>         // check config class
>>
>>         String configClassName = System
>>
>>                 .getProperty("java.util.logging.config.class");
>>
>>         if (null == configClassName
>>
>>                 || null == getInstanceByClass(configClassName)) {
>>
>>             // if config class failed, check config file
>>
>>             String configFile = System
>>
>>                     .getProperty("java.util.logging.config.file");
>>
>>
>>             if (null == configFile) {
>>
>>                 // if cannot find configFile, use default logging.properties
>>
>>                 configFile = new StringBuilder().append(
>>
>>
>>  System.getProperty("java.home")).append(File.separator)
>>
>>                         .append("lib").append(File.separator).append(
>>
>>                                 "logging.properties").toString();
>>
>>             }
>>
>>
>>             InputStream input = null;
>>
>>             try {
>>
>>                 input = new BufferedInputStream(new
>> FileInputStream(configFile));
>>
>>                 readConfiguration(input);
>>
>>             } finally {
>>
>>                 if (input != null) {
>>
>>                     try {
>>
>>                         input.close();
>>
>>                     } catch (Exception e) {// ignore
>>
>>                     }
>>
>>                 }
>>
>>             }
>>
>>         }
>>
>>     }
>>
>>
>> By using an eager return, the code needs fewer layers of indentation and
>> thus feels simpler (at least to the colleagues I've asked):
>>
>>     public void readConfiguration() throws IOException {
>>
>>         // check config class
>>
>>         String configClassName = System
>>
>>                 .getProperty("java.util.logging.config.class");
>>
>>         if (null != configClassName
>>
>>                 && null != getInstanceByClass(configClassName)) {
>>
>>             return;
>>
>>         }
>>
>>
>>         // if config class failed, check config file
>>
>>         String configFile = System
>>
>>                 .getProperty("java.util.logging.config.file");
>>
>>
>>         if (null == configFile) {
>>
>>             // if cannot find configFile, use default logging.properties
>>
>>             configFile = new StringBuilder().append(
>>
>>                     System.getProperty("java.home")).append(File.separator)
>>
>>                     .append("lib").append(File.separator).append(
>>
>>                             "logging.properties").toString();
>>
>>         }
>>
>>
>>         InputStream input = null;
>>
>>         try {
>>
>>             input = new BufferedInputStream(new
>> FileInputStream(configFile));
>>
>>             readConfiguration(input);
>>
>>         } finally {
>>
>>             if (input != null) {
>>
>>                 try {
>>
>>                     input.close();
>>
>>                 } catch (Exception e) {// ignore
>>
>>                 }
>>
>>             }
>>
>>         }
>>
>>     }
>>
>>
>> In patches I'll be submitting, I'll use the second form, which is idiomatic
>> Java. I may also submit patches that convert the first style to the second,
>> but usually only when there's other useful cleanup to accompany it. If you'd
>> rather I not, please let me know and we'll arm-wrestle.
>>
>>
>> Cheers,
>> Jesse
>>
>



-- 
With best regards / с наилучшими пожеланиями,
Alexei Fedotov / Алексей Федотов,
http://www.telecom-express.ru/
http://harmony.apache.org/
http://www.expressaas.com/
http://openmeetings.googlecode.com/

Mime
View raw message