Solved Any way to clean up this code?

Below are methods used in a plugin I work on that's used to monitor block placement/item use. The code below, frankly, makes my eyes bleed, and I'm the one that wrote it! Can anyone offer some tips for how I can (hopefully) get rid of a lot of this redundancy so I can make this code work any better/be shorter?

Code:

    //Call when a monitored block is placed
    @EventHandler(priority = EventPriority.MONITOR)
    public void onPlayerPlaceBlock(BlockPlaceEvent event) {
        Player player = event.getPlayer();
     
        for(Iterator<Integer> iter = RedAlert.config.placeDisabled.iterator(); iter.hasNext();) {
            int disabledID = ((Integer)iter.next()).intValue();
            if(disabledID == event.getBlock().getTypeId() && !pl.isPermitted(player, "redalert.exempt")) {
                playerLocation = player.getLocation();
                lastBlock = event.getBlockPlaced().getType().name();
                logAlert("placed", player, lastBlock, playerLocation);
            } else {
                return;
            }
        }
    }
 
    //Call when a monitored block is broken
    @EventHandler(priority = EventPriority.MONITOR)
    public void onPlayerBreakBlock(BlockBreakEvent event) {
        Player player = event.getPlayer();
     
        for(Iterator<Integer> iter = RedAlert.config.breakDisabled.iterator(); iter.hasNext();) {
            int disabledID = ((Integer)iter.next()).intValue();
            if(disabledID == event.getBlock().getTypeId() && !pl.isPermitted(player, "redalert.exempt")) {
                playerLocation = player.getLocation();
                lastBlock = event.getBlock().getType().name();
                logAlert("destroyed", player, lastBlock, playerLocation);
            } else {
                return;
            }
        }
    }
 
    //Call when a monitored block is interacted with (doors, switches, etc.)
    @EventHandler(priority = EventPriority.MONITOR)
    public void onPlayerInteract(PlayerInteractEvent event) {
        Player player = event.getPlayer();
     
        if(event.getAction() == Action.RIGHT_CLICK_BLOCK) {
            for(Iterator<Integer> iter = RedAlert.config.useDisabled.iterator(); iter.hasNext();) {
                int disabledID = ((Integer)iter.next()).intValue();
                if(disabledID == event.getClickedBlock().getTypeId() && !pl.isPermitted(player, "redalert.exempt")) {
                    playerLocation = player.getLocation();
                    lastBlock = event.getClickedBlock().getType().name();
                    plugin.logMessage(lastBlock);
                    logAlert("used", player, lastBlock, playerLocation);
                } else {
                    return;
                }
            }
        }
    }
 
    //Call when a monitored item is used (food, flint and steel, etc.)
    @EventHandler(priority = EventPriority.MONITOR)
    public void onPlayerItemUse(PlayerInteractEvent event) {
        Player player = event.getPlayer();
     
        if(event.getAction() == Action.RIGHT_CLICK_BLOCK) {
            for(Iterator<Integer> iter = RedAlert.config.useDisabled.iterator(); iter.hasNext();) {
                int disabledID = ((Integer)iter.next()).intValue();
                if(disabledID == event.getPlayer().getItemInHand().getTypeId() && !pl.isPermitted(player, "redalert.exempt")) {
                    playerLocation = player.getLocation();
                    lastBlock = event.getPlayer().getItemInHand().getType().name();
                    logAlert("used", player, lastBlock, playerLocation);
                } else {
                    return;
                }
            }
        }
    }

Seriously, bleeding eyes. I've wrestled with ways of making "catch-all" methods, but the event API is kinda weird that way. Each event call works just differently enough that it never works out the way I want.

 

You could always put the events in another class... I had to do that with my plugin... It was 4000 lines in events.

 

Flawedspirit You could replace pl.isPermitted with player.hasPermission("permisson.node");

 

Oh, I didn't mention, this is an excerpt from my EventListener class. So I take it from what you're saying that I may have to just bite the bullet regarding events and leave it (sorta) as-is?

So far I've tried finding ways of running the Iterator in a single class, and having the information it needs passed to it via method call, but that decided to not work.

So instead I run the iterator in every event method, and then call the "logAlert" method, which passes a verb that describes what happened (or tried to happen), the offending player, the item's name, and the player's location, which I can extract coordinates from.

GodzOfMadness - Yeah, that's from a class I implemented a while ago for debugging purposes. It contains methods for getting permissions and getting/setting a player's location. It's pretty redundant, but I may decide to keep it around just for future debugging purposes anyway. That way I can keep that sort of code mostly out of my main classes.

 

You are correct that directly calling iterables is pretty ugly, but luckily you don't have to.

Code:

