2
\$\begingroup\$

This class updates the console's title so it can tell the person using it the programs uptime, which can be pretty useful. The class holds a timer and each time the timer executes it will call the function to update the title (interval is in config file).

How can I improve on this?

using Apple.Application.Base.Config;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Timers;

namespace Apple.Application.Base.Core
{
    class ConsoleWorker
    {
        /// <summary>
        /// Console timer
        /// </summary>
        private Timer _consoleTimer;

        /// <summary>
        /// ConsoleWorker
        /// </summary>
        /// <param name="config">Instance of AppleConfig</param>
        public ConsoleWorker(AppleConfig config)
        {
            _consoleTimer = new Timer(int.Parse(config.GetConfigElement("console.timer.interval")));
            _consoleTimer.Elapsed += new ElapsedEventHandler(OnElapsed);
            _consoleTimer.Enabled = true; // Enable it
        }

        public void OnElapsed(object sender, ElapsedEventArgs e)
        {
            DateTime Startup = Apple.ServerInformation.ServerStarted;
            TimeSpan Uptime = DateTime.Now - Startup;

            string days = Uptime.Days + " day" + (Uptime.Days != 1 ? "s" : "") + ", ";
            string hours = Uptime.Hours + " hour" + (Uptime.Hours != 1 ? "s" : "") + ", and ";
            string mins = Uptime.Minutes + " min" + (Uptime.Minutes != 1 ? "s" : "");
            string UptimeString = days + hours + mins;

            Console.Title = "Apple Server [" + UptimeString + "]";
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ You used a config file!! +10! \$\endgroup\$ – RubberDuck Oct 28 '15 at 23:30
4
\$\begingroup\$

I think for the given task, the code is fine.

If you want to do some architecting, here are some thoughts

  • I'd say the class does two things, timing and updating console
  • I'd create a worker class that would do the timer stuff
  • Inject (using dependency injection) an interface that can output stuff
  • Create an interface that defines "outputting stuff"
  • Create a console version of that interface
  • Feed it to the concrete one

It's just refactoring so that you can use the same stuff to write output to different places such as disk, web service etc..

| improve this answer | |
\$\endgroup\$
4
\$\begingroup\$

I love that you're storing the interval in a config, but there's some issues with your implementation of that.

  1. This class doesn't need the whole config file.

    There's no reason for this class to have knowledge of the entire configuration and how to access it. It only needs to know what the interval should be. I'd change your constructor so that you're just passing the interval.

  2. GetConfigSection isn't a very nice way to access the configuration.

    It relies on a string key and string keys are very easy to misspell. It's much preferable to access these through the strong typing option that Settings.Default provides. See here for more info.


Lastly, this block looks like a wonderful place to extract a method and use String.Format

string days = Uptime.Days + " day" + (Uptime.Days != 1 ? "s" : "") + ", ";

string hours = Uptime.Hours + " hour" + (Uptime.Hours != 1 ? "s" : "") + ", and ";

string mins = Uptime.Minutes + " min" + (Uptime.Minutes != 1 ? "s" : "");

string UptimeString = days + hours + mins;

For example:

string days = String.Format("{0} day{1}  , ", Uptime.Days, (Uptime.Days != 1 ? "s" : ""));

Or, even better, but only if you're using C# 6

string days = $"{Uptime.Days} day{(Uptime.Days != 1 ? "s" : "")}  , ";

Once you do this for all 3, you'll see pretty quickly your opportunity to extract a method.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ GetConfigSection ? I don't see GetConfigSection anywhere.. \$\endgroup\$ – Liam Hardy Oct 29 '15 at 0:14
  • \$\begingroup\$ GetConfigElememt... Which I just realized is form a custom class you wrote when I found your other question. The advice still applies though. It'd be much better to use the strongly typed configuration than to retrieve them via string keys. \$\endgroup\$ – RubberDuck Oct 29 '15 at 0:18
  • \$\begingroup\$ I see, also can you further your explanation on string format? uptime string. \$\endgroup\$ – Liam Hardy Oct 29 '15 at 1:43
  • 1
    \$\begingroup\$ Isn't it redundant to use the string interpolation and Format at the same time? I guess a copy&paste error ;-] \$\endgroup\$ – t3chb0t Jul 5 '16 at 19:55
  • 1
    \$\begingroup\$ Maybe @t3chb0t. I wrote this answer from my phone shortly after the features were just released. Please make edits if one is necessary. \$\endgroup\$ – RubberDuck Jul 5 '16 at 19:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.