6
\$\begingroup\$

I've got the following code to monitor the network usage on a network interface. I'm mainly looking for feedback on the design, but if you see any code improvements please let me know! This is also the first time I've actually added 'official' documentation so tell me if I'm forgetting something. For those that feel more comfortable browsing github: you can find the code here.

Statistics:

public interface IStatistics {
    string NetworkInterface { get; }
    float DataSent { get; }
    float DataReceived { get; }
    float UploadSpeed { get; }
    float DownloadSpeed { get; }
    Queue<float> LatestDownTransfers { get; }
    Queue<float> LatestUpTransfers { get; }
}

public class Statistics : IStatistics {
    public Statistics(string name) {
        NetworkInterface = name;
        LatestDownTransfers = new Queue<float>(3);
        LatestUpTransfers = new Queue<float>(3);
    }

    // <summary>
    // Holds the name of the selected network interface
    // </summary>
    public string NetworkInterface { get; set; }

    // <summary>
    // Contains the data sent in the most recent time interval
    // </summary>
    public float DataSent { get; set; }

    // <summary>
    // Contains the data received in the most recent time interval
    // </summary>
    public float DataReceived { get; set; }

    // <summary>
    // Returns the upload speed in KiloBytes / Second
    // </summary>
    public float UploadSpeed {
        get { return LatestUpTransfers.Sum() / LatestUpTransfers.Count / 1028 / StatisticsFactory.MULTIPLIER; }
    }

    // <summary>
    // Returns the download speed in KiloBytes / Second
    // </summary>
    public float DownloadSpeed {
        get { return LatestDownTransfers.Sum() / LatestDownTransfers.Count / 1028 / StatisticsFactory.MULTIPLIER; }
    }

    // <summary>
    // Contains the data received in the three most recent time intervals
    // </summary>
    public Queue<float> LatestDownTransfers { get; set; }

    // <summary>
    // Contains the data sent in the three most recent time intervals
    // </summary>
    public Queue<float> LatestUpTransfers { get; set; }
}

Statistics Factory:

public static class StatisticsFactory {
    private static Queue<float> _latestDownTransfers = new Queue<float>();
    private static Queue<float> _latestUpTransfers = new Queue<float>();
    private static Statistics stats;

    public const int MULTIPLIER = 25;

    // <summary>
    // Creates a new statistic and uses the latest transferrates from the previous stats
    // </summary>
    public static Statistics CreateStatistics(string interfaceName) {
        stats = new Statistics(interfaceName) {
            LatestDownTransfers = _latestDownTransfers,
            LatestUpTransfers = _latestUpTransfers
        };

        return stats;
    }

    // <summary>
    // Adds a value to the current running statistics summary's upload list
    // </summary>
    public static void AddSentData(float d) {
        stats.DataSent = d;
        if (_latestUpTransfers.Count == 3) {
            _latestUpTransfers.Dequeue();
        }
        _latestUpTransfers.Enqueue(d);

        stats.LatestUpTransfers = _latestUpTransfers;
    }

    // <summary>
    // Adds a value to the current running statistics summary's download list
    // </summary>
    public static void AddReceivedData(float d) {
        stats.DataReceived = d;

        if (_latestDownTransfers.Count == 3) {
            _latestDownTransfers.Dequeue();
        }

        _latestDownTransfers.Enqueue(d);

        stats.LatestDownTransfers = _latestDownTransfers;
    }
}

Utilities:

public static class Utilities {
    static Utilities() {
        Thread.CurrentThread.CurrentCulture = System.Globalization.CultureInfo.CreateSpecificCulture("en-US");
    }

    // <summary>
    // Returns statistics about the given network interface
    // </summary>
    public static IStatistics GetNetworkStatistics(string interfaceName) {
        var stats = StatisticsFactory.CreateStatistics(interfaceName);

        var dataSentCounter = new PerformanceCounter("Network Interface", "Bytes Sent/sec", interfaceName);
        var dataReceivedCounter = new PerformanceCounter("Network Interface", "Bytes Received/sec", interfaceName);

        float sendSum = 0;
        float receiveSum = 0;

        for (var index = 0; index < StatisticsFactory.MULTIPLIER; index++) {
            sendSum += dataSentCounter.NextValue();
            receiveSum += dataReceivedCounter.NextValue();
        }

        if (sendSum > 0 || receiveSum > 0) {
            StatisticsFactory.AddReceivedData(receiveSum);
            StatisticsFactory.AddSentData(sendSum);
        }

        return stats;
    }

    // <summary>
    // Returns a list of all available network interfaces
    // </summary>
    public static IList<string> GetNetworkInterfaces() {
        return new PerformanceCounterCategory("Network Interface").GetInstanceNames().ToList();
    }
}

Program:

