fixed in revision 11826

The patch file works fine for me with the current version of the trunk. It modfies 1 file in the documentation folder and 3 files in the source folder, adds one file in the test-unit folder (RosterOfflineTest.java) and adds/modifies 4 files in the test folder. I can apply the patch with Eclipse (Team->Apply Patch...) to the root of the Smack project folder.

Just in case there is something wrong with the patch file I recreated the patch using the "svn diff" command. Maybe this one works better.

Hello Henning,

These are good improvements. I like particularly like the IllegalStateException instead of null values.

The v2 patch does not apply properly against my checkout of Smack. Is that me, or is the patch not correct?

I noticed some new Test classes the are now in the same source directory as the 'regular' code in /source/. This might not be intentional (and caused by the erroneous patch problem). In any case, I would prefer not to have any test classes (unit tests or other tests) in the source directory that is used to build the deliverables (smack.jar, smackx.jar, etc). Instead, I'd prefer if they go in another directory instead. This will make it very easy for us to avoid having those classes in the deliverables (without having to resort to filtering on classname). There are at least two other directories that you could use for this purpose (/test/ and /test-unit/). If you need yet another directory, thats fine by me too.

Hi Guus,

thanks for the review.
The XMPPConnection#getRoster() method always returns a Roster now. I've changed the javadoc so that it explicitly mentions that the method will not return null. I also added comments to the code snippet you mentioned to make clear how the roster is initialized. The snippet is now surrounded by a synchronized block to assure thread-safety in case #login() is executed concurrently.

All if statements now have curly braces.

The test cases are now separated in unit test and Smack test cases. The test methods may now throw an exception and I removed the try/catch/finally blocks. Where appropriate I used the @Test(expected = ???) annotation to verify an exception is thrown.

Additionally I changed the condition when the modifying methods of Roster will throw an IllegalStateException. It is thrown if the connection is not authenticated or the login was anonymously (which are the same conditions when the old #getRoster() implementation returned null).
So the only odd thing left is that it is now possible to get a roster and register roster listener which will have no effect if the user logs in anonymously. But I think that's OK.

Hello Henning,

Thank you for your patch. I have reviewed it, and would like you to consider the following:

((XMPPConnection#getRoster())) seems to rely on the fact that a roster will be instantiated while the connection is not (yet) authenticated. From the snippet of code below, it is unclear what happens if getRoster() is called for the first time only after the connection was authenticated. If this cannot happen, please document this. In any case, the javadoc (as defined in the interface) should explicitly tell users if this method can be expected to return null values. As we're changing existing behavior, it is wise to be overly-clear.

The snippet of code above contains a check-then-act pattern. How thread-safe is this code?

Please add curly braces to the code blocks of if-statements, even if they are one line only.

My last comment perhaps more a matter of style, but for your consideration, here goes: I notice that you wrap the methods of a unit tests in big try/catch/finally blocks. I typically do not. Instead, I (by default/template!) have my unit tests throw Exception. This allows you to have two distinct types of failure: a failure that is caused by an (unexpected) exception, and another type of failure related to something that you assert. This helps you to distinct between problems in unit test code (an exception is thrown, or something unexpected happened) and problems caused by a pattern that you're specifically testing for. In places, this can be a gray area, and it's certainly not a pattern that is fail-proof or has huge benefits, but in general, this pattern has helped me to clarify unexpected test results.