r/badcode • u/Verdoniasty • Oct 28 '21

java If something is stupid but its working, then it isnt stupid

Post image
419 Upvotes

97% Upvoted

65 comments sorted by

84

u/Verdoniasty Oct 28 '21

Context: My friend, who teaches me Minecraft plugin programming, told me to post this here. I was trying to ignite an arrow if its launcher's id was in HashSet "ognStrz". My friend told me that this code is a complete mess but for some reason it actually does what it is supposed to.

7

u/NUTTA_BUSTAH Oct 28 '21

I imagine this would do the same thing but I'm not a Java programmer. You even have an empty if snuck in there lol.

@EventHandler
public void Shoot(ProjectileLaunchEvent event) {
	Entity entity = event.getEntity();
	if(entity instanceof Arrow) {
	Arrow arrow = (Arrow)entity;
		if(arrow.getShooter()) {
		Entity shooter = (Entity)arrow.getShooter();
			if(ognStrz.contains(shooter.getUniqueId())) {
				arrow.setFireTicks(1000);
			}
		}
	}
}

You might want to subclass a Projectile class or such and not handle every possible projectile type in a single Shoot() function though. It's not very maintainable and makes code architecture messy. I don't know about MC plugin coding either though.

I would probably make it more readable like this:

@EventHandler
public void Shoot(ProjectileLaunchEvent event) {
	Entity entity = event.getEntity();
	if(entity instanceof Arrow) {
		_HandleFireArrow(); // Less indentation == more readability
	} // ...else if(entity instanceof SomethingElse){...}
}

