Hi all, I just integrated Carlos Vara's suggestions (thanks a lot!!!) and tests seem working nicely. I'd ask once again a little review and, if it is ok for you, I'd start moving the module from the sandbox to the top level. Please let me know!!! :) Best regards, have a nice day, Simo
http://people.apache.org/~simonetripodi/ On Mon, May 10, 2010 at 10:37 PM, Donald Woods <[email protected]> wrote: > Great review Carlos! > > Simone, I haven't had time to look thoroughly at the code, but it looks > like it uses BVAL correctly and is a good complement module. I was able > to build the module and it passed rat:check, so I'd vote for bringing it > over into trunk. > > > -Donald > > > On 5/9/10 6:10 PM, Carlos Vara wrote: >> Hi Simone, >> >> I took a look at the code, very good work there! >> >> I'm afraid I can't comment much about the guice part as I'm more of a spring >> man myself, but I can comment a little on the validation part. >> >> First, just to check that I understood what it does right: >> - It creates an interceptor for method calls annotated with @Validate and >> validates all the arguments. In case any validation error is found, it >> throws a CVE. >> >> If the above is right, a few pointers on the validation part: >> >> - If you validate the arguments individually, maybe having an extra >> annotation in them can help improve the speed of the code. Take care that >> calling validator.validate(..) for all basic types (Strings, Integers, etc.) >> will always output an empty set, and those arguments are very common. >> You can use @Valid as a marker for the arguments that you want to validate, >> this way, a validated method call would be like this: >> >> @Validate >> public String myMethod(String notValidated, @Valid Person validatedPerson) { >> ... } >> >> You may decide to validate all arguments in case @Validate is present and no >> argument is annotated with @Valid. >> >> - Although it is not portable, you may also want to take a look at the >> method validation API that the Apache implementation has. It works quite >> well in this scenario. I posted a quick implementation I did with AspectJ >> here: >> http://carinae.net/2010/04/automatic-validation-method-calls-with-jsr-303-appendix-c/ >> If you are interested, you could do a second interceptor that takes >> advantage of it in case the JSR-303 implementation is the apache one. >> >> - One last thing, in ValidateMethodInterceptor.invoke(), you may want to >> also validate the return value before returning. >> >> Oh, and my non-binding opinion about moving it out of the sandbox would be a >> yes. All the above comments are just possible improvements, IMHO code uses >> the validation API correctly to solve an interesting problem. >> >> Regards, >> Carlos >> >> >> >> On Sat, May 8, 2010 at 1:07 PM, Simone Tripodi >> <[email protected]>wrote: >> >>> Hi all guys, >>> I'd like to ask you what you think about moving the actual google >>> guice integration support from sandbox to current modules. >>> I just integrated the groups support in the AOP interceptor and it >>> seems working nicely, at least for me, I'd really would like if >>> someone could provide feedbacks. >>> Every kind of help will be more than appreciated, thanks in advance! >>> Simo >>> >>> http://people.apache.org/~simonetripodi/<http://people.apache.org/%7Esimonetripodi/> >>> >> >
