CODEWITHSUNDEEP
javaconditional-statementssimplify
Pocketkid2
Pocketkid2

Reputation: 853

How to simplify this Java code?

I'm writing some Java code for a Minecraft server software called Spigot (spigotmc.org) and I've just written this. The goal is to check each side of the variable block and check for a wall sign as you can see. It should only need to find one. Then it will update it and whatnot. I know this code can be simplified, but for the moment I can't see how without making an entire new function for the code inside the conditionals, which I don't want to do. Is there a better way to write this conditional? 

// Get an attached sign
Block sign;
if ((sign = block.getRelative(BlockFace.NORTH)).getType() == Material.WALL_SIGN) {
    org.bukkit.block.Sign data = (Sign) sign.getState();
    data.setLine(1, "OFF");
    data.update();
} else if ((sign = block.getRelative(BlockFace.EAST)).getType() == Material.WALL_SIGN) {
    org.bukkit.block.Sign data = (Sign) sign.getState();
    data.setLine(1, "OFF");
    data.update();
} else if ((sign = block.getRelative(BlockFace.WEST)).getType() == Material.WALL_SIGN) {
    org.bukkit.block.Sign data = (Sign) sign.getState();
    data.setLine(1, "OFF");
    data.update();
} else if ((sign = block.getRelative(BlockFace.SOUTH)).getType() == Material.WALL_SIGN) {
    org.bukkit.block.Sign data = (Sign) sign.getState();
    data.setLine(1, "OFF");
    data.update();
}

Upvotes: 0

Views: 139

Answers (3)

Aivean
Aivean

Reputation: 10882

According to DRY principle you need to reuse repeating parts. In your example the whole if block is the same, except for the face.

  1. Replacing ifs with for:

    List<BlockFace> blockFaces = Arrays.asList(BlockFace.NORTH, BlockFace.EAST, BlockFace.WEST, BlockFace.SOUTH);
for (BlockFace face : blockFaces) {
    Sign sign = block.getRelative(face);
    if (sign.getType() == Material.WALL_SIGN) {
        org.bukkit.block.Sign data = (Sign) sign.getState();
        data.setLine(1, "OFF");
        data.update();
        break;
    }
}

  2. Extracting method:

    private boolean updateAttachedSign(Block block, BlockFace face) {
    Sign sign = block.getRelative(face);
    if (sign.getType() == Material.WALL_SIGN) {
        org.bukkit.block.Sign data = (Sign) sign.getState();
        data.setLine(1, "OFF");
        data.update();
        return true;
    }
    return false;
}

{
    boolean signIsUpdated =
        updateAttachedSign(block, BlockFace.NORTH) ||
        updateAttachedSign(block, BlockFace.EAST) ||
        updateAttachedSign(block, BlockFace.WEST) ||
        updateAttachedSign(block, BlockFace.SOUTH);
}

The second solution, despite of being superior approach in general, seems to be a bit overcomplicated for your case. I'd stick with the first one.

Edit by @Constantin:

Use short-circuited conditionals prudently. Please refer to this excellent post on where this makes sense https://softwareengineering.stackexchange.com/questions/284415/short-circuit-evaluation-is-it-bad-practice

Upvotes: 2

Constantin
Constantin

Reputation: 1506

Maybe a loop?

// Get an attached sign
Block sign;
BlockFace[] faces = new BlockFace[] {
    BlockFace.NORTH,
    BlockFace.EAST,
    BlockFace.WEST,
    BlockFace.SOUTH};

for(BlockFace face : faces) {
    if ((sign = block.getRelative(face)).getType() == Material.WALL_SIGN) {
        org.bukkit.block.Sign data = (Sign) sign.getState();
        data.setLine(1, "OFF");
        data.update();
        break;
    }
}

This approach works well when you have many options to look at. Also, if you are doing similar logic elsewhere, definitely create a function that all clients with same needs can call on!

Upvotes: 2

Roy Shmuli
Roy Shmuli

Reputation: 5019

Your inner if scopes always the same, so you can use "or" command between the conditions:

// Get an attached sign
Block sign;
if (((sign = block.getRelative(BlockFace.NORTH)).getType() == Material.WALL_SIGN) 
 || ((sign = block.getRelative(BlockFace.EAST)).getType()  == Material.WALL_SIGN)
 || ((sign = block.getRelative(BlockFace.WEST)).getType()  == Material.WALL_SIGN)
 || ((sign = block.getRelative(BlockFace.SOUTH)).getType() == Material.WALL_SIGN)
    org.bukkit.block.Sign data = (Sign) sign.getState();
    data.setLine(1, "OFF");
    data.update();
}

Upvotes: 1

Related Questions