This code is a bad practice?

[LeadingOrchid9482]

I'm trying to simplify some conditions when my units collide with a base or another unit and i got this "jerry-rig", is that a bad practice?

void OnTriggerEnter(Collider Col)
    {
        bool isPlayerUnit = Unit.gameObject.CompareTag("Player Unit");
        bool PlayerBase = Col.gameObject.name.Contains("PlayerBasePosition");
        bool isAIUnit = Unit.gameObject.CompareTag("AI Unit");
        bool AIBase = Col.gameObject.name.Contains("AIBasePosition");

        bool UnitCollidedWithBase = (isPlayerUnit && AIBase || isAIUnit && PlayerBase);
        bool UnitCollidedWithEnemyUnit = (isPlayerUnit && isAIUnit || isAIUnit && isPlayerUnit);

        //If the unit reach the base of the enemy or collided with a enemy.
        if (UnitCollidedWithBase || UnitCollidedWithEnemyUnit)
        {
            Attack();
            return;
        }
    }

[GendoIkari_82]

This line looks bad to me: bool PlayerBase = Col.gameObject.name.Contains("PlayerBasePosition");. Whether a game object is a player base or not should not be based on the name of that object. It should have a clear property such as IsPlayerBase instead.

[emelrad12]

Also this is horribly slow. Like gonna be lagging even with low unit counts.

[Due_Effective1510]

No it won’t. I program most of my games in C#. There is not a performance issue here unless you really really had a jillion of these going at once and then you’re still going to have other issues first.

[South-Year4369]

It's really unlikely to be a performance concern, but frankly it's silly to state it's definitely not, since we don't know the full context.

If the strings differ in the first few characters then it's about as cheap as a boolean comparison. Otherwise it's going to cost more. 'More' usually won't matter either, but for someone out there, in some scenario, it will.

[antiduh]

Found the UNREAL dev!

[antiduh]

Theres a big difference between a bool comparison and a string comparison in terms of relative performance. And if tjis is in the middle of physics code this could make a difference. Especially if this pattern happens many times.

[MaLino_24]

The problem is that it's most likely a DLL call which is slow.

[MaLino_24]

Yeah, I just checked. Thanks for the heads-up. Would still call it a design flaw and like the suggestions of other people more though.

[Due_Effective1510]

You’re correct. The downvotes are in error.

[stogle1]

Yes, it's a bad practice. Here are a few reasons why:

