ConsoleWorker update console timer with how long application has been open

Question

earlier I coded a quick console worker that runs a timer every 1 second and updates the consoles timer with how long the app has been open, how can I improve this?

using Faze.Other.App;
using log4net;
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
namespace Faze.Other.Util.Console
{
 class ConsoleWorker : IDisposable
 {
 /// <summary>
 /// Holds an instance of Timer.
 /// </summary>
 private Timer consoleWorkerTimer;
 /// <summary>
 /// Interval of the ConsoleWorker timer.
 /// </summary>
 private int consoleWorkerInterval = 1000;
 /// <summary>
 /// Holds an instance of ILog.
 /// </summary>
 private ILog classLogger;
 /// <summary>
 /// Initializes a new instance of ConsoleWorker.
 /// </summary>
 public ConsoleWorker()
 {
 if (StaticSettings.includeSecondsInUptimeString && consoleWorkerInterval < 1000)
 consoleWorkerInterval = 1000;
 classLogger = LogManager.GetLogger(typeof(ConsoleWorker));
 consoleWorkerTimer = new Timer(new TimerCallback(timerElapsed), null, TimeSpan.FromMilliseconds(consoleWorkerInterval), TimeSpan.FromMilliseconds(consoleWorkerInterval));
 }
 /// <summary>
 /// Starts the worker (timer) process.
 /// </summary>
 public void startWorker()
 {
 // I'll leave this for now..
 }
 /// <summary>
 /// Handles the timers callback once elapsed.
 /// </summary>
 /// <param name="timerObject">ConsoleWorker timer's object.</param>
 private void timerElapsed(object timerObject)
 {
 if (Faze.getServer().getServerInformation().hasStarted())
 {
 if (StaticSettings.advancedLoggingWhenDebugging && Debugger.IsAttached)
 classLogger.Info("ConsoleWorker has elapsed -> " + DateTime.Now.ToLongTimeString());
 TimeSpan serverUptime = DateTime.Now - Faze.getServer().getServerInformation().getStartedTime();
 string consoleTitleString = string.Empty;
 if (StaticSettings.includeSecondsInUptimeString)
 {
 consoleTitleString = Faze.getServer().getServerInformation().getProjectName() + " / " + serverUptime.Days + " day" + (serverUptime.Days != 1 ? "s" : "")
 + ", " + serverUptime.Hours + " hour" + (serverUptime.Hours != 1 ? "s" : "") + ", " +
 serverUptime.Minutes + " minute" + (serverUptime.Minutes != 1 ? "s" : "") + " and " + serverUptime.Seconds + " second" + (serverUptime.Seconds != 1 ? "s" : "");
 }
 else
 {
 consoleTitleString = Faze.getServer().getServerInformation().getProjectName() + " / " + serverUptime.Days + " day" + (serverUptime.Days != 1 ? "s" : "")
 + ", " + serverUptime.Hours + " hour" + (serverUptime.Hours != 1 ? "s" : "") + " and " +
 serverUptime.Minutes + " minute" + (serverUptime.Minutes != 1 ? "s" : "");
 }
 System.Console.Title = consoleTitleString;
 }
 }
 public void Dispose()
 {
 consoleWorkerTimer.Dispose();
 }
 }
}

• Malachi: why didn't you take this user's advice about the String.Format and/or Interpolation? --> codereview.stackexchange.com/a/109061/18427

Answer 1

1. Drop the field's comments, they are absolut useless
2. classLogger should be static readonly
3. consoleWorkerTimer should be readonly

• Liam Hardy: Why static for classLogger? wouldn't private readonly cut it..
• t3chb0t: + use CamelCase for method names or otherwise it looks like javaScript ;-)
• JanDotNet: @Liam Hardy: Because you need only one logger instance for all ConsoleWorker instances.

Answer 2

I think the first thing that you should do is create one more variable for Faze.getServer().getServerInformation().getProjectName() don't type that twice, and please don't make me read it twice.

I would also use String Interpolation (if you can) it makes this look a little cleaner.

