4
\$\begingroup\$

I'm writing an assistant for the tabletop RPG Pathfinder in Java. I intend to use the MVVM pattern using JavaFX, possibly through mvvmFX. Before I go into the V-VM phase, I have built a first iteration of the model, and would like some comments on it. The full code is on GitHub, I only include in this question the relevant classes. I have three main questions but I accept all feedback, especially on comments and package names if they do not follow standard conventions.

Question 1: Can global parameters be accessed to build a view, or does it break the pattern?

In my case, I have six abilities, defined in the AbilityName enum: Strength, Dexterity, etc. I will have six labels in the character sheet to display them to the user. Can I define my abilities in a service package accessed by both the model and the view?

CreatureParameters.java, where I define the enum, follows. Should it be located in a model package and, if so, am I doomed to have some weird code clumsily duplicating it to bind data to the corresponding labels?

package service.parameters;
/**
 * Non-instanciable container for the global parameters of the system relative
 * to the creatures.
 * These are not made to be tweaked but only to be shared between the layers 
 * of the program.
 */
public class CreatureParameters {   
    /**
     * Names of the six abilities.
     */
    public static enum AbilityName{
        STRENGTH,
        DEXTERITY,
        CONSTITUTION,
        INTELLIGENCE,
        WISDOM,
        CHARISMA;
    }
    private CreatureParameters() {};
}

Question 2: Are my exceptions sane, or I am doing something wrong?

In the model, when the input for a method call is invalid, I throw an exception. The classes defining the abilities may throw several such exceptions, and I am not sure I am defining them correctly and discriminating them enough.

Value.java, the building block which I will use to store most numerical values and handle bonuses in the future :

package model.values;

/**
 * Container for the base numerical unit of measurement. A Value contains a 
 * base value that may be modified by different instances of {@link Bonus}.
 */
public class Value {
    private int value;

    /**
     * Initialises a simple Value object.
     * @param value to store in the object.
     */
    public Value(int value) {
        this.value = value;
    }

    /**
     * @return the total value of the object.
     */
    public int getValue() {
        return this.value;
    }
}

AbilityScore.java, where I define a generic ability score interface (I use an interface to let the AbilityScores class handle input validation):

package model.values;

/**
 * Read-only container for one of the six ability scores of a creature: 
 * Strength, Dexterity, Constitution, Intelligence, Wisdom, Charisma.
 */
public interface AbilityScore {

    /**
     * @return the modifier of the ability score.
     */
    public int getModifier();

    /**
     * Computes the modifier associated with a given value for an ability 
     * score.
     * @param value to take into account.
     * @return the modifier associated with the given value.
     */
    public static int computeModifier(int value) {
        return value/2 - 5;
    }

    /**
     * @return  the value of the ability score, taking all bonuses into account.
     */
    public int getValue();

}

ValueParameters.java, where I define the min and max acceptable values for abilities and dice (the Roll class is left out because I do not use it yet).

package service.parameters;

/**
 * Non-instanciable container for the global parameters of the system relative
 * to the values.
 * These are not made to be tweaked but only to be shared between the layers 
 * of the program.
 */
public final class ValueParameters {

    /**Prevents the class from being instantiated.*/
    private ValueParameters() {}
    /**Private parameter to set a maximum for the input values.*/
    private static final int MAX_INT_VALUE = 999;
    /**Minimum valid value for an ability score. Must be 0.*/
    public static final int MIN_ABILITY_SCORE = 0;
    /**Maximum valid value for an ability score. Must be at least 40.*/
    public static final int MAX_ABILITY_SCORE = MAX_INT_VALUE;
    /**Minimum valid number of dice in a roll. Must be 1.*/
    public static final int MIN_NUMBER_OF_DICE = 1;
    /**Minimum valid number of sides for a die. Must be at most 4.*/
    public static final int MIN_NUMBER_OF_SIDES = 2;
    /**Maximum valid number of dice in a roll. Must be at least 10.*/
    public static final int MAX_NUMBER_OF_DICE = MAX_INT_VALUE;
    /**Maximum valid number of sides for a dice. Must be at least 100.*/
    public static final int MAX_NUMBER_OF_SIDES = MAX_INT_VALUE;
}

