How to compact my custom tree code

[HarryTechReviews]

I have been working on custom trees and know how to add a block anywhere I want but it seems like there is probably a much better way of doing it. Thought more experienced people might be able to compact it a bit. Thanks in advance

 

int branchHeight = 4;
            	for(int branchLength = 0; branchLength < 3; branchLength++)
            	{
            		BlockPos north = pos.north(branchLength);
            		BlockPos east = pos.east(branchLength);
            		BlockPos south = pos.south(branchLength);
            		BlockPos west = pos.west(branchLength);
            		
            		IBlockState nState = world.getBlockState(north);
            		IBlockState eState = world.getBlockState(east);
            		IBlockState sState = world.getBlockState(south);
            		IBlockState wState = world.getBlockState(west);
            		
            		if (nState.getBlock().isAir(nState, world, north) || nState.getBlock().isLeaves(nState, world, north))
                    {
                        this.setBlockAndNotifyAdequately(world, pos.north(branchLength).up(branchHeight), LOG);
                    }
            		if (eState.getBlock().isAir(eState, world, east) || eState.getBlock().isLeaves(eState, world, east))
                    {
                        this.setBlockAndNotifyAdequately(world, pos.east(branchLength).up(branchHeight), LOG);
                    }
            		if (sState.getBlock().isAir(sState, world, south) || sState.getBlock().isLeaves(sState, world, south))
                    {
                        this.setBlockAndNotifyAdequately(world, pos.south(branchLength).up(branchHeight), LOG);
                    }
            		if (wState.getBlock().isAir(wState, world, west) || wState.getBlock().isLeaves(wState, world, west))
                    {
                        this.setBlockAndNotifyAdequately(world, pos.west(branchLength).up(branchHeight), LOG);
                    }
            		
            	}

 

[jabelar]

I don't think there is any reason to "compact" it. It is already short, doesn't have any performance impact, and is fairly readable.

 

 

[HarryTechReviews]

It just seems hugely long to add every few blocks at once. Was hoping for a loop of some kind to create all states and set block

[Jay Avery]

What's the difference between this-

BlockPos north = pos.north(branchLength);
...
IBlockState nState = world.getBlockState(north);
...
if (nState.getBlock().isAir(nState, world, north) || nState.getBlock().isLeaves(nState, world, north))
{
  this.setBlockAndNotifyAdequately(world, pos.north(branchLength).up(branchHeight), LOG);
}

- and this -

BlockPos east = pos.east(branchLength);
...
IBlockState eState = world.getBlockState(east);
...
if (eState.getBlock().isAir(eState, world, east) || eState.getBlock().isLeaves(eState, world, east))
{
  this.setBlockAndNotifyAdequately(world, pos.east(branchLength).up(branchHeight), LOG);
}

?

 

The only difference is that every instance of north in the first example is replaced with east in the second example. You could streamline this by looping through the horizontal directions (EnumFacing.HORIZONTALS), and write out this procedure just once inside the loop.

Edited August 21, 2017 by Jay Avery

[jabelar]

Yeah, but you'll note that none of your code is really repeated -- you're checking different blocks / locations and placing blocks in different locations. You've already shortened the amount of code by assigning things like north, east, south, west.

 

Now the code you posted seems to be about checking specific location. Do you have other code that loops through lots of locations?  Or maybe you're just started and will add a lot more lines like the ones you show? In those cases, yes you might be able to compact it.

 

But there is nothing wrong with "brute" force approaches to programming. It is often less buggy to do it the way you're doing it, because if you try to get clever you can end up doing something wrong in a loop that creates very strange results. It also doesn't save much typing because you can cut and paste easily enough.

 

So my recommendation is to only look at compacting code if you have a lot of repetition or if you feel performance is being hampered due to inefficiency. Otherwise, as long as it is short and readable it is good code.

[jabelar]

11 minutes ago, Jay Avery said:

The only difference is that every instance of north in the first example is replaced with east in the second example. You could streamline this by looping through the horizontal directions (EnumFacing.HORIZONTALS), and write out this procedure just once inside the loop.

Not that easy though because it isn't just the conditions that cycle through the directions but also the action -- the positions are different in each case. So if he had an iterator for the direction he'd still have to look up what to do, probably using a bunch of if statements, when the condition is met. I really don't think it would save many lines of code.

 

If he was doing the SAME thing in each case then definitely it could be compacted, but harder when it does different things.

Edited August 21, 2017 by jabelar

[HarryTechReviews]

I guess I'll leave it then. Thanks for the help nontheless

[Jay Avery]

32 minutes ago, jabelar said:

Not that easy though because it isn't just the conditions that cycle through the directions but also the action -- the positions are different in each case. So if he had an iterator for the direction he'd still have to look up what to do, probably using a bunch of if statements, when the condition is met. I really don't think it would save many lines of code.

 

If he was doing the SAME thing in each case then definitely it could be compacted, but harder when it does different things.

The position is just moved in the relevant direction, so you can do pos.offset(facing).

Edited August 21, 2017 by Jay Avery

[jabelar]

Okay, fair enough. Generally looping through enums in Java is kinda annoying, but I guess EnumFacing class provides a couple of arrays that can help looping.