Would it be better to just combine these two scripts?

Question

I've got these two scripts that are supposed to share variables, but I'm having a ton of trouble getting the first (Hourglass.js) to pull the values from the second one (HourOfTheDayCalculator.js). Really, the second one is just one giant function that takes up a ton of lines so I moved it over to a separate script to make the first more readable.

Is there any drawbacks to combining the two scripts together for simplicity's sake?

Hourglass.js

 #pragma strict
 public class Hourglass extends MonoBehaviour {
     /*********************************************/
     /*                Script Links                */
     /*********************************************/
 
     public var HourOfTheDayCalculatorScript : HourOfTheDayCalculator;
     public var TimeTravelScript : TimeTravel;
     
 
     /*********************************************/
     /*         Time Machine On/Off               */
     /*********************************************/
      
     public var normalPassageOfTime : boolean = true;
     var timeTravelActive: boolean;
 
 
     /*********************************************/
     /*             Clock Variables               */
     /*********************************************/
 
     //How many degrees the sun rotates per second
     private var degreesOfRotation : float = 360.0 /*degrees*/ / 24.0 /*hours*/ / 60.0 /*minutes*/ / 60.0 /*seconds*/;
 
     //Editor slider variable to adjust speed of time flow
     @Range (0.5, 10000.0)
     public var speedOfLight : float = 1.0;
 
     //Calculation of time flow speed
     @System.NonSerialized
     public var speedModifier : float;
 
     //Get Hours, Minutes, and Seconds as a string
     @System.NonSerialized 
     public var currentHour = System.DateTime.Now.ToString("HH");
     @System.NonSerialized 
     public var currentMin = System.DateTime.Now.ToString("mm");
 
 
     //Convert current number of hours/minutes passed since midnight into usable seconds
     private var hoursInSeconds : int = parseInt(currentHour) * 60.0 * 60.0;
     private var minutesInSeconds : int = parseInt(currentMin) * 60.0;
 
     //Total seconds passed
     @System.NonSerialized
     public var timeOfDay : int = hoursInSeconds + minutesInSeconds;
     @System.NonSerialized
     public var clockHour : int;
     @System.NonSerialized
     public var timeOfDayPhrase;
 
     /*********************************************/
     /*              Color Variables              */
     /*********************************************/
     public var sun : Light;
     public var sunNightColor : Color;
     public var sunDayColor : Color;
 
 
 
 
     /*********************************************/
     /*                Functions                  */
     /*********************************************/
 
     //set position of the sun based on time of day
     function setSun() 
             {
             transform.Rotate(0,0,degreesOfRotation * (hoursInSeconds + minutesInSeconds));
             //sun.color = Color.Lerp (sunNightColor, sunDayColor, .5 * Time.deltaTime);
             }
 
     //rotate the sun based on set speed
     function moveSun() {
             while (speedOfLight > 0) {
             //Multiplies speed variable by number of degrees to rotate per second
             speedModifier = degreesOfRotation * speedOfLight;
             transform.Rotate(0,0,speedModifier * Time.deltaTime);
             yield;    
             }
     }
 
     function normalPassageOfTimeToggle () {
         //Setting the normalPassageOfTime to opposite of timeTravelActive
         if (timeTravelActive == false){
             normalPassageOfTime = true;
         } else {
             normalPassageOfTime = false;
         }
     }
 
     function importVariables(){
         clockHour = HourOfTheDayCalculatorScript.clockHour;
         timeOfDayPhrase = HourOfTheDayCalculatorScript.timeOfDayPhrase;
         timeTravelActive = TimeTravelScript.timeTravelActive;
         }
 }
 
 /*********************************************/
 /*           ****   BEGIN   ****             */
 /*********************************************/
 
 function Awake() {
     //Starting normalPassageOfTime as true
     normalPassageOfTime = true;
     
     //Linking Scripts
     TimeTravelScript = GetComponent("TimeTravel");
     HourOfTheDayCalculatorScript = GetComponent("HourOfTheDayCalculator");
     importVariables();
 }
 
 /*********************************************/
 /*                  Start                    */
 /*********************************************/
 
 function Start () {
     
     normalPassageOfTimeToggle();
 
     if (normalPassageOfTime == true) {
         setSun();
         moveSun();
     }
     
     Debug.Log("TODPhrase within Start function is " + timeOfDayPhrase);
     Debug.Log("clockHour within Start function is " + clockHour);
 }
 
 
 /*********************************************/
 /*               Update Loop                 */
 /*********************************************/
 
 function Update () {
     
 }
 
 //clockHour and tODP not importing for some reason... Fix it.

