You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@wicket.apache.org by Per Newgro <pe...@gmx.ch> on 2011/06/08 08:53:31 UTC
[WICKET-3552] Is adding another parameter to the constructor required?
Hi,
i've traced changes in 1.5-RC4.2. Found the issue 3552 [
https://issues.apache.org/jira/browse/WICKET-3552 ]. If i'm not
completely in the wood the solution breaks the
SingleResponsibilityPrinciple by adding a flag to the constructor
which negates the class behavior.
Wouldn't it be more clear and reusable if we had another
AttributePrepender which acts like the AttributeAppender.
Both could extend the base-class AbstractAttributeRegister or
something. Then only the different part of the newValue method has
to be implemented by the concrete class.
I only ask because if 1.5 is out it will be hard to exchange because
many apps will be upgraded and feature is maybe used :-).
What do you think?
Cheers
Per
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [WICKET-3552] Is adding another parameter to the constructor required?
Posted by Igor Vaynberg <ig...@gmail.com>.
why not roll this into attributemodifier and have mode { replace,
append, prepand } where replace is how the modifier works now...
-igor
On Wed, Jun 8, 2011 at 12:07 AM, Martin Grigorov <mg...@apache.org> wrote:
> Hi,
>
> I also had the same thoughts when I added the flag (as the patch
> suggested) but AttributeAppender is a class with just constructor
> overrides and one method override (#newValue()). If we introduce yet
> another class for prepend then there is no sense in "the one common
> parent" because they have nothing to share. Both of them will have the
> same number of constructors and this override of #newValue().
>
> I agree that now the name AttributeAppender is not the best but I
> cannot find a better one and even if we find it then we will have to
> rename this class which I think is used a lot
>
> On Wed, Jun 8, 2011 at 8:53 AM, Per Newgro <pe...@gmx.ch> wrote:
>> Hi,
>>
>> i've traced changes in 1.5-RC4.2. Found the issue 3552 [
>> https://issues.apache.org/jira/browse/WICKET-3552 ]. If i'm not
>> completely in the wood the solution breaks the
>> SingleResponsibilityPrinciple by adding a flag to the constructor
>> which negates the class behavior.
>> Wouldn't it be more clear and reusable if we had another
>> AttributePrepender which acts like the AttributeAppender.
>> Both could extend the base-class AbstractAttributeRegister or
>> something. Then only the different part of the newValue method has
>> to be implemented by the concrete class.
>>
>> I only ask because if 1.5 is out it will be hard to exchange because
>> many apps will be upgraded and feature is maybe used :-).
>>
>> What do you think?
>> Cheers
>> Per
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
>> For additional commands, e-mail: users-help@wicket.apache.org
>>
>>
>
>
>
> --
> Martin Grigorov
> jWeekend
> Training, Consulting, Development
> http://jWeekend.com
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [WICKET-3552] Is adding another parameter to the constructor required?
Posted by Martijn Dashorst <ma...@gmail.com>.
On Wed, Jun 8, 2011 at 11:26 AM, Martin Grigorov <mg...@apache.org> wrote:
> If you have the karma you can miss the attaching part and commit
> directly in SVN
> dashorst ^^
Moved it to dev@ instead...
Martijn
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [WICKET-3552] Is adding another parameter to the constructor required?
Posted by Martin Grigorov <mg...@apache.org>.
Jira and up and running and the last time I checked it still supports
attaching files (patches) :-)
If you have the karma you can miss the attaching part and commit
directly in SVN
dashorst ^^
2011/6/8 Robert Dahlström <ro...@ongame.com>:
> +1 on skipping a boolean flag in the constructor. Igors suggestion to
> provide a mode parameter feels like a good one.
>
> /Robert
>
> On 06/08/2011 10:18 AM, Martin Grigorov wrote:
>>
>> Hi Per,
>>
>> I see your point.
>> It is just my opinion (and ticket's reporter) that this additional
>> boolean parameter to the constructor is adds much less noise than
>> adding additional 2 classes.
>> If the name of AttributeAppender is changed to AtrributeAdder (because
>> we are used that add(obj) appends and add(0, obj) prepends) then it
>> will be much cleaner.
>>
>> Igor's suggestion also sounds OK to me. Then we can either deprecate
>> AttributeAppender or add AttributePrepender and both of them will just
>> have constructors that pass the proper mode to AttributeModifier.
>>
>> Create a ticket.
>>
>> On Wed, Jun 8, 2011 at 10:04 AM, Per Newgro<pe...@gmx.ch> wrote:
>>>
>>> Hi Martin Grigorov,
>>>
>>> But i made naming proposals:
>>> AbstractAttributeRegister
>>> | |
>>> AttributeAppender AttributePrepender
>>>
>>> My pain in the ... is only that we have now 2 more constructors. This (in
>>> my
>>> opinion)
>>> really important class should be clear in design and responsibility.
>>> Another pain could become important if we decide to exchange the default
>>> values
>>> for prepend. Wicket did that already in history and it lead to confusion
>>> and
>>> required
>>> app changes.
>>>
>>> The sense in "the one common parent" would be to provide the abstract
>>> method
>>> for value registering. The subclasses would have to implement that.
>>>
>>> package org.apache.wicket.behavior;
>>>
>>> import org.apache.wicket.AttributeModifier;
>>> import org.apache.wicket.model.IModel;
>>> import org.apache.wicket.util.string.AppendingStringBuffer;
>>> import org.apache.wicket.util.string.Strings;
>>>
>>> public class AbstractAttributeRegister extends AttributeModifier
>>> {
>>> private static final long serialVersionUID = 1L;
>>>
>>> private final String separator;
>>>
>>> public AbstractAttributeRegister(String attribute, boolean
>>> addAttributeIfNotPresent,
>>> IModel<?> appendModel, String separator)
>>> {
>>> this(attribute, addAttributeIfNotPresent, appendModel, separator);
>>> }
>>>
>>> public AbstractAttributeRegister(String attribute, IModel<?>
>>> appendModel,
>>> String separator)
>>> {
>>> this(attribute, true, appendModel, separator);
>>> }
>>>
>>>
>>> /**
>>> * @see org.apache.wicket.AttributeModifier#newValue(java.lang.String,
>>> java.lang.String)
>>> */
>>> @Override
>>> protected String newValue(String currentValue, String appendValue)
>>> {
>>> // Shortcut for empty values
>>> if (Strings.isEmpty(currentValue))
>>> {
>>> return appendValue != null ? appendValue : "";
>>> }
>>> else if (Strings.isEmpty(appendValue))
>>> {
>>> return currentValue;
>>> }
>>>
>>> final AppendingStringBuffer sb = new
>>> AppendingStringBuffer(currentValue.length() +
>>> appendValue.length() + separator.length());
>>>
>>> return registerValue(sb, currentValue, appendValue);
>>> }
>>>
>>> protected abstract String registerValue(AppendingStringBuffer target,
>>> String currentValue, String appendValue);
>>> }
>>>
>>> public class AttributeAppender extends AbstractAttributeRegister
>>> {
>>> private static final long serialVersionUID = 1L;
>>>
>>> public AttributeAppender(String attribute, boolean
>>> addAttributeIfNotPresent,
>>> IModel<?> appendModel, String separator)
>>> {
>>> super(attribute, addAttributeIfNotPresent, appendModel,
>>> separator);
>>> }
>>>
>>> public AttributeAppender(String attribute, IModel<?> appendModel,
>>> String
>>> separator)
>>> {
>>> super(attribute, appendModel, separator);
>>> }
>>>
>>> @Override
>>> protected String registerValue(AppendingStringBuffer target, String
>>> currentValue, String value, String separator)
>>> {
>>> sb.append(currentValue);
>>> sb.append(separator);
>>> sb.append(value);
>>> return sb.toString();
>>> }
>>> }
>>>
>>> public class AttributePrepender extends AbstractAttributeRegister
>>> {
>>> private static final long serialVersionUID = 1L;
>>>
>>> public AttributePrepender(String attribute, boolean
>>> addAttributeIfNotPresent,
>>> IModel<?> appendModel, String separator)
>>> {
>>> super(attribute, addAttributeIfNotPresent, appendModel,
>>> separator);
>>> }
>>>
>>> public AttributePrepender(String attribute, IModel<?> appendModel,
>>> String
>>> separator)
>>> {
>>> super(attribute, appendModel, separator);
>>> }
>>>
>>> @Override
>>> protected String registerValue(AppendingStringBuffer target, String
>>> currentValue, String value, String separator)
>>> {
>>> sb.append(value);
>>> sb.append(separator);
>>> sb.append(currentValue);
>>> return sb.toString();
>>> }
>>> }
>>>
>>>
>>>
>>>> Hi,
>>>>
>>>> I also had the same thoughts when I added the flag (as the patch
>>>> suggested) but AttributeAppender is a class with just constructor
>>>> overrides and one method override (#newValue()). If we introduce yet
>>>> another class for prepend then there is no sense in "the one common
>>>> parent" because they have nothing to share. Both of them will have the
>>>> same number of constructors and this override of #newValue().
>>>>
>>>> I agree that now the name AttributeAppender is not the best but I
>>>> cannot find a better one and even if we find it then we will have to
>>>> rename this class which I think is used a lot
>>>>
>>>> On Wed, Jun 8, 2011 at 8:53 AM, Per Newgro<pe...@gmx.ch> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> i've traced changes in 1.5-RC4.2. Found the issue 3552 [
>>>>> https://issues.apache.org/jira/browse/WICKET-3552 ]. If i'm not
>>>>> completely in the wood the solution breaks the
>>>>> SingleResponsibilityPrinciple by adding a flag to the constructor
>>>>> which negates the class behavior.
>>>>> Wouldn't it be more clear and reusable if we had another
>>>>> AttributePrepender which acts like the AttributeAppender.
>>>>> Both could extend the base-class AbstractAttributeRegister or
>>>>> something. Then only the different part of the newValue method has
>>>>> to be implemented by the concrete class.
>>>>>
>>>>> I only ask because if 1.5 is out it will be hard to exchange because
>>>>> many apps will be upgraded and feature is maybe used :-).
>>>>>
>>>>> What do you think?
>>>>> Cheers
>>>>> Per
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
>>>>> For additional commands, e-mail: users-help@wicket.apache.org
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
>>> For additional commands, e-mail: users-help@wicket.apache.org
>>>
>>>
>>
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
>
--
Martin Grigorov
jWeekend
Training, Consulting, Development
http://jWeekend.com
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [WICKET-3552] Is adding another parameter to the constructor
required?
Posted by Robert Dahlström <ro...@ongame.com>.
+1 on skipping a boolean flag in the constructor. Igors suggestion to
provide a mode parameter feels like a good one.
/Robert
On 06/08/2011 10:18 AM, Martin Grigorov wrote:
> Hi Per,
>
> I see your point.
> It is just my opinion (and ticket's reporter) that this additional
> boolean parameter to the constructor is adds much less noise than
> adding additional 2 classes.
> If the name of AttributeAppender is changed to AtrributeAdder (because
> we are used that add(obj) appends and add(0, obj) prepends) then it
> will be much cleaner.
>
> Igor's suggestion also sounds OK to me. Then we can either deprecate
> AttributeAppender or add AttributePrepender and both of them will just
> have constructors that pass the proper mode to AttributeModifier.
>
> Create a ticket.
>
> On Wed, Jun 8, 2011 at 10:04 AM, Per Newgro<pe...@gmx.ch> wrote:
>> Hi Martin Grigorov,
>>
>> But i made naming proposals:
>> AbstractAttributeRegister
>> | |
>> AttributeAppender AttributePrepender
>>
>> My pain in the ... is only that we have now 2 more constructors. This (in my
>> opinion)
>> really important class should be clear in design and responsibility.
>> Another pain could become important if we decide to exchange the default
>> values
>> for prepend. Wicket did that already in history and it lead to confusion and
>> required
>> app changes.
>>
>> The sense in "the one common parent" would be to provide the abstract method
>> for value registering. The subclasses would have to implement that.
>>
>> package org.apache.wicket.behavior;
>>
>> import org.apache.wicket.AttributeModifier;
>> import org.apache.wicket.model.IModel;
>> import org.apache.wicket.util.string.AppendingStringBuffer;
>> import org.apache.wicket.util.string.Strings;
>>
>> public class AbstractAttributeRegister extends AttributeModifier
>> {
>> private static final long serialVersionUID = 1L;
>>
>> private final String separator;
>>
>> public AbstractAttributeRegister(String attribute, boolean
>> addAttributeIfNotPresent,
>> IModel<?> appendModel, String separator)
>> {
>> this(attribute, addAttributeIfNotPresent, appendModel, separator);
>> }
>>
>> public AbstractAttributeRegister(String attribute, IModel<?> appendModel,
>> String separator)
>> {
>> this(attribute, true, appendModel, separator);
>> }
>>
>>
>> /**
>> * @see org.apache.wicket.AttributeModifier#newValue(java.lang.String,
>> java.lang.String)
>> */
>> @Override
>> protected String newValue(String currentValue, String appendValue)
>> {
>> // Shortcut for empty values
>> if (Strings.isEmpty(currentValue))
>> {
>> return appendValue != null ? appendValue : "";
>> }
>> else if (Strings.isEmpty(appendValue))
>> {
>> return currentValue;
>> }
>>
>> final AppendingStringBuffer sb = new
>> AppendingStringBuffer(currentValue.length() +
>> appendValue.length() + separator.length());
>>
>> return registerValue(sb, currentValue, appendValue);
>> }
>>
>> protected abstract String registerValue(AppendingStringBuffer target,
>> String currentValue, String appendValue);
>> }
>>
>> public class AttributeAppender extends AbstractAttributeRegister
>> {
>> private static final long serialVersionUID = 1L;
>>
>> public AttributeAppender(String attribute, boolean
>> addAttributeIfNotPresent,
>> IModel<?> appendModel, String separator)
>> {
>> super(attribute, addAttributeIfNotPresent, appendModel, separator);
>> }
>>
>> public AttributeAppender(String attribute, IModel<?> appendModel, String
>> separator)
>> {
>> super(attribute, appendModel, separator);
>> }
>>
>> @Override
>> protected String registerValue(AppendingStringBuffer target, String
>> currentValue, String value, String separator)
>> {
>> sb.append(currentValue);
>> sb.append(separator);
>> sb.append(value);
>> return sb.toString();
>> }
>> }
>>
>> public class AttributePrepender extends AbstractAttributeRegister
>> {
>> private static final long serialVersionUID = 1L;
>>
>> public AttributePrepender(String attribute, boolean
>> addAttributeIfNotPresent,
>> IModel<?> appendModel, String separator)
>> {
>> super(attribute, addAttributeIfNotPresent, appendModel, separator);
>> }
>>
>> public AttributePrepender(String attribute, IModel<?> appendModel, String
>> separator)
>> {
>> super(attribute, appendModel, separator);
>> }
>>
>> @Override
>> protected String registerValue(AppendingStringBuffer target, String
>> currentValue, String value, String separator)
>> {
>> sb.append(value);
>> sb.append(separator);
>> sb.append(currentValue);
>> return sb.toString();
>> }
>> }
>>
>>
>>
>>> Hi,
>>>
>>> I also had the same thoughts when I added the flag (as the patch
>>> suggested) but AttributeAppender is a class with just constructor
>>> overrides and one method override (#newValue()). If we introduce yet
>>> another class for prepend then there is no sense in "the one common
>>> parent" because they have nothing to share. Both of them will have the
>>> same number of constructors and this override of #newValue().
>>>
>>> I agree that now the name AttributeAppender is not the best but I
>>> cannot find a better one and even if we find it then we will have to
>>> rename this class which I think is used a lot
>>>
>>> On Wed, Jun 8, 2011 at 8:53 AM, Per Newgro<pe...@gmx.ch> wrote:
>>>>
>>>> Hi,
>>>>
>>>> i've traced changes in 1.5-RC4.2. Found the issue 3552 [
>>>> https://issues.apache.org/jira/browse/WICKET-3552 ]. If i'm not
>>>> completely in the wood the solution breaks the
>>>> SingleResponsibilityPrinciple by adding a flag to the constructor
>>>> which negates the class behavior.
>>>> Wouldn't it be more clear and reusable if we had another
>>>> AttributePrepender which acts like the AttributeAppender.
>>>> Both could extend the base-class AbstractAttributeRegister or
>>>> something. Then only the different part of the newValue method has
>>>> to be implemented by the concrete class.
>>>>
>>>> I only ask because if 1.5 is out it will be hard to exchange because
>>>> many apps will be upgraded and feature is maybe used :-).
>>>>
>>>> What do you think?
>>>> Cheers
>>>> Per
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
>>>> For additional commands, e-mail: users-help@wicket.apache.org
>>>>
>>>>
>>>
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
>> For additional commands, e-mail: users-help@wicket.apache.org
>>
>>
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [WICKET-3552] Is adding another parameter to the constructor required?
Posted by Martijn Dashorst <ma...@gmail.com>.
Boolean parameters are the bane of software development. I'd urge
anyone to reconsider their train of thought when they declare
foo(boolean x). It is really bad to have AttributeAppender(boolean
appendOrPrepend, ...) if you can have both AttributeAppender() and
AttributePrepender()
Classes are cheap compared to the cognitive dissonance of the boolean
parameter. In any case, the SimpleAttributeModifier, AttributeAppender
and AttributePrepender classes are basically stop gap measures to work
around issues with AttributeModifier's API.
I think that we could do better API wise with three factory methods on
AttributeModifier, and deprecate SimpleAttributeModifier and
AttributeAppender. I'd also merge the appender/prepender logic into
attribute modifier, such that user specializations can make use of
them.
AttributeModifier.setAttribute(String attribute, IModel<?> value);
AttributeModifier.prependAttribute(String attribute, IModel<?> value);
AttributeModifier.appendAttribute(String attribute, IModel<?> value);
Or perhaps:
AttributeModifier.overwrite(String attribute, IModel<?> value);
AttributeModifier.prepend(String attribute, IModel<?> value);
AttributeModifier.append(String attribute, IModel<?> value);
Martijn
On Wed, Jun 8, 2011 at 10:18 AM, Martin Grigorov <mg...@apache.org> wrote:
> Hi Per,
>
> I see your point.
> It is just my opinion (and ticket's reporter) that this additional
> boolean parameter to the constructor is adds much less noise than
> adding additional 2 classes.
> If the name of AttributeAppender is changed to AtrributeAdder (because
> we are used that add(obj) appends and add(0, obj) prepends) then it
> will be much cleaner.
>
> Igor's suggestion also sounds OK to me. Then we can either deprecate
> AttributeAppender or add AttributePrepender and both of them will just
> have constructors that pass the proper mode to AttributeModifier.
>
> Create a ticket.
>
> On Wed, Jun 8, 2011 at 10:04 AM, Per Newgro <pe...@gmx.ch> wrote:
>> Hi Martin Grigorov,
>>
>> But i made naming proposals:
>> AbstractAttributeRegister
>> | |
>> AttributeAppender AttributePrepender
>>
>> My pain in the ... is only that we have now 2 more constructors. This (in my
>> opinion)
>> really important class should be clear in design and responsibility.
>> Another pain could become important if we decide to exchange the default
>> values
>> for prepend. Wicket did that already in history and it lead to confusion and
>> required
>> app changes.
>>
>> The sense in "the one common parent" would be to provide the abstract method
>> for value registering. The subclasses would have to implement that.
>>
>> package org.apache.wicket.behavior;
>>
>> import org.apache.wicket.AttributeModifier;
>> import org.apache.wicket.model.IModel;
>> import org.apache.wicket.util.string.AppendingStringBuffer;
>> import org.apache.wicket.util.string.Strings;
>>
>> public class AbstractAttributeRegister extends AttributeModifier
>> {
>> private static final long serialVersionUID = 1L;
>>
>> private final String separator;
>>
>> public AbstractAttributeRegister(String attribute, boolean
>> addAttributeIfNotPresent,
>> IModel<?> appendModel, String separator)
>> {
>> this(attribute, addAttributeIfNotPresent, appendModel, separator);
>> }
>>
>> public AbstractAttributeRegister(String attribute, IModel<?> appendModel,
>> String separator)
>> {
>> this(attribute, true, appendModel, separator);
>> }
>>
>>
>> /**
>> * @see org.apache.wicket.AttributeModifier#newValue(java.lang.String,
>> java.lang.String)
>> */
>> @Override
>> protected String newValue(String currentValue, String appendValue)
>> {
>> // Shortcut for empty values
>> if (Strings.isEmpty(currentValue))
>> {
>> return appendValue != null ? appendValue : "";
>> }
>> else if (Strings.isEmpty(appendValue))
>> {
>> return currentValue;
>> }
>>
>> final AppendingStringBuffer sb = new
>> AppendingStringBuffer(currentValue.length() +
>> appendValue.length() + separator.length());
>>
>> return registerValue(sb, currentValue, appendValue);
>> }
>>
>> protected abstract String registerValue(AppendingStringBuffer target,
>> String currentValue, String appendValue);
>> }
>>
>> public class AttributeAppender extends AbstractAttributeRegister
>> {
>> private static final long serialVersionUID = 1L;
>>
>> public AttributeAppender(String attribute, boolean
>> addAttributeIfNotPresent,
>> IModel<?> appendModel, String separator)
>> {
>> super(attribute, addAttributeIfNotPresent, appendModel, separator);
>> }
>>
>> public AttributeAppender(String attribute, IModel<?> appendModel, String
>> separator)
>> {
>> super(attribute, appendModel, separator);
>> }
>>
>> @Override
>> protected String registerValue(AppendingStringBuffer target, String
>> currentValue, String value, String separator)
>> {
>> sb.append(currentValue);
>> sb.append(separator);
>> sb.append(value);
>> return sb.toString();
>> }
>> }
>>
>> public class AttributePrepender extends AbstractAttributeRegister
>> {
>> private static final long serialVersionUID = 1L;
>>
>> public AttributePrepender(String attribute, boolean
>> addAttributeIfNotPresent,
>> IModel<?> appendModel, String separator)
>> {
>> super(attribute, addAttributeIfNotPresent, appendModel, separator);
>> }
>>
>> public AttributePrepender(String attribute, IModel<?> appendModel, String
>> separator)
>> {
>> super(attribute, appendModel, separator);
>> }
>>
>> @Override
>> protected String registerValue(AppendingStringBuffer target, String
>> currentValue, String value, String separator)
>> {
>> sb.append(value);
>> sb.append(separator);
>> sb.append(currentValue);
>> return sb.toString();
>> }
>> }
>>
>>
>>
>>> Hi,
>>>
>>> I also had the same thoughts when I added the flag (as the patch
>>> suggested) but AttributeAppender is a class with just constructor
>>> overrides and one method override (#newValue()). If we introduce yet
>>> another class for prepend then there is no sense in "the one common
>>> parent" because they have nothing to share. Both of them will have the
>>> same number of constructors and this override of #newValue().
>>>
>>> I agree that now the name AttributeAppender is not the best but I
>>> cannot find a better one and even if we find it then we will have to
>>> rename this class which I think is used a lot
>>>
>>> On Wed, Jun 8, 2011 at 8:53 AM, Per Newgro<pe...@gmx.ch> wrote:
>>>>
>>>> Hi,
>>>>
>>>> i've traced changes in 1.5-RC4.2. Found the issue 3552 [
>>>> https://issues.apache.org/jira/browse/WICKET-3552 ]. If i'm not
>>>> completely in the wood the solution breaks the
>>>> SingleResponsibilityPrinciple by adding a flag to the constructor
>>>> which negates the class behavior.
>>>> Wouldn't it be more clear and reusable if we had another
>>>> AttributePrepender which acts like the AttributeAppender.
>>>> Both could extend the base-class AbstractAttributeRegister or
>>>> something. Then only the different part of the newValue method has
>>>> to be implemented by the concrete class.
>>>>
>>>> I only ask because if 1.5 is out it will be hard to exchange because
>>>> many apps will be upgraded and feature is maybe used :-).
>>>>
>>>> What do you think?
>>>> Cheers
>>>> Per
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
>>>> For additional commands, e-mail: users-help@wicket.apache.org
>>>>
>>>>
>>>
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
>> For additional commands, e-mail: users-help@wicket.apache.org
>>
>>
>
>
>
> --
> Martin Grigorov
> jWeekend
> Training, Consulting, Development
> http://jWeekend.com
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
>
--
Become a Wicket expert, learn from the best: http://wicketinaction.com
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [WICKET-3552] Is adding another parameter to the constructor required?
Posted by Martin Grigorov <mg...@apache.org>.
Hi Per,
I see your point.
It is just my opinion (and ticket's reporter) that this additional
boolean parameter to the constructor is adds much less noise than
adding additional 2 classes.
If the name of AttributeAppender is changed to AtrributeAdder (because
we are used that add(obj) appends and add(0, obj) prepends) then it
will be much cleaner.
Igor's suggestion also sounds OK to me. Then we can either deprecate
AttributeAppender or add AttributePrepender and both of them will just
have constructors that pass the proper mode to AttributeModifier.
Create a ticket.
On Wed, Jun 8, 2011 at 10:04 AM, Per Newgro <pe...@gmx.ch> wrote:
> Hi Martin Grigorov,
>
> But i made naming proposals:
> AbstractAttributeRegister
> | |
> AttributeAppender AttributePrepender
>
> My pain in the ... is only that we have now 2 more constructors. This (in my
> opinion)
> really important class should be clear in design and responsibility.
> Another pain could become important if we decide to exchange the default
> values
> for prepend. Wicket did that already in history and it lead to confusion and
> required
> app changes.
>
> The sense in "the one common parent" would be to provide the abstract method
> for value registering. The subclasses would have to implement that.
>
> package org.apache.wicket.behavior;
>
> import org.apache.wicket.AttributeModifier;
> import org.apache.wicket.model.IModel;
> import org.apache.wicket.util.string.AppendingStringBuffer;
> import org.apache.wicket.util.string.Strings;
>
> public class AbstractAttributeRegister extends AttributeModifier
> {
> private static final long serialVersionUID = 1L;
>
> private final String separator;
>
> public AbstractAttributeRegister(String attribute, boolean
> addAttributeIfNotPresent,
> IModel<?> appendModel, String separator)
> {
> this(attribute, addAttributeIfNotPresent, appendModel, separator);
> }
>
> public AbstractAttributeRegister(String attribute, IModel<?> appendModel,
> String separator)
> {
> this(attribute, true, appendModel, separator);
> }
>
>
> /**
> * @see org.apache.wicket.AttributeModifier#newValue(java.lang.String,
> java.lang.String)
> */
> @Override
> protected String newValue(String currentValue, String appendValue)
> {
> // Shortcut for empty values
> if (Strings.isEmpty(currentValue))
> {
> return appendValue != null ? appendValue : "";
> }
> else if (Strings.isEmpty(appendValue))
> {
> return currentValue;
> }
>
> final AppendingStringBuffer sb = new
> AppendingStringBuffer(currentValue.length() +
> appendValue.length() + separator.length());
>
> return registerValue(sb, currentValue, appendValue);
> }
>
> protected abstract String registerValue(AppendingStringBuffer target,
> String currentValue, String appendValue);
> }
>
> public class AttributeAppender extends AbstractAttributeRegister
> {
> private static final long serialVersionUID = 1L;
>
> public AttributeAppender(String attribute, boolean
> addAttributeIfNotPresent,
> IModel<?> appendModel, String separator)
> {
> super(attribute, addAttributeIfNotPresent, appendModel, separator);
> }
>
> public AttributeAppender(String attribute, IModel<?> appendModel, String
> separator)
> {
> super(attribute, appendModel, separator);
> }
>
> @Override
> protected String registerValue(AppendingStringBuffer target, String
> currentValue, String value, String separator)
> {
> sb.append(currentValue);
> sb.append(separator);
> sb.append(value);
> return sb.toString();
> }
> }
>
> public class AttributePrepender extends AbstractAttributeRegister
> {
> private static final long serialVersionUID = 1L;
>
> public AttributePrepender(String attribute, boolean
> addAttributeIfNotPresent,
> IModel<?> appendModel, String separator)
> {
> super(attribute, addAttributeIfNotPresent, appendModel, separator);
> }
>
> public AttributePrepender(String attribute, IModel<?> appendModel, String
> separator)
> {
> super(attribute, appendModel, separator);
> }
>
> @Override
> protected String registerValue(AppendingStringBuffer target, String
> currentValue, String value, String separator)
> {
> sb.append(value);
> sb.append(separator);
> sb.append(currentValue);
> return sb.toString();
> }
> }
>
>
>
>> Hi,
>>
>> I also had the same thoughts when I added the flag (as the patch
>> suggested) but AttributeAppender is a class with just constructor
>> overrides and one method override (#newValue()). If we introduce yet
>> another class for prepend then there is no sense in "the one common
>> parent" because they have nothing to share. Both of them will have the
>> same number of constructors and this override of #newValue().
>>
>> I agree that now the name AttributeAppender is not the best but I
>> cannot find a better one and even if we find it then we will have to
>> rename this class which I think is used a lot
>>
>> On Wed, Jun 8, 2011 at 8:53 AM, Per Newgro<pe...@gmx.ch> wrote:
>>>
>>> Hi,
>>>
>>> i've traced changes in 1.5-RC4.2. Found the issue 3552 [
>>> https://issues.apache.org/jira/browse/WICKET-3552 ]. If i'm not
>>> completely in the wood the solution breaks the
>>> SingleResponsibilityPrinciple by adding a flag to the constructor
>>> which negates the class behavior.
>>> Wouldn't it be more clear and reusable if we had another
>>> AttributePrepender which acts like the AttributeAppender.
>>> Both could extend the base-class AbstractAttributeRegister or
>>> something. Then only the different part of the newValue method has
>>> to be implemented by the concrete class.
>>>
>>> I only ask because if 1.5 is out it will be hard to exchange because
>>> many apps will be upgraded and feature is maybe used :-).
>>>
>>> What do you think?
>>> Cheers
>>> Per
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
>>> For additional commands, e-mail: users-help@wicket.apache.org
>>>
>>>
>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
>
--
Martin Grigorov
jWeekend
Training, Consulting, Development
http://jWeekend.com
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [WICKET-3552] Is adding another parameter to the constructor
required?
Posted by Per Newgro <pe...@gmx.ch>.
Hi Martin Grigorov,
But i made naming proposals:
AbstractAttributeRegister
| |
AttributeAppender AttributePrepender
My pain in the ... is only that we have now 2 more constructors. This
(in my opinion)
really important class should be clear in design and responsibility.
Another pain could become important if we decide to exchange the default
values
for prepend. Wicket did that already in history and it lead to confusion
and required
app changes.
The sense in "the one common parent" would be to provide the abstract method
for value registering. The subclasses would have to implement that.
package org.apache.wicket.behavior;
import org.apache.wicket.AttributeModifier;
import org.apache.wicket.model.IModel;
import org.apache.wicket.util.string.AppendingStringBuffer;
import org.apache.wicket.util.string.Strings;
public class AbstractAttributeRegister extends AttributeModifier
{
private static final long serialVersionUID = 1L;
private final String separator;
public AbstractAttributeRegister(String attribute, boolean
addAttributeIfNotPresent,
IModel<?> appendModel, String separator)
{
this(attribute, addAttributeIfNotPresent, appendModel, separator);
}
public AbstractAttributeRegister(String attribute, IModel<?>
appendModel, String separator)
{
this(attribute, true, appendModel, separator);
}
/**
* @see
org.apache.wicket.AttributeModifier#newValue(java.lang.String,
java.lang.String)
*/
@Override
protected String newValue(String currentValue, String appendValue)
{
// Shortcut for empty values
if (Strings.isEmpty(currentValue))
{
return appendValue != null ? appendValue : "";
}
else if (Strings.isEmpty(appendValue))
{
return currentValue;
}
final AppendingStringBuffer sb = new
AppendingStringBuffer(currentValue.length() +
appendValue.length() + separator.length());
return registerValue(sb, currentValue, appendValue);
}
protected abstract String registerValue(AppendingStringBuffer
target, String currentValue, String appendValue);
}
public class AttributeAppender extends AbstractAttributeRegister
{
private static final long serialVersionUID = 1L;
public AttributeAppender(String attribute, boolean
addAttributeIfNotPresent,
IModel<?> appendModel, String separator)
{
super(attribute, addAttributeIfNotPresent, appendModel, separator);
}
public AttributeAppender(String attribute, IModel<?> appendModel,
String separator)
{
super(attribute, appendModel, separator);
}
@Override
protected String registerValue(AppendingStringBuffer target, String
currentValue, String value, String separator)
{
sb.append(currentValue);
sb.append(separator);
sb.append(value);
return sb.toString();
}
}
public class AttributePrepender extends AbstractAttributeRegister
{
private static final long serialVersionUID = 1L;
public AttributePrepender(String attribute, boolean
addAttributeIfNotPresent,
IModel<?> appendModel, String separator)
{
super(attribute, addAttributeIfNotPresent, appendModel, separator);
}
public AttributePrepender(String attribute, IModel<?> appendModel,
String separator)
{
super(attribute, appendModel, separator);
}
@Override
protected String registerValue(AppendingStringBuffer target, String
currentValue, String value, String separator)
{
sb.append(value);
sb.append(separator);
sb.append(currentValue);
return sb.toString();
}
}
> Hi,
>
> I also had the same thoughts when I added the flag (as the patch
> suggested) but AttributeAppender is a class with just constructor
> overrides and one method override (#newValue()). If we introduce yet
> another class for prepend then there is no sense in "the one common
> parent" because they have nothing to share. Both of them will have the
> same number of constructors and this override of #newValue().
>
> I agree that now the name AttributeAppender is not the best but I
> cannot find a better one and even if we find it then we will have to
> rename this class which I think is used a lot
>
> On Wed, Jun 8, 2011 at 8:53 AM, Per Newgro<pe...@gmx.ch> wrote:
>> Hi,
>>
>> i've traced changes in 1.5-RC4.2. Found the issue 3552 [
>> https://issues.apache.org/jira/browse/WICKET-3552 ]. If i'm not
>> completely in the wood the solution breaks the
>> SingleResponsibilityPrinciple by adding a flag to the constructor
>> which negates the class behavior.
>> Wouldn't it be more clear and reusable if we had another
>> AttributePrepender which acts like the AttributeAppender.
>> Both could extend the base-class AbstractAttributeRegister or
>> something. Then only the different part of the newValue method has
>> to be implemented by the concrete class.
>>
>> I only ask because if 1.5 is out it will be hard to exchange because
>> many apps will be upgraded and feature is maybe used :-).
>>
>> What do you think?
>> Cheers
>> Per
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
>> For additional commands, e-mail: users-help@wicket.apache.org
>>
>>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [WICKET-3552] Is adding another parameter to the constructor required?
Posted by Martin Grigorov <mg...@apache.org>.
Hi,
I also had the same thoughts when I added the flag (as the patch
suggested) but AttributeAppender is a class with just constructor
overrides and one method override (#newValue()). If we introduce yet
another class for prepend then there is no sense in "the one common
parent" because they have nothing to share. Both of them will have the
same number of constructors and this override of #newValue().
I agree that now the name AttributeAppender is not the best but I
cannot find a better one and even if we find it then we will have to
rename this class which I think is used a lot
On Wed, Jun 8, 2011 at 8:53 AM, Per Newgro <pe...@gmx.ch> wrote:
> Hi,
>
> i've traced changes in 1.5-RC4.2. Found the issue 3552 [
> https://issues.apache.org/jira/browse/WICKET-3552 ]. If i'm not
> completely in the wood the solution breaks the
> SingleResponsibilityPrinciple by adding a flag to the constructor
> which negates the class behavior.
> Wouldn't it be more clear and reusable if we had another
> AttributePrepender which acts like the AttributeAppender.
> Both could extend the base-class AbstractAttributeRegister or
> something. Then only the different part of the newValue method has
> to be implemented by the concrete class.
>
> I only ask because if 1.5 is out it will be hard to exchange because
> many apps will be upgraded and feature is maybe used :-).
>
> What do you think?
> Cheers
> Per
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
>
--
Martin Grigorov
jWeekend
Training, Consulting, Development
http://jWeekend.com
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org