1
\$\begingroup\$

I made my own class that inherits from button and has an OnHold event. It seems to work ok but I'm wondering if I can improve it or if you can spot any glaring mistakes that I made.

Here is the code:

using UnityEngine;
using UnityEngine.UI;
using UnityEngine.EventSystems;
using UnityEngine.Events;
[AddComponentMenu("UI/Hold Button")]
public class HoldButton : Button
{
    private bool isBeingPressed;
    private float currTime;
    private static float TimeMax = 2;
    private bool startTimer;
    public ButtonHoldEvent onHold { get; set; }
    public override void OnPointerDown(PointerEventData eventData)
    {
        startTimer = true;
        currTime = 0;
    }
    public override void OnPointerUp(PointerEventData eventData)
    {
        startTimer = false;
        currTime = 0;
        isBeingPressed = false;
    }
    void Update()
    {
      if(startTimer)
        {
            currTime += Time.deltaTime;
            if(currTime >= TimeMax)
            {
                isBeingPressed = true;
            }else
            {
                isBeingPressed = false;
            }
        }
        if (isBeingPressed)
        {
            onHold.Invoke(); 
        }
    }

    public class ButtonHoldEvent : UnityEvent
    {
        public ButtonHoldEvent() : base()
        {

        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Quick tip since isBeingPressed is a boolean variable you can simply give it the value of a specific condition, instead of checking if(currTime >= TimeMax) then setting the value of isBeingPressed to true, You can just do isBeingPressed = currTime >= TimeMax \$\endgroup\$ – Denis Jul 30 '16 at 17:58
2
\$\begingroup\$

There is a few thing you can improve :

  • TimeMax is used like a constant here, but is not given the access modifier const.
  • Try to group your class members in a meaningful way. It could depend on their function(member of same interface), access-modifier, or what makes sense to you.

    public ButtonHoldEvent onHold { get; set; }
    
    private static float TimeMax = 2;
    
    private bool isBeingPressed;
    private float currTime;
    private bool startTimer;
    
  • The name currTime doesn't reflect what it contains. heldDuration is more appropriate. The same applies for TimeMax and should be renamed TriggerTime.

  • isPressed is fine, but isHeldDown fits more the theme here.

Full code :

[AddComponentMenu("UI/Hold Button")]
public class HoldButton : Button
{
    public ButtonHoldEvent onHold { get; set; }

    private const float TriggerTime = 2;

    private bool isHeldDown;
    private float heldDuration;

    public override void OnPointerDown(PointerEventData eventData)
    {
        isHeldDown = true;
        heldDuration = 0;
    }
    public override void OnPointerUp(PointerEventData eventData)
    {
        isHeldDown = false;
        heldDuration = 0;
    }

    void Update()
    {
        if (!isHeldDown)
        {
            heldDuration += Time.deltaTime;

            if (heldDuration >= TriggerTime)
            {
                onHold.Invoke();

                // uncomment next line, to make `onHold` trigger only once per mouse press-hold
                //isHeldDown = false;
            }
        }
    }

    public class ButtonHoldEvent : UnityEvent
    {
        public ButtonHoldEvent() : base()
        {

        }
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.