Part of the AbilityScores.java file (I made a read-only and a read-write versions of the interface to make sure that only the creature with the ability scores can modify them; any other object must contend with a read-only version and ask the creature to modify them. The read-only version is left out as it is not that relevant to the question):

package model.creatures;

import java.util.AbstractMap;
import java.util.Collections;
import java.util.EnumMap;
import java.util.EnumSet;
import java.util.Iterator;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;

import model.exceptions.IllegalAbilityScoreException;
import model.values.AbilityScore;
import model.values.Value;
import service.parameters.CreatureParameters.AbilityName;
import service.parameters.ValueParameters;

/**
 * Set of the six abilities of any creature.
 */
public interface AbilityScores extends Iterable<Map.Entry<AbilityName, AbilityScore>>{
    /**
     * Public unmodifiable reference to the subset of abilities that all 
     * creatures must have. The other abilities are optional, some creatures 
     * may not have them.
     */
    public final static Set<AbilityName> MANDATORY_ABILITIES = Collections.unmodifiableSet(EnumSet.of(
            AbilityName.DEXTERITY, AbilityName.WISDOM, AbilityName.CHARISMA));

    /**
     * Returns the modifier associated with the given ability.
     * @param ability
     * @return  an integer which may be positive or negative depending on the 
     * value of the ability, or zero if the ability is not defined.
     */
    public int getModifier(AbilityName ability);

    /**
     * Returns specified the {@link model.values.AbilityScore}. May return 
     * {@link null} if the ability is not defined for the creature.
     * @param ability
     * @return an {@link AbilityScore} object or {@link null}.
     */
    public AbilityScore getScore(AbilityName ability);

    /**
     * Checks that the input is valid to build an AbilityScore.
     * @param values    the input to check.
     * @param fail      if true, raise an {@link IllegalAbilityScoreException}
     * instead of returning false.
     * @return  true if all mandatory abilities are present, and all present 
     * abilities have valid values.
     */
    public static boolean isValidAbilityScoreInput(Map<AbilityName, Integer> values, boolean fail) {
        //Reject null input
        if(values == null) {
            if(fail) {
                throw new IllegalAbilityScoreException(IllegalAbilityScoreException.Cause.NULL);
            }
            return false;
        }
        //Check if all mandatory abilities are present
        if(!MANDATORY_ABILITIES.containsAll(values.keySet())) {
            //If not, find the missing one and throw an exception
            for(AbilityName ability: MANDATORY_ABILITIES) {
                if (!values.containsKey(ability)) {
                    if(fail) {
                        throw new IllegalAbilityScoreException(ability.toString(), values.keySet().toString());
                    }
                    return false;
                }
            }
        }
        //Verify that all model.values are valid
        for(Entry<AbilityName, Integer> entry : values.entrySet()) {
            if(entry.getValue() < ValueParameters.MIN_ABILITY_SCORE 
                    || entry.getValue() > ValueParameters.MAX_ABILITY_SCORE) {
                if(fail) {
                    throw new IllegalAbilityScoreException(entry.getKey().toString(), entry.getValue());
                }
                return false;
            }
        }
        return true;
    }

    /**
     * Initialises an {@link AbilityScores} object with model.values for at least 
     * some of the abilities.
     * @param model.values  must contain the mandatory abilities: DEXTERITY, 
     * WISDOM, and CHARISMA. May also contain the optional abilities.
     * @throws {@link model.exceptions.IllegalAbilityScoreException} if a mandatory 
     * ability is missing or if a value is invalid.
     */
    public static AbilityScores create(Map<AbilityName, Integer> values) {
        return new RWAbilityScores(values);
    }
}

/**
 * Read-write implementation of the {@link AbilityScores} interface.
 * This class offers additional methods to increment abilities or add bonuses.
 * Classes using a {@link RWAbilityScores} attribute should never expose it 
 * directly but only expose its {@link ROAbilityScores} counterpart.
 */
class RWAbilityScores implements AbilityScores{
    /**
     * Map of the scores associated with the abilities.
     */
    private EnumMap<AbilityName, AbilityScoreType> abilities;

