Skip to main content
added 568 characters in body
Source Link
Heslacher
  • 51k
  • 5
  • 83
  • 177

Comments

I am confused by the xml comment of the interface CardLoader<I>. It says

@param <I>   The input file type of the card loader on each individual load action  

and the XmlCardLoader`` implements CardLoader. In my opinion Path!=input file type`. So it would better state

@param <I>   The input source of the card loader on each individual load action

and the signatur in the interface should be changed to

Collection<Entity> loadCards(I source, ...)  

and the xml comment for the method itself also.

Comments

I am confused by the xml comment of the interface CardLoader<I>. It says

@param <I>   The input file type of the card loader on each individual load action  

and the XmlCardLoader`` implements CardLoader. In my opinion Path!=input file type`. So it would better state

@param <I>   The input source of the card loader on each individual load action

and the signatur in the interface should be changed to

Collection<Entity> loadCards(I source, ...)  

and the xml comment for the method itself also.

Source Link
Heslacher
  • 51k
  • 5
  • 83
  • 177

Readability

For me this is clearly to many lines of code inside this method. So let us see, what you are doing.

  1. you are creating a CardInfo object by using the Path path.
  2. you are creating a List<String>'s and validate them
  3. you are createing Map<String, T's and use them to prodcude the result

As it seems 1. and 2. won't change in the near future but add a lot of codelines to this method. So let us extract them to separate methods.

private CardList readFromFile(File file)
{
    SAXBuilder saxBuilder = new SAXBuilder();
    Document document = saxBuilder.build(file);

    XMLOutputter xmlOutputter = new XMLOutputter(Format.getCompactFormat().setExpandEmptyElements(true));
    String unformattedXmlString = xmlOutputter.outputString(document);

    JacksonXmlModule xmlModule = new JacksonXmlModule();
    xmlModule.setDefaultUseWrapper(false);
    ObjectMapper xmlMapper = new XmlMapper(xmlModule);

    return xmlMapper.readValue(unformattedXmlString, CardInfo.class);
}

private List<String> getValidatedTags(List<ECSResource> resourcesList, List<ECSAttribute> attributesList)
{
    List<String> tags = Stream.concat(resourcesList.stream(), attributesList.stream())
        .map(ecsElement -> sanitizeTag(ecsElement.toString()))
        .collect(Collectors.toList());

    validateTags(tags);

    return tags;
}  

private void validateTags(List<String> tags)
{

    if (requiredTags().stream().anyMatch(tags::contains)) {
        throw new UncheckedCardLoadingException("Tags " + requiredTags() + " are required by default you cannot submit them in the resources or attributes.");
    }

    List<String> duplicateTags = tags.stream()
        .collect(Collectors.groupingBy(i -> i))
        .entrySet()
        .stream()
        .filter(entry -> entry.getValue().size() > 1)
        .map(Entry::getKey)
        .collect(Collectors.toList());

    if (!duplicateTags.isEmpty()) {
        throw new UncheckedCardLoadingException("Tags " + duplicateTags + " have been input multiple times, this is not allowed.");
    }

}

private List<Card> getValidatedCards(CardInfo cardInfo)
{
    List<Card> cards = cardInfo.getCards().getCards();
 
    List<String> duplicateIds = cards.stream()
        .collect(Collectors.groupingBy(Card::getId))
        .entrySet()
        .stream()
        .filter(entry -> entry.getValue().size() > 1)
        .map(Entry::getKey)
        .collect(Collectors.toList());

    if (!duplicateIds.isEmpty()) {
        throw new UncheckedCardLoadingException("Card ids " + duplicateIds + " are duplicaties, this is not allowed.");
    }

    return cards;
}

Now, after renaming cardList to cards and rearanging the creating of the Map<String, T>'s the former loadCards() method looks like

@Override
public Collection<Entity> loadCards(final Path path, final Supplier<Entity> entitySupplier, final ECSResource[] resources, final ECSAttribute[] attributes) throws CardLoadingException {
    Objects.requireNonNull(path, "path");
    Objects.requireNonNull(entitySupplier, "entitySupplier");

    List<ECSResource> resourcesList = (resources == null) ? Arrays.asList() : Arrays.asList(resources);
    List<ECSAttribute> attributesList = (attributes == null) ? Arrays.asList() : Arrays.asList(attributes);

    try {

        CardInfo cardInfo = readFromFile(path.toFile());

        List<String> tags = getValidatedTags(resourcesList, attributesList);

        List<Card> cards = getValidatedCards(cardInfo);
        
        Map<String, ECSResource> ecsResourcesMap = resourcesList.stream()
            .collect(Collectors.toMap(ecsResource -> sanitizeTag(ecsResource.toString()), i -> i));

        Map<String, ECSAttribute> ecsAttributesMap = attributesList.stream()
            .collect(Collectors.toMap(ecsAttribute -> sanitizeTag(ecsAttribute.toString()), i -> i));
        
        return cards.stream()
            .map(card -> {
                Entity entity = entitySupplier.get();

                entity.addComponent(new IdComponent(card.getId()));

                ECSResourceMap resourceMap = ECSResourceMap.createFor(entity);
                ECSAttributeMap attributeMap = ECSAttributeMap.createFor(entity);

                card.getElements().forEach((sanitizedTag, value) -> {
                    if (ecsResourcesMap.containsKey(sanitizedTag)) {
                        resourceMap.set(ecsResourcesMap.get(sanitizedTag), Integer.parseInt(value.toString()));
                    }
                    else if (ecsAttributesMap.containsKey(sanitizedTag)) {
                        attributeMap.set(ecsAttributesMap.get(sanitizedTag), value.toString());
                    }
                    else {
                        throw new UncheckedCardLoadingException("Element " + sanitizedTag + " has not been found in the supplied resource and attribute mappings where card id = " + card.getId());
                    }
                });

                return entity;
            })
            .collect(Collectors.toList());
    } catch (UncheckedCardLoadingException ex) {
        throw new CardLoadingException(ex.getMessage(), ex.getCause());
    } catch (Exception ex) {
        throw new CardLoadingException(ex);
    }
}

What you are needed to do is add the trows part to the extracted methods, as I don't have Java 8 or any of these used libraries installed.

CardInfo,Cards and Card classes

What I don't like here is that CardInfo holds a field Cards cards and that Cards holds a field Card[] cards. I don't see any need for the Cards class here.