LegacyFactions

[markhughes]

Hello! Thanks so much for adding LegacyFactions support! 🕺

I noticed you are casting LegacyFactions Board and FactionColl to MemoryBoard and MemoryFactions. Then accessing the fields from there.

You store a reference flocationIds and factions and use these. Is there a reason for this? As holding this reference is not safe for future plans when more database type support is added (e.g. with upcoming MySQL support these types are going to be shifted).

I'm going to add a warning to the code so there is a better understanding about this (sorry for not having that there already 😨)

Here are some suggestions:

getFactionIds

    @Override
    public Set<String> getFactionIds() {
-       return factions.keySet();
+       return FactionColl.all().stream()
+           .map(faction -> faction.getId())
+           .collect(Collectors.toList());
    }

getClaims

    @Override
    public List<ChunkPos> getClaims() {
        List<ChunkPos> target = new LinkedList<>();
-       target.addAll(getChunkPos(flocationIds.keySet()));
+       target.addAll(getChunkPos(Board.get().getAllClaims()));
        return target;
    }

I would create a PR but don't have time to setup the project and test it.

What do you think? Sorry for the rushed issue! 😄

[novucs]

No high frequency calls access these methods, so by all means this change sounds good. So long as getClaims() remains thread safe. I had held onto these references before in order to simply access them from other builds of Factions as they required some reflection. The code in this case just managed to worm its way in after copying and pasting.

[markhughes]

Sorry didn't see your response!

Looks great :) I tested it myself and noticed LegacyFactions really holds onto /f top so I added an integration for your plugin to pass on /f top -> /ftop

😄

[novucs]

Oh cool nice work, thanks! I must've only tested with /ftop