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