if(event.getAction() == Action.RIGHT_CLICK_BLOCK) {
for(Iterator<Integer> iter = RedAlert.config.useDisabled.iterator(); iter.hasNext() {
int disabledID = ((Integer)iter.next()).intValue();
if(disabledID == event.getClickedBlock().getTypeId() && !pl.isPermitted(player, "redalert.exempt")) {
playerLocation = player.getLocation();
lastBlock = event.getClickedBlock().getType().name();
plugin.logMessage(lastBlock);
logAlert("used", player, lastBlock, playerLocation);
} else {
return;
}
}
}

becomes

Code:

[FONT=Consolas]if(event.getAction() == Action.RIGHT_CLICK_BLOCK) {[/FONT]
[FONT=Consolas] for(int disabledID : RedAlert.config.useDisabled.iterator()) {[/FONT]
[FONT=Consolas] if(disabledID == event.getClickedBlock().getTypeId() && !pl.isPermitted(player, "redalert.exempt")) {[/FONT]
[FONT=Consolas] playerLocation = player.getLocation();[/FONT]
[FONT=Consolas] lastBlock = event.getClickedBlock().getType().name();[/FONT]
[FONT=Consolas] plugin.logMessage(lastBlock);[/FONT]
[FONT=Consolas] logAlert("used", player, lastBlock, playerLocation);[/FONT]
[FONT=Consolas] } else {[/FONT]
[FONT=Consolas] return;[/FONT]
[FONT=Consolas] }[/FONT]
[FONT=Consolas] }[/FONT]
[FONT=Consolas] }[/FONT]

Splitting it up into multiple methods would also help, and making a local variable called block that is event.getClickedBlock() would decrease the line length on the if. You can also split the if statement into two lines if it is still too long.

Also, you might want to look at hasPermission(permission) so it would be

!pl.hasPermission("redalert.exempt")

Speaking of which, you could move that entirely outside the loop, because if the player has that permission you can just early-return.

EDIT: It messed up my formatting :\

 

Trying out your code now, Tjstretchalot, but my compiler's complaining about it. Specifically, the

Code:

for(int disabledID : RedAlert.config.useDisabled.iterator()) {}

line.

I'm getting an error that says: "Can only iterate over an array or an instance of java.lang.Iterable"

I'm assuming the implementation present in the way the Bukkit processes the arrays found in .yml config files is not Iterable? Because "RedAlert.config.placeDisabled" is totally an array.

Edit: Well, ok, it's technically a List<Integer>, but that's how I always see it implemented in config file handlers, so I don't know if I have any recourse on this.

 

I see few diferences in the code, it's too repeatitive, I would convert that code into a single method and use that method in the events, it would be much cleaner.

And arrays are iterable but you should not give it an interator, give it the array directly:

Code:

for(int id : RedAlert.config.useDisabled)

But... all I see you doing is iterating through an array looking for an ID... why don't you just use contains() ? Or better yet why don't you use a HashSet which is way faster than an array at contains()... look, it's so easy:

Code:

Set<Integer> disabled = new HashSet<Integer>();

// ...

if(disabled.contains(event.getClickedBlock().getTypeId()) && player.hasPermission("whatever"))
{
    // do stuff
}

 

Yes, I know the code is too repetitive, but I have not managed to actually get this down to a single method, because events in Bukkit don't seem to work that way. Each type of event seems to have its own unique methods, most of which aren't compatible. I wish I could just pass something like a "BlockBreakEvent" as a generic "Event" and then extract all the info I need in a separate method, but alas, I can't seem to do that.

Also, how would I integrate the whole Hashset stuff into my config handler? The API for dealing with .yml files seems to be rather strict regarding how you retrieve and where you store lists of integers.

 

Flawedspirit
Send the variables as arguments... player, block, the action condition still must be checked in PlayerInteractEvent... and I really don't know why you're using 2 same events there either, they seem to do the exact same thing... just use one!
And you can even input the list you need to check.

You'd need to convert them fromt list to hashset and vice versa if you're dealing with configs...
But regardless, that contains() way works with lists and sets.

EDIT: here's an example method I'd use for that:

Code:

private void checkBlock(Player player, String permission, Block block, Collection<Integer> list)
{
   // do stuff
}

 

Alright, I think I've done it. I just finished testing the plugin and everything works as it should. I've shoved my foot down my throat by handling all the events in one method (after saying I probably couldn't), and each "onEvent" method has far fewer redundant statements. The result of my work is below. It's not perfect, and I didn't implement some of the suggestions you guys had (if it ain't broke...) but the code looks way better now and I think I shaved a good 15-20 SLOC off the class.

Code:

    //Call when a monitored block is interacted with (doors, switches, etc.)
    @EventHandler(priority = EventPriority.MONITOR)
    public void onPlayerInteract(PlayerInteractEvent event) {
        Player player = event.getPlayer();
        Block block = event.getClickedBlock();
        if(event.getAction() == Action.RIGHT_CLICK_BLOCK) {
            handleEvent(event, player, block);
        }
    }
 
    private void handleEvent(Event event, Player player, Block block) {   
        String eventType = event.getEventName();
        playerLocation = player.getLocation();
        lastBlock = block.getType().name();
        itemName = null;
           
        if(player.hasPermission("redalert.exempt")) {return;}
        switch(eventType) {
            case "BlockPlaceEvent":
                for(int disabled : RedAlert.config.placeDisabled) {
                    if(disabled == block.getTypeId()) {
                        logAlert("placed", player, lastBlock, playerLocation);
                    }
                }
                break;
            case "BlockBreakEvent":
                for(int disabled : RedAlert.config.breakDisabled) {
                    if(disabled == block.getTypeId()) {
                        logAlert("destroyed", player, lastBlock, playerLocation);
                    }
                }
                break;
            case "PlayerInteractEvent":
                for(int disabled : RedAlert.config.useDisabled) {
                    if(disabled == player.getItemInHand().getTypeId()) {
                        itemName = player.getItemInHand().getType().name();
                        logAlert("used", player, itemName, playerLocation);
                    } else if(disabled == block.getTypeId()) {
                        logAlert("used", player, lastBlock, playerLocation);
                    }
                }
        }
    }

Thank you for everyone who helped me out (in a timely manner I might add!)