3
\$\begingroup\$

I'm creating a town plugin for the game Minecraft using the Bukkit API. It works fine and all, however I'm not satisfied with the quality of the code and I'd like to know how I can improve. I've been programming in Java for about a year and a half now, any advice is appreciated.

I have a few concerns

  • I'm not sure if I'm checking for null values too often. I use both annotations and the Validate#notNull function
  • I only recently (today) started writing JavaDocs for my code, should I be more/less detailed? Should I put them on more/less functions?
  • I use enums a lot. I'm not sure if this is bad practice, but in my opinion it makes the code more flexible
  • I take away a lot of the flexibility of the code. With my setters, most of them both set the actual value, and also validate it, and insert it into a MySQL database. My constructor also inserts the newly created object into a HashMap.

Nation.java

package com.shepherdjerred.sttowns.objects;

import com.shepherdjerred.sttowns.Main;
import com.shepherdjerred.sttowns.messages.MessageHelper;
import com.shepherdjerred.sttowns.mysql.objects.managers.NationInviteManager;
import com.shepherdjerred.sttowns.mysql.objects.managers.NationManager;
import org.apache.commons.lang.Validate;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

import java.util.*;

import static com.shepherdjerred.sttowns.objects.Nation.NationRank.CAPITAL;
import static com.shepherdjerred.sttowns.objects.Nation.NationRank.MEMBER;


public class Nation {

    @NotNull
    private static HashMap<UUID, Nation> nations = new HashMap<>();

    @NotNull
    private String name, motd;
    @NotNull
    private HashMap<Town, NationRank> towns;
    @NotNull
    private UUID uuid;
    @NotNull
    private HashMap<Nation, NationRelationship> relationships;
    @NotNull
    private EnumMap<NationFlag, Integer> flags;

    /**
     * Creates a new Nation object which is added to the nations Map
     * Doesn't add to database
     *
     * @param uuid The UUID of the nation
     * @param name The name of the nation
     * @param motd The MOTD of the nation
     */
    public Nation(@NotNull UUID uuid, @NotNull String name, @NotNull String motd) {
        Validate.notNull(uuid);
        Validate.notNull(name);
        Validate.notNull(motd);
        this.name = name;
        this.uuid = uuid;
        this.motd = motd;
        this.flags = new EnumMap<>(NationFlag.class);
        this.towns = new HashMap<>();
        this.relationships = new HashMap<>();
        setDefaultFlags();
        nations.put(uuid, this);
    }

    /**
     * Gets all stored nations
     *
     * @return HashMap with all nations
     */
    @NotNull
    public static HashMap<UUID, Nation> getNations() {
        return nations;
    }

    /**
     * Creates a new nation, adds it to the database, initializes variables
     *
     * @param name    The name of the nation
     * @param capital The town to be the nations capital
     */
    public static void createNation(@NotNull String name, @NotNull Town capital) {
        Validate.notNull(name);
        Validate.notNull(capital);
        Nation nation = new Nation(UUID.randomUUID(), name, MessageHelper.colorConfigString("nations.defaults.motd"));
        NationManager.createNation(nation);
        capital.setNation(nation);
        nation.setTown(capital, CAPITAL);
        NationManager.setNation(capital, nation, CAPITAL);
    }

    /**
     * Gets a nation from the given string
     *
     * @param name The nations name
     * @return Returns a Nation if a nation with the given name exists, null otherwise
     */
    @Nullable
    public static Nation getNation(@NotNull String name) {
        Validate.notNull(name);
        for (Nation nationLoop : getNations().values())
            if (nationLoop.getName().equalsIgnoreCase(name))
                return nationLoop;
        return null;
    }

    /**
     * Check if a nation with the given name exists
     *
     * @param name The nations name
     * @return Return true if a nation with the given name exists, false otherwise
     */
    public static boolean nationExists(@NotNull String name) {
        Validate.notNull(name);
        for (Nation nationLoop : getNations().values())
            if (nationLoop.getName().equalsIgnoreCase(name))
                return true;
        return false;
    }

    /**
     * Gets a nation from the given uuid
     *
     * @param uuid The nations uuid
     * @return Returns a Nation if a nation with the given uuid exists, null otherwise
     */
    @Nullable
    public static Nation getNation(@NotNull UUID uuid) {
        Validate.notNull(uuid);
        return getNations().getOrDefault(uuid, null);
    }

    /**
     * Check if a nation with the given uuid exists
     *
     * @param uuid The nations uuid
     * @return Return true if a nation with the given uuid exists, false otherwise
     */
    public static boolean nationExists(@NotNull UUID uuid) {
        Validate.notNull(uuid);
        return getNations().containsKey(uuid);
    }

