----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3882/#review5157 -----------------------------------------------------------
src/org/waveprotocol/wave/model/wave/ParticipantId.java <https://reviews.apache.org/r/3882/#comment11265> Construct -> Constructs src/org/waveprotocol/wave/model/wave/ParticipantId.java <https://reviews.apache.org/r/3882/#comment11266> I think it would be better to add @Nullable annotation on the parameter src/org/waveprotocol/wave/model/wave/ParticipantId.java <https://reviews.apache.org/r/3882/#comment11267> an vector -> the array src/org/waveprotocol/wave/model/wave/ParticipantId.java <https://reviews.apache.org/r/3882/#comment11268> @throws InvalidParticipantAddress if at least one of the addresses failed validation. src/org/waveprotocol/wave/model/wave/ParticipantId.java <https://reviews.apache.org/r/3882/#comment11264> I think it would be better to put this logic into a static util class, or even just a static method in ParticipantController. The logic of building a list of participants from a string with optional default domain is relevant mostly for client side, and not generic enough to be put inside ParticipantId class. src/org/waveprotocol/wave/model/wave/ParticipantId.java <https://reviews.apache.org/r/3882/#comment11261> Please add spaces int i=0;i<addressList.length;i++ -> int i = 0; i < addressList.length; i++ src/org/waveprotocol/wave/model/wave/ParticipantId.java <https://reviews.apache.org/r/3882/#comment11262> Seems like this check can be done outside of the loop. src/org/waveprotocol/wave/model/wave/ParticipantId.java <https://reviews.apache.org/r/3882/#comment11263> Please remove empty line test/org/waveprotocol/wave/model/wave/ParticipantIdTest.java <https://reviews.apache.org/r/3882/#comment11272> Please makes sure we follow consistent indentation test/org/waveprotocol/wave/model/wave/ParticipantIdTest.java <https://reviews.apache.org/r/3882/#comment11269> Please add spaces before and after all arithmetical operators. test/org/waveprotocol/wave/model/wave/ParticipantIdTest.java <https://reviews.apache.org/r/3882/#comment11271> Why using compareTo instead of equals? test/org/waveprotocol/wave/model/wave/ParticipantIdTest.java <https://reviews.apache.org/r/3882/#comment11270> Please fix indentation. - Yuri On 2012-02-15 22:54:41, rocklund wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3882/ > ----------------------------------------------------------- > > (Updated 2012-02-15 22:54:41) > > > Review request for wave. > > > Summary > ------- > > Implemented as suggested in the jira-issue as a client side implementation in > ParticipantController.java. I selected the separator for the multiple > participant list to be semi-colon(;). > > > This addresses bug wave-317. > https://issues.apache.org/jira/browse/wave-317 > > > Diffs > ----- > > > src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java > ec9e25a > src/org/waveprotocol/wave/model/wave/ParticipantId.java a5dbdf6 > test/org/waveprotocol/wave/model/wave/ParticipantIdTest.java 58a2772 > > Diff: https://reviews.apache.org/r/3882/diff > > > Testing > ------- > > Compiled and run. > > * Tested to add single as well as multiple participants > * Tested to add participants both with and without @localhost > * Tested to add invalid participants > > > Thanks, > > rocklund > >
