Parsing one UserData type to a preview

Question

I am currently developing a web app and am using Spring as a Backend. I have some user data that is provided by one of our services and I need to display it in my frontend.

The subject of the code review is the part, where I process the data in my backend to create a simpler data type that I can directly use in the frontend. This avoids that I have to mess with the incoming data in my frontend app and can directly take the data I want to display from this data type.

This is my parser I wrote for this task. I tried some stuff that was relatively unknown to me before e.g. this return switch thingy. Any feedback is greatly appreciated!

@Service
public class UserDataParser {
    public ProfilePreviewData toProfilePreviewData(ExtendedUserData user) {
        ProfilePreviewData profilePreviewData = new ProfilePreviewData();
        profilePreviewData.setAccountId(user.getId());
        profilePreviewData.setAccountName(user.getName());
        profilePreviewData.setAccounts(getAccountPreviewData(user));
        return profilePreviewData;
    }

    private List<AccountPreviewData> getAccountPreviewData(ExtendedUserData user) {
        List<AccountPreviewData> accountsPreviewData = new ArrayList<>();
        for (AccountData account : user.getAccounts()) {
            if (!account.getPublish()) continue;
            accountsPreviewData.add(getMinecraftAccountPreviewData(account));
        }

        for (ExternalAccountData externalAccount : user.getExternalAccounts()) {
            if (!externalAccount.getPublish()) continue;
            accountsPreviewData.add(getExternalAccountPreviewData(externalAccount));
        }

        return accountsPreviewData;
    }

    private AccountPreviewData getMinecraftAccountPreviewData(AccountData account) {
        AccountPreviewData accountPreviewData = new AccountPreviewData();
        accountPreviewData.setAccountType(AccountType.MINECRAFT);
        accountPreviewData.setDisplayName(account.getName());
        accountPreviewData.setUniqueIdentifier(account.getUuid().toString());
        return accountPreviewData;
    }

    private AccountPreviewData getExternalAccountPreviewData(ExternalAccountData accountData) {
        AccountPreviewData accountPreviewData = new AccountPreviewData();

        AccountType type = getTypeOfExternalAccount(accountData.getTypeName());
        accountPreviewData.setAccountType(type);

        accountPreviewData.setDisplayName(accountData.getAccountId());
        accountPreviewData.setUniqueIdentifier(accountData.getName());
        return accountPreviewData;
    }

    private AccountType getTypeOfExternalAccount(String typeName) {
        return switch (typeName) {
            case "Discord" -> AccountType.DISCORD;
            case "TeamSpeak" -> AccountType.TEAMSPEAK;
            default -> AccountType.UNKNOWN;
        };
    }


}

```

Answer 1

Naming

The class name is a bit abstract. I prefer naming services based on the concrete things they provide instead of what they consume. You of course follow the naming conventions of your organization, but to me a Parser converts transportation data format, such as an XML or JSON document, to a Java object. I prefer to use Mapper suffix for classes that convert between different type of Java objects and I use the result object type as the mapper name. So my choice for naming this class would be ProfilePreviewDataMapper.

When your mapper is named after the result type, you don't need to repeat it in the method name, since the information is in the class name. The method becomes ProfilePreviewDataMapper.from(ExtendedUserData extendedUserData). Also by following this naming style, you automatically limit the scope of each class to it's return type and "accidentally" adding more and more responsibilities becomes harder, because you have to figure out different method names that break the pattern. It's easier to keep classes small and manageable.

In Java, get is a special prefix for a method and it marks an accessor to a field. Getters are not expected to create new objects. Using get prefix in a method that accepts parameters and creates objects breaks one of the most widely used Java idioms, so I would recommend against that. If you want to follow your class structure, use something like createAccountPreviewData instead, as that prefix immediately signals that something new is being returned instead of returning a reference to an existing object. Also use the same prefix for all methods. Now you have to and get prefixes in methods that perform similar tasks.

Structure

Consider the testability of your class. What data do you need to set up in order to unit test the getMinecraftAccountPreviewData method? I would take the three private methods and extract them into standalone mappers. The getAccountPreviewData method would become AccountPreviewDataMapper.from(ExtendedUserData extendedUserData) which gets injected into ProfilePreviewDataMapper. Apply same pattern to getMinecraftAccountPreviewData and getExternalAccountPreviewData and then you can create individual unit tests for each mapper without having to set up a lot of unnecessary data for each of them.

Instead of implemeting the mappers straight up as ProfilePreviewDataMapper etc, make them interfaces instead and write the implementation in ProfilePreviewDataMapperImpl.

You have now implemented S, O, L and D of the SOLID principles

Style

I find this "if not something, skip" logic unnecessarily difficult to follow and not using curly braces only adds to it. I would replace

for (AccountData account : user.getAccounts()) {
    if (!account.getPublish()) continue;
    accountsPreviewData.add(getMinecraftAccountPreviewData(account));
}

with

for (AccountData account : user.getAccounts()) {
    if (account.getPublish()) {
        accountsPreviewData.add(getMinecraftAccountPreviewData(account));
    }
}