You should pull out the stuff that is the same from the if/else statement and then use a single if statement if you want to add seconds onto the console title string, and then you can create the variable in one declaration minimizing the lines of code.

Here is what I have:

if (Faze.getServer().getServerInformation().hasStarted())
{
 if (StaticSettings.advancedLoggingWhenDebugging && Debugger.IsAttached)
 classLogger.Info("ConsoleWorker has elapsed -> " + DateTime.Now.ToLongTimeString());
 TimeSpan serverUptime = DateTime.Now - Faze.getServer().getServerInformation().getStartedTime();
 var projectName = Faze.getServer().getServerInformation().getProjectName();
 var consoleTitleString = $"{projectName} / {serverUptime.Days} " +
 $"day{(serverUptime.Days != 1 ? "s" : "")}, {serverUptime.Hours} " +
 $"hour{(serverUptime.Hours != 1 ? "s" : "")}, {serverUptime.Minutes} " +
 $"minute{(serverUptime.Minutes != 1 ? "s" : "")}";
 if (StaticSettings.includeSecondsInUptimeString)
 {
 consoleTitleString += $" and {serverUptime.Seconds} second{(serverUptime.Seconds != 1 ? "s" : "")}";
 }
 System.Console.Title = consoleTitleString;
}

---

Another change that I might attempt would be to minimize some typing by creating an object from Faze.getServer().getServerInformation() so that I don't have to type that 3 times in my code, it might look something like this.

var serverInfo = Faze.getServer().getServerInformation();
if (serverInfo.hasStarted())
{
 if (StaticSettings.advancedLoggingWhenDebugging && Debugger.IsAttached)
 classLogger.Info("ConsoleWorker has elapsed -> " + DateTime.Now.ToLongTimeString());
 TimeSpan serverUptime = DateTime.Now - serverInfo.getStartedTime();
 var projectName = serverInfo.getProjectName();
 var consoleTitleString = $"{projectName} / {serverUptime.Days} " +
 $"day{(serverUptime.Days != 1 ? "s" : "")}, {serverUptime.Hours} " +
 $"hour{(serverUptime.Hours != 1 ? "s" : "")}, {serverUptime.Minutes} " +
 $"minute{(serverUptime.Minutes != 1 ? "s" : "")}";
 if (StaticSettings.includeSecondsInUptimeString)
 {
 consoleTitleString += $" and {serverUptime.Seconds} second{(serverUptime.Seconds != 1 ? "s" : "")}";
 }
 System.Console.Title = consoleTitleString;
}

---

I don't see anything that would change the private consoleWorkerInterval to anything other than a value of 1000 so I really don't see a need for this

if (StaticSettings.includeSecondsInUptimeString && consoleWorkerInterval < 1000)
 consoleWorkerInterval = 1000; 

---

Resharper also noted that you don't need the Redundant explicit delegate creation on this line of code

consoleWorkerTimer = new Timer(new TimerCallback(timerElapsed), null, TimeSpan.FromMilliseconds(consoleWorkerInterval), TimeSpan.FromMilliseconds(consoleWorkerInterval));

you don't need to create a new TimerCallback because you are creating this Timer and giving it a method to call when the time passes, so this line of code can be shorter as well

consoleWorkerTimer = new Timer(timerElapsed, null, TimeSpan.FromMilliseconds(consoleWorkerInterval), TimeSpan.FromMilliseconds(consoleWorkerInterval));

• Liam Hardy: Thanks for all the advice I have took it all in to consideration, the only thing I wanted to make sure you knew is that I added the if to check if the interval is < 1000 because I wanted to implement the interval to come from a config file in the future. Also "In your Ternary Statements you should use a greater than symbol (>) instead of a not equals, it is less characters and it looks neater, in my opinion" Where is the > ? Anyway, answer accepted thank you. P.S if you mean the != 1 for the uptime checking I did that because 0 also needs a S, anything but 1 would.
• Malachi: @LiamHardy that makes sense, I will change it back to the !=