Console Worker Follow-Up

Question

A few hours ago I posted a similar question but this is a follow-up, I have revised the code and added and removed certain lines of code and hopefully improved it a bit, I just brought it here for a final cleaning, can anyone notice anything than can be improved even more? Some of the code has been totally changed such as the way I wrote the uptime string

var consoleTitleString = string.Format("{0} / {1}, {2}, and {3}", 
                    projectName, 
                    serverUptime.Days + " day" + (serverUptime.Days != 1 ? "s" : ""),
                    serverUptime.Hours + " hour" + (serverUptime.Hours != 1 ? "s" : ""),
                    (StaticSettings.includeSecondsInUptimeString) ? serverUptime.Minutes + 
                    " minute" + (serverUptime.Minutes != 1 ? "s" : "") + " and " + 
                    serverUptime.Seconds + " second" + (serverUptime.Seconds != 1 ? "s" : "") : 
                    serverUptime.Minutes + " and minute" + (serverUptime.Minutes != 1 ? "s" : "")
                    );

I feel its a bit squeezed in, is there a way to make it a bit more clean without the need of an if statement?

Just a few of the improvements I've made

• Removed field comments
• Improved long-lined code
• Removed useless code

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
    {
        private readonly Timer consoleWorkerTimer;
        private readonly int consoleWorkerInterval;
        private readonly ILog classLogger;

        public ConsoleWorker()
        {
            consoleWorkerInterval = 1000;
            classLogger = LogManager.GetLogger(typeof(ConsoleWorker));
            consoleWorkerTimer = new Timer(timerElapsed, null, TimeSpan.FromMilliseconds(consoleWorkerInterval), TimeSpan.FromMilliseconds(consoleWorkerInterval));
        }

        /// <summary>
        /// Handles the timers callback once elapsed.
        /// </summary>
        /// <param name="timerObject">ConsoleWorker timer's object.</param>
        private void timerElapsed(object timerObject)
        {
            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 = string.Format("{0} / {1}, {2}, and {3}", 
                    projectName, 
                    serverUptime.Days + " day" + (serverUptime.Days != 1 ? "s" : ""),
                    serverUptime.Hours + " hour" + (serverUptime.Hours != 1 ? "s" : ""),
                    (StaticSettings.includeSecondsInUptimeString) ? serverUptime.Minutes + 
                    " minute" + (serverUptime.Minutes != 1 ? "s" : "") + " and " + 
                    serverUptime.Seconds + " second" + (serverUptime.Seconds != 1 ? "s" : "") : 
                    serverUptime.Minutes + " and minute" + (serverUptime.Minutes != 1 ? "s" : "")
                    );

                System.Console.Title = consoleTitleString;
            }
        }

        public void Dispose()
        {
            consoleWorkerTimer.Dispose();
        }
    }
}

One final thing, Someone on the old post said use CamalCase because I would seem like I'm coding JavaScript otherwise, is this a do do and a don't don't go against thing or is that just his personal preference?

Answer 1

I still find the string creation awful. If you cannot use C# 6 the I suggest the StringBuilder:

var consoleTitle = new StringBuilder()
    .Append(projectName).Append(" / ")
    .Append(serverUptime.Days).Append(" day".Pluralize(serverUptime.Days))
    .Append(serverUptime.Hours).Append(" hour".Pluralize(serverUptime.Hours))
    // ... 
    .ToString();

and an extension method to get rid of conditionals:

static class StringExtensions
{
    public static string Pluralize(this string text, int value)
    {
        return (value != 1 ? text + "s" : text)
    }
}

Answer 2

In order to reduce nesting you could perform an early return at the beginning

if (!serverInfo.hasStarted()) return;

I really feel like you are trying to golf this code and it is making it really hard to read

You really should break out all the variables that you are using to build this string and build them then use String Interpolation or String.Format.

The main reason I say this is because you have a lot going on in the different pieces of the string.

Here is what I would do, so that I can see what is going on, I wouldn't try to shorten this up like you are doing because it looks messy, this is my opinion only.

