Question

I am doing a project using Unity and C# but I don't feel this is necessarily Unity related. I have two separate hands that are represented by 2 instances of a hand class.

public class HandController
{//....class}

HandController LeftHand  = new HandController();
HandController RightHand = new HandController();

I am constantly doing twice the work in a lot of areas to affect the hands because each hand needs to be treated independently. So for instance I am using a Leap motion controller and if one of the hands is not detected I want to inform the user of this. So I change the color of the hand in the update method.

Color notDetected   = Color.red;
Color detected      = new Color(189/255.0f, 165/255.0f, 134/255.0f);

if (!LeftHandTracked)
    LeftHand.renderer.material.color = notDetected;

if (!RightHandTracked)
    RightHand.renderer.material.color = notDetected;

if (LeftHandTracked)
    LeftHand.renderer.material.color = detected;

if (RightHandTracked)
    RightHand.renderer.material.color = detected;

Is there a more efficient way of doing this? I hate having duplicate if conditionals sprawled all over my code. I also am tracking fingers, so each finger needs to be recognized and I get an even worse chain of if conditionals

    if (TappedFingers[0] && !_keySpamBlock)
        LeftHand.SetSide(true, _pointer);

    if (TappedFingers[1] && !_keySpamBlock)
        LeftHand.SetSide(true, _middle);

    if (TappedFingers[2] && !_keySpamBlock)
        LeftHand.SetSide(true, _ring);

    if (TappedFingers[3] && !_keySpamBlock)
        LeftHand.SetSide(true, _pinky); 

    if (TappedFingers[4] && !_keySpamBlock)
        RightHand.SetSide(true, _pointer);

    if (TappedFingers[5] && !_keySpamBlock)
        RightHand.SetSide(true, _middle);

    if (TappedFingers[6] && !_keySpamBlock)
        RightHand.SetSide(true, _ring);

    if (TappedFingers[7] && !_keySpamBlock)
        RightHand.SetSide(true, _pinky);

_pinky and _ middle and etc.. are hash values I pass into SetSide method in the HandController class that allow me to access the animationcontroller booleans I have set in Unity. SetSide() basically just sends the true if a user taps their finger and it plays an animation on the appropriate finger.

EDIT: To Clarify a little more whats going on

I am connecting to an API by inheriting a class and establishing an event listener:

public class AppListener : ErghisListener {


    public delegate void onUpdate(Data d);
    public event onUpdate Updated;

    public override void OnErghisFrame(Data data)
    {
        Loom.QueueOnMainThread(() => { this.Updated(data); });
    }
}

Then I have a MainController where recieve the data object from the API:

public class MainController: MonoBehaviour{

private AppListener _appListener;    

private int _pointer;
private int _middle;
private int _ring;
private int _pinky;

void Start()
{
    this._appListener = new AppListener();
    this._appListener.Updated += callback;

    this._pointer  = Animator.StringToHash("Pointer");
    this._middle   = Animator.StringToHash("Middle");
    this._ring     = Animator.StringToHash("Ring");
    this._pinky    = Animator.StringToHash("Pinky");

}

public void callback(Data d)
{
    // Here is where all my annoying if conditionals were. 
    bool[] TappedFingers = d.tappedF;
}
Was it helpful?

Solution

Your first code snippet would look much more logical if it was something like this:

class HandController : MonoBehaviour
{
    bool m_Tracked;

    Color NotDetected { get { return Color.red; } }
    Color Detected { get { return new Color(189/255.0f, 165/255.0f, 134/255.0f); } }

    public bool Tracked
    {
        if (m_Tracked == value) return;
        m_Tracked = value;
        renderer.material.color = value ? Detected : NotDetected;
    }
}

// ...

LeftHand.Tracked = LeftHandTracked;

if (TappedFingers.Length != FINGERS*2)
{
    Debug.LogError("Unexpected amount of fingers: " + TappedFingers.Length);
    return;
}

for(int i = 0; i < FINGERS; i++)
{    
    LeftHand.SetSide(TappedFingers[i], i);
}

for(int i = FINGERS; i < FINGERS*2; i++)
{    
    RightHand.SetSide(TappedFingers[i], i-FINGERS);
}

OTHER TIPS

You could simply use else here.

if (LeftHandTracked)
    LeftHand.renderer.material.color = detected;
else
    LeftHand.renderer.material.color = notDetected;

I would prefer inline conditional.

LeftHand.renderer.material.color = LeftHandTracked ? detected : notDetected;

As for the second example, you can wrap all the statements in a single if block to remove visual noise:

if (!_keySpamBlock)
{
    if (TappedFingers[0])
        LeftHand.SetSide(true, _pointer);

    if (TappedFingers[1])
        LeftHand.SetSide(true, _middle);
}

The other option with Linq (which is not more efficient, but so much prettier):

var sides = new[] { _pointer, _middle };

if (!_keySpamBlock)
    TappedFingers.Zip(sides, (x, y) => { if (x) { LeftHand.SetSide(true, y); });

I'd say this is clear and looks good enough. I don't see how polymorphism could help here, but you could investigate it yourself.

for the first part:

LeftHand.renderer.material.color = LeftHandTracked ? detected : notDetected; 
RightHand.renderer.material.color = RightHandTracked ? detected : notDetected;

second part:

if(!_keySpamBlock)
{
    int index=Array.FindLastIndex(TappedFingers.Take(8).ToArray(), i => i);
    switch (index)
    {
       case 0:  {LeftHand.SetSide(true, _pointer); break;}
       case 1:  {LeftHand.SetSide(true, _middle); break;}
       //.........

       case 7: {RightHand.SetSide(true, _pinky); break;}
    }
}
Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top