Help cleaning up code.

This is my first real attempt at coding for unity. Well, I used someone else’s code as my base but changed it quite a bit. Basically, it’s a code to make items in the game clickable and gives the player choices based on what they click. Then it clears out the variables (to clear the GUI text) on a timer. I’m far from done with it but want some help cleaning it up before I go on. It “works” but I know my code is sloppy and probably has some problems with it I haven’t yet discovered. What I have discovered is sometimes the “clearVar” function runs when I don’t want it to rather than timed like I want it. If there is a better way to clear the variables/GUI text after a few seconds, please tell me…

And before anyone asks “why did you do X that way and not this way”, I did it this way because it’s the only way I currently know how to do it… But I’m learning! Anyway, here is my sloppy code:

var target1: Transform;
var target2: Transform;
var whatHit = 0;
var playerAction = 0;
var playerResponse = 0;

function Update () {
	if (Input.GetMouseButton(0)) {
		var ray: Ray = Camera.main.ScreenPointToRay(Input.mousePosition);
		var hit: RaycastHit;
		
		
		
		if (Physics.Raycast(ray, hit)) {
			if (hit.transform == target1) {
				print("Hit target 1");
				whatHit = 1;
				//print(whatHit);
				playerResponse = 0;

			} else if (hit.transform == target2) {
				print("Hit target 2");
				whatHit = 2;
				playerResponse = 0;
			}
		} else {
			print("Hit nothing");
			whatHit = 0;
			playerResponse = 0;
		}
	}
		if (playerAction == 1) {
			if (Input.GetKeyDown("y")) {
			playerResponse = 1;

			}
		else if (Input.GetKeyDown("u")) {
			playerResponse = 2;

			}
			if (Input.GetKeyDown("i")) {
			playerResponse = 3;

			}
	}		
}

function OnGUI () {
if (whatHit == 1) {
GUI.Label(Rect(10,10,300,200), "You clicked the cube");
GUI.Label(Rect(10,30,300,200), "Press Y to attack");
GUI.Label(Rect(10,50,300,200), "Press U to talk");
GUI.Label(Rect(10,70,300,200), "Press I to ignore");
playerAction = 1;
}

else if (whatHit == 2){
GUI.Label(Rect(10,10,300,200), "You clicked the ground");
playerResponse = 0;

}
else if (whatHit == 0){
GUI.Label(Rect(10,10,300,200), " ");
playerResponse = 0;
}

if (playerResponse == 1) {
GUI.Label(Rect(10,90,300,200), "You attack the cube");
Invoke("clearVars", 2);
}
else if (playerResponse == 2) {
GUI.Label(Rect(10,90,300,200), "Hello Cube!");
Invoke("clearVars", 2);
}
else if (playerResponse == 3) {
GUI.Label(Rect(10,90,300,200), "You ignore the cube");
Invoke("clearVars", 2);
}

else if (playerResponse == 0) {
GUI.Label(Rect(10,90,300,200), " ");
}
}



function clearVars () {
whatHit = 0;
playerAction = 0;
playerResponse = 0;
}

Thanks!

If you’re aiming to clean-up your code, my general comment would be to choose clearer variable names, consider your types, avoid magic numbers, and modularize. Let’s go bit by bit…

if (hit.transform == target1)
if (hit.transform == target2)

Long-term, are you only going to have two targets? Chances are, you will have many (monsters, items, etc.) in which case instead of target1, tartget2, … targetN you’re going to want to create a targets[ ] array that you dynamically create based on what is on the scene using GameObject.FindGameObjectsWithTag() and iterate through with a for or foreach loop. That will make your code scalable.

var playerAction = {0,1};

This is better than var a501 = {13,39}, but what do the values 0 and 1 really mean? Simple variable names and precise type selection makes coding and logic errors less likely, and makes it easier to extend or maintain your code. Perhaps try bool isTargetSelected = {true,false}.