HourOfTheDayCalculator.js

 #pragma strict
 public class HourOfTheDayCalculator extends MonoBehaviour {
     /*********************************************/
     /*                Variables                  */
     /*********************************************/
 
 
     var HourglassScript : Hourglass;
     
     public var timeOfDay : int;
     
     @System.NonSerialized
     var clockHour : int;
     
     @System.NonSerialized
     var timeOfDayPhrase;
     
     @System.NonSerialized
     var speedModifier : float;
     
     @System.NonSerialized
     var normalPassageOfTime;
 
     /*********************************************/
     /*              Color Variables              */
     /*********************************************/
 
     public var nightFogColor : Color;
     public var duskFogColor : Color;
     public var morningFogColor : Color;
     public var middayFogColor : Color;
 
     public var nightAmbientColor : Color;
     public var duskAmbientColor : Color;
     public var morningAmbientColor : Color;
     public var middayAmbientColor : Color;
 
 
     /*********************************************/
     /*                Functions                  */
     /*********************************************/
     
     function setClockHour (){
         
         while (normalPassageOfTime == true) {
             //00:00 - 00:59 (12:00am)
             if (timeOfDay <= 3599) {
             clockHour = 0;
             timeOfDayPhrase = "night";
             RenderSettings.ambientLight = nightAmbientColor;
             RenderSettings.fogColor = nightFogColor;
             }
             
             //01:00 - 01:59
             if (timeOfDay >= 3600 && timeOfDay <= 7199){
             clockHour = 1;
             timeOfDayPhrase = "night";
             RenderSettings.ambientLight = nightAmbientColor;
             RenderSettings.fogColor = nightFogColor;
             }
             
             
             //02:00 - 02:59
             if (timeOfDay >= 7200 && timeOfDay <= 10799){
             clockHour = 2;
             timeOfDayPhrase = "night";
             RenderSettings.ambientLight = nightAmbientColor;
             RenderSettings.fogColor = nightFogColor;
             }
             
             //03:00 - 03:59
             if (timeOfDay >= 10800 && timeOfDay <= 14399){
             clockHour = 3;
             timeOfDayPhrase = "night";
             RenderSettings.ambientLight = nightAmbientColor;
             RenderSettings.fogColor = nightFogColor;
             }
             
             //04:00 - 04:59
             if (timeOfDay >= 14400 && timeOfDay <= 17999){
             clockHour = 4;
             timeOfDayPhrase = "night";
             RenderSettings.ambientLight = nightAmbientColor;
             RenderSettings.fogColor = nightFogColor;
             }
             
             //05:00 - 05:59
             if (timeOfDay >= 18000 && timeOfDay <= 21599){
             clockHour = 5;
             timeOfDayPhrase = "morning";
             RenderSettings.ambientLight = Color.Lerp (nightAmbientColor, duskAmbientColor, speedModifier * Time.deltaTime);
             RenderSettings.fogColor = Color.Lerp (nightFogColor, duskFogColor, speedModifier * Time.deltaTime);
             }
             
             //06:00 - 06:59
             if (timeOfDay >= 21600 && timeOfDay <= 25199){
             clockHour = 6;
             timeOfDayPhrase = "morning";
             RenderSettings.ambientLight = Color.Lerp (nightAmbientColor, duskAmbientColor, speedModifier * Time.deltaTime);
             RenderSettings.fogColor = Color.Lerp (nightFogColor, duskFogColor, speedModifier * Time.deltaTime);
             }
             
             //07:00 - 07:59
             if (timeOfDay >= 25200 && timeOfDay <= 28799){
             clockHour = 7;
             timeOfDayPhrase = "morning";
             RenderSettings.ambientLight = Color.Lerp (duskAmbientColor, morningAmbientColor, speedModifier * Time.deltaTime);
             RenderSettings.fogColor = Color.Lerp (duskFogColor, morningFogColor, speedModifier * Time.deltaTime);
             }
             
             //08:00 - 08:59
             if (timeOfDay >= 28800 && timeOfDay <= 32399){
             clockHour = 8;
             timeOfDayPhrase = "morning";
             RenderSettings.ambientLight = Color.Lerp (duskAmbientColor, morningAmbientColor, speedModifier * Time.deltaTime);
             RenderSettings.fogColor = Color.Lerp (duskFogColor, morningFogColor, speedModifier * Time.deltaTime);
             }
             
             //09:00 - 09:59
             if (timeOfDay >= 32400 && timeOfDay <= 35999){
             clockHour = 9;
             timeOfDayPhrase = "morning";
             RenderSettings.ambientLight = Color.Lerp (duskAmbientColor, morningAmbientColor, speedModifier * Time.deltaTime);
             RenderSettings.fogColor = Color.Lerp (duskFogColor, morningFogColor, speedModifier * Time.deltaTime);
             }
             
             //10:00 - 10:59
             if (timeOfDay >= 36000 && timeOfDay <= 39599){
             clockHour = 10;
             timeOfDayPhrase = "morning";
             RenderSettings.ambientLight = Color.Lerp (morningAmbientColor, middayAmbientColor, speedModifier * Time.deltaTime);
             RenderSettings.fogColor = Color.Lerp (morningFogColor, middayFogColor, speedModifier * Time.deltaTime);
             //yield;
             }
             
             //11:00 - 11:59
             if (timeOfDay >= 39600 && timeOfDay <= 43199){
             clockHour = 11;
             timeOfDayPhrase = "morning";
             RenderSettings.ambientLight = middayAmbientColor;
             RenderSettings.fogColor = middayFogColor;
             }
             
             //12:00 - 12:59
             if (timeOfDay >= 43200 && timeOfDay <= 46799){
             clockHour = 12;
             timeOfDayPhrase = "afternoon";
             RenderSettings.ambientLight = middayAmbientColor;
             RenderSettings.fogColor = middayFogColor;
             }
             
             //13:00 - 13:59 (1:00pm)
             if (timeOfDay >= 46800 && timeOfDay <= 50399){
             clockHour = 13;
             timeOfDayPhrase = "afternoon";
             RenderSettings.ambientLight = middayAmbientColor;
             RenderSettings.fogColor = middayFogColor;
             }
             
             //14:00 - 14:59 (2:00pm)
             if (timeOfDay >= 50400 && timeOfDay <= 53999){
             clockHour = 14;
             timeOfDayPhrase = "afternoon";
             RenderSettings.ambientLight = middayAmbientColor;
             RenderSettings.fogColor = middayFogColor;
             }
             
             //15:00 - 15:59 (3:00pm)
             if (timeOfDay >= 54000 && timeOfDay <= 57599){
             clockHour = 15;
             timeOfDayPhrase = "afternoon";
             RenderSettings.ambientLight = middayAmbientColor;
             RenderSettings.fogColor = middayFogColor;
             }
             
             //16:00 - 16:59 (4:00pm)
             if (timeOfDay >= 57600 && timeOfDay <= 61199){
             clockHour = 16;
             timeOfDayPhrase = "afternoon";
             RenderSettings.ambientLight = middayAmbientColor;
             RenderSettings.fogColor = middayFogColor;
             }
             
             //17:00 - 17:59 (5:00pm)
             if (timeOfDay >= 61200 && timeOfDay <= 64799){
             clockHour = 17;
             timeOfDayPhrase = "evening";
             RenderSettings.ambientLight = middayAmbientColor;
             RenderSettings.fogColor = middayFogColor;
             }
             
             //18:00 - 18:59 (6:00pm)
             if (timeOfDay >= 64800 && timeOfDay <= 68399){
             clockHour = 18;
             timeOfDayPhrase = "evening";
             }
             
             //19:00 - 19:59 (7:00pm)
             if (timeOfDay >= 68400 && timeOfDay <= 71999){
             clockHour = 19;
             timeOfDayPhrase = "evening";
             }
             
             //20:00 - 20:59 (8:00pm)
             if (timeOfDay >= 72000 && timeOfDay <= 75599){
             clockHour = 20;
             timeOfDayPhrase = "night";
             }
             
             //21:00 - 21:59 (9:00pm)
             if (timeOfDay >= 75600 && timeOfDay <= 79199){
             clockHour = 21;
             timeOfDayPhrase = "night";
             RenderSettings.ambientLight = nightAmbientColor;
             RenderSettings.fogColor = nightFogColor;
             }
             
             //22:00 - 22:59 (10:00pm)
             if (timeOfDay >= 79200 && timeOfDay <= 82799){
             clockHour = 22;
             timeOfDayPhrase = "night";
             RenderSettings.ambientLight = nightAmbientColor;
             RenderSettings.fogColor = nightFogColor;
             }
             
             //23:00 - 23:59 (11:00pm)
             if (timeOfDay >= 82800 && timeOfDay <= 86399){
             clockHour = 23;
             timeOfDayPhrase = "night";
             RenderSettings.ambientLight = nightAmbientColor;
             RenderSettings.fogColor = nightFogColor;
             }
             yield;
         }    
     }
 
     //Gets the value of timeOfDay from Hourglass script
     function getTimeOfDay(){
         while (normalPassageOfTime == true){
         timeOfDay = HourglassScript.timeOfDay;
         yield;
         }
     }
     
     function importVariables(){
         normalPassageOfTime = HourglassScript.normalPassageOfTime; 
         speedModifier = HourglassScript.speedModifier;
     }
     
 }
 /*********************************************/
 /*           ****   BEGIN   ****             */
 /*********************************************/
 
 
 function Awake () {
     HourglassScript = GetComponent("Hourglass");
         
     
 }
 
 function Start () {
     importVariables();
     getTimeOfDay();
     setClockHour();
     
     Debug.Log("HourOfTheDayCalculator : Current TODP: " + timeOfDayPhrase);
     Debug.Log("HourOfTheDayCalculator : Current ClockHour: " + clockHour);
     
     
     
 }
 
 function Update () {
     
 }

