2
\$\begingroup\$

I'm currently writing a card-game in Unity and the below code handles the player's ability to drag/drop cards onto various sections of the game board. Right now, I have this nagging feeling that as it is written now, it violates the "Single Responsibility Principle" by handling the drag/drop operation as well as the dropzone/emptyspaceshaker interactions simultaneously.

A dropzone is a gameobject that contains an emptyspaceshaker component and signifies an object suitable for dropping the player's card on. The emptyspaceshaker component simply takes the object that it is attached to and mutates its transform.position each frame to provide a visual indicator to the player that this is the empty-space that the card will be dropped onto.

using UnityEngine;
using System.Linq;

public class CardDragDrop : MonoBehaviour
{
  // These variables are needed to complete a drag-drop process
  private bool _isDragging = false;
  private bool _isOverDropZone = false;
  private EmptySpaceShaker _dropZone;

  // These variable allow us to abort a drag-drop process
  private Vector3 _startPosition;
  private Transform _parent;
  private GameObject _emptySpaceShaker;

  public void OnTriggerEnter2D(Collider2D collision) {
    EndDropzoneShaking();
    _dropZone = collision.gameObject.GetComponent<EmptySpaceShaker>();
    StartDropzoneShaking();
  }

  public void OnTriggerExit2D(Collider2D collision) {
    if (_dropZone == collision.gameObject.GetComponent<EmptySpaceShaker>()) {
      EndDropzoneShaking();
    }
  }

  public void OnBeginDrag()
  {
    if (transform.parent.CompareTag("Undraggable"))
      return;

    ShowHiddenEmptySpaceShaker();
    BeginDragDropOperation();
  }

  public void OnDragging() {
    if (!_isDragging)
      return;

    transform.position = new Vector3(Input.mousePosition.x, Input.mousePosition.y, 0.0f);
  }

  public void OnEndDrag() {
    if (!_isDragging)
      return;

    _isDragging = false;
    if (!CanCompleteOperation()) {
      AbortDragDropOperation();
      return;
    }

    CompleteDragDropOperation();
    DisableDropZone();
  }

  private Transform GetTopmostCanvas() {
    Canvas[] c = gameObject.GetComponentsInParent<Canvas>();
    return c[c.Length - 1].transform;
  }

  private void ShowHiddenEmptySpaceShaker() {
    EmptySpaceShaker[] objs = GameObject.FindObjectsOfType<EmptySpaceShaker>(true)
                              .Where(o => !o.gameObject.activeInHierarchy && o.transform.parent == transform.parent)
                              .ToArray();
    _emptySpaceShaker = objs[objs.Length-1].gameObject;

    if (_emptySpaceShaker != null) {
      _emptySpaceShaker.gameObject.SetActive(true);
      _emptySpaceShaker.transform.SetSiblingIndex(transform.GetSiblingIndex());
    }
  }

  private void EndDropzoneShaking() {
    if ((_dropZone != null) && (_dropZone.IsShaking)) {
      _dropZone.EndShaking();
    }

    _isOverDropZone = false;
    _dropZone = null;
  }

  private void StartDropzoneShaking() {
    if (_dropZone != null) {
      _isOverDropZone = true;
      _dropZone.BeginShaking();
    }
  }

  private bool CanCompleteOperation() {
    if (!_isOverDropZone) 
      return false;

    if (_dropZone == null)
      return false;

    if (_dropZone.transform.parent.name.Substring(0, 3) != "Row")
      return false;

    return true;
  }

  private void BeginDragDropOperation() {
    _parent = transform.parent;
    _startPosition = transform.position;
    transform.SetParent(GetTopmostCanvas());

    _isDragging = true;
  }

  private void AbortDragDropOperation() {
    _emptySpaceShaker.gameObject.SetActive(false);

    transform.position = _startPosition;
    transform.SetParent(_parent);
    transform.SetSiblingIndex(_emptySpaceShaker.transform.GetSiblingIndex());
  }

  private void CompleteDragDropOperation() {
    transform.SetParent(_dropZone.transform.parent, false);
    transform.SetPositionAndRotation(_dropZone.transform.position, _dropZone.transform.rotation);
    transform.SetSiblingIndex(_dropZone.transform.GetSiblingIndex());
    transform.parent.gameObject.tag = "Undraggable";
  }

  private void DisableDropZone() {
    _dropZone.GetComponent<EmptySpaceShaker>().CanShake = false;
    _dropZone.gameObject.SetActive(false);
  }
}

This code works as is, however I'm concerned that as the project develops I will be extending this class just a little more with each iteration, ie: sound effects, particle effects, gradients etc.

Is there a way to separate these interactions (dropzone/emptyspaceshaker) and if so, how might I go about it? And, finally, how would I extend the process for future feature additions? Or am I worried about nothing because a little bit of interactions are just fine (perhaps expected at times)?

The whole idea of using GetComponent<> makes me think that there's just a little to much coupling going on and I want to address it now before it gets out of control.

I'm looking forward to any input that you can provide.

Thanks,

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I changed the title so that it describes what the code does per site goals, rather than how it's implemented: "State what your code does in your title.". Please check that I haven't misrepresented your code, and correct it if I have. \$\endgroup\$ Nov 23, 2022 at 7:11

1 Answer 1

1
\$\begingroup\$

So first thing I did was reverse the roles of the colliders. Player cards are now the triggers and the shakers handle the collisions themselves. This way makes way more sense to me.

Also, I created a class that handles the showing/hiding of the empty space under a player card when dragging and dropping. Another class that handles whether or not the intended drop is valid;and finally, the initial card drag and drop handler itself.

Except for reversing the colliders, I don’t really like the changes I’ve made. Seriously, in how many situations will I ever need to show/hide an empty space under a player card except when a player drags it away? Or validate a drop zone except when a card is drug over it?

It seems like it way over complicates things; reminiscent of my dealings with normalized data in databases. It makes sense at a theoretical level but practicality should also be a consideration.

I’m gonna leave it the way it is because the work is already done but I’m not sure if I’ll go through such extremes in the future without first coming across a need to do it.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Since no one has answered the question yet but you, you can change the code you posted by editing the question. As far as the answer goes, you aren't really commenting on the original code. Please read How do I write a good answer. \$\endgroup\$
    – pacmaninbw
    Nov 28, 2022 at 15:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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