Code from the internet

published: Mon, 14-Jul-2008   |   updated: Sat, 3-Nov-2018

I've done it, you've done it, if anyone programs for a living I'll bet they've done it. What, exactly? Got some code off the internet to solve a particular problem and used it in your current program.

That's all very well, but you should practice some what I may call prophylactic procedures first.

Take a look at the code you're about to use. Do you understand it? Is it using classes, methods, properties you've not met before? Read up on them. Put it into a test harness and run it. Does it run as you'd expect? Think of failure cases, and try them out: does it behave?

But most of all: read the code and understand it.

Let's take an example. There's a blog I read occasionally, found though Jeff Duntemann, hosted by Michael Covington. Very interesting blog, especially with regard to astronomy photos. He takes some stunning photos of planets, stars, clusters, galaxies. Magic.

I wish however he'd keep away from programming. Here's a very recent example: finding the speed of the CPU using C# and WMI (Windows Management Instrumentation).

I learnt a whole bunch about WMI when I worked at Configuresoft, and in those days I learned how flaky it could be (I dare say things have improved since then). In essence it's an interface to a whole bunch of configuration options and values from the current or perhaps remote system. It runs as a service that you can stop or disable.

So Covington has this code, the idea for which was obtained from the internet, I'm guessing.

    static double? CpuGHz() {
      double? GHz = null;
      try {
        ManagementClass mc = new ManagementClass("Win32_Processor");
        foreach (ManagementObject mo in mc.GetInstances()) {
          GHz = 0.001 *
             Int16.Parse(mo.Properties["CurrentClockSpeed"].Value.ToString());
          break;
        }
      }
      catch {
        // do nothing
      }
      return GHz;
    }

As soon as I read the piece and the code red flags appeared.

The first was the blithe justification for the empty catch block. This is just bad coding practice. You should never have an empty catch block because it will catch everything, not just exceptions form your own code. Even very bad exceptions that really say "shut down now, abandon all hope, Buster" will be caught and tossed aside. Equally as bad is a catch block that catches objects of type Exception. Nuts to that: you should catch only those exceptions you are willing to catch and process, If you are not willing to handle any exceptions then don't catch them, let something else further up the food chain worry about handling them. If you find out later that there's some exception type you can catch and, say, ignore, then modify the code to do so. Ignoring all exceptions is never "precisely the desired action".

So I decided to play around with the code. First I paused the WMI service. I then added a couple of lines to the catch block to actually catch the damn exception and print it out. The call to create the ManagementClass takes a long time to execute now, but eventually it fails with a System.Runtime.InteropServices.COMException. The message is "Server execution failed (Exception from HRESULT: 0x80080005 (CO_E_SERVER_EXEC_FAILURE))". So now I know what to put in my catch block should the code be run on a system without the WMI service running. (Some other research revealed that System.Management.ManagementException could be thrown too.)

Habing done thnis research I decided that a "low-level" method such as this shouldn't be in the business of catching anything, expecially COMExceptions. So I took out the try..catch.

The next red flag is just as visible, since Covington brings attention to it as well. The result value of the GetInstances() call is some kind of enumerable collection that does not indexing defined on it, so you have to use an enumerator. Covington just breaks after the first item, but my immediate reaction was are there any more items in this collection. What is so special about the first?

So I altered the code to print out all of the items. As it happened there was only one. I did wonder, since I'm using a quad core, whether there might be 4 items in this collection. Guess not.

Next up is the requirement to convert the Value property of the CurrentClockSpeed value to a string and then parse it back into an integer. Yuk. Let's write a little code to check what type Value is in this instance, ah, UInt32. (In fact, browsing through the MSDN documentation for WMI reveals the same type for this property.) Cool: we can get rid of the ToString() and Parse() calls.

The next red flag is more subtle, but it's something I've learned from the school of hard knocks to watch out for. If there's a class being instantiated I don't know, especially when it's going to be discarded pretty much immediately, I go check to see whether it's implementing IDisposable. If it is, I should use the using() construct in C# to ensure that Dispose is called.

With Refactor! Pro, this check is really simple. Put the caret on the new object being created, press the Refactor key, and look to see if the "Introduce Using Statement" option is present. If it is, select it.

So, after that little bit of research and experimentation, here's a better (more efficient, more robust, won't knife you in the back) routine to get the current CPU speed.

using System;
using System.Management;

namespace CpuSpeed {
  class Program {

    static double? GetCpuSpeedInGHz() {
      double? GHz = null;
      using (ManagementClass mc = new ManagementClass("Win32_Processor")) {
        foreach (ManagementObject mo in mc.GetInstances()) {
          GHz = 0.001 * (UInt32) mo.Properties["CurrentClockSpeed"].Value;
          break;
        }
      }
      return GHz;
    }

    static void Main(string[] args) {
      Console.WriteLine("The current CPU speed is {0}", (GetCpuSpeedInGHz() ?? -1.0).ToString());
      Console.ReadLine();
    }
  }
}

My point here is you really should make some effort to sanitize code you download from the internet. This is not an NIH attitude, but a recognition that you really don't know how experienced the developer at the other end of the URL really is.