7
\$\begingroup\$

I am using a combination of the following two classes to achieve a quite simple task. I have Users in my application. These Users can be assigned Roles. And these Roles need to be assigned Rights.

Currently I do this with a primefaces selectCheckboxMenu. This menu requires the entries to be Strings. In itself this is not really hard. But the fun just begins. I need to assign the selected entries back to the processed Role before passing it to the businessInterface.save() method for persistence.

That's why I wrote a ModelConverter class to provide a static method doing that conversion for me. This is mainly due to the fact that I am limited to Java 6 in the project. I'd love to go for Java 7 or even 8, but oh well ;)

But when I look at the usage of that class it looks ugly, and I'd like to change that.

RoleEditController.java:

import static company.crm.controller.ControllerSettings.CONVERSATION_STANDARD_TIMEOUT;
import company.crm.business.RoleDistributor;
import company.crm.controller.Mode;
import company.crm.controller.Pages;
import company.crm.framework.ModelConverter;
import company.crm.model.Right; 
import company.crm.model.Role;

@Named
@ConversationScoped
public class RoleEditController implements Serializable {

    private static final long serialVersionUID = 4002592161426207130L;

    @Inject
    private RoleListController roleListController;

    @Inject
    private RoleDistributor businessInterface;

    @Inject
    private Conversation conversation;

    private Mode mode;
    private Role currentRole;
    private static List<Right> allRights = new ArrayList<Right>();
    private List<String> selectedRights = new ArrayList<String>();

    @PostConstruct
    public void init() {
        conversation.setTimeout(CONVERSATION_STANDARD_TIMEOUT);
        conversation.begin();
        this.mode = Mode.ADD;
        reloadAllRightsList();
    }

    public String save() {
        currentRole.setRights(new ArrayList<Right>(ModelConverter
                .objectifyStringifiedCollectionWithOriginalItems(allRights,
                        selectedRights)));
        boolean success = businessInterface.save(mode, currentRole);

        roleListController.fillRoleList();
        if (success) {
            conversation.end();
            return Pages.ROLE_LIST;
        } else {
            return Pages.ROLE_EDIT;
        }
    }

    public void reloadCurrentRole() {
        reloadAllRightsList();
        this.currentRole = businessInterface.loadSingle(currentRole.getId());
        selectedRights = new ArrayList<String>(
                ModelConverter.stringifyCollection(currentRole.getRights()));
    }

    private void reloadAllRightsList() {
        allRights.clear();
        allRights.addAll(businessInterface.loadPossibleRights());
    }

    public String abort() {
        conversation.end();
        return Pages.ROLE_LIST;
    }

    public void recieveRole(final long roleId) {
        currentRole = businessInterface.loadSingle(roleId);
        selectedRights = new ArrayList<String>(
                ModelConverter.stringifyCollection(currentRole.getRights()));

        this.mode = roleId == 0 ? Mode.ADD : Mode.EDIT;
    }

    public Role getCurrentRole() {
        return currentRole;
    }

    public Mode getMode() {
        return mode;
    }

    public List<String> getStringifiedRightList() {
        return new ArrayList<String>(
                ModelConverter.stringifyCollection(allRights));
    }

    public List<String> getSelectedRights() {
        return selectedRights;
    }

    public void setSelectedRights(final List<String> selectedRights) {
        this.selectedRights = selectedRights;
    }
}

ModelConverter.java:

package novatec.crm.framework;

import java.util.Collection;
import java.util.HashSet;

public final class ModelConverter {

    public static Collection<String> stringifyCollection(
            final Collection<? extends Object> items) {
        if (items == null) {
            return new HashSet<String>();
        }

        Collection<String> stringifiedCollection = new HashSet<String>();
        for (Object item : items) {
            stringifiedCollection.add(item.toString());
        }
        return stringifiedCollection;
    }

