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
>
