How to Refactor this big if condition

user_15096668 0
Viewed 220

How to refactoring this big if condition? weapon is a list and it;s gameobject

How to refactoring this big if condition? weapon is a list and it;s gameobject

public class Customanger : MonoBehaviour
{
    public List<GameObject> weapon;
    public static Customanger singleton;
    private void Awake() => singleton = this;
};

if (Customanger.singleton.weapon[1].activeSelf ||
    Customanger.singleton.weapon[2].activeSelf ||
    Customanger.singleton.weapon[3].activeSelf ||
    Customanger.singleton.weapon[4].activeSelf ||
    Customanger.singleton.weapon[5].activeSelf ||
    Customanger.singleton.weapon[8].activeSelf ||
    Customanger.singleton.weapon[10].activeSelf ||
    Customanger.singleton.weapon[12].activeSelf ||
    Customanger.singleton.weapon[13].activeSelf ||
    Customanger.singleton.weapon[14].activeSelf ||
    Customanger.singleton.weapon[15].activeSelf ||
    Customanger.singleton.weapon[16].activeSelf ||
    Customanger.singleton.weapon[17].activeSelf)
{
    dosomething();
}
refactoring c#
6 Answers

You can try .Any(),

Determines whether any element of a sequence exists or satisfies a condition.

//Here I considered length of Weapon array/list is 17
if (Customanger.singleton.weapon.Any(x => x.activeSelf))
{
    dosomething();
}

If you would like to check same condition for subset of weapons, then you can try below,

var subSetOfWeapons = Customanger.singleton.weapon.GetRange(1, 17);

if (subSetOfWeapons.Any(x => x.activeSelf))
{
    dosomething();
}

For more details: List.GetRange(int index, int count)

user_62998570

I don't know which types you are using. What I did is check which pattern is recurrent. I see that you always access the weapon list/array. Normally indexers can be iterated.

Some assumes are made because of the incompleteness of information.

You could write a loop for it:

private bool CheckActiveSelf(List<WeaponThing> weapons)
{
    // iterate each weapon
    foreach(var weapon in weapons)
        // when activeSelf, return true
        if(weapon.activeSelf)
            return true;
   
    return false;
}


// instead of passing the Customanger, you should pass the deepest
// level. So when the weapon system is used elsewhere, you can still
// use the method.
if(CheckActiveSelf(Customanger.singleton.weapon))
{
    DoSomething();
}
user_22661300

Whatever this check means, in OOP world it should be implemented as instance method (or property) with meaningful name in Customanger class:

class Customanger
{
    private static int[] activeWeaponIndexes = { 1, 2, 3, 4, 5, 8, 10, 12, 13, 14, 15, 16, 17 };
    public bool HasActiveWeapon => activeWeaponIndexes.Any(x => weapon[x].activeSelf);
}

Then condition will be reduced to:

if (Customanger.singleton.HasActiveWeapon) { \*do smth*\ }
user_15064540

There are multiple ways to do it, you can include or exclude items from your collection and than check for the "activeSelf" flag ex:

List<GameObject> excludedList = new List<GameObject>() { 
Customanger.singleton.weapon[1], 
Customanger.singleton.weapon[3], 
Customanger.singleton.weapon[5]};
if (Customanger.singleton.weapon.Except(excludedList).Any(x => x.activeSelf))
    dosomething();
    ```
user_52114060

You could do something like that:

var weaponIds = new List<int>() { 1, 2, 3, 4, 5, 8, 10, 12, 13, 14, 15, 16, 17 };

var weapons = weaponIds.Select(x => Customanger.singleton.weapon[x]);

if (weapons.Where(x => x.activeSelf).Any())
{
    DoSomething();
}

You could probably do better than that by splitting your weapon arrays into categories maybe? I can't help you more with the code you provided.

EDIT: Removed the completely unecessary ToList()

user_79021800

How did you decide that specifically { 1, 2, 3, 4, 5, 8, 10, 12, 13, 14, 15, 16, 17 } are the relevant weapon indices?

If they are all indices of weapons that distinguish themselves from the remaining weapons by fulfilling a certain condition, you could identify the relevant weapons directly and not bother with the indices.

As an example, you could use use .Where() to filter out the relevant weapons:

Customanger.singleton.weapon
    .Where(weapon => <weapon fulfills condition>)

Let's say the condition is that the relevant weapons all have a common trait: they all belong to scenes that have been loaded. The condition can then be defined as:

<weapon fulfills condition> = weapon.scene.isLoaded

, and you can filter your relevant weapons as follows:

IEnumerable<GameObject> weaponsInLoadedScenes = Customanger.singleton.weapon
    .Where(weapon => weapon.scene.isLoaded);

Now, you can use .Any() to perform your original if check on these relevant weapons:

if (weaponsInLoadedScenes.Any(weapon => weapon.activeSelf))
{
    DoSomething();
}

Both .Where() and .Any() are found in the namespace System.Linq.

If, on the other hand, the relevant weapon indices are not selected based on common weapon traits, I would basically refer to Another One's answer.

First, create a collection of the relevant weapon indices (preferably using a variable name that somehow describes why these are relevant weapon indices):

var relevantWeaponIndices = new List<int>() { 1, 2, 3, 4, 5, 8, 10, 12, 13, 14, 15, 16, 17 };

Then, verify conditions; either by first explicitly defining the relevant weapons:

var relevantWeapons = relevantWeaponIndices
    .Select(index => Customanger.singleton.weapon[index]);

if (relevantWeapons.Any(weapon => weapon.activeSelf))
{
    DoSomething();
}

or by getting the relevant weapons inline:

if (relevantWeaponIndices.Any(index => Customanger.singleton.weapon[index].activeSelf))
{
    DoSomething();
}

Both .Select() and .Any() are found in the namespace System.Linq.

user_172135260
Related