search

fabriciocolombo / sonar-delphi

SonarQube Delphi Plugin
91 stars 46 forks source link

Refactored DelphiRule.visit and DelphiRule.addViolation methods #22

Closed Teloah closed 8 years ago

Teloah commented 8 years ago

Update. I found AbstractJavaRule javadoc and discovered that these methods simply mimic the signature of all those other visit methods. So I'm not so sure about this PR anymore. On the other hand - do we need to keep this signature? Can this change be the source of problems sometime later?

I started to learn what information I have available in the rule's visit method. And the first intriguing thing was that mystical Object data that's received, used only in addViolation call and then returned back. So I found the only place where visit gets called to see what's really passed in. And found out that it is an instance of RuleContext. Also I noticed that return value is not used, which means that return data; line in the rule is meaningless. So I updated the signature to match the way it really is used.

Then I noticed that the same pattern of receiving plain Object and then casting it to the real type of RuleContext is used a little below in 3 addViolation methods. Again, I saw no bonus to do that (and nothing else can be passed in anyway, cause then we'll immediately get java.lang.ClassCastException, right?) and updated the signature to match the reality.

And all the tests still pass, so why not?

Teloah commented 8 years ago

I fixed a couple of bugs in CountRule and ClassPerFileRule, but am waiting for this PR to be accepted or rejected first.

fabriciocolombo commented 8 years ago

I will review today

fabriciocolombo commented 8 years ago

I can not see problems in changing the signature. Now it's clear the meaning of the arguments. And we can always go back if something goes wrong. Thank you