    /**
     * Initialises an {@link AbilityScores} object with model.values for at least 
     * some of the abilities.
     * @param model.values  must contain the mandatory abilities: DEXTERITY, 
     * WISDOM, and CHARISMA. May also contain the optional abilities.
     * @throws {@link model.exceptions.IllegalAbilityScoreException} if a mandatory 
     * ability is missing or if a value is invalid.
     */
    public RWAbilityScores(Map<AbilityName, Integer> values) {
        //Validate input
        AbilityScores.isValidAbilityScoreInput(values, true);
        this.abilities = new EnumMap<AbilityName, AbilityScoreType>(AbilityName.class);
        for(Entry<AbilityName, Integer> entry : values.entrySet()) {
            this.abilities.put(entry.getKey(), new AbilityScoreType(entry.getValue()));
        }
    }

    /**
     * Initialises a {@link RWAbilityScores} object by making a deep-copy of
     * the input {@link AbilityScores} object.
     * @param abilities object to copy.
     */
    public RWAbilityScores(AbilityScores abilities) {
        //Reject null input
        if(abilities == null) {
            throw new IllegalAbilityScoreException(IllegalAbilityScoreException.Cause.NULL);
        }
        this.abilities = new EnumMap<AbilityName, AbilityScoreType>(AbilityName.class);
        for(Map.Entry<AbilityName, AbilityScore> entry : abilities) {
            if(entry.getValue() != null) {
                this.abilities.put(entry.getKey(), new AbilityScoreType(entry.getValue()));
            }
        }
    }

    @Override
    public int getModifier(AbilityName ability) {
        return abilities.getOrDefault(ability, AbilityScoreType.UNDEFINED).getModifier();
    }
    @Override
    public AbilityScore getScore(AbilityName ability) {
        return abilities.get(ability);
    }

    /**
     * @return a read-only object encapsulating this one.
     */
    //This method is commented because I left out the ROAbilityScores class.
    //public AbilityScores getROAbilityScores() {
    //  return new ROAbilityScores(this);
    //}

    /**
     * Implementation of the {@link AbilityScore} interface based on the 
     * {@link Value} class. It adds mutability to the interface, which must be 
     * managed by the {@link AbilityScores} container.
     */
    private static class AbilityScoreType extends Value implements AbilityScore{

        final static AbilityScoreType UNDEFINED = new AbilityScoreType(ValueParameters.MIN_ABILITY_SCORE) {
            @Override
            public int getModifier() {
                return 0;
            }
            @Override
            public int getValue() {
                throw new IllegalAbilityScoreException(IllegalAbilityScoreException.Cause.UNDEFINED);
            }
        };
        /**
         * Basic constructor directly derived from {@link Value#Value(int)}.
         * @param value
         */
        AbilityScoreType(int value) {
            super(value);
        }

        /**
         * Makes a deep copy of the input {@link AbilityScore} object.
         * @param value to copy.
         */
        AbilityScoreType(AbilityScore value){
            super(value.getValue());
        }

        @Override
        public int getModifier() {
            return AbilityScore.computeModifier(this.getValue());
        }

    }

    @Override
    public Iterator<Map.Entry<AbilityName, AbilityScore>> iterator() {
        return new Iterator<Map.Entry<AbilityName, AbilityScore>>(){
            //Build atop an iterator for the names
            Iterator<AbilityName> nameIterator = EnumSet.allOf(AbilityName.class).iterator();
            @Override
            public boolean hasNext() {
                return nameIterator.hasNext();
            }

            @Override
            public Entry<AbilityName, AbilityScore> next() {
                AbilityName name = nameIterator.next();
                return new AbstractMap.SimpleImmutableEntry<AbilityName, AbilityScore>(name, getScore(name));
            }
        };
    };
}

And finally, the IllegalAbilityScoreException.java :

package model.exceptions;

/**
 * Exception raised when an object tries to create an illegal 
 * {@link model.values.AbilityScore} or {@link model.creatures.AbilityScores}.
 */
public class IllegalAbilityScoreException extends RuntimeException {
    private static final long serialVersionUID = 6846962934628169718L;

    public static enum Cause{
        NULL,
        UNDEFINED
    }

