Now we have supports for Valued Parameters #290
Conversation
| import br.com.caelum.vraptor.http.ParanamerNameProvider; | ||
| import br.com.caelum.vraptor.http.ValuedParameter; | ||
|
|
||
| public class ValuedParameterProducer { |
There was a problem hiding this comment.
This class is useful to create ValuedParameters for a method. But I don't know the best location for this class.
| @@ -97,7 +98,8 @@ public void deserializes(@Observes ReadyToExecuteMethod event, HttpServletReques | |||
| // return a defensive copy | |||
| for (int i = 0; i < deserialized.length; i++) { | |||
| if (deserialized[i] != null) { | |||
| parameters[i] = deserialized[i]; | |||
| // FIXME ot - change deserialize return to void, and change valued parameters into deserializer | |||
| parameters[i].setValue(deserialized[i]); | |||
There was a problem hiding this comment.
I'd put this invocation in method info.
methodInfo.setParameter(i, deserialized[i]);
There was a problem hiding this comment.
Why, since ValuedParameter already have the method setValue? If I already have an instance of ValuedParameter, I can simple call setValued instead methodInfo.setParameter.
There was a problem hiding this comment.
this way there is no need for the line 92 and DeserializerObserver knowing about ValueParameter. Less coupling ;)
There was a problem hiding this comment.
There are more changes or suggestions before merge?
|
Overall I didn't see much gain in this refactoring yet =( Maybe moving some of the parameter behavior to MethodInfo would be sufficient. |
|
After changes, can I merge? |
|
Ok, let's see where it is going ;) |
| public ValuedParameter[] getValuedParameters() { | ||
| if (valuedParameters == null) { | ||
| valuedParameters = new ValuedParameter[0]; | ||
| } |
There was a problem hiding this comment.
empty array is better than null? AIOOBoundsEx is better than NullPEx? Is this if necessary?
I think it is better to define where this changes are going, before merge this |
| public ControllerMethod getControllerMethod() { | ||
| return controllerMethod; | ||
| } | ||
|
|
||
| public void setControllerMethod(ControllerMethod controllerMethod) { | ||
| createValuedParameter(controllerMethod); |
There was a problem hiding this comment.
I think this method should be called on getValuedParameters because even if i'll never use,
it always create valued parameters when VRaptor sets ControllerMethod (and to create this, we
use parameterNameProvider, etc... it is kind of expensive). Maybe a simple lazy strategy:
if (valuedParameters != null) createValuedParameter(controllerMethod);
Now we have supports for Valued Parameters
Initial support for valued parameters. Now we can simple inject
MethodInfoto get all info about parameters like names, types, values ao so on.We don't need to to inject Paranamer neither to foreach a pair of arrays with names and values.
There are some task to do after this pull request, like change deserializers to void, and change values of parameters in deserializer. But I think that is better to send small pull requests, to delivery faster.
This pull request closes #104