CODEWITHSUNDEEP
javastatic
XLordalX
XLordalX

Reputation: 594

Am I using static correctly?

Lately I've been reading lots of posts about using static. I read a lot about static being abused. I want to be sure I am using it correctly in this Manager class:

public class SignManager
{
    private static HashMap<String, List<GameSign>> signsBySection   = new HashMap<>();
    private static HashMap<String, List<GameServer>> serversBySection = new HashMap<>();
    private static HashMap<String, GameServer>serverNames = new HashMap<>();
    private static HashMap<Sign, GameSign> gameSignBySign = new HashMap<>();
    private static List<GameServer> availableServers = new ArrayList<>();
    private static List<GameServer> displayedServers = new ArrayList<>();

    public static void addSign(String section, Sign sign)
    {
        List<GameSign> signs = signsBySection.get(section);

        if(signs == null)
            signsBySection.put(section, signs = new ArrayList<>());

        GameSign gameSign = new GameSign(section, sign.getLocation());

        signs.add(gameSign);
        gameSignBySign.put(sign, gameSign);
    }

    public static void addServers(String section, List<String> range)
    {
        List<GameServer> servers = SignManager.serversBySection.get(section);

        if(servers == null)
            SignManager.serversBySection.put(section, servers = new ArrayList<>());

        for(String s : range)
        {
            GameServer server = new GameServer(s);

            servers.add(server);
            serverNames.put(s, server);
        }
    }

    public static void setAvailable(GameServer server)
    {
        availableServers.add(server);
    }

    public static void replaceDisplayed(GameServer old, GameServer newServer)
    {
        removeDisplayed(old);
        displayedServers.add(newServer);
    }

    public static void removeDisplayed(GameServer server)
    {
        displayedServers.remove(server);

        if(server != null)
            server.setSign(null);
    }

    public static boolean isDisplayed(GameServer server)
    {
        return displayedServers.contains(server);
    }

    public static boolean isAvailable(GameServer server)
    {
        return availableServers.contains(server);
    }

    public static void tick()
    {
        for(GameSign sign : getAllGameSigns())
            sign.tick();

        GameSign.addDot();
    }

    public static GameServer getGameServer(String name)
    {
        return serverNames.get(name);
    }

    public static GameServer getNextAvailableServer()
    {
        if(availableServers.size() == 0)
            return null;

        GameServer server = availableServers.get(0);
        availableServers.remove(0);
        return server;
    }

    public static GameSign getGameSign(Sign sign)
    {
        return gameSignBySign.get(sign);
    }

    public static Set<Map.Entry<String, List<GameSign>>> getSignsBySection()
    {
        return signsBySection.entrySet();
    }

    public static Collection<GameServer> getAllServers()
    {
        return serverNames.values();
    }

    public static Collection<GameSign> getAllGameSigns()
    {
        return gameSignBySign.values();
    }
}

I also read that if the class has a state, it shouldn't be static. So does using static Maps mean that the class has a state and am I using static correctly here?

Thanks in advance.

Upvotes: 3

Views: 89

Answers (2)

BretC
BretC

Reputation: 4209

If you make everything static as you have above, it will make you code more tightly coupled. You could remove all of the static keywords from this class and use instances of it in other components and your code will still be correct.

Now if you wanted to make a different implementation of your class (or have more than one instance of it for some reason), you don't have to change that much, just inject the new implementation of the class into the components that use it.

It is also easier to write unit tests for classes that do not use static calls.

This article might help shed more light on why it's best to avoid them... http://www.devtrends.co.uk/blog/how-not-to-do-dependency-injection-the-static-or-singleton-container

Upvotes: 1

DaveH
DaveH

Reputation: 7335

On the face of it, I would say that you are not using static correctly. The class maintains state which is affected by the addServers and addSign methods. The state is maintained statically which means that if you had two distinct instance of the SignManager object, both would share the same state. This may be what you want - it may not.

If it is what you want, then using the Singleton pattern provides a more usual way of achieving this. If it is not what you want, then you should consider changing the static variable to instance variables ( and making the corresponding changes to the method signatures )

Upvotes: 1

Related Questions