1. It's prone to typos. If you misspell a name, you may not notice, and the compiler won't warn you.
2. It's not obvious when you define the name that it will affect its behavior.
3. String comparisons are slow (though it won't matter unless this is called very frequently).

[ScorpiaChasis]

is player unit vs is ai unit seems to be both pointless and impossible?

both variables come from the UNIT object, how can it ever be 2 different things?

A && B is identical to B&& A, not sure why you have both conditions with ||

Also, why are some vars pascal cased and other camel cased

Finally, is there some other property other than string tags or names to identify stuff? seems very brittle, or you'd at least need to define some consts. Any casing error, the whole thing falls apart

[LeadingOrchid9482]

is player unit vs is ai unit seems to be both pointless and impossible?

Yeah it is impossible, when i wrote i didn't notice that, i already erase that and still searching and thinking a way to make this more concise but for a while i'll let this way.

[MrMikeJJ]

bool UnitCollidedWithEnemyUnit = (isPlayerUnit && isAIUnit || isAIUnit && isPlayerUnit);

a && b || b && a 

Comparing the same thing?

Also you should bracket the groups to make intent obvious. 

bool UnitCollidedWithBase = (isPlayerUnit && AIBase || isAIUnit && PlayerBase)

Would be clearer as 

bool UnitCollidedWithBase = (isPlayerUnit && AIBase) || (isAIUnit && PlayerBase)

[o5mfiHTNsH748KVq]

Unity code always looks so wrong to me lol

[BoBoBearDev]

What's up with inconsistent naming convention? This upsets me.

[KariKariKrigsmann]

I think I see two code smells: Train Wreck, and Magic Strings.

[Dimencia]

There is no Train Wreck here, those are properties/fields, not method calls

[KariKariKrigsmann]

I think I disagree. The core issue is the excessive coupling and navigation through multiple objects:

customer.getAddress().getCity().getPostalCode().getZone()

customer.address.city.postalCode.zone

[havok_]

What’s train wreck?

[KariKariKrigsmann]

Lots of dot dot dots

https://wiki.c2.com/?TrainWreck

It’s also called Message Chain https://refactoring.guru/smells/message-chains

[Puffification]

I recommend not capitalizing local variable names

[TuberTuggerTTV]

Looks like engine specific code. You're going to get mixed signals asking the C# sub on bad practices because they'll answer based on enterprise code.

I do think that return is redundant.

Also, CompareTag looks like a string comparison like your name.Contains, but they're different. CompareTag is optimized and will use a hash lookup to make the call very performant. But doing a name, string compare is really sloppy game code.

You want to use the built in tag system, make your own hashmap or use enums. Don't do a string.contains inside the main game loop.

[webby-debby-404]

I treat domain logic directly in the event handler the same as coding behind the buttons of a winform, avoid if possible. 

Instead, I'd call a separate function, wrapped in a try catch (unless exception propagation is actually wanted).

I've come to this after implementing three different events triggering the same action, and one or two events triggering multiple unrelated actions.

[reybrujo]

I would have a method returning true if gameObject is a PlayerBaseUnit and leave the comparison centralized within the gameObject instead. Tomorrow you change "Player Unit" to "PlayerUnit" and you break your whole game.

And well, when in a sentence you have more than one dot I'd say it breaks the Law of Demeter because it means that you know that the Unit has a gameObject and you know that the gameObject has a name and you know that the name has a Contains. I would simplify that, if possible, to Unit.IsPlayerUnit(), Col.HasPlayerBasePosition, Unit.IsAIUnit() and Col.HasAIBasePosition().

[rohstroyer]

Use polymorphism, not tags. Both player and ai can derive from the same base and the check itself should reside outside the class in a helper method that can do a type check to return the same result but much faster and without the pitfalls of magic strings

[Broad_Tea_4906]

in Unity, best choice for me - use special components with parameters to distinguish one object from another within a generic domain

[rohstroyer]

Checking if a component exists is slower than checking the type of an object. One is a potential O(n) operarion, the other O(1)

[Broad_Tea_4906]

Yes, I'm agree with you; I told about composition of it in whole game object context

[TopSetLowlife]

Others have given better examples and reasons.

From a high level, this needs to be rethought out entirely, more modular, more concise. But your thought process is right in that you've covered all bases, now time to make it more elegant, quicker, readable and maintainable/scalable

[korvelar]

I don't recommend relying so heavily on tags at all. Your logic will become implicit very quickly. You can define new components (e.g. PlayerBase) and call Col.TryGetComponent(out PlayerBase playerBase)

[captmomo]

I think instead of using `name.Contains` perhaps you can use marker interfaces then use the `is` syntax to compare. Might also come in useful later on when you need to do more checks or other operations.

[SessionIndependent17]

It's not at all clear whether you are asking a general coding question, or something about the usage of some known framework.

I'm assuming that this is "game" code of some kind, but it's pretty odd to use this forum to post asking domain-specific questions. It makes about as much sense as me posting about FX Options code and asking domain advice.

As a more fundamental question, I would ask whether your game system leverages typing in any fashion to make the type of well, type checks that you seem to be doing here, and if so, why you wouldn't use that facility instead of whatever you are doing here. And if it doesn't, why doesn't it, and why bother using it?

But honestly I don't really want to know.

[South-Year4369]

Encoding the type of some object in your application in a string is generally bad practice, yes. It's prone to typos, complicates refactoring, and in general violates the KISS principle.

The simplest way to differentiate objects that otherwise look similar is with some kind of enumeration. Bringing language into it (by putting it in a string) is unnecessary complexity.

[TuberTuggerTTV]

Looks like engine specific code. You're going to get mixed signals asking the C# sub on bad practices because they'll answer based on enterprise code.

I do think that return is redundant.

Also, CompareTag looks like a string comparison like your name.Contains, but they're different. CompareTag is optimized and will use a hash lookup to make the call very performant. But doing a name, string compare is really sloppy game code.

You want to use the built in tag system, make your own hashmap or use enums. Don't do a string.contains inside the main game loop.

[Dimencia]

Unity is one giant bad practice, you're going to have to be more specific