    /**
     * Initialises an {@link IllegalAbilityScoreException} for when an illegal
     * value is given to the constructor.
     * @param ability   name of the ability with an illegal value.
     * @param value     value in the illegal call to the AbilityScore
     * constructor.
     */
    public IllegalAbilityScoreException(String ability, int value){
        super("An ability score must be positive or null, received " + value 
                + "for ability " + ability + ".");
    }

    /**
     * Initialises an {@link IllegalAbilityScoreException} for when a required
     * ability is not initialised.
     * @param missingAbility
     * @param listOfAbilities
     */
    public IllegalAbilityScoreException(String missingAbility, String listOfAbilities) {
        super("Ability score " + missingAbility 
                + " must be defined, received no value for it in map with " 
                + listOfAbilities + ".");
    }

    /**
     * Initialises an {@link IllegalAbilityScoreException} for when an illegal 
     * call to {@link model.values.Value#getValue()} is performed on the UNDEFINED
     * AbilityScore or when an {@link model.creatures.AbilityScores} object is 
     * initialised with a null input.
     */
    public IllegalAbilityScoreException(Cause type) {
        super(pickMessage(type));
    }

    /**
     * Picks the message to set in the 
     * {@link IllegalAbilityScoreException#IllegalAbilityScoreException(Cause)}
     * constructor.
     * @param cause of the issue raising the exception
     * @return the adequate message for the given situation
     */
    static private String pickMessage(Cause cause) {
        String message;
        switch(cause) {
        case NULL:
            message = "Tried to create an AbilityScores object with a null input";
            break;
        case UNDEFINED:
            message = "Tried to call getValue() on the UNDEFINED AbilityScore.";
            break;
        default:
            message = "An unknown error has occurred";
        }
        return message;
    }
}

Question 3: is it reasonnable to unit-test the parameters to make sure that they are not set to a value that will break stuff? Some parameters must be exactly or above some values. This is written in a specific comment for each value, but to add one more safety net, I have written simple unit tests to check these conditions. Can I?

For example, I have a ValueParametersTest.java file which includes the following code to check the conditions of the ValueParameters class defined in question 2:

package service.parameters;

import static org.junit.Assert.*;

import org.junit.Test;

public class ValueParametersTest {

    /**
     * Checks that the ability score parameters have reasonable values.
     */
    @Test
    public void testAbilityScoreParameters() {
        assertEquals("MIN_ABILITY_SCORE must be 0.", 0, ValueParameters.MIN_ABILITY_SCORE);
        assertTrue("MAX_ABILITY_SCORE must be at least 40.", ValueParameters.MAX_ABILITY_SCORE > 40);
    }

    /**
     * Checks that the roll parameters have reasonable values.
     */
    @Test
    public void testRollParameters() {
        assertEquals("MIN_NUMBER_OF_DICE must be 1.", 1, ValueParameters.MIN_NUMBER_OF_DICE);
        assertTrue("MIN_NUMBER_OF_SIDES must be at least 1.", ValueParameters.MIN_NUMBER_OF_SIDES >= 1);
        assertTrue("MIN_NUMBER_OF_SIDES must be at most 4.", ValueParameters.MIN_NUMBER_OF_SIDES <= 4);
        assertTrue("MAX_NUMBER_OF_DICE must be at least 10.", ValueParameters.MAX_NUMBER_OF_DICE >= 10);
        assertTrue("MAX_NUMBER_OF_SIDES must be at least 100.", ValueParameters.MAX_NUMBER_OF_SIDES >= 100);
    }
}
\$\endgroup\$
1
\$\begingroup\$

Question 1: Can global parameters be accessed to build a view, or does it break the pattern?

I understand your question as:
Should parameters controlling the view itself be part of the model layer?

My answer to that would be: yes.

Question 2: Are my exceptions sane, or I am doing something wrong?

The questions are:

  • does the caller need to destinguish between different of your exceptions (for other reason then showing the right error message)?

    If not you don't need custom exceptions at all.

  • does the caller need to destinguish between your exceptions and prefefines system Exceptions like i.e. NullPointerException (for other reason then showing the right error message)?

    If you answered the previous question with "yes" and this with "no" a single custom exception may be enough.

Question 3: is it reasonnable to unit-test the parameters to make sure that they are not set to a value that will break stuff?

