I agree that the code readability is good. No doubt here.
Unfortunately, rewriting the code which works fine but does not follow
some our internal vision can easily offend the original authors and
provoke holy wars.
I'm following the same policy while accepting or rejecting the code
into the project: if the code works fine, do not have visible
performance or quality issues, but formatted in the way I do not like
much then I better accept it with minimal modifications to keep the
author happy. And let him work on the other real problems but try to
convince me to return back his original code or explain my
modifications.
However, I feel ok to rewrite the code I do not like completely if I'm
making some important modifications :)
Thanks.
Alexey
2009/10/27 Tim Ellison <t.p.elli...@gmail.com>:
> 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
>>
>
