Long Player movement script

All of this does just one thing. Move my player by finding the first finger that was used to touch the ship and drag it.

I have a few questions:

• The first thing is I have an embedded class of MotionState. At first glance it makes a lot of sense to start with that. I latter ended up not using it. How do I go about refactoring this part of the code to scrap it without breaking my code?

• My other question is on syntax. Further along down my code I have helper classes that use some variables. I've read that when you use variables with helper functions you should just declare those variables right above the function. Is that correct and am I using that methodology correctly?

• Finally if there is something you see that should be done another way because of some easier way to do things please let me know.

```
public class PlayerMovement : MonoBehaviour {
MeshRenderer meshRenderer;
[SerializeField] bool usingTouch;
Vector3 force;
ShipManager shipManager;
float topSpeed = 20;

public Canvas Canvas;
DebugController dc;
bool isBeingTouched = false;
int beingTouchedByFingerNumber = -1;
float zPos;
float distFromCamera;

MotionState motionState;
enum MotionState {
waiting,
start,
drag,
}

public void Start() {
meshRenderer = GetComponent();
shipManager = GetComponent();
dc = Canvas.GetComponent();
zPos = gameObject.transform.position.z;
GameObject camera = GameObject.Find("Main Camera");
distFromCamera = transform.position.z - camera.transform.position.z;//Used for ScreenToWorldPoint()
}

public LayerMask touchInputMask;
RaycastHit hit;
void Update() {
if (Input.touchCount > 0) {
foreach (Touch touch in Input.touches) {
Ray ray = Camera.main.ScreenPointToRay(touch.position);

if (Physics.Raycast(ray, out hit, touchInputMask)) {
GameObject recipient = hit.tra

The first thing is I have an embedded class of MotionState. At first
glance it makes a lot of sense to start with that. I latter ended up
not using it. How do I go about refactoring this part of the code to
scrap it without breaking my code?

First, MotionState is not a class but an enum. Setting this aside, you say that you are not using it, but you have used it 7 times in the current code.


My other question is on syntax. Further along down my code I have
helper classes that use some variables. I've read that when you use
variables with helper functions you should just declare those
variables right above the function. Is that correct and am I using
that methodology correctly?

These classes aren't contained in the question so it is a little bit hard to answer but I try it nevertheless.

You should always declare your variables as near as possible to their usage. If a variable is only used in one method it should be declared inside this method to give it the most limited scope.


Finally if there is something you see that should be done another way
because of some easier way to do things please let me know.

-
your indention is completely off. If you are using Visual Studio with the C# key binding, the default keyboard shortcuts are Ctrl + E,D to format the document or Ctrl + E,F to format the selected code.

-
you should stick to the style you have choosen.

Right now you have sometimes stated explicitly the scope of methods private sometimes you left it to default which also is private.

Sometimes you use braces {} for single statement if sometimes you don't.

I would like to encourage you to use them always which helps to make your code less error prone.

-
Comments should describe why something is done in the way it is done. Let the code speak for itself what is done by using meaningful names for variables, methods and classes.

-
Based on the naming guidelines methods should be named using PascalCase casing. Right now your are mixing PascalCase and camelCase casing for naming your methods.

-
Shortening variable names like DebugController dc; reduces the readability of the code. You should avoid using abbreviations.

-
you have some public variables in your class. This open up the class for beeing manipulated from the outside which can lead to unecpected behaviour.

If you need to read the values from outside of the class you should turn them into properties with a public getter and a private setter.

Now let us dig into your code

//Turns a Vector2 touchInput to a world point
private Vector3 turnTouchInputToWorldPoint(Vector2 v2) {
    Vector3 v3;
    if (usingTouch) {
        v3 = new Vector3(v2.x, v2.y, distFromCamera);
    } else {
        v3 = new Vector3(Input.mousePosition.x, Input.mousePosition.y, gameObjectToScreenPoint.z);
    }
    return v3;
}

could be written like

private Vector3 TurnTouchInputToWorldPoint(Vector2 v2) {

    if (usingTouch) {
        return new Vector3(v2.x, v2.y, distFromCamera);
    } 

    return new Vector3(Input.mousePosition.x, Input.mousePosition.y, gameObjectToScreenPoint.z);
}

btw, I just noticed you are using the K&R style for placing your braces instead of the Allman style. If this is written in your coding guidelines you should stick to it, if not it would be better to switch to Allman because most C# developer uses it. In this way a new developer can easier read your code.

private void OnTouchEnter(Touch touch) {
    beingTouchedByFingerNumber = touch.fingerId;
    isBeingTouched = true;
    shipManager.GetComponent().isKinematic = true;
    Vector2 v2 = Input.GetTouch(beingTouchedByFingerNumber).position;
    GetOffsetOfMouseFromShip(v2);
    motionState = MotionState.drag;
    meshRenderer.enabled = false;
}

public void OnTouchEnter() {
    isBeingTouched = true;
    shipManager.GetComponent().isKinematic = true;
    Vector2 v2 = Input.GetTouch(beingTouchedByFingerNumber).position;
    GetOffsetOfMouseFromShip(v2);
    motionState = MotionState.drag;
    meshRenderer.enabled = false;
}

Here the first method could be made smaller by calling the second method after using the Touch parameter like so

private void OnTouchEnter(Touch touch) {
    beingTouchedByFingerNumber = touch.fingerId;
    OnTouchEnter();
}

private void HandleMovement() {
    switch (motionState) {
        case MotionState.start:
            break;
        case MotionState.drag:
            Vector2 v2 = (Input.GetTouch(beingTouchedByFingerNumber).position);
            handlePlayerDrag(v2);
            break;
        case MotionState.waiting:
            break;
        default:
            break;
    }
}

by replacing the switch with a simple if like so

``
private void HandleMovement() {
if (motionState != MotionState.drag) { return; }

Vector2 v2 = (Input.GetTouch(beingTouchedByFingerNumber).position);
handlePlayerDrag(v2);
}


StackExchange Code Review Q#96302, answer score: 3