Answer 1

Best Answer

Answer by Alec-Slayden · Jul 25, 2015 at 09:48 PM

I would say separate them. Your calculator is an object that may be useful in non-hourglass scenarios.

Objects that represent different concepts should be as self-contained as possible, allowing you to use them in different scenarios. Have your calculator be it's own object, requiring no references to other objects (no reference to hourglass). Hourglass will be an object that utilizes the calculator and can initialize the speed values. To do this, remove references to hourglass in your calculator, and instead of calling ImportVariables in your calculator, have the hourglass initialize them.

This way you can use your calculator for any number of things. Wristwatches, day-night cycles, alarm clocks, without having to rely on any of them.

Edit: I should also add that from a maintainability stand point, having separately contained classes and scripts for actions can be very beneficial. Having your attack script separate from your move script for a unit, for example, I believe to be much better than a single unit "super-script", unless those two actions are very simple.

Edit 2: Here's a link to a relevant answer for coding practices:

http://answers.unity3d.com/questions/15713/what-are-some-good-scripting-habits.html

Add comment · Show 3 · Share

Bengooch7 · Jul 25, 2015 at 11:38 PM 0

Share

@akec-slayden Thank you for answering!

I thought it was the best idea to have the two separated, but the main function of the hourglass script is to calculate the time of day and then put the sun where it needs to be for that. Then Calculator takes that TOD and does it's calculations to set the ambient colors and variables like clockHour and timeOfDayPhrase. timeOfDayPhrase, I thought might be helpful, for NPCs to use as a greeting, which means the NPC scripts would have to import that value from calculator. clockHour doesn't really have any purpose yet, I was more so using it as a way to ensure the game was using the right IF statement to set the ambient lighting.

