EditController and ModelConverter – Does this make my ass look fat?

By Posted on

Problem

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.

Solution

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.

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.

Leave a Reply

Your email address will not be published. Required fields are marked *