    public static <T> Collection<T> objectifyStringifiedCollectionWithOriginalItems(
            final Collection<? extends T> originalItems,
            final Collection<String> stringifiedItems) {
        if (originalItems.size() < stringifiedItems.size()) {
            throw new IllegalArgumentException(
                    "The original collection may not be smaller than the subcollection of stringified Items");
        }

        Collection<T> convertedItems = new HashSet<T>();

        for (T item : originalItems) {
            for (String stringifiedItem : stringifiedItems) {
                if (item.toString().equals(stringifiedItem)) {
                    convertedItems.add(item);
                }
            }
        }

        return convertedItems;
    }
}

Questions:

  • Am I overly complicating things?
  • Is this another case of German overengineering™?
  • Are my names telling the intention behind my code?

And before anyone calls it: I know that the raw string error message doesn't look nice, and that my Converter does automagically remove duplicates in the Collections that are passed to him.

\$\endgroup\$
2
  • \$\begingroup\$ I would move the creation of the HashSet<String> to the top of the method and then check if the collection parameter ! null (talking about the converter class) (from my phone) \$\endgroup\$
    – Heslacher
    Jul 15, 2014 at 14:15
  • \$\begingroup\$ @Heslacher this is actually a nice idea, but upon consideration I like to have my guard-clauses first, before any initialization. This is personal preference though ;) \$\endgroup\$
    – Vogel612
    Jul 15, 2014 at 14:33

2 Answers 2

4
\$\begingroup\$

Some things about your current converter:

Collection<? extends Object> can just be Collection<?>. Technically it would be enough to specify it as Iterable<?> as Iterable is a superinterface of Collection, but declaring it as Collection is fine as well.

To support null values (even though you might not need it) you can use String.valueOf(item) instead of item.toString()


In your objectifyStringifiedCollectionWithOriginalItems method you are.... complicating things.

First of all, this part:

for (String stringifiedItem : stringifiedItems) {
    if (item.toString().equals(stringifiedItem)) {
        convertedItems.add(item);
    }
}

Can be written as:

