Moving GameObject in a square pattern (C# Unity) [closed]

Question

I'm new to Unity/C# and I'm trying to get a 3D GameObject in a square pattern, using transform.position:

 void Update() { bool flag = true; if (transform.position.x < 4 && transform.position.y >= -4 && flag == true) { Debug.Log("right"); transform.position = new Vector3(transform.position.x + 1, transform.position.y, 0); } else if (transform.position.x == 4 && transform.position.y >= -4 && transform.position.y <= 4 && flag == true) { transform.position = new Vector3(transform.position.x, transform.position.y + 1, 0); Debug.Log("up"); if (transform.position.y >= 4) { flag = false; } } else if (transform.position.x <= 4 && transform.position.x > -4 && transform.position.y == 4 && flag == false) { transform.position = new Vector3(transform.position.x - 1, transform.position.y, 0); Debug.Log("left"); } else if (transform.position.x == -4 && transform.position.y <= 4 && flag == false) { transform.position = new Vector3(transform.position.x, transform.position.y - 1, 0); Debug.Log("down"); if (transform.position.y == -4) { flag = true; } } 

As you can see, it's very limited in it's functionality because it uses hardcoded values on position to work.

Is there any way to improve this? Thanks so much!

Answer 1

General development and readability advice

This code may make sense to you because you wrote it, but it doesn't make sense to a reader who has no context.

The first example here: what is flag? What does it represent? What does it do? Based on my prior knowledge I can make an educated guess that it involves keeping track of the direction of travel, but if you were to ask me "what happens when flag is set to false?", I wouldn't be able to answer you even after having read your code several times.

There were claims in the comments that your code does not currently work as intended. I am genuinely uncertain of that, simply because your logic is so hard to follow.
This is one of those cases where you took a very complex route from the beginning, which made it hard to spot any alleged mistakes in the described behavior. Had you take a step back, analysed and abstracted your approach, and then implemented it; it would've been easier to keep it all straight.

The problem here is that you've overly condensed your logic. You've done the precise math and logic on what you want to have happen, and then you wrote it in the most terse and dense way you could.
This may be very efficient to write, but it is horribly inefficient to read. Code is read more than it is written, and you should invest more effort into the writing of your code in ways that it saves you effort on having to read the code afterwards.

To that end, let's start cleaning up the code by labeling things clearly, rather than putting the raw numbers in the logic directly. A prime candidate here is the logic for executing a move.

Move execution logic

Did you know that you can add Vector3 together without needing to pull it apart into its coordinates, individually adjust those, and then put it back together? This significantly simplifies the code.

First, define the vectors that describe the movement:

private const Vector3 MoveUp = new Vector3( 0, -1, 0); private const Vector3 MoveDown = new Vector3( 0, 1, 0); private const Vector3 MoveLeft = new Vector3(-1, 0, 0); private const Vector3 MoveRight = new Vector3( 1, 0, 0); 

And then your logic can simply add this vector to the original position:

if (...) { Debug.Log("right"); transform.position += this.MoveRight; } // and so on 

This significantly cuts down on the amount of logic and reading complexity in your snippet.

Move decision logic

Secondly, the other meaty bit of logic in your code is your if evaluations. Just by reading it, I don't know what it is that each if is trying to verify. It requires a lot of deep thought to understand it. Again, we must improve the readability.

I'm also going to slightly change your approach. Rather than have them execute the move, I'm going to have them decide what the move is. The execution of the chosen move is not something that needs to be repeated in every individual if, it can be done afterwards based on the move that has been decided.

I suspect that what led you to using your flag boolean is the fact that you "forget" which direction you were traveling in between subsequent calls of the Update method. This flag logic is very contrived. While it's possible to get it working, it's not easy (nor necessary) to do so. The better approach here is to remember which direction you were moving in, so you don't have to repeatedly figure it out. This is achieved by storing the "current direction" in a class field (or property), so that its value it retained inbetween subsequent calls to the Update method.

I'm also going to change the style of your evaluations. Right now, you're evaluating if you should continue moving. But it makes more sense to (by default) intend to move in the same direction as before; but check if you reach a certain boundary, in which case you change your desired direction. In other words, the if blocks will evaluate if you should change your direction, not if you should continue it.

Think of your square as a bounding box. Each side of the box represents a "stop" when you're moving in the direction that would cross the boundaries.

 stop IF moving up ┌───────────────────────┐ │ │ stop IF moving left │ │ stop IF moving right │ │ └───────────────────────┘ stop IF moving down 

If you use this approach, it simplifies the evaluations you have to do. I'm assuming a clockwise movement. For counter-clockwise, you'll have to change what the next direction is when you hit a boundary.

Vector3 currentDirection = this.MoveRight; // default movement public void Update() { // stop IF moving right if(currentDirection == this.MoveRight && transform.position.x >= 4) { // start moving down currentDirection = this.MoveDown; } // stop IF moving down else if(currentDirection == this.MoveDown && transform.position.y >= 4) { // start moving left currentDirection = this.MoveLeft; } // stop IF moving left else if(currentDirection == this.MoveLeft && transform.position.x <= -4) { // start moving up currentDirection = this.MoveUp; } // stop IF moving up else if(currentDirection == this.MoveUp && transform.position.y <= -4) { // start moving right currentDirection = this.MoveRight; } // Whatever the chosen move is, execute it transform.position += currentDirection; } 

Some remarks:

• The comments really help a new reader with finding your way through the ifs very quickly. I actually made a mistake in the original snippet, but I was able to quickly fix it because the comments made it clear which block did what.
• In most cases, none of the if blocks will be entered - these are all the cases where the character can keep moving in the direction that they were already moving. The if blocks will only be entered when you need to change your direction.
• Make sure to define currentDirection outside of the Update method, as you need it to be remembered inbetween subsequent calls to the Update method
• Note the use of Vector3 equality checks. I opted for == as it is less strict than .Equals and more likely to yield the desired behavior.
• Because I'm using <= and >=, this will also work for a character who is currently outside of the intended boundary. After doing a full loop (4 direction changes), they will have encountered the boundary and from then on stay inside the boundary you've defined.

Avoiding hardcoded values

Now that we've reworked the logic, we can remove the hardcoded 4 and -4 magic numbers. Just like we did for the movements, we're going to define private fields:

private int TopBoundary = -4; private int RightBoundary = 4; private int BottomBoundary = 4; private int LeftBoundary = -4; 

How you set/change these values is up to you. Either they're hardcoded (which is what I did here), but you could also have these be set via a constructor or public method, or make them into publically settable properties in their own right.

In any case, just replace the original hardcoded value with the field/property that represents the boundary:

// stop IF moving right if(currentDirection == this.MoveRight && transform.position.x >= this.RightBoundary) { // start moving down currentDirection = this.MoveDown; } 

I hope you agree that the final result is much more readable for anyone who doesn't have hands on knowledge of how it was written, whether they are other developers or you in the future when it's not as fresh in your memory anymore.

Comment
For your everyday business development practices, I would argue that transform.position.x >= this.RightBoundary is a bit too meaty to put in a chain of if blocks, and would be better abstracted into a clearly labeled variable or method.
However, your context is one of game development; where coordinate-based operations are so very common that abstracting them all into their own methods can lead to a massive explosion of "use once" methods.
The assumption here is that a game developer will learn to innately read and understand simple coordinate operations without needing to further abstract them for readability's sake.