So originally, I wrote Hourglass with that huge setClockHour() in it, then after seeing how huge it was, decided to move it to it's own script. At this point, really, all Hourglass does is set the position of the sun based on the time of day, and tells it to move. I could work that into Calculator and treat the whole thing as like a "Space/Time/Environment" controller. It comes out to around 350 lines of code (including comments, which take up a ton of room, as I like to remember what the hell the code I wrote does).

At what point is a script too long? Is there such a thing?

It's mostly variable and function declarations and the actual execution sections only take up about 50 lines.

Alec-Slayden · Jul 26, 2015 at 12:08 AM 1

Share

If combining them will create a cohesive script with a clear purpose then that's fine too. Personally, I tend to lean towards generic and reusable scripts. I can see a lot of places I might want to calculate or label the time of day, and having a utility for just that would be nice. But my preference doesn't make it incorrect to do otherwise.

Similarly, when it comes to script length there's no definitive answer for when it becomes too long. It's important that they are cohesive and have a clear purpose (there are few circumstances in which you would want only one script to handle enemy spawn times, time of day, and player jumping, for example), but if that purpose is complicated sometimes the scripts can get pretty long.

When it starts nearing 1k lines I sometimes see if it can be broken down into separate elements, but that isn't always the answer, and having too many scripts that don't do much can be just as much of a burden as having one script that does too much.

Some people like to use partial classes and separate functionality into those partials, and that's a valid approach, but unless I'm working in a $$anonymous$$m environment where that is necessary, I prefer single document scripts.

I think the sweet spot may be a delicate mix of preference and intuition that varies from script to script. Sometimes I open up a third party script to make changes and sigh at how long the script is. Other times I might open up an even longer one and think it makes total sense.

There are a lot of "Best Practices" style guides out there, and they can be invaluable for maintaining efficiency. I advise taking a look.

https://unity3d.com/learn/tutorials/modules/intermediate/scripting/coding-practices

http://answers.unity3d.com/questions/15713/what-are-some-good-scripting-habits.html

Bengooch7 · Jul 26, 2015 at 12:55 AM 0

Share

Thanks man, all good advice!

I really appreciate you taking the time to help a new programmer learning the ropes!

Would it be better to just combine these two scripts?

Hourglass.js

HourOfTheDayCalculator.js

1 Reply

Your answer

Follow this Question

Related Questions