if (stringifiedItems.contains(item.toString()) {
    convertedItems.add(item);
}

Which would boost performance when stringifiedItems is a Set.

However, what it seems to be doing is to Map a String to a <T>. Did you see that? Map? Does that tell you something? Create a Map<String, T> of all your Rights (which sounds like it should be named Permission instead) and you would reduce your mapper to a simple map.get(string).

So basically:

Am I overly complicating things?

Yes.

Is this another case of german overengineering™?

Yes.

Are my names telling the intention behind my code?

Besides Right (where's Left?), yes.

\$\endgroup\$
2
\$\begingroup\$

I think you complicated the wrong things.

Your root issue seems to be

  1. Given a string, get the matching item (items?)
  2. Given an item, get the matching string

So you should probably write that bit of functionality, as clearly as you can, first, then worry about manipulating the collections.

Guava, for example, has an explicit Function that accepts an F and returns a T. So you can encapsulate your choice for how to produce a label for a Right in one location. You can easily write your unit tests for just that bit.

After that, you can start working on converting the collections -- again, Guava has good support for that.

It also feels like you are stumbling because you have your view code and your model code tangled. Given the constraints you are working with, I would expect to see a representation of your dialog, that only knows about Strings, and a representation of your persistence layer, that only knows about Rights, and an Adapter in the middle, knows how to switch back and forth between Strings and Rights, but doesn't care what you do with either.

For instance, consider this

public String save() {
    currentRole.setRights(new ArrayList<Right>(ModelConverter
            .objectifyStringifiedCollectionWithOriginalItems(allRights,
                    selectedRights)));
    boolean success = businessInterface.save(mode, currentRole);

    roleListController.fillRoleList();
    if (success) {
        conversation.end();
        return Pages.ROLE_LIST;
    } else {
        return Pages.ROLE_EDIT;
    }
}

This method is doing real work, AND acting in both the Rights world and the String world. That's too many different concerns in one place. Let's try refactoring to see what's going on...

public String save() {
    // This is the checkbox collection
    List<String> selectedCheckboxes = selectedRights;

    // This is the adapter
    List<Right> selectedRights = new ArrayList<Right>(ModelConverter
            .objectifyStringifiedCollectionWithOriginalItems(allRights,
                    selectedRights));

    // here's work being done in the model
    currentRole.setRights(selectedRights);

    // here's a save of the current state of the model...
    boolean success = businessInterface.save(mode, currentRole);

    roleListController.fillRoleList();
    if (success) {
        conversation.end();
        return Pages.ROLE_LIST;
    } else {
        return Pages.ROLE_EDIT;
    }
}

If you squint a little bit, I think that you can see that this code wants to look more like

public String save() {
    // This is the adapter
    List<Right> selectedRights = rightsCheckbox.getSelected();

    // here's work being done in the model
    currentRole.setRights(selectedRights);

    // here's a save of the current state of the model...
    boolean success = businessInterface.save(mode, currentRole);

    roleListController.fillRoleList();
    if (success) {
        conversation.end();
        return Pages.ROLE_LIST;
    } else {
        return Pages.ROLE_EDIT;
    }
}

The obvious objection being that the checkbox code you are talking to knows about strings, not rights. We solve this by creating an abstraction for a generic checkbox

interface Checkbox<T> {
    // We could make this an Iterable, or an ImmutableList
    List<T> getSelected();
    void setSelected(List<T> choices);
}

Now we need two implementations of this. One is an encapsulation of your existing String based selection management. The other is the adapter...

class RightsCheckboxAdapter implements Checkbox<Right> {
    private static Function<List<Right>, List<String>> TO_LABEL_LIST = ...;
    private static Function<List<String>, List<Right>> FROM_LABEL_LIST = ...;

    private final Checkbox<String> target;

    RightsCheckboxAdapter (Checkbox<String> target) {
        this.target = target;
    }

    public void setSelected(List<Right> selections) {
        target.setSelected(TO_LABEL_LIST.apply(selectsions));
    }

    public List<Right> getSelected() {
        return FROM_LABEL_LIST.apply(target.getSelected());
    }
}

This in turn is obviously pretty generic; really it just needs non static Functions to do the work

class CheckboxAdapter<F,T> implements Checkbox<F> {
    private Checkbox<T> target;
    private Function<List<F>, List<T>> toTarget;
    private Function<List<T>, List<F>> fromTarget;

    CheckboxAdapter(Checkbox<T> target, Function<List<F>, List<T>> toTarget, Function<List<T>, List<F>> fromTarget) {
        this.target = target;
        this.toTarget = toTarget;
        this.fromTarget = fromTarget;
    }

    public void setSelected(List<F> selections) {
        target.setSelected(toTarget.apply(selectsions));
    }

    public List<F> getSelected() {
        return fromTarget.apply(target.getSelected());
    }
}

In passing, I'll mention that this...

    if (success) {
        conversation.end();
        return Pages.ROLE_LIST;
    } else {
        return Pages.ROLE_EDIT;
    }

looks suspiciously like there's a state machine that you should be teasing out of the code.

\$\endgroup\$
2
  • \$\begingroup\$ A few points I want to address in this review. The Controller-Method save is supposed to return a navigation rule for the site to go to, after evaluating the entered data. This leads to the fact that it depends on the result of a business-layer (Right-world) call. The controller uses the entered Data to pass them down to business. unfortunately the View can only give him Strings. What you describe here as a state-machine is not a machine, but a request, with 2 different outcomes :( \$\endgroup\$
    – Vogel612
    Jul 16, 2014 at 7:10
  • \$\begingroup\$ Furthermore: What makes your RightsCheckboxAdapter different from my ModelConverter (aside from the interface and the non-static functionality)? Your refactoring of the save-method is also incorrect. selectedRights will get set by the view-side. Before submitting a "request" to the business Layer they need to be transformed into Rights, and that is what the method does, using the ModelConverter... \$\endgroup\$
    – Vogel612
    Jul 16, 2014 at 7:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.