search

Benoker / DockingFrames

http://www.docking-frames.org
96 stars 46 forks source link

squid:S1941 - Variables should not be declared before they are relevant #47

Closed m-ezzat closed 8 years ago

m-ezzat commented 8 years ago

This pull request is focused on resolving occurrences of Sonar rule squid:S1941 - Variables should not be declared before they are relevant

You can find more information about the issue here: https://dev.eclipse.org/sonar/coding_rules#q=squid:S1941

Please let me know if you have any questions.

M-Ezzat

Benoker commented 8 years ago

I took me about 5 minutes to find at least 3 serious bugs that would be introduced by this request. It's one of these "shotgun"-requests, blindly following a rule without looking at and understanding the code, that I do not like very much.

DockSituation line 551 You did change the way an InputStream is read - this will read to an exception on runtime

PredefinedDockStation line 825 You did change the way an InputStream is read - this will read to an exception on runtime

DefaultSplitLayoutManager line 180 It would be better to move make the constant a field (instead of keeping it as variable)

SplitNode line 1042 While the change makes the code more "correct", it does not make it more readable

MenuLineLayoutPane line 296 Switching from a "gather all information, then decide" style to "gather, decide, gather, decide, gather dicide" style does not make the code more readable

WizardSplitDockStationFactorz line 151 You did change the way an InputStream is read - this will read to an exception on runtime

Benoker commented 8 years ago

After some more thinking and locking at the code I decided to not accept this pull request. I really don't see how this change adds enough value to the project. And also: the bugs with the InputStream make me very nervous about changes that look as if they were done by an automated tool.