Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

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 @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:

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:

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:

Source Link
forsvarir
  • 11.8k
  • 7
  • 39
  • 72

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.

lang-cs

AltStyle によって変換されたページ (->オリジナル) /