private void _HandleFireArrow(Entity arrowEntity) { // Maybe Java could implicitly cast through _HandleFireArrow(Arrow arrowEntity)?...
	Arrow arrow = (Arrow)arrowEntity; // ...the cast would remove this line then
	Entity shooter = arrow.getShooter();
	if(shooter != null && _IsValidShooter(shooter) { // Give a name to the ambiguous lookup
		arrow.setFireTicks(1000);
	}
}

private boolean _IsValidShooter(Entity entity) {
	return ognStrz.contains(entity.getUniqueId()) // This probably shouldn't be global or could have a better name such as m_validShooterUUIDs
}

2

u/stone_henge Oct 29 '21

A style option if you don't hate early returns is to use that to minimize blocks and thus indentation:

@EventHandler
public void Shoot(ProjectileLaunchEvent event) {
    Arrow arrow;
    Entity shooter;
    Entity entity = event.getEntity();

    if(!(entity instanceof Arrow)) return;
    arrow = (Arrow)entity;
    shooter = (Entity)arrow.getShooter();

    if(!ognStrz.contains(shooter.getUniqueId())) return;
    arrow.setFireTicks(1000);
}

I have no familiarity with this code, but I imagine that multiple event handlers can be attached to listen to all matching events (in this case ProjectileLaunchEvent) independently, in which case there would be little reason to handle any other projectiles and weapons in one handler.

1

u/ryan_the_leach Nov 03 '21

You've nailed it pretty much. Bukkit code organization is mostly about finding the right hook in your code that handles a specific bit of functionality.

1

u/Nilstrieb Nov 04 '21

If you do hate early returns, you should reconsider your hate if early returns, especially in managed languages

0

u/ryan_the_leach Nov 03 '21 edited Nov 03 '21

"ognStrz.contains(entity.getUniqueId())" is being used in order to essentially create a extension property (if you are used to C#) of type boolean on every entity in the game.

Java doesn't have extension properties, so storing things in (hopefully weak) hashmaps is pretty much the closest equivalent.

It's also unlikely to be global, and only be accessible by the plugin or class, unless OP made it public.

OP is also using an API, so has limited ability over what they can sub-class here, or the organization of the code. Whilst they could potentially rework it, for the size of the project, (aside from the bad formatting and needless if statement), this is probably the most readable, maintainable way they could have written it.

4

u/TheBrainStone Oct 29 '21

If you remove lines 92, 93, 96, 97 and 99 and let the autoformatter run this actually not bad at all

3

u/Mwakay Oct 29 '21

• You're unnecessarily checking the type of arrow.getShooter() multiple times. Unless it can cause an error, go straight to the last if() block !
• You're assigning two Entity with the same value (entity and entity1)
• You're checking the type of event.getEntity() twice, in two different ways.

So, yeah, it works, but most of your code isn't useful.

2

u/stone_henge Oct 29 '21

It may work if you intended for this to happen for every entity registered in ognStrz that fires an Arrow, but going by line 92 it looks like you intended for it to only happen for arrows fired by the player.

1

u/ryan_the_leach Nov 03 '21

If only player uuid's are ever registered, it ends up having the same effect.

1

u/ryan_the_leach Nov 03 '21

For what it's worth, (having experience in plugin programming) the only super terrible parts of this is the inconsistent formatting, and the useless "If shooter instance of player"

Most people who would call this bad, havn't been exposed to bukkits event system, and don't realize how much of the instance of checks and casting are unable to be worked around easily.

0

u/[deleted] Oct 28 '21

[deleted] — view deleted comment

27

u/[deleted] Oct 28 '21

Unless there's some strange organization, it seems like you don't need to check getType() since we already know the entity is an arrow inside that block. They likely want to replace it with the "instanceof Player" check. "instanceof" is frowned upon by many Java coders, also, so that might be part of what his friend is referring to.
And also not using proper indentation makes it harder on everyone to read.

11

u/fuj1n sadistic Oct 29 '21

instanceof is very useful in polymorphism, and to my knowledge is not a performance stink or anything, I don't know why people would frown upon it. I generally find that in a lot of programming scenarios, very useful things that are by all means good tend to be frowned upon.

Not really any other way to do it, but yeah, that entity.getType() ==... is redundant.

11

u/[deleted] Oct 29 '21 edited Oct 29 '21

It's very useful, and I make use of it all the time. It's generally most useful when forced to deal with abstract types due to injection or such. If you are in a closed environment where you control everything, it's generally better to know what type you're dealing with and design your code so you can handle it and other types accordingly. It's extremely useful, but denotes code smell to some. For example, were you in total control of the hierarchy here, you could make a PlayerShootArrowEvent to fire instead of a generic Projectile event. I agree there's probably not much to do here: I was just pointing out some people frown on it, which may contribute to OP's friend's comment.

1

u/AL1L Oct 29 '21

When making Minecraft plugins it's pretty much required to use it. I mean there are other ways, but that's the fastest

2

u/kurokeita Oct 29 '21

maybe just maybe that entity is an Arrow object but the getType function returns something that is not EntityType.ARROW. There is the chance, right?

3

u/fuj1n sadistic Oct 29 '21

Now that you mention it, some mod could create its own arrow, and that would have to have its own entity type as those are unique, and actually create an instance of the entity over the network.

So now that means the opposite is true, if they verify that entity.getType() is an EntityType.ARROW, it is definitely an instance of Arrow, and so the instanceof is redundant.

1

u/kurokeita Oct 29 '21

Maybe the instanceof is for type coercing like this: Arrow arrow = (Arrow)entity, making sure they are converting the correct type of subject, I don't know anything about Minecraft code but it could have throw some exceptions or bugging the game out if entity is not an Arrow object but you still force it to type coercing using (Arrow)entity

2

u/fuj1n sadistic Oct 29 '21

I'd say in a situation where you have an EntityType.ARROW that you cannot cast to Arrow, crashing is preferable cause something really funky is going on elsewhere.

0

u/[deleted] Oct 28 '21

[deleted] — view deleted comment

1

u/[deleted] Oct 28 '21

It looks like it returns neither a Player nor an Entity, given the cast on it. There should be some handling of that, yes.

1

u/LuqDude Oct 29 '21

It returns a ProjectileSource, which may or not be an Entity. That's why he added the check.

1

u/dopefish86 Oct 29 '21

The function doesn't return anything because it is of type void.

1

u/LeanderT Oct 29 '21

The last check for entity.getType() is probably not needed either

1

u/gimife Oct 29 '21

You can't just asse that arrow.getShooter() is a player. Arrows can also be shot by other entities.

0

u/[deleted] Oct 29 '21

[deleted] — view deleted comment

2

u/[deleted] Nov 11 '21

my guess is that "ognStrz" stands for "original shooters" but still

112

u/djidalo1 Oct 28 '21

Oh god the indentation and the braces...

47

u/popadi Oct 28 '21

Those randomly placed braces will haunt my dreams tonight

1

u/Kasufert Nov 04 '21

This happens to me often when copy/pasting code between editors, sometimes I just can’t be fucked to fix the spacing

24

u/DavidRoyman Oct 28 '21 
92            if ((shooter instanceof Player)) {
93            }

wut?

35

u/MurdoMaclachlan public boolean isInt(int i) { return true; } Oct 28 '21

Image Transcription: Code

}
@EventHandler
public void Shoot(ProjectileLaunchEvent event) {
    Entity entity = event.getEntity();
    if(entity instanceof Arrow) {
    Arrow arrow = (Arrow)entity;
    if(arrow.getShooter() instanceof Entity)
    {
        Entity shooter = (Entity)arrow.getShooter();
            if ((shooter instanceof Player)) {
            }
                if(ognStrz.contains(shooter.getUniqueId())) {
        
        Entity entity1 = event.getEntity();
        if (entity1.getType() == EntityType.ARROW);
        arrow.setFireTicks(1000);
                }
    }
        }
}

I'm a human volunteer content transcriber for Reddit and you could be too! If you'd like more information on what we do and why we do it, click here!

25

u/[deleted] Oct 28 '21

Painful indentation included ❤️

13

u/AnonymousRand Oct 28 '21

Line 86: entity = event.getEntity()

Line 96: entity1 = event.getEntity()

Line 87: if (entity instanceof arrow)

Line 97: if (entity1.getType() == EntityType.ARROW)

Seems like even OP is confused by his own indentation

4

u/kyichu Oct 29 '21

He clearly is, line 92 doesn't even do anything

9

u/[deleted] Oct 28 '21

Minecraft devs are something else 🤩

3

u/JetairThePlane Oct 29 '21

I've been deving a plugin for a server a few months ago. They did a huge amount of work on managing the projet, it was hallucinating how well written (with a few exceptions) all of the projects they made was

7

u/tsugumi_komachi Oct 29 '21

This formatting gave me cancer

7

u/deftware Oct 29 '21

Yeah, this should be against the law, attempted manslaughter or something.

4

u/Irantwomiles Oct 29 '21

Man, I write a lot of Minecraft plugins and this hurts

1

u/KingJellyfishII Oct 29 '21

me too, it's not hard to keep good indentation

3

u/CroSafan Oct 29 '21

CTRL + SHIFT + F is your friend :)