Unittest verify any behavior that is important for your business logic. This means, you don't test data like "parameters".

\$\endgroup\$
2
  • \$\begingroup\$ Thanks! So, I should get rid of my XXXParametersTest class and possibly some of my exceptions if I don't intend on catching them specifically to respond to them, is that right? \$\endgroup\$
    – Anab
    May 24 '20 at 8:51
  • \$\begingroup\$ @Anab there usually is no "right or wrong" ;o) But yes, that is my suggestion. \$\endgroup\$ May 24 '20 at 10:02
2
\$\begingroup\$

You've got some answers to your initial questions, which you seem to be happy with, so I'm not going to address them. There are some things that I noticed about your code, however which I think are worth mentioning.

Tests

  • Naming Consider losing 'test' from the front of your test names. All the methods are annotated and in test class. Having the word test at the front of the method name just adds noise. Consider instead adding some indicator as to what it is you're testing for / the expected value. maxAbilityScore_aboveLowerBound. This will make it easier possible at a glance to see what the test is validating. It doesn't make a lot of difference on these tests, however larger tests (which you have in your github) will be clearer.
  • Size Tests should be aimed at testing one thing. How small 'one thing' is, can be open to interpretation. I tend to come down on the side of smaller is better, because if something breaks it's usually pretty clear what / why. testRollParameters is really testing the number of sides on the die and also the number of dice to roll. These seem like two separate things, so I'd probably break the test up. This might be too small for some people, however if I look at your github, the testGetModifier method contains 4 asserts, with two of them inside a for loop, wrapped in a while loop. This feels like it's testing too much and the intention of the test is lost. Things like parameterised tests and good naming can help maintain the intention if you need to the same test for a different set of inputs.

Magic Numbers

Consider

public static int computeModifier(int value) {
    return value/2 - 5;
}

It may be that 2 and 5 make perfect sense to you, or it may be that they are based on some formula from a book, in which case having them as actual numbers might make sense. However, I'm left wondering why 5, why not 4 or 6. If a constant would make sense here, consider using it to make the calculation clearer.

Fail flag / YAGNI

The way you're using a parameter to determine failure behaviour adds unnecessary complexity to your isValidAbilityScoreInput method. Every time you encounter an error condition you have if(fail) throw... else return false. This makes the intention of the code harder to follow. Personally, I would rather see two functions isValidAbilityScoreInput, which returns true/false and validateAbilityScoreInput, which throws an exception if it's invalid. This makes it clearer to the caller what behaviour to expect and simplifies the method implementation. This might seem like it's duplicating code, however you could implement it in a way to reduce this duplication. Also, as it stands your code base only ever calls the method with fail set to true, so you only really need the 'throws exception' version currently. Worry about the non-throwing version at the point that you actually need it...

There can be only one...or can there?

The way you're validating abilities will only flag up the first failing item. Consider:

for (Entry<AbilityName, Integer> entry : values.entrySet()) {
   if (entry.getValue() < ValueParameters.MIN_ABILITY_SCORE
            || entry.getValue() > ValueParameters.MAX_ABILITY_SCORE) {
       throw new IllegalAbilityScoreException(entry.getKey().toString(), entry.getValue());
   }
}

The check stops as soon as there is an entry that is invalid. You have a similar approach when checking for mandatory values. This might be acceptable / desired behaviour, however consider if it's worth collecting a list of failing abilities so that they can all be put into the exception.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your review! Your first two points are interesting and will go in my todo list. I think I have already addressed the last two. In commit 6193, the AbilityScores interface contains a method with signature public static Map<AbilityName, InvalidityCode> checkAbilityScoresValidity(Map<AbilityName, Integer> values) instead of the previous isValidAbilityScoreInput. What do you think? I'll check for other places where the points apply. \$\endgroup\$
    – Anab
    Jun 17 '20 at 19:09
  • \$\begingroup\$ @Anab If you need the error information, it seems like an improvement. However, consider the value of your comments. If the comment simply states what the code does 'reject null input', you probably don't need it. They can easily become out of date when you don't update them as you change the code '//If not, find the missing one and throw an exception'... \$\endgroup\$
    – forsvarir
    Jun 18 '20 at 8:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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