6

To disguise players as another Entity, I made a disguise class as you can see here:

public class Disguise
{
    private static HashSet<Disguise> disguises = new HashSet<>();
    private net.minecraft.server.v1_8_R2.EntityLiving nmsEntity;
    private Player disguise;

    public Disguise(Player disguise, EntityLiving entity, boolean affectLogin)
    {
        if(affectLogin)
            disguises.add(this);

        this.disguise = disguise;
        this.nmsEntity = entity;
    }

    public Disguise(Player disguise, EntityLiving entity)
    {
        this(disguise, entity, true);
    }

    public void send(Player visible)
    {
        if(visible == disguise)
            return;

        EntityPlayer player = NMSUtils.getNMSPlayer(visible);

        nmsEntity.setPosition(player.locX, player.locY, player.locZ);
        nmsEntity.d(disguise.getEntityId());
        nmsEntity.setCustomName(disguise.getDisplayName());
        nmsEntity.setCustomNameVisible(true);

        PacketPlayOutSpawnEntityLiving spawn = new PacketPlayOutSpawnEntityLiving(nmsEntity);
        PacketPlayOutEntityDestroy destroy = new PacketPlayOutEntityDestroy(disguise.getEntityId());

        player.playerConnection.sendPacket(destroy);
        player.playerConnection.sendPacket(spawn);
    }

    public void send(List<Player> visible)
    {
        for(Player player : visible)
            send(player);
    }

    public void send(Player... visible)
    {
        send(Arrays.asList(visible));
    }

    public void send()
    {
        send(new ArrayList<>(Bukkit.getOnlinePlayers()));
    }

    public Player getDisguised()
    {
        return disguise;
    }

    public static HashSet<Disguise> getDisguises()
    {
        return disguises;
    }
}

I also have a static HashSet which stores all the instances made. I am doing this because I want players who login to see the disguise aswell and I want to remove the disguise from the player when the player logs out. Is a static HashSet the way to do it (like I'm doing it)? And if not, how should it be done?

Makoto
  • 104,088
  • 27
  • 192
  • 230
XLordalX
  • 584
  • 1
  • 7
  • 25
  • First of all, I would encapsulate `return disguises;`, otherwise other classes can start mutating the `HashSet`, never return the raw collections themselves... – Willem Van Onsem May 18 '15 at 15:16
  • 7
    you are abusing static, if you are trying to do OOP and instead are making it reference programming by never having to create a new object and instead just need to call the class without ever making an instance. – jgr208 May 18 '15 at 15:16
  • What do you perceive as the problem with having the HashSet be static? I'm not sure I see it. He has a centralized repository, and needs no instance for it. Is it because he's hiding it within Disguise? I would only have accessors/mutators for it though....never expose a collection directly. –  May 18 '15 at 15:17
  • @jgr208 I'm not sure if I understand what you're saying, but as far as I understand it I should remove the constructor and the HashSet and make send static? – XLordalX May 18 '15 at 15:22
  • Well does your program need to be or do you want it to be OOP, if so then don't use static unless you do not need an instance of a class like a utilities class. If you don't care about OOP and making a new instance of classes when needing to use a function for the class go ahead and use static everywhere. It's all about your design for what you want to do. – jgr208 May 18 '15 at 15:29
  • I like to stick to OOP unless not possible. – XLordalX May 18 '15 at 15:32
  • well is there any reason why your program may have two different instances of the hasmap? if so then you may want to keep it static so that you don't have two different instances of the hashmap floating around creating weird logic errors. if there is no reason that you will by accident have the chance of creating more then one hashmap if the api or program is used correctly then don't use static. – jgr208 May 18 '15 at 15:40
  • No, there is no reason to have multiple instances of it. – XLordalX May 18 '15 at 15:43
  • well then there should be no reason for it to be static if by design another instance of the hashmap will never be made. you can use a singleton to insure this doesn't happen. – jgr208 May 18 '15 at 15:46

3 Answers3

4

static was asking for it. By its nature, it is prone to "abuse", but that's just part of the challenge.

When all is said and done, if your mod does what you need it to do without bugginess, don't stress too much about best practices at this level of granularity (a particular variable). It's not likely to ever scale in scope to the point where poor design would cause problems for you. It's not a life-support system, after all.

If you want to practice good form for fun, my first instinct would be to move your management logic from Disguise to a (e.g.) DisguiseManager class, and handle all Disguise creation/destruction through a manager class. Less complex would be private constructor and static create/destroy methods on Disguise. Global side-effects in constructors like you posted is generally bad form.

Cheezmeister
  • 4,895
  • 3
  • 31
  • 37
  • Then would I have to use Singleton for the Manager class since I want to have one list of disguises only. – XLordalX May 18 '15 at 20:12
  • I don't see why you "have" to do anything at all. Singleton is essentially a static global in nicer clothes. If you only want one list, then only instantiate one. No need to overthink it :) – Cheezmeister May 19 '15 at 20:53
2

Basically everytime a constructor is called, you want to add this to a global place.

That's fine, but there are two concerns:

  1. exposing this in constructor is dangerous and requires careful analysis. (your code is buggy in this aspect)
  2. concurrency - if the app is multi-threaded, it needs to be thread-safe. (exposing this in constructor is more problematic in a concurrent environment)
  3. garbage collection - when the object becomes "garbage", how to remove it from the global place.
ZhongYu
  • 19,446
  • 5
  • 33
  • 61
2

Using static objects can get really frustrating when your code grows in size, and there are many accessors of the said object. If you were to debug the code, how would you catch the exact code to manipulate the HashSet?

Why don't you refactor the clients using the HashSet, to get it through a getter? What about encapsulation the HashSet instance as a Singleton ? It sounds like only one HashSet is ever created to store Players/Disguies.

Having any sort of getter method, say through Singleton, you could easily add additional code before or after accessing the HashSet. For example, after each usage of the method returning the HashSet, you can print the HashSet's contents. You could do that with static object as well, but the nightmare of finding all the usages of the static object...

Evdzhan Mustafa
  • 3,645
  • 1
  • 24
  • 40
  • Wouldn't using Singleton lead to the same point? Because I'm sure there won't be any future implementations. – XLordalX May 18 '15 at 15:34
  • @XLordalX updated my answer highlight what would be different. – Evdzhan Mustafa May 18 '15 at 15:48
  • Please don't use a singleton, it's well documented as to the problems this can cause -> http://stackoverflow.com/questions/12755539/why-is-singleton-considered-an-anti-pattern – tddmonkey May 18 '15 at 15:55
  • @MrWiggles the accepted answer from your link states "If you find that you want to use a singleton, you may want to consider your design, **but there are times where it is useful.**" – Jonny Henly May 18 '15 at 16:39
  • And those times are extremely rare, in fact I would argue not at all in a well designed system – tddmonkey May 18 '15 at 16:40
  • @MrWiggles I agree, but still the accepted answer further states "For example, once I had to write an application that could have at most one database connection, to process thousands of requests. So, a singleton makes sense since I am resource constrained to having only one instance." This example seems almost identical to the OP's scenario. – Jonny Henly May 18 '15 at 16:42
  • If I would use Singleton I won't be able to pass in the Player object though? – XLordalX May 18 '15 at 18:00
  • I'd also argue in that case it should have been the DataSource that is constrained with a pool size of 1 (or single connection data source). The Singleton constrains you in that if the amount of DB connections can increase, the code has to change. – tddmonkey May 19 '15 at 09:39