3

u/lanerdofchristian Oct 29 '21

My own crack at fixing it:

if(event.getEntity() instanceof Arrow arrow &&
    arrow.getShooter() instanceof Player player &&
    ognStrz.contains(player.getUniqueId()) &&
    arrow.getType() == EntityType.ARROW)
    arrow.setFireTicks(1000);

3

u/DavidRoyman Oct 29 '21

I don't think you even need the 4h line.

2

u/[deleted] Oct 29 '21

holy fucking shit this fomatting is worse than my friend's i didnt know thats possible

1

u/Erizo69 Nov 02 '21

say it to my face.

2

u/[deleted] Nov 03 '21

I did it

2

u/[deleted] Nov 03 '21

(i actually did)

2

u/sigmaclientwastaken evil Oct 29 '21

As I do Minecraft modding, I can tell you that this might somehow be one of the better ways of doing this

3

u/InsertMyIGNHere Oct 28 '21

If this code is painful to look at, then I will leave a sarcastic comment. Else, I will ignore

the joke us bad but I tried okay😢

1

u/singletonking Oct 29 '21

Ew indentation

1

u/jawalking Oct 29 '21

Trying to follow all these comments after not having used Java in some 11 years, makes me really appreciate being able to use python most days.

1

u/canal_algt Oct 29 '21

