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 <[email protected]> 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<[email protected]> 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: [email protected] >>> For additional commands, e-mail: [email protected] >>> >>> >> >> > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > > -- Martin Grigorov jWeekend Training, Consulting, Development http://jWeekend.com --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
