Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

refactoring and clean code in project #52

Closed
wants to merge 1 commit into from

2 participants

@otaviojava

Loose coupling
unnecessary local before return
final
Unused fields and imports
if, else, do, while are clearer when have the braces
Redundant 'public' modifier in interface

@otaviojava otaviojava String to String is unnecessary
Loose coupling
unnecessary local before return
final 
Unused fields and imports
if, else, do, while are clearer when have the braces
Redundant 'public' modifier in interface
d877dfc
@pcmanus
Owner

Most of this happens to be adding braces after if and else, which I'm afraid I don't particularly like, especially since the patch doesn't respect the code style of the rest of the code. I have nothing against most of the rest (though it groups some imports in ways I'd rather not too, but that's a detail) but that's lost in the middle of braces addition and is at best cosmetic.

@otaviojava

I swept all the code.
I believe that all code will have only one style.

@pcmanus
Owner

By code style, I was referring to 1) the lack of space before the opening bracket and 2) there is a few places where a 'else' is not on the same line that the preceding closing bracket.

But I'm sorry I wasn't clear, this was a detail. The "important" part is that I like the code the way it is, without braces after one liner 'if'. I fully respect that you may prefer otherwise and I'm sorry our preferences differ on that issue, but as this really is a matter of preferences, I'm going to stick to my own (and I must add that I'm not really interested in debate on code style). And since 95% of the changes of the patch concerned braces addition, I'm not going to merge this patch.

As for the other changes contained in the patch, it's hard to check them all given the size of the patch, but if you pull them out in more digestible commits, I'm fine having a look.

@pcmanus pcmanus closed this
@otaviojava

What do you think about the other changes ?
Loose coupling
unnecessary local before return
final
Unused fields and imports
Redundant 'public' modifier in interface

@pcmanus
Owner

To be honest with you, and while I don't pretend having looked at all the details, this mostly seems to make code style changes so that it looks the way you like, but I don't really prefer your changes myself. For instance, I kind of like having 'public' for interface members (yes it's redundant, but I find it handy to have the visibility in the method signature directly for a few minor reasons). That being said, I'm clearly fine adding a few missing 'final' and removing unused fields for instance. As for the 'loose coupling', I'm not sure to what you are refering to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 30, 2013
  1. @otaviojava

    String to String is unnecessary

    otaviojava authored
    Loose coupling
    unnecessary local before return
    final 
    Unused fields and imports
    if, else, do, while are clearer when have the braces
    Redundant 'public' modifier in interface
Something went wrong with that request. Please try again.