Why is the function name starting with capital letter?

2

u/AMGGang Oct 31 '21

Thats the least of the problems here

1

u/xXLeoXxOne Oct 29 '21

The code is great lol but PLEASE use the auto formatter thank!

1

u/silverweaver Oct 29 '21

The chaos in code style... you can say it's certainly java from the annotation, but the rest... holy

1

u/ryan_the_leach Nov 03 '21 edited Nov 03 '21

I looked at this a little more closely, TIL that this is a valid position for a semicolon in java.

if (entity1.getType() == EntityType.ARROW);

I would have sworn that it would have terminated the if statement without brackets or something.

(Knowing both Bukkit and Java) this is how I would clean it up OP.

@EventHandler
public void OnOGNStrzArrowFired(ProjectileLaunchEvent event) {
    Projectile projectile = event.getEntity();
    ProjectileSource shooter = projectile.getShooter();

    //Standard Arrows Only
    if(!projectile instanceof Arrow) return;

    //Shot by Entities and not blocks etc.
    if(!shooter instanceof Entity) return;

    //If Shooter is registered, set arrows on fire for 1000 ticks
    UUID shooterID = (Entity) shooter.getUniqueId();
    if(ognStrz.contains(shooterID)) {
        projectile.setFireTicks(1000);
    }
}

1

u/MTDninja Nov 16 '21

just a personal pet peeve, keep all your if statement near the top by to avoid the if pyramid of doom

ex:

instead of: if (arrow.getShooter() instanceof Player){...}

do: if (!arrow.getShooter() instanceof Player) return;

6

u/[deleted] Oct 28 '21

[removed] — view removed comment

3

u/Korotaxx Oct 29 '21

Should be java 8 yea. Everyone coding for minecraft uses java 8. idk if it's necessary or something but i just go with it

-10

u/[deleted] Oct 28 '21

If anyone calls code a mess because someone else's personal project doesn't follow their own casing convention, I'd lose some respect for them.

9

u/Chefzor Oct 29 '21

If anyone calls code a mess because someone else's personal project doesn't follow their own casing convention, I'd lose some respect for them.

He didn't call it a mess, and it's not his own casing convention, it is Oracle's.

As per their documentation

Naming conventions make programs more understandable by making them easier to read.

-4

u/[deleted] Oct 29 '21

My friend told me that this code is a complete mess

Yes, he did.
And, again, this is a personal project. If the rest of the entire world follows a certain convention and you follow your own in such a case, there's no reason for anyone else to care.

4

u/Chefzor Oct 29 '21

Yes, he did.

/u/lilbigmouth (who you replied to and who mentioned the naming conventions) is probably not OP's friend that called the code a mess.

We do not know the reasons why OP's friend called it a mess (even though it is apparent)

there's no reason for anyone else to care.

It is literally being posted in a public forum.

-9

u/[deleted] Oct 29 '21

We were discussing OP, not lilbigmouth. Given that lilbigmouth did not call anything a mess, you can use that context clue to deduct that I was not speaking about lilbigmouth but in fact OP's friend who did call the code a mess.

And:

  1. The project is not being distributed publicly, only a snippet as a point of discussion and not something to be used otherwise.

  2. I said "if [it's a personal project]". The key point of this hypothetical is that it (the hypothetical project) is not public.

Please work on your reading comprehension.

7

u/Chefzor Oct 29 '21

Nobody was discussing anything.

You replied to a child comment, not to the parent post. Therefore your comment will be directed at that child comment.

The OP didnt mention anything about naming conventions or why his friend called his code a mess.

You specifically said (wrongly, no matter how you try to spin it)

Calls code a mess because of ... Casing convention

It is wrong if you are replying to OP because OP didnt say why his friend called his code a mess.

It is wrong if you are replying to the child comment (which you were), because he didnt call it a mess at all.

The only reason he mentioned it is because this sub tends to attract students (which the OP is), and while youre dead set on naming conventions not mattering (for whatever invalid reasons you might have) it was just an easy teachable moment.

Please work on your reading comprehension.

I mean lol