Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

And some more SonarQube issues #634

Merged
merged 4 commits into from Feb 15, 2015

Conversation

Projects
None yet
3 participants
@hansjoachim
Copy link
Contributor

commented Feb 15, 2015

This mainly focuses on:
A) Array designator should be on the type, not the variable.
B) Prefer interfaces over concrete classes for collections.

Also sprinkled with some Override annotations and various other fixes at the same time.

When it comes to use of interfaces in B, I've fixed most of the instances regarding either internal usage or parameters coming from the outside. I don't see any regression potential in the former and for the latter a parameter which accepts HashMaps can be generalized to accept Maps because any HashMap will also be a Map. However, I have not touched return types for public methods. A method returning a LinkedList could potentially return a Collection, but since it is part of the public API I have no control over what people have built around and used the return value for. Changing this could potentially break code on the outside, so I've left them alone for now.

There were also a couple of places which specify a LinkedList as the type and are using methods from both List and Deque, so I wasn't able to reduce the type to a single interface.

@amolenaar amolenaar added this to the Next release milestone Feb 15, 2015

@amolenaar

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2015

You did a lot of code refinements over the passed row PR's. Thanks.

However, I have not touched return types for public methods. A method returning a LinkedList could potentially return a Collection, but since it is part of the public API I have no control over what people have built around and used the return value for.

I think changing return types to their most specific interface should not pose any harm. We can not move forward without improving the existing code.

@hansjoachim

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2015

You're welcome. I think a good first step is to reduce the overall amount of issues both because most of them are straight-forward fixes which improves code quality but also to make it easier to see when new issues are introduced. You may already be aware of this, but the SonarQube page allows us to see changes over time and between versions. It looks like it is scanned each Saturday so it is easy to see which issues were introduced during the last week of development.

I think changing return types to their most specific interface should not pose any harm. We can not move forward without improving the existing code.

Ok, I figured I'd better check first before I started working on them. It is part of the public API afterall, and I'm not familiar with FitNesse's policy on API changes.
Changing to the most specific interface is probably the least intrusive change, though it will still require changes for calling code. For instance, say we have the following piece of code as part of the API:

  public LinkedList<Object> getSomething() {
    return new LinkedList<Object>();
  }

and someone using FitNesse has used this to implement the following method in their code:

  public void outside() {
    LinkedList<Object> l = getSomething();
    doStuff(l);
  }

So they will need to change the type they assign the result to. That shouldn't be too hard if we go for the most specific one as you mentioned, but still require changes.

As mentioned above, I'm really not sure how to replace LinkedList though, since it implements both List and Deque, which makes it a bit hard to pick which one should be used. (I realize this is proabably one of the reason interfaces are prefered over classes...) "Single-interface" classes shouldn't be a problem though.

@ggramlich

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2015

Hi Hans-Joachim, hi Arjan,

I am not quite sure, if there is a misunderstanding between both of you (and probably me as well).
Arjan wrote: changing return types to their most specific interface
I am not sure, if he specifically meant interface vs. class or just interface as sth. like "type".
But be aware, that he did not write least specific.

Be strict in what you return and flexible in what you accept.

See http://stackoverflow.com/a/18124954/32679 and http://en.wikipedia.org/wiki/Robustness_principle

@amolenaar amolenaar merged commit b2d03ea into unclebob:master Feb 15, 2015

amolenaar added a commit that referenced this pull request Feb 15, 2015

Merge pull request #634 from hansjoachim/otherissues
And some more SonarQube issues
@amolenaar

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2015

Hi, I referred to Java interfaces (not classes).

LinkedList is a tricky one, I agree. I tend to go for List by default, since that's more generally used.

@hansjoachim

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2015

Hi ggramlich, thanks for stopping by and fixing my misconceptions. :)

Btw, ActionFixture.java contains a TODO(code review) which I planned on bringing up in the code review, because I wasn't sure whether to fix the issue or not. Moving the [] to the end of the type seems like it would look rather cluttered so I don't know whether that would be better or worse. (I should probably have mentioned it, but I've gotten into the habit of marking things for future discussion and didn't think about it when submitting the PR.)

@hansjoachim hansjoachim deleted the hansjoachim:otherissues branch Mar 1, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.