private void timerElapsed(object timerObject)
{
    var serverInfo = Faze.getServer().getServerInformation();

    if (!serverInfo.hasStarted()) return;

    if (StaticSettings.advancedLoggingWhenDebugging && Debugger.IsAttached)
        classLogger.Info("ConsoleWorker has elapsed -> " + DateTime.Now.ToLongTimeString());

    TimeSpan serverUptime = DateTime.Now - serverInfo.getStartedTime();

    var projectName = serverInfo.getProjectName();
    var daysString = string.Format(serverUptime.Days + " day" + (serverUptime.Days != 1 ? "s" : ""));
    var hoursString = string.Format(serverUptime.Hours + " hour" + (serverUptime.Hours != 1 ? "s" : ""));
    var minutesString = string.Format(serverUptime.Minutes + " minute" + (serverUptime.Minutes != 1 ? "s" : ""));

    var consoleTitleString = projectName + " / " + daysString + " " + hoursString + " " + minutesString;

    if (StaticSettings.includeSecondsInUptimeString)
    {
        consoleTitleString += " and " + serverUptime.Seconds + " second" +
                              (serverUptime.Seconds != 1 ? "s" : "");
    }
    System.Console.Title = consoleTitleString;           
}

The only way that I think you should shorten this anymore is by creating another method that will build your strings based on the specified time piece that you pass into it.

---

Other than that I like the rest of the changes that you made to your code.

Answer 3

consoleWorkerInterval - consts Vs readonly

Currently you've got this as a readonly int, that you're initialising in the constructor. This value doesn't need to be set for every instance of the class, you already know it at compile time. Instead you could set it up either as a static readonly field:

private static readonly int consoleWorkerInterval = 1000;

Or a const one:

private const int consoleWorkerInterval = 1000;

timerElapsed - Naming convention

It's unusual to see methods that start with a lowercase letter in C#. Even private methods would typically follow Pascal naming convention, so I would expect the method to be called TimerElapsed. Your public methods/properties/fields should always be following the naming guidelines, which methods on the objects you're calling like getServer and getServerInformation clearly aren't.

ConsoleTitle

All those ternary operators does make the title generation code look a bit messy. The first thing I'd suggest doing is extracting the section that formats the a value's output with or without the 's' in a similar way to the method suggested by @t3chb0t. However, I'd build this on top of the StringBuilder class. I'd then add another method that is responsible for formatting a whole TimeSpan, which uses the extension method to ensure the correct 's'ness. Because you want to only format seconds if they're required, a third extension method might be useful to achieve this. Putting it all together you could end up with a class like this:

static class StringBuilderConsoleExtensions
{
    internal static StringBuilder FormatForTitle(this StringBuilder sb, TimeSpan timeSpan, bool includeSeconds)
    {
        return sb.FormatForTitle("{0} day", timeSpan.Days)
                 .FormatForTitle(", {0} hour", timeSpan.Hours)
                 .FormatForTitle(", and {0} minute", timeSpan.Minutes)
                 .OnlyIfTrue(includeSeconds, s => s.FormatForTitle(" and {0} second", timeSpan.Seconds));
    }

    internal static StringBuilder FormatForTitle(this StringBuilder sb, string format, int value)
    {
        sb.AppendFormat(format, value);
        if (1 != value) sb.Append('s');
        return sb;
    }

    internal static StringBuilder OnlyIfTrue(this StringBuilder target, bool condition, Func<StringBuilder, StringBuilder> funcToRun)
    {
        if (condition) return funcToRun(target);
        return target;
    }
}

Putting all of this together, means that the timerElapsed method is a lot more manageable:

public  void timerElapsed(object timerObject)
{
    var serverInfo = Faze.getServer().getServerInformation();

    if(!serverInfo.hasStarted())
    {
        return;
    }

    if (StaticSettings.advancedLoggingWhenDebugging && Debugger.IsAttached)
        classLogger.Info("ConsoleWorker has elapsed -> " + DateTime.Now.ToLongTimeString());

    TimeSpan serverUptime = DateTime.Now - serverInfo.getStartedTime();

    System.Console.Title = new StringBuilder().AppendFormat("{0} / ", serverInfo.getProjectName())
                                              .FormatForTitle(serverUptime, StaticSettings.includeSecondsInUptimeString)
                                              .ToString();
}

This is a fair bit more code than just using if to break up your logic, however each bit has a more distinct role in the process of building up the string.