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

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 allRights = new ArrayList();
private List selectedRights = new ArrayList();

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

public String save() {
currentRole.setRights(new ArrayList(ModelConverter
.objectifyStringifiedCollection

Some things about your current converter:

Collection 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 `. Did you see that? Map? Does that tell you something? Create a Map 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.

StackExchange Code Review Q#57093, answer score: 4