public class Program {
    private static void Main(string[] args) {
        var instances = Utilities.GetNetworkInterfaces();
        Console.WriteLine("All available network interfaces:\n");

        for (var i = 0; i < instances.Count; i++) {
            Console.WriteLine(i + ": " + instances[i]);
        }

        var choice = Convert.ToInt32(Console.ReadLine());
        Console.WriteLine("Selected network interface:\n" + instances[choice] + "\n\n");

        while (true) {
            var stats = Utilities.GetNetworkStatistics(instances[choice]);
            Console.WriteLine("Download speed: " + stats.DownloadSpeed + " KBytes/s");
            Console.WriteLine("Upload speed: " + stats.UploadSpeed + " KBytes/s");
            Console.WriteLine("--------------------------------------------------------------\n\n");
            Thread.Sleep(1000);
        }
    }
}

A few remarks:

The current download/upload speed is determined by calculating the mean of the three latest time intervals. In one interval, the data transmitted according to the PerformanceCounter is checked 25 times to get a pretty accurate result. Have I approached this the right way?

Second of all: the factory is using a static list to store the data of the latest statistics across statistic instances. This makes sure I keep the previous data, even though I create a new object. It works, but it feels dirty. Thoughts?

\$\endgroup\$
5
  • \$\begingroup\$ Is there a reason why everything is Static? \$\endgroup\$ Jul 11, 2013 at 15:34
  • \$\begingroup\$ I personally feel like static methods make a lot of sense for utility classes, certainly because there will be only one interface tracked at a time. And as far as I've seen a factory is usually statically implemented as well. Would you approach this differently? \$\endgroup\$ Jul 11, 2013 at 15:43
  • \$\begingroup\$ To me, static classes are reminiscent of procedural programming, and even worse, emulate VB6 with the global methods and variables. I also find in some instances they interfere with unit testing, both as a change of state problem, but they also restrict the ability to test a specific section of code because you are tying that code to the static method. \$\endgroup\$ Jul 11, 2013 at 16:47
  • \$\begingroup\$ How are you unit testing/isolation testing this code? \$\endgroup\$
    – Gayot Fow
    Jul 17, 2013 at 22:54
  • \$\begingroup\$ I'm not actually: the entire concept of network programming and decapsulation of packets is new to me so I had to figure out what could work along the way. Now that I have found what works I'll look into writing unit tests for future reference (I haven't got experience with mocking network data). Any suggestions? I've been thinking about my design and I will probably remove the static methods and use objects instead. \$\endgroup\$ Jul 17, 2013 at 22:58

1 Answer 1

5
\$\begingroup\$

Style

  1. IStatistics is a bit of a generic name (although I guess if you put it in a sensible namespace it might be ok). I'd consider renaming it to something like INetworkStatistics.
  2. From looking at the code it is not obvious what units the upload and download speeds are (apart from one comment). Consider abstracting it behind a TransferSpeed class from which you can get any sensible transfer speed (e.g. KB/sec, MB/sec, MB/min, ...) or at least put the unit in the property name.

Design

  1. Your interface IStatistics exposes some internals which should not be of any interest to the user, namely DataSent, DataReceived and LatestDownTransfers, LatestUpTransfers. I'd say the user only cares about the fact that he can get the up/download rate and add new data points. So your interface should look like this:

    public interface INetworkStatistics 
    {
        string NetworkInterface { get; }
        float UploadSpeed { get; }
        float DownloadSpeed { get; }
        void AddSentData(float data);
        void AddReceivedData(float data);
    }
    

    You can then implement the Add methods as you implemented them in your factory class.

  2. Rename your utility function GetNetworkStatistics into UpdateNetworkStatistics and pass it as parameter an IStatistics instance to record the data points for:

    public static void UpdateNetworkStatistics(IStatistics stats)
    {
        var dataSentCounter = new PerformanceCounter("Network Interface", "Bytes Sent/sec", stats.NetworkInterface);
        var dataReceivedCounter = new PerformanceCounter("Network Interface", "Bytes Received/sec", stats.NetworkInterface);
    
        float sendSum = 0;
        float receiveSum = 0;
    
        for (var index = 0; index < Statistics.MULTIPLIER; index++)
        {
            sendSum += dataSentCounter.NextValue();
            receiveSum += dataReceivedCounter.NextValue();
        }
    
        if (sendSum > 0 || receiveSum > 0)
        {
            stats.AddReceivedData(receiveSum);
            stats.AddSentData(sendSum);
        }
    }
    

    The while(true) loop in your main program would now become:

    stats = new Statistics(instances[choice]);
    while (true) 
    {
        Utilities.UpdateNetworkStatistics(stats);
        ...
    
  3. Lo and behold you can now delete the StatisticsFactory class. It serves no purpose other than making it near impossible to get statistics for more than one interface at a time and interfere with unit testing big time.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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