var playerResponse = {0,1,2,3}
else if (playerResponse == 2) {

That’s really cryptic. What does playerResponse=2 mean? You won’t know without scrolling up to check another section of your code. A better approach here is to use enumerations, eg, enum PlayerResponse { 0 = NONE, 1 = ATTACK, 2 = TALK, 3 = IGNORE} Then down below you can use more natural syntax such as “else if (playerResponse == TALK)”. Again, less chance of bugs and easier to maintain.

That’s one of the better questions I’ve seen. Cheers and good luck with your application. :slight_smile:

– Ostagar

Also, suppose if the player did the following:

t=0.0: Clicked on target1. You display “Press Y, U, or I”.
t=1.0: Presses U. You display “Hello Cube” and set cleanup() to run in 2s.
t=2.9: Presses Y. You display “Attacking Cube” and set cleanup() to run a second time in 2s.
t=3.0: CleanupVars() runs without player having time to read the attacking cube message.
t=4.9: CleanupVars() runs again for no apparent reason.

I’ve noticed that… Would making each of the if statements into functions in separate scripts and using GetComponent to call them help?

Moving Attack(), Talk(), and Ignore() into their own procedures makes sense when you’re ready to write real code to implement those actions, but for now your main issue is how to debug and improve your control logic. The way I would do that is to simplify per the advice in my first message, then simplify further by removing all your Invoke() calls and cleaning up using a cleanVarsTimer variable instead.

Thanks! I’m about to go step by step through your suggestions and see what I can do. Hopefully I won’t screw it up too bad. :slight_smile:

I’m having problems with the array. This is what I have thus far:

var clickableGO : GameObject[];
clickableGO = GameObject.FindGameObjectsWithTag("clickables");

function Update () {
	if (Input.GetMouseButton(0)) {
		var ray: Ray = Camera.main.ScreenPointToRay(Input.mousePosition);
		var hit: RaycastHit;
		
		
		
		if (Physics.Raycast(ray, hit)) {
			
			if (hit.transform == clickableGO) {
				print("Hit target 1");
				whatHit = 1;
				//print(whatHit);
				playerResponse = 0;

I’ve created the tag and assigned it to a game object but it doesn’t register the click. I’ve been scouring the forums and help files about arrays and thought I had a pretty good grasp on what I was doing… guess I was wrong.

From what I heard from pro guys - you gotta make things work first and then start cleaning up. The best in an enemy for the good. You can polish small things forever, while you are not actually progressing. I suggest making it iteratively. Achieve big goal, make small gameplay. Then start cleaining

Nikolay, actually, I am a “pro guy”, a very long-time pro at that, and I disagree. It’s my experience that it’s more expensive (money, time, reputation) to hack away at your code and then “clean it later” than it is to aim to develop reliable software from the get-go. It’s generally accepted that the earlier in the development cycle a bug is discovered, the cheaper it is to fix (and the faster you ship a good product).

True, but getting to specifics, using magic numbers throughout your code instead of enumerations makes bug and coding errors more likely. That’s the opposite of what you want if you want to finish your code quickly, have it work well, and have it work reliably.

Absolutely. A common example is the mission to land on the Moon. In the beginning, NASA wasn’t entirely certain what such a mission would look like, so they completed several smaller projects and missions first. It’s a great idea to divide your project into milestones and releases/iterations with smaller feature sets. That doesn’t mean that any of them should be hacked out or half-assed.

Argh. No! Unless by cleaning, you mean something that doesn’t contribute to the bottom line, unlike the suggestions above, in which case I agree we should move that deliverable to Never-Never Land. :slight_smile:

Getting back to your drewdradley’s question…

var clickableGO : GameObject[ ];
hit.transform == clickableGO

Well, hit.transform is a Transform and clickableGO is a GameObject[ ], so comparing them directly doesn’t work so well. When accessing elements of arrays you need to use array[element] syntax and usually a for or foreach loop.

Arrays are a bit tricky at first but very, very powerful once you get the hang of them. Try Googling “Javascript array tutorial” and you should find a wealth of pages explaining how to use them.

Here’s a bit of code from a game I made this weekend in 14 hours, using a switch statement and enums to make it easy to remember which color of enemy ship I was making changes to.

switch (myColor) {
   case EnemyColor.silver:	// do something..
   case EnemyColor.amber:	// do something..
   case EnemyColor.red:		// do something..
   default:			// do something..
}

If I were to use so-so variable names, magic numbers, and go to if’s this is what it looks like:

if (theShip == 1) {
   // do something
}
if (theShip == 2) {
  // do something
}
if (theShip == 3) {
  // do something
}
if ( (theShip <= 0) || (theShip >= 4) {
  // do something
}

Which do you think makes logic errors more likely?

They are tricky and make my head hurt. I did google “Javascript array tutorial” as well as “Javascript dynamic array tutorial”. They only made my head hurt worse. :slight_smile:

I get the concept of an array and the basics of it, but I think I need to spend some time on simple arrays before I can make them work in my script. The thing I am most confused about is populating the array from the tags. I mean, am I even close with what I tried?

var clickableGO : GameObject[];
clickableGO = GameObject.FindGameObjectsWithTag("clickables");

You’re right on. Assuming you actually tagged at least one GameObject as “clickables”.

I did. Next I need a for statement? Something like this?

for (i=0; i < clickableGO.length; i++) {
//some code here
}
for (i=0; i < clickableGO.length; i++) {
//some code here
}

Exactly! Now you’re cooking… :slight_smile:

I suppose you’re working towards:

for (i=0; i < clickableGO.length; i++) { 
    if (hit.transform == clickableGO[i].transform) {
        Debug.Log("Hit target 1");
    }
}

Another option to consider is:

if ( hit.transform.tag == "clickables" ) {
     Debug.Log("Hit target " + hit.transform.name );
}

Warning: I code in C# more than UnityScript and haven’t tested either of these snippets.

Next I need something like:

clickableGO = X
but what replaces X ?

I’m working form the example on this thread:

http://forum.unity3d.com/threads/38772-using-GameObject.Find-to-get-an-array-of-gameobjects?highlight=array+PlanetA

It seemed to be the most clear explanation but something looks wrong with it to me. Looks like it has too many variables but I’m a complete noob so I could be wrong.

You totally rock! That works! Thank you so much for your help and the help your probably going to continue to give.

Unfortunately now my code is even more confusing to me and it’ll probably be better for me to rewrite from scratch using what I’ve learned today.