You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@camel.apache.org by bvahdat <ba...@swissonline.ch> on 2011/07/26 16:22:30 UTC
Favor static member classes over nonstatic
Hi Claus,
There're many places in Camel (including the unit-tests) where an inner
class is declared to be nonstatic although a static one would also do the
job. On ther other hand as stands in the Java-Bible[1] it makes sense in
such cases to move towards the static-ones. Best reason I see for this (as
already mentioned in [1]) is the avoidance of the extraneous reference to
it's enclosing instance which costs *time* and *space*. So that I can even
imagine that running the tests on the Apache CI-Servers would/could even run
faster!
I've already a patch available on my workspace, but before creating a ticket
(including the patch), I first wanted to check what the Lead-Rider thinks
about such a change.
Another question I have is regarding the patch I provided by [2]. Just
wonder why you didn't want to take over the 2 changes with
assertTrue(file.delete()) stuff in TestSupport:
// assert on the return value below so that we surely know that the
file has been deleted
// as it could be that some native processes're sitting on the file
prohibiting the deletion
// (i.e. on windows that could be the windows explorer, etc.)
assertTrue(file.delete());
Or maybe even better:
assertTrue("deletion of " + file.getAbsolutePath() + " failed!",
file.delete());
[1]
http://books.google.ch/books?id=ka2VUBqHiWkC&lpg=PA106&dq=effective%20java%20non%20static%20inner%20classes&pg=PA106#v=onepage&q&f=false
[2] https://issues.apache.org/jira/browse/CAMEL-4255
Regards, Babak
--
View this message in context: http://camel.465427.n5.nabble.com/Favor-static-member-classes-over-nonstatic-tp4634964p4634964.html
Sent from the Camel - Users mailing list archive at Nabble.com.
Re: Favor static member classes over nonstatic
Posted by bvahdat <ba...@swissonline.ch>.
By the way the change would involve 158 classes in trunk!
--
View this message in context: http://camel.465427.n5.nabble.com/Favor-static-member-classes-over-nonstatic-tp4634964p4635016.html
Sent from the Camel - Users mailing list archive at Nabble.com.
Re: Favor static member classes over nonstatic
Posted by bvahdat <ba...@swissonline.ch>.
FYI, I reopened the ticket [1] with some comments, feel free to close it
again if the missing patch part was REALLY on purpose.
[1] https://issues.apache.org/jira/browse/CAMEL-4255
Regards, Babak
--
View this message in context: http://camel.465427.n5.nabble.com/Favor-static-member-classes-over-nonstatic-tp4634964p4642102.html
Sent from the Camel - Users mailing list archive at Nabble.com.
Re: Favor static member classes over nonstatic
Posted by bvahdat <ba...@swissonline.ch>.
Hi Claus,
thanks for applying those 2 changes as well...
While I was looking into the conversation we had in this thread, I suddenly
realized that I misunderstood your answer completely regarding unit-tests,
that's the following answer of yours:
> For unit tests I doubt it have any effect, as the unit test is short
> lived and executed once when testing. In camel-core it could be
> another matter as that code is what you run in production.
Which I thought it has to do with the following question of mine:
> Another question I have is regarding the patch I provided by [2]. Just
> wonder why you didn't want to take over the 2 changes with
> assertTrue(file.delete()) stuff in TestSupport:
> // assert on the return value below so that we surely know that
> the file has been deleted
> // as it could be that some native processes're sitting on the
> file prohibiting the deletion
> // (i.e. on windows that could be the windows explorer, etc.)
> assertTrue(file.delete());
> Or maybe even better:
> assertTrue("deletion of " + file.getAbsolutePath() + " failed!",
> file.delete());
Just wonder what you really think about this change which was not taken over
in Subversion Commits for https://issues.apache.org/jira/browse/CAMEL-4255
In my case it was really vital, see my previous reply with HSQL stuff. I
just want to make it sure that it was not your IDE rejecting that change, as
it apparently happened with the last patch.
Regards, Babak
--
View this message in context: http://camel.465427.n5.nabble.com/Favor-static-member-classes-over-nonstatic-tp4634964p4639321.html
Sent from the Camel - Users mailing list archive at Nabble.com.
Re: Favor static member classes over nonstatic
Posted by Claus Ibsen <cl...@gmail.com>.
Hi Babak
Thanks for spotting. Somehow patch rejected those 2 files. I have
correct them now.
On Wed, Jul 27, 2011 at 4:06 PM, bvahdat <ba...@swissonline.ch> wrote:
> FYI, just realized that there're two more classes I've already provided in
> the patch which are missing by your revision 1151350:
>
> http://svn.apache.org/viewvc?view=revision&revision=1151350
>
> That's:
>
> EventRecord @
> https://svn.apache.org/repos/asf/camel/trunk/components/camel-quickfix/src/test/java/org/apache/camel/component/quickfixj/QuickfixjEngineTest.java
>
> EchoServiceImpl @
> https://svn.apache.org/repos/asf/camel/trunk/components/camel-jms/src/test/java/org/apache/camel/component/jms/temp/JmsReconnectTest.java
>
> Regards, Babak
>
> --
> View this message in context: http://camel.465427.n5.nabble.com/Favor-static-member-classes-over-nonstatic-tp4634964p4638912.html
> Sent from the Camel - Users mailing list archive at Nabble.com.
>
--
Claus Ibsen
-----------------
FuseSource
Email: cibsen@fusesource.com
Web: http://fusesource.com
Twitter: davsclaus, fusenews
Blog: http://davsclaus.blogspot.com/
Author of Camel in Action: http://www.manning.com/ibsen/
Re: Favor static member classes over nonstatic
Posted by bvahdat <ba...@swissonline.ch>.
FYI, just realized that there're two more classes I've already provided in
the patch which are missing by your revision 1151350:
http://svn.apache.org/viewvc?view=revision&revision=1151350
That's:
EventRecord @
https://svn.apache.org/repos/asf/camel/trunk/components/camel-quickfix/src/test/java/org/apache/camel/component/quickfixj/QuickfixjEngineTest.java
EchoServiceImpl @
https://svn.apache.org/repos/asf/camel/trunk/components/camel-jms/src/test/java/org/apache/camel/component/jms/temp/JmsReconnectTest.java
Regards, Babak
--
View this message in context: http://camel.465427.n5.nabble.com/Favor-static-member-classes-over-nonstatic-tp4634964p4638912.html
Sent from the Camel - Users mailing list archive at Nabble.com.
Re: Favor static member classes over nonstatic
Posted by bvahdat <ba...@swissonline.ch>.
For any case I created the ticket (including the patch), see
https://issues.apache.org/jira/browse/CAMEL-4273 feel free to include the
patch or ignore/close the ticket.
> For unit tests I doubt it have any effect, as the unit test is short
> lived and executed once when testing. In camel-core it could be
> another matter as that code is what you run in production.
Indeed it happened to me while I was working on my project, while running
'mvn install' on one terminal, accidentally I ran the same test case inside
IDE. The tests had to do with my routes (containing camel-jpa component
using filesystem-based-hsql-db locally). At the start-up of the test cases I
used to cleanup the hsql-database on the file-system (@BeforeClass) to avoid
any side-effect of the previous run (using TestSupport.deleteDirectory())
and the assertions of my test-cases failed until I realized the other
terminal with transactions running on the *same* hsql-db using the *same*
test-data. So to say suddently I got 2 rows fulfilling the jpa-query instead
of only expected one row.
So I just copy/pasted TestSupport.deleteDirectory() into my utility-class
and adapted with that assertTrue so that by the next accident I realize that
before it baffles me.
Regards, Babak
--
View this message in context: http://camel.465427.n5.nabble.com/Favor-static-member-classes-over-nonstatic-tp4634964p4635206.html
Sent from the Camel - Users mailing list archive at Nabble.com.
Re: Favor static member classes over nonstatic
Posted by Claus Ibsen <cl...@gmail.com>.
On Tue, Jul 26, 2011 at 4:22 PM, bvahdat <ba...@swissonline.ch> wrote:
> Hi Claus,
>
> There're many places in Camel (including the unit-tests) where an inner
> class is declared to be nonstatic although a static one would also do the
> job. On ther other hand as stands in the Java-Bible[1] it makes sense in
> such cases to move towards the static-ones. Best reason I see for this (as
> already mentioned in [1]) is the avoidance of the extraneous reference to
> it's enclosing instance which costs *time* and *space*. So that I can even
> imagine that running the tests on the Apache CI-Servers would/could even run
> faster!
Yeah I can imagine there may be some inner clases that could be
converted to static, if they dont need access to their enclosing
class.
For unit tests I doubt it have any effect, as the unit test is short
lived and executed once when testing. In camel-core it could be
another matter as that code is what you run in production.
>
> I've already a patch available on my workspace, but before creating a ticket
> (including the patch), I first wanted to check what the Lead-Rider thinks
> about such a change.
>
> Another question I have is regarding the patch I provided by [2]. Just
> wonder why you didn't want to take over the 2 changes with
> assertTrue(file.delete()) stuff in TestSupport:
>
This caused unit tests to fail.
> // assert on the return value below so that we surely know that the
> file has been deleted
> // as it could be that some native processes're sitting on the file
> prohibiting the deletion
> // (i.e. on windows that could be the windows explorer, etc.)
> assertTrue(file.delete());
>
> Or maybe even better:
>
> assertTrue("deletion of " + file.getAbsolutePath() + " failed!",
> file.delete());
>
> [1]
> http://books.google.ch/books?id=ka2VUBqHiWkC&lpg=PA106&dq=effective%20java%20non%20static%20inner%20classes&pg=PA106#v=onepage&q&f=false
> [2] https://issues.apache.org/jira/browse/CAMEL-4255
>
> Regards, Babak
>
> --
> View this message in context: http://camel.465427.n5.nabble.com/Favor-static-member-classes-over-nonstatic-tp4634964p4634964.html
> Sent from the Camel - Users mailing list archive at Nabble.com.
>
--
Claus Ibsen
-----------------
FuseSource
Email: cibsen@fusesource.com
Web: http://fusesource.com
Twitter: davsclaus, fusenews
Blog: http://davsclaus.blogspot.com/
Author of Camel in Action: http://www.manning.com/ibsen/