How do I improve this code stink?

Question

Heya all.

During my game development, I've written a small piece of code that just reeks of code smell. However, I can't seem to pin point one specific cause. Any improvement suggestions are welcome; I'd like to improve my code quality in general.

This is the piece of code I'm talking about:

   // Seconds a player gets to hide
         public int secondsToHide = 90;
 
     // Runs a timer. If player hides during the timer, run other method. If
     // failed to hide within timer, run a method which makes the player 
     public IEnumerator CheckIfHiddenInHidingFase()
     {
         // GameObject which is a collection of UI elements for the hiding timer. Is a child of a general collection of UI elements.
         GameObject HideTimer = CanvasPlayerUI.transform.Find("HideTimerHider").gameObject;
 
         // Is the script component of the UI that handles changing of HideTimer UI text.
         CountdownUIScript HideTimerCountdown = HideTimer.GetComponent<CountdownUIScript>();
 
         for (int i = secondsToHide; i > 0; i--)
         {
             // Changes text in UI.
             HideTimerCountdown.ChangeCountdownNumber(i);
 
             yield return new WaitForSecondsRealtime(1.0f);
 
             // Ends timer is player hides so player doesnt have to wait for the timer to run out
             if (playerIsHidden) { i = 0; }
         }
         if (playerIsHidden)
         {
             EndHiderHidingFaseHidden();
         }
         else
         {
             EndHiderHidingFaseTimerRanOut();
         }
     }

I'm feeling particularly icky that the "If (playerIsHidden)" statement is called and does two different things both inside and after the for loop.

I appreciate any advice!

Answer 1

Well I don't really see any issues with your if statements. The only thing that you should avoid is changing the for loop variable inside the for loop. If you want to exit the for loop, use break;. Having your two outcomes after the for loop actually is a bit more readable compared to the "compact" solution. By compact I mean something like this:

     // [ ... ]
     for (int i = secondsToHide; i > 0; i--)
     {
         // [ ... ]
         yield return new WaitForSecondsRealtime(1.0f);
         if (playerIsHidden)
         {
             EndHiderHidingFaseHidden();
             yield break;
         }
     }
     EndHiderHidingFaseTimerRanOut();
 }

In this case we only check playerIsHidden in one place. If it turns true while the for loop is still running, we call "EndHiderHidingFaseHidden" and then terminate the coroutine completely (with yield break;). If the for loop actually reaches the end we simply call "EndHiderHidingFaseTimerRanOut" at the end.

ps: You might want to reduce your usage of comments, especially those which are superfluous. Maybe have a look at this.

How do I improve this code stink?

1 Reply

Your answer

Welcome to Unity Answers

Follow this Question

Related Questions