Smaller. -- Jeremy Thomerson http://www.wickettraining.com
On Thu, Mar 25, 2010 at 12:51 PM, nino martinez wael < [email protected]> wrote: > Yup, no problem i'll try to do it while it's still spring :) Are > smaller patches better or is just a large chunk better? > > regards Nino > > 2010/3/25 Igor Vaynberg <[email protected]>: > > feel free to do this, but for trunk only. i would rather not tweak > > working code for the sake of making it "cleaner". > > > > also for this specific example there is Streams.close() that does this > > already... > > > > -igor > > > > On Thu, Mar 25, 2010 at 5:26 AM, nino martinez wael > > <[email protected]> wrote: > >> Well I do think there are some places that where code are somewhat > >> unreadable or hard to read. So it was just to make the code more > >> readable.. And making maintenance time go down. So some of it would be > >> following up against rule violations, but some of it would also be > >> tidying up code like this (from fileupload.java on 1.4.x branch), and > >> it can be further reduced below are just example: > >> > >> [original] > >> public final void closeStreams() > >> { > >> if (inputStreamsToClose != null) > >> { > >> for (Iterator<InputStream> inputStreamsIterator = > >> inputStreamsToClose.iterator(); inputStreamsIterator.hasNext();) > >> { > >> InputStream inputStream = > inputStreamsIterator.next(); > >> > >> try > >> { > >> inputStream.close(); > >> } > >> catch (IOException e) > >> { > >> // We don't care aobut the > exceptions thrown here. > >> } > >> } > >> > >> // Reset the list > >> inputStreamsToClose = null; > >> } > >> } > >> [modified] > >> public final void closeStreams() > >> { > >> if (inputStreamsToClose != null) > >> { > >> for (Iterator<InputStream> inputStreamsIterator = > >> inputStreamsToClose.iterator(); inputStreamsIterator.hasNext();) > >> { > >> > closeInputStream(inputStreamsIterator.next()); > >> } > >> > >> // Reset the list > >> inputStreamsToClose = null; > >> } > >> } > >> > >> private void closeInputStream(InputStream inputStream){ > >> try > >> { > >> inputStream.close(); > >> } > >> catch (IOException e) > >> { > >> // We don't care aobut the > exceptions thrown here. > >> } > >> > >> } > >> > >> It's up to you guys, it's defiantly not an easy task. Im just > >> following the example from Robert C Martin > >> ( > http://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882 > ) > >> ... In my daily work I've found it to be a very good practice. > >> > >> > >> > >> 2010/3/23 Igor Vaynberg <[email protected]>: > >>> lets not start tweaking code for tweaking's sake. > >>> > >>> -igor > >>> > >>> On Tue, Mar 23, 2010 at 11:57 AM, nino martinez wael > >>> <[email protected]> wrote: > >>>> Yeah I know, one has to have a sense of code regarding some of those > >>>> violations. But are very good to have as an indicator, at work I've > >>>> removed some of the rules from the rule set as they do not fit our > >>>> programming style. We value code readability very high, although it's > >>>> a constant battle between developers. > >>>> > >>>> Regarding reading code is a completely different matter. When I see > >>>> something like "!foo", I just read it "not foo". You can miss the > >>>> exclamation, but you can also miss one equal sign and have a fatal > >>>> problem. > >>>> > >>>> One of the most annoying ones are if you create something serializable > >>>> you also have to declare an id. Another one are that eclipse by > >>>> default generates methods that are pulled to an interface abstract, > >>>> but one of the rules says it's duplicate signature since it's an > >>>> interface :) So theres a secret battle between eclipse and the > >>>> ruleset.. > >>>> > >>>> But most of the are good. > >>>> > >>>> regards Nino > >>>> > >>>> 2010/3/23 Jeremy Thomerson <[email protected]>: > >>>>> I would reject patchs to fix some of those. Some of those so-called > >>>>> "violations" are just their coding style not being the same as ours. > >>>>> > >>>>> For instance, they say there are 218 "violations" where we have 'if > (foo == > >>>>> false)' - which they say should be simplified, I'm assuming to be 'if > >>>>> (!foo)'. Personally, I write mine as "foo == false" because it is > much > >>>>> harder to miss that than it is to miss "!" as you're reading through > the > >>>>> code. > >>>>> > >>>>> Another example: "empty method in abstract class should be abstract". > No, > >>>>> it shouldn't. It's a method designed to be overridden for additional > >>>>> functionality if you so desire. > >>>>> > >>>>> There might be some that are worth fixing. But as I mention, there > are some > >>>>> that are better left alone. > >>>>> > >>>>> -- > >>>>> Jeremy Thomerson > >>>>> http://www.wickettraining.com > >>>>> > >>>>> > >>>>> > >>>>> On Tue, Mar 23, 2010 at 6:39 AM, nino martinez wael < > >>>>> [email protected]> wrote: > >>>>> > >>>>>> Hi I wondered > >>>>>> > >>>>>> if it would be interesting if I started to make wicket more in > >>>>>> compliance with the rules defined here: > >>>>>> > http://nemo.sonarsource.org/drilldown/violations/44196?priority=MAJOR > >>>>>> ? > >>>>>> > >>>>>> I'd of course start by submitting patches.. > >>>>>> > >>>>>> So are it interesting? > >>>>>> > >>>>>> regards Nino > >>>>>> > >>>>> > >>>> > >>> > >> > > >
