• Products
  • Solutions
  • Made with Unity
  • Learning
  • Support & Services
  • Community
  • Asset Store
  • Get Unity

UNITY ACCOUNT

You need a Unity Account to shop in the Online and Asset Stores, participate in the Unity Community and manage your license portfolio. Login Create account
  • Blog
  • Forums
  • Answers
  • Evangelists
  • User Groups
  • Beta Program
  • Advisory Panel

Navigation

  • Home
  • Products
  • Solutions
  • Made with Unity
  • Learning
  • Support & Services
  • Community
    • Blog
    • Forums
    • Answers
    • Evangelists
    • User Groups
    • Beta Program
    • Advisory Panel

Unity account

You need a Unity Account to shop in the Online and Asset Stores, participate in the Unity Community and manage your license portfolio. Login Create account

Language

  • Chinese
  • Spanish
  • Japanese
  • Korean
  • Portuguese
  • Ask a question
  • Spaces
    • Default
    • Help Room
    • META
    • Moderators
    • Topics
    • Questions
    • Users
    • Badges
  • Home /
  • Help Room /
avatar image
0
Question by SteampunkEngineer · Jun 30, 2020 at 06:41 PM · getcomponentfindfor-loop

How do I improve this code stink?

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!

Comment
Add comment
10 |3000 characters needed characters left characters exceeded
▼
  • Viewable by all users
  • Viewable by moderators
  • Viewable by moderators and the original poster
  • Advanced visibility
Viewable by all users

1 Reply

· Add your reply
  • Sort: 
avatar image
0

Answer by Bunny83 · Jul 02, 2020 at 02:42 AM

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.

Comment
Add comment · Share
10 |3000 characters needed characters left characters exceeded
▼
  • Viewable by all users
  • Viewable by moderators
  • Viewable by moderators and the original poster
  • Advanced visibility
Viewable by all users

Your answer

Hint: You can notify a user about this post by typing @username

Up to 2 attachments (including images) can be used with a maximum of 524.3 kB each and 1.0 MB total.

Welcome to Unity Answers

The best place to ask and answer questions about development with Unity.

To help users navigate the site we have posted a site navigation guide.

If you are a new user to Unity Answers, check out our FAQ for more information.

Make sure to check out our Knowledge Base for commonly asked Unity questions.

If you are a moderator, see our Moderator Guidelines page.

We are making improvements to UA, see the list of changes.



Follow this Question

Answers Answers and Comments

207 People are following this question.

avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image avatar image

Related Questions

What to use instead of GameObject.Find and GetComponent 2 Answers

Talking to GameObject and components problem 1 Answer

Unity GetComponent Script? 1 Answer

Referencing variable from another script on another object 3 Answers

Help with script , Change tag depending on material attached 1 Answer

  • Anonymous
  • Sign in
  • Create
  • Ask a question
  • Spaces
  • Default
  • Help Room
  • META
  • Moderators
  • Explore
  • Topics
  • Questions
  • Users
  • Badges