    /**
     * Sets the nation and rank of the given town, and updates it in the database
     *
     * @param town The town to set
     * @param rank The rank to give the town, if this is NationRank.CAPITAL, there must not already be a nation capital
     */
    public void setTown(@NotNull Town town, @NotNull NationRank rank) {
        Validate.notNull(town);
        Validate.notNull(rank);
        if (rank == CAPITAL)
            Validate.isTrue(getCapital() == null);
        towns.put(town, rank);
        town.setNation(this);
        NationManager.setNation(town, this, rank);
    }

    /**
     * Accepts an invite to a nation
     *
     * @param town The town to join, this town must be invited
     */
    public void acceptInvite(@NotNull Town town) {
        Validate.isTrue(town.isInvited(this));
        NationInviteManager.removeInvite(town.getNationInvites().get(this));
        town.getNationInvites().remove(this);
        setTown(town, MEMBER);
    }

    /**
     * Removes the town from the nation, and updates it in the database
     *
     * @param town The town to remove
     */
    public void removeTown(@NotNull Town town) {
        Validate.notNull(town);
        towns.remove(town);
        town.setNation(null);
        NationManager.removeTownNation(town);
    }

    /**
     * Sets the relationship with the given nation, and updates it in the database
     *
     * @param nation The nation to set the relationship
     * @param status The status to set between the two
     */
    public void setRelationship(@NotNull Nation nation, @NotNull NationRelationship status) {
        Validate.notNull(nation);
        Validate.notNull(status);
        relationships.put(nation, status);
        NationManager.setStatus(this, nation);
    }

    /**
     * Sets the given nation flag, and updates it in the database
     *
     * @param flag  The flag to set
     * @param value The value to set the flag to
     */
    public void setFlag(@NotNull NationFlag flag, @NotNull Integer value) {
        Validate.notNull(flag);
        Validate.notNull(value);
        flags.put(flag, value);
        NationManager.setFlag(this, flag);
    }

    /**
     * Deletes a nation, and removes it from the database
     */
    public void delete() {
        getNations().remove(this.getUuid());
        towns.keySet().forEach(town -> town.setNation(null));
        // Remove nation_towns (MySQL only)
        NationManager.deleteTowns(this);
        // Delete flags (MySQL only)
        NationManager.deleteFlags(this);
        // Remove relationships
        getNations().values().forEach(nation -> {
            if (nation.relationships.containsKey(this))
                nation.relationships.remove(nation);
        });
        NationManager.deleteRelationships(this);
        // Remove town nation invites (MySQL only)
        NationInviteManager.removeTownInvites(this);
        NationManager.deleteNation(this);
    }

    /**
     * Load the default flags for the nation.
     * Doesn't write over existing flags.
     * Doesn't save to database.
     */
    public void setDefaultFlags() {
        for (NationFlag flag : NationFlag.values())
            if (!flags.containsKey(flag))
                flags.put(flag, Main.getInstance().getConfig().getInt("nations.defaults.flags." + flag.toString().toLowerCase()));
    }

    /**
     * Get the capital of this nation.
     * Can return null, but it shouldn't, all nations are supposed to have one capital
     *
     * @return A Town object which is the capital of this nation
     */
    @Nullable
    public Town getCapital() {
        for (Map.Entry<Town, NationRank> entry : towns.entrySet())
            if (entry.getValue().equals(CAPITAL))
                return entry.getKey();
        return null;
    }

    @NotNull
    public String getName() {
        return name;
    }

    /**
     * Sets the name of the nation, and updates it in the database
     *
     * @param name The new name, name should be no longer than 16 characters, and not already taken
     */
    public void setName(@NotNull String name) {
        Validate.notNull(name);
        Validate.isTrue(name.length() <= 16);
        Validate.isTrue(!nationExists(name));
        this.name = name;
        NationManager.updateName(this);
    }

    @NotNull
    public HashMap<Town, NationRank> getTowns() {
        return towns;
    }

    /**
     * Check if a uuid is the owner of the nation
     *
     * @param uuid The uuid to check
     * @return True if the uuid is the owner, false otherwise
     */
    public boolean isOwner(@NotNull UUID uuid) {
        Validate.notNull(name);
        for (Town town : towns.keySet())
            if (town.isCapital())
                return town.getOwner().equals(uuid);
        return false;
    }

    /**
     * Get all members in the nation.
     * This does not include the capital
     *
     * @return A list of town members
     */
    @NotNull
    public List<Town> getMembers() {
        List<Town> members = new ArrayList<>();
        getTowns().forEach((town, rank) -> {
            if (rank != CAPITAL)
                members.add(town);
        });
        return members;
    }

    @NotNull
    public UUID getUuid() {
        return uuid;
    }

    @NotNull
    public String getMotd() {
        return motd;
    }

    /**
     * Sets the nations motd, and updates it in the database
     *
     * @param motd The new motd
     */
    public void setMotd(@NotNull String motd) {
        Validate.notNull(motd);
        Validate.isTrue(motd.length() <= 65535);
        this.motd = motd;
        NationManager.updateMotd(this);
    }

    @NotNull
    public HashMap<Nation, NationRelationship> getAllRelationships() {
        return relationships;
    }

    /**
     * Sets all nation relationships.
     * Does not update the database
     *
     * @param relationships HashMap containing these relationships
     */
    public void setAllRelationships(@NotNull HashMap<Nation, NationRelationship> relationships) {
        Validate.notNull(relationships);
        this.relationships = relationships;
    }

    @NotNull
    public Integer getFlag(@NotNull NationFlag flag) {
        Validate.notNull(flag);
        return flags.get(flag);
    }

    @NotNull
    public List<Nation> getAllies() {
        List<Nation> allies = new ArrayList<>();
        getAllRelationships().forEach((nation, relationship) -> {
            if (relationship == NationRelationship.ALLY)
                allies.add(nation);
        });
        return allies;
    }

    @NotNull
    public List<Nation> getEnemies() {
        List<Nation> enemies = new ArrayList<>();
        getAllRelationships().forEach((nation, relationship) -> {
            if (relationship == NationRelationship.ENEMY)
                enemies.add(nation);
        });
        return enemies;
    }

    /**
     * Set all flags for this nation
     * Doesn't effect database
     *
     * @param flags HashMap with this nations flags
     */
    public void setAllFlags(@NotNull EnumMap<NationFlag, Integer> flags) {
        Validate.notNull(flags);
        this.flags = flags;
    }

    /**
     * Set all towns in this nation
     * Doesn't effect database
     *
     * @param towns HashMap with this nations towns
     */
    public void setAllTowns(@NotNull HashMap<Town, NationRank> towns) {
        Validate.notNull(name);
        this.towns = towns;
    }

    @Nullable
    public NationRelationship getRelationship(@NotNull Nation nation) {
        Validate.notNull(nation);
        return relationships.get(nation);
    }

    /**
     * Send a message to all members of this nation
     *
     * @param message The message to send
     */
    public void broadcastToNation(@NotNull String message) {
        Validate.notNull(message);
        getTowns().keySet().forEach(town -> town.broadcastToTown(message));
    }

    /**
     * Get the owner of the nation
     *
     * @return The uuid of the owner of the nations capital town
     */
    public UUID getOwner() {
        return getCapital().getOwner();
    }

    public enum NationFlag {
        OPEN, TIER
    }

    public enum NationRank {
        CAPITAL, MEMBER
    }

    public enum NationRelationship {
        ALLY, NEUTRAL, ENEMY
    }

}

If anything is unclear, or more context is needed, the repository is located here https://github.com/ShepherdJerred/stTowns/tree/ea6e5b08fbfa72b462d183ad280c91cf68926c5e

\$\endgroup\$
  • \$\begingroup\$ Welcome to code review! Thanks is implicit in every post, so you can remove it. You could also list your particular concerns about the code (if any) like readability, performance, design. You can do it by editing the post. Other than that, nice first post! \$\endgroup\$ – Incomputable Jul 19 '16 at 21:04
  • \$\begingroup\$ I went ahead and added some concerns I had, and removed my thanks :) \$\endgroup\$ – Jerred Shepherd Jul 19 '16 at 21:14
1
\$\begingroup\$

It's quite an interesting plugin you've got there. This isn't a full review, just some initial thoughts I had while I was looking through the code.

Constraints

I've never used the @NotNull constraints, but from what I can tell they basically introduce compiler warnings so if you do something that could result in assigning a null to one of your fields marked with a @NotNull constraint you should get a compiler warning in the IDE. If you're paying attention to the warnings then you shouldn't really need to check for null in your methods (although I probably would anyway). If on the other hand you're not paying any attention to these warnings, then the constraints probably aren't doing anything for you.

Responsibilities

Your Nation class is quite large. It seems like it's got at least two responsibilities. The first is maintaining the static list of all nations and the second is supporting the behaviour of an individual Nation, adding towns, relationships with other Nations etc. I'd consider trying to split this functionality into two classes. This would make each classes responsibilities more cohesive and easier to follow.

Static State

The static nations map is shared across all instances of Nation. I don't really know much about the way Bukkit manages calls, but does it support any kind of threading that can result in multiple calls hitting your plugin at the same time? If it does you might need to protect the map with some locks.

\$\endgroup\$
  • \$\begingroup\$ I know the class is large and I was having trouble splitting it up. I'll try creating a NationHelper class with all the static methods inside of it. Bukkit is partially multi-threaded, and I only don't do any writing off of the main thread, only reading. \$\endgroup\$ – Jerred Shepherd Jul 19 '16 at 22:20

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.