14
\$\begingroup\$

I have included the code from a second year project that I would like some advice on. Basically it utilises WMI to query a NIC card from a selection (depending how many are installed in the host machine) and shows various settings such as IP info, DHCP info etc. It also has a ping function.

How can I improve this to make it a more professional application? I'm looking for things like code structure, functionalities (bearing in mind mark 2 will have the ability to actually interact with the NIC to turn DHCP off and change addresses, plus I hope to incorporate a TRACERT function).

This project is now finished so I'm not cheating by asking this. I just want to take the step from creating a rough but working piece of code to a polished professional one.

The program is written in Visual Studio 2010.

using System;
using System.Collections.Generic;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;
using System.Management;
using System.Management.Instrumentation;
/////////////////////////////////////
using System.Net;
using System.Net.NetworkInformation;
using System.ComponentModel;
using System.Threading;

namespace WindowsFormsApplication3
{
    public partial class Form1 : Form
    {
        private int pingsSent;
        AutoResetEvent resetEvent = new AutoResetEvent(false);
        public Form1()
        {
            {
                InitializeComponent();
            }
            // win32 class
            //http://msdn.microsoft.com/en-us/library/aa394216(v=vs.85).aspx
            try
            {
                ManagementObjectSearcher query = new ManagementObjectSearcher(@"SELECT * FROM Win32_NetworkAdapter WHERE  Manufacturer != 'Microsoft' AND NOT PNPDeviceID LIKE 'ROOT\\%'");
                ManagementObjectCollection queryCollection = query.Get();
                foreach (ManagementObject mo in queryCollection)
                {
                    if (mo["MacAddress"] != null)
                    {
                        comboBox1.Items.Add(mo["Description"].ToString());
                    }
                }
            }
            catch (Exception ex)
            {
            }
        }
        string[] availabilityArray = new string[] { "", "Other", "Unknown", "Running or Full Power", "Warning", "In Test", "Not Applicable", "Power Off", "Off Line", "Off Duty", "Degraded", "Not Installed", "Install Error", "Power Save - Unknown" + "\n" + "The device is known to be in a power save state, but its exact status is unknown.", "Power Save - Low Power Mode" + "/n" + "The device is in a power save state, but still functioning, and may exhibit degraded performance.", "Power Save - Standby" + "/n" + "The device is not functioning, but could be brought to full power quickly.", "Power Cycle", "Power Save - Warning" + "/n" + "The device is in a warning state, though also in a power save state.", };
        string[] errorArray = new string[] { "Device is working properly.", "Device is not configured correctly.", "Windows cannot load the driver for this device.", "Driver for this device might be corrupted, or the system may be low on memory or other resources.", "Device is not working properly. One of its drivers or the registry might be corrupted.", "Driver for the device requires a resource that Windows cannot manage.", "Boot configuration for the device conflicts with other devices.", "Cannot filter.", "Driver loader for the device is missing.", "Device is not working properly. The controlling firmware is incorrectly reporting the resources for the device.", "Device cannot start.", "Device failed.", "Device cannot find enough free resources to use.", "Windows cannot verify the device's resources.", "Device cannot work properly until the computer is restarted.", "Device is not working properly due to a possible re-enumeration problem.", "Windows cannot identify all of the resources that the device uses.", "Device is requesting an unknown resource type.", "Device drivers must be reinstalled.", "Failure using the VxD loader.", "Registry might be corrupted.", "System failure. If changing the device driver is ineffective, see the hardware documentation. Windows is removing the device.", "Device is disabled.", "System failure. If changing the device driver is ineffective, see the hardware documentation.", "Device is not present, not working properly, or does not have all of its drivers installed.", "Windows is still setting up the device.", "Windows is still setting up the device.", "Device does not have valid log configuration.", "Device drivers are not installed", "Device is disabled. The device firmware did not provide the required resources.", "Device is using an IRQ resource that another device is using.", "Device is not working properly. Windows cannot load the required device drivers.", };
        private void comboBox1_SelectedIndexChanged(object sender, EventArgs e)
        {
            ManagementObjectSearcher intquery = new ManagementObjectSearcher("SELECT * FROM Win32_NetworkAdapter WHERE Description ='" + comboBox1.Items[comboBox1.SelectedIndex].ToString() + "'");
            ManagementObjectCollection queryCollection = intquery.Get();
            foreach (ManagementObject mo in queryCollection)
            {
                string Dev = (mo["DeviceID"].ToString());
                txtMacAdd.Text = (" " + mo["MacAddress"].ToString());
                TxtAdapter.Text = (" " + mo["AdapterType"].ToString());
                txtAvailability.Text = (" " + mo["Availability"].ToString());
                txtCaption.Text = (" " + mo["Caption"].ToString());
                txtDeviceID.Text = (" " + mo["DeviceID"].ToString());
                txtErrorCode.Text = (mo["ConfigManagerErrorCode"].ToString());
                ManagementObjectSearcher intquery1 = new ManagementObjectSearcher("ASSOCIATORS OF {Win32_NetworkAdapter.DeviceID='" + Dev + "'}WHERE ResultClass=Win32_NetworkAdapterConfiguration");
                ManagementObjectCollection queryCollection1 = intquery1.Get();
                queryCollection1 = intquery1.Get();
                foreach (ManagementObject mo1 in queryCollection1)
                {
                    string[] addresses = (string[])mo1["IPAddress"];
                    string[] gateways = (string[])mo1["DefaultIPGateway"];
                    string[] Subnets = (string[])mo1["IPSubnet"];
                    foreach (string ipaddress in addresses)
                    {
                        listIp.Items.Add(ipaddress);
                    }
                    foreach (string gateway in gateways)
                    {
                        TxtGateway.Text = (gateway);
                    }
                    foreach (string subnet in Subnets)
                    {
                        listSubnet.Items.Add(subnet);
                    }
                    /////// SHOW DCHP CONFIG/////////
                    if ((bool)mo1["DHCPEnabled"] == true)
                    {
                        checkBox1.Checked = true;
                    }
                    if ((bool)mo1["DHCPEnabled"] != true)
                    {
                        checkBox1.Checked = !true;
                    }
                    DateTime dt = System.Management.ManagementDateTimeConverter.ToDateTime(mo1["DHCPLeaseExpires"].ToString());
                    txtDHCPExpires.Text = dt.ToString();
                    DateTime dte = System.Management.ManagementDateTimeConverter.ToDateTime(mo1["DHCPLeaseObtained"].ToString());
                    txtDHCPObtained.Text = dte.ToString();
                    txtDHCPServer.Text = (mo1["DHCPServer"].ToString());
                    //////////////////////Security////////////////////////////
                    if ((bool)mo1["IPFilterSecurityEnabled"] == true)
                    {
                        chkboxIPFilter.Checked = true;
                    }
                    if ((bool)mo1["IPFilterSecurityEnabled"] != true)
                    {
                        chkboxIPFilter.Checked = !true;
                    }
                    string[] IPSecProtocols = (string[])mo1["IPSecPermitIPProtocols"];
                    string[] IPSecPermitTCPPorts = (string[])mo1["IPSecPermitTCPPorts"];
                    string[] IPSecPermitUDPPorts = (string[])mo1["IPSecPermitUDPPorts"];
                    foreach (string IPSecProtocol in IPSecProtocols)
                    {
                        //richTextBox1.AppendText(IPSecProtocol);
                        listPermitIP.Items.Add(IPSecProtocol);
                    }
                    foreach (string TCPPort in IPSecPermitTCPPorts)
                    {
                        listPermitTCP.Items.Add(TCPPort);
                    }
                    foreach (string UDPPort in IPSecPermitUDPPorts)
                    {
                        listPermitUDP.Items.Add(UDPPort);
                    }
                    ///////////////////DNS/////////////////////////////
                    try
                    {
                        txtDNSDomain.Text = (mo1["DNSDomain"].ToString());
                        txtDNShostName.Text = (mo1["DNSHostName"].ToString());
                        string[] DNSDomainSuffSearch = (string[])mo1["DNSDomainSuffixSearchOrder"];
                        string[] DNSServerSearchOrder = (string[])mo1["DNSServerSearchOrder"];
                        foreach (string domainSuff in DNSDomainSuffSearch)
                        {
                            listDomainSuffSearchOrder.Items.Add(domainSuff);
                        }
                        foreach (string DNSServer in DNSServerSearchOrder)
                        {
                            listDNSServerSearchOrder.Items.Add(DNSServer);
                        }
                        if ((bool)mo1["DNSEnabledForWINSResolution"] == true)
                        {
                            chkDNSWinsResolution.Checked = true;
                        }
                        if ((bool)mo1["DNSEnabledForWINSResolution"] != true)
                        {
                            chkDNSWinsResolution.Checked = !true;
                        }
                        if ((bool)mo1["DomainDNSRegistrationEnabled"] == true)
                        {
                            chkDNSRegEnabled.Checked = true;
                        }
                        if ((bool)mo1["DomainDNSRegistrationEnabled"] != true)
                        {
                            chkDNSRegEnabled.Checked = !true;
                        }
                    }
                    catch (Exception ex3)
                    { }
                }
            }
        }
        private void button2_Click(object sender, EventArgs e)
        {
            try
            {
                ManagementObjectSearcher intquery;
                intquery = new ManagementObjectSearcher("SELECT * FROM Win32_NetworkAdapter WHERE Description ='" + comboBox1.Items[comboBox1.SelectedIndex].ToString() + "'");
                ManagementObjectCollection queryCollection = intquery.Get();
                queryCollection = intquery.Get();
                foreach (ManagementObject mo in queryCollection)
                {
                    string thisstring = (mo["ConfigManagerErrorCode"].ToString());
                    int error;
                    int.TryParse(thisstring, out error);
                    MessageBox.Show(errorArray[error]);
                }
            }
            catch (Exception ex1)
            {
                MessageBox.Show("Please Select a Device");
            }
        }
        private void BtnAvailability_Click(object sender, EventArgs e)
        {
            try
            {
                ManagementObjectSearcher intquery;
                intquery = new ManagementObjectSearcher("SELECT * FROM Win32_NetworkAdapter WHERE Description ='" + comboBox1.Items[comboBox1.SelectedIndex].ToString() + "'");
                ManagementObjectCollection queryCollection = intquery.Get();
                queryCollection = intquery.Get();
                foreach (ManagementObject mo in queryCollection)
                {
                    string thisstring = (mo["Availability"].ToString());
                    int Avail;
                    int.TryParse(thisstring, out Avail);
                    MessageBox.Show(availabilityArray[Avail]);
                }
            }
            catch (Exception ex2)
            {
                MessageBox.Show("Please Select a Device");
            }
        }
        private void btnPing_Click(object sender, EventArgs e)
        {
            // Reset the number of pings
            pingsSent = 0;
            // Clear the textbox of any previous content
            txtResponse.Clear();
            txtResponse.Text += "Pinging " + txtIP.Text + " with 32 bytes of data:\r\n\r\n";
            // Send the ping
            SendPing();
        }
        private void SendPing()
        {
            System.Net.NetworkInformation.Ping pingSender = new System.Net.NetworkInformation.Ping();
            // Create an event handler for ping complete
            pingSender.PingCompleted += new PingCompletedEventHandler(pingSender_Complete);
            // Create a buffer of 32 bytes of data to be transmitted.
            byte[] packetData = Encoding.ASCII.GetBytes("................................");
            // Jump though 50 routing nodes tops, and don't fragment the packet
            PingOptions packetOptions = new PingOptions(50, true);
            // Send the ping asynchronously
            pingSender.SendAsync(txtIP.Text, 5000, packetData, packetOptions, resetEvent);
        }
        private void pingSender_Complete(object sender, PingCompletedEventArgs e)
        {
           // If the operation was canceled, display a message to the user.
            if (e.Cancelled)
            {
                txtResponse.Text += "Ping was canceled...\r\n";
                // The main thread can resume
                ((AutoResetEvent)e.UserState).Set();
            }
            else if (e.Error != null)
            {
                txtResponse.Text += "An error occured: " + e.Error + "\r\n";
                // The main thread can resume
                ((AutoResetEvent)e.UserState).Set();
            }
            else
            {
               PingReply pingResponse = e.Reply;
                // Call the method that displays the ping results, and pass the information with it
               ShowPingResults(pingResponse);
            }
        }
        public void ShowPingResults(PingReply pingResponse)
        {
            if (pingResponse == null)
            {
                // We got no response
                txtResponse.Text += "There was no response.\r\n\r\n";
                return;
            }
            else if (pingResponse.Status == IPStatus.Success)
            {
                // We got a response, let's see the statistics
                txtResponse.Text += "Reply from " + pingResponse.Address.ToString() + ": bytes=" + pingResponse.Buffer.Length + " time=" + pingResponse.RoundtripTime + " TTL=" + pingResponse.Options.Ttl + "\r\n";
            }
            else
            {
                // The packet didn't get back as expected, explain why
                txtResponse.Text += "Ping was unsuccessful: " + pingResponse.Status + "\r\n\r\n";
            }
            // Increase the counter so that we can keep track of the pings sent
            string strpings = txtNumPackets.Text;
            int numbpings = int.Parse(strpings);
            if (txtNumPackets.Text == null)
            {
                numbpings = 4;
            }
            pingsSent++;
            // Send 4 pings
            if (pingsSent < numbpings)
            {
                SendPing();
            }
        }
        private void panel2_Paint(object sender, PaintEventArgs e)
        {
        }
        //////////////////////////////////////////////////////////////////////////////////////////
    }
}
\$\endgroup\$

migrated from stackoverflow.com Jun 7 '11 at 12:14

This question came from our site for professional and enthusiast programmers.

  • 12
    \$\begingroup\$ First casualty would be all those extraneous blank lines. \$\endgroup\$ – BoltClock Jun 7 '11 at 9:06
  • 3
    \$\begingroup\$ One very easy way to make this code vastly more readable is to remove the many redundant blank lines. Whitespace generally increases readability, but only up to a point. Your code forces the reader to scroll all the time, nothing ever fits completely onto the screen. This is bad. \$\endgroup\$ – Konrad Rudolph Jun 7 '11 at 9:08
  • \$\begingroup\$ There seem to be new lines where they are not needed and no new lines where they are required. \$\endgroup\$ – Jodrell Jun 7 '11 at 9:26
12
\$\begingroup\$

One or rarely two blank lines can aid readability, any more degrades readability.

If a line of code is long and goes off the screen you could put a new line in to make it readable without scrolling.

Why the meaningless ///////////////////////////////////// spacer?

The code has very few comments, especially near start, and they don't add much to the readability, if you want to split the code into regions the use region and end region keywords.

Your namespace namespace WindowsFormsApplication3 and your classes public partial class Form1 : Form, and controls all have automatically generated names. These should be changed to something that actually imparts some indication of the their purpose.

Catch generic exceptions sparingly, only do it if you really want to treat all exceptions the same and comment this accordingly, just catch on its own works for this.

If you are going suppress the exception you should comment accordingly, you don't need to provide a name for the exception when doing this, just use catch (SomeException).

Consider declaring literals as constants or as resources, this is not always expedient, especially when the string is effectively code and self descriptive but is generally good practice.

!true is false.

Use camel case declaration names, pascal case for constants. Variable names should comprise of whole words or recognised acronyms or abbreviations. Acronyms of 3 or over chars should be written lower case.

Use String.Format with an IFormatProvider when building or concatenating a string, not the + operator.

Don't have empty event handlers.

Consider using FXCop or Resharper.

\$\endgroup\$
10
\$\begingroup\$

A couple of things based on a quick read through the code:

First, catching exceptions that you don't know how to handle is generally not a good idea. And squashing exceptions is almost always a bad idea. For example, you have:

try
{
    // do stuff here
}
catch (Exception)
{
    // do nothing
}

You should catch only those exceptions that you know how to handle. There are many unexpected things that could happen in your code that could put the program in an unstable state. If that happens, you should let the program crash. By catching and squashing all exceptions, you allow the program to potentially corrupt data or return incorrect results.

You have this code in one of your methods:

if ((bool)mo1["DNSEnabledForWINSResolution"] == true)
{
    chkDNSWinsResolution.Checked = true;
}
if ((bool)mo1["DNSEnabledForWINSResolution"] != true)
{
    chkDNSWinsResolution.Checked = !true;
}

You can shorten that and make it more readable by using if...else:

if ((bool)mo1["DNSEnabledForWINSResolution"] == true)
{
    chkDNSWinsResolution.Checked = true;
}
else
{
    chkDNSWinsResolution.Checked = !true;
}

Also, !true is the same thing as false in C#. bool is a type that can only take one of two values, unlike the old C BOOL that was just an alias for int. C# programmers are used to seeing false. !true will confuse them.

The code can be further simplified to:

chkDNSWinsResolution.Checked = (bool)mo1["DNSEnabledForWINSResolution"];

without losing clarity. In fact, one could argue that it's more clear. Terseness isn't a goal in itself, but here the meaning is quite clear, and it eliminates several lines of extraneous lines of code.

Your availabilityArray and errorArray arrays should be defined as static readonly, and formatted across multiple lines. For example:

static readonly string[] availabilityArray = new string[]
{
    "",
    "Other",
    "Unknown",
    // etc.
}

That's much more readable. Making them readonly prevents them from being modified and also tells others looking at the program that they won't be modified. static ensures that only one copy will be used. If it's not static, then every instance of the class will create a separate array to hold the same constant strings.

You need to handle error return values. For example, you have:

int error;
int.TryParse(thisstring, out error);

If thisstring is not a valid integer, then error will be 0. You probably should write instead something like:

int error;
string errMsg;
if (int.TryParse(thisstring, out error) && error >= 0 && error < errorArray.Length)
{
    errMsg = errorArray[error];
}
else
{
    errMsg = string.Format("Unknown error code: {0}", thisstring);
}
MessageBox.Show(errMsg);

Otherwise your code will throw an exception if error is outside the range of possible error codes, and if thisstring isn't a valid integer you'll display a bogus error message.

In general, if a conversion can cause an error, assume that it will at some point, and be prepared to handle it. In one place, for example, you're using int.Parse to parse user input from a text box. If the user enters a non-numeric value or a value that can't be converted to an integer (for example, 3000000000), that's going to throw an exception. You should use TryParse, check the return value, and display an error message if it fails.

\$\endgroup\$
  • \$\begingroup\$ @Jim Thankyou, this is exactly the kind of advice i'm lookinf for and can apply in many situations :) \$\endgroup\$ – Dylan Jackson Jun 7 '11 at 9:22
  • \$\begingroup\$ @Jim would validation on the textbox do the same thing? \$\endgroup\$ – Dylan Jackson Jun 7 '11 at 9:37
  • \$\begingroup\$ @Dylan: Validation on the textbox can do the same thing, as long as you guarantee that the user can't exit the textbox without it being validated. That turns out to be something of a difficult problem in the general case, and often turns out to be annoying to the user because it interferes with editing. \$\endgroup\$ – Jim Mischel Jun 7 '11 at 9:55
10
\$\begingroup\$

@KerriShotts already mentioned Whitespace in declaring your string arrays, but what no one talks about is the trailing (,) comma in your string array

string[] errorArray = new string[] 
{ 
    "Device is working properly.", 
    "Device is not configured correctly.", 
    "Windows cannot load the driver for this device.", 
    "Driver for this device might be corrupted, or the system may be low on memory or other resources.", 
    "Device is not working properly. One of its drivers or the registry might be corrupted.",
};

The compiler won't like this because it will be looking for another item in the array,

It's kind of like writing a list of things but ending with a comma, or not using and before the last item and instead ending the thought with a ,

You do this in both String Arrays errorArray, and availabilityArray

Note: it seems that the Compiler just removes the Comma (Visual Studio 2013) but I wouldn't get into the habit of doing this, it isn't compliant with standards I am sure.

Another thing about these two arrays is that the naming isn't the best, personally I would name them something like errorMessages and availabilityMessages. not much of a change, but now I know that the error array isn't an array of exceptions, rather it's an array of type string

These should be declared before the constructor because they are class variables, some may disagree on this.

You may want to look into List<string> or even a dictionary for these arrays.

With a dictionary you would have a key/value pair, which might suit your usage a lot better than an array.


Your naming conventions allow me to see what is a text box just from looking at the code, but hungarian naming is really going out of style (if it isn't already dead and buried), if you ask anyone on Code Review they will say pretty much the same thing.

Use descriptive variable names, even for form elements. (And don't disemvowel)


I noticed that you have newline characters in your string and that one of them is different from the others.

 , "Power Save - Low Power Mode" + "/n" + "The device is in a power save state, but still functioning, and may exhibit degraded performance."

There is no need to concatenate these into the string either, so what you would have for this one is something like this.

, "Power Save - Unknown /nThe device is known to be in a power save state, but its exact status is unknown."

no concatenation needed.


You have a double assignment inside of your first foreach loop.

 ManagementObjectCollection queryCollection1 = intquery1.Get();
 queryCollection1 = intquery1.Get();

completely unnecessary, but I am going to change this in a little bit as well.

You do this in every button click method.


your TxtGateway breaks naming first of all and second it is only going to be set to the last gateway in the collection.

Going through your code I see a lot of stuff that is being overwritten over and over again, like

 if ((bool)mo1["DHCPEnabled"] == true)
 {
     checkBox1.Checked = true;
 }
 if ((bool)mo1["DHCPEnabled"] != true)
 {
     checkBox1.Checked = !true;
 }

Which is inside of a foreach loop (that is inside a foreach loop), this checkbox is going to get confused because it is set so many times, why don't we set a boolean variable and set the check box once at the end of the method?

Let us also do the same with the TxtGateway: create a variable to hold the value and set the .text once at the end of the method.


Pretty sure that this code is left over or accidental double click on the form.

 private void panel2_Paint(object sender, PaintEventArgs e)
 {
 }

so let's just get rid of it.


In btnPing_Click your comment says that you are resetting the number of pings, but this isn't completely true, the private variable has been declared but no assignment was made, this means that until you set it to 0 it has a value of null.

While we are talking about the pingsSent variable, why are you incrementing it inside of a display method(ShowPingResults)?

In order to check to make sure that you are receiving all ping replies you should increment pingsSent inside of the SendPing method, that way you can compare between pings sent and pings received and see if there is an issue with dropped (pings).

then you can count how many cancelled, error, or reply events you receive to determine what is going on with your pings.


DateTime objects are nice and neat.

DateTime dt = System.Management.ManagementDateTimeConverter.ToDateTime(mo1["DHCPLeaseExpires"].ToString());
txtDHCPExpires.Text = dt.ToString();
DateTime dte = System.Management.ManagementDateTimeConverter.ToDateTime(mo1["DHCPLeaseObtained"].ToString());
txtDHCPObtained.Text = dte.ToString();

There is no need to create DateTime variables for this, just assign straight to the text field of the textbox like this

txtDHCPExpires.Text = System.Management.ManagementDateTimeConverter.ToDateTime(mo1["DHCPLeaseExpires"].ToString());
txtDHCPObtained.Text = System.Management.ManagementDateTimeConverter.ToDateTime(mo1["DHCPLeaseObtained"].ToString());

Your Management objects are IDisposable so you should make use of the using construct like your code looks like this

    private void button2_Click(object sender, EventArgs e)
    {
        try
        {
            ManagementObjectSearcher intquery;
            intquery = new ManagementObjectSearcher("SELECT * FROM Win32_NetworkAdapter WHERE Description ='" + comboBox1.Items[comboBox1.SelectedIndex].ToString() + "'");
            ManagementObjectCollection queryCollection = intquery.Get();
            queryCollection = intquery.Get();
            foreach (ManagementObject mo in queryCollection)
            {
                string thisstring = (mo["ConfigManagerErrorCode"].ToString());
                int error;
                int.TryParse(thisstring, out error);
                MessageBox.Show(errorArray[error]);
            }
        }
        catch (Exception ex1)
        {
            MessageBox.Show("Please Select a Device");
        }
    }

and with using statements stacked nicely it would look like this.

try
{
    using (ManagementObjectSearcer intquery = new ManagementObjectSearcer("SELECT * FROM Win32_NetworkAdapter WHERE Description ='" + comboBox1.Items[comboBox1.SelectedIndex].ToString() + "'"))
    using (ManagementObjectCollection queryCollection = intquery.Get())
    {
        foreach (ManagementObject mo in queryCollection)
        {
            string thisstring = (mo["ConfigManagerErrorCode"].ToString());
            int error;
            int.TryParse(thisstring, out error);
            MessageBox.Show(errorArray[error]);
        }
    }
}
catch (Exception ex1)
{
    MessageBox.Show("Please Select a Device");
}

and the object will be disposed of appropriately so that there are no leaks anywhere.


with all of those changes, this is what the code looks like, I think there is still plenty to do, please feel free to post a follow-up question with your new(er) code!

public partial class Form1 : Form
{
    private int pingsSent;
    AutoResetEvent resetEvent = new AutoResetEvent(false);
    public Form1()
    {
        {
            InitializeComponent();
        }
        // win32 class
        //http://msdn.microsoft.com/en-us/library/aa394216(v=vs.85).aspx
        try
        {
            ManagementObjectSearcher query = new ManagementObjectSearcher(@"SELECT * FROM Win32_NetworkAdapter WHERE  Manufacturer != 'Microsoft' AND NOT PNPDeviceID LIKE 'ROOT\\%'");
            ManagementObjectCollection queryCollection = query.Get();
            foreach (ManagementObject mo in queryCollection)
            {
                if (mo["MacAddress"] != null)
                {
                    comboBox1.Items.Add(mo["Description"].ToString());
                }
            }
        }
        catch (Exception ex)
        {
        }
    }
    string[] availabilityArray = new string[] 
        { ""
            , "Other"
            , "Unknown"
            , "Running or Full Power"
            , "Warning", "In Test"
            , "Not Applicable"
            , "Power Off"
            , "Off Line"
            , "Off Duty"
            , "Degraded"
            , "Not Installed"
            , "Install Error"
            , "Power Save - Unknown /nThe device is known to be in a power save state, but its exact status is unknown."
            , "Power Save - Low Power Mode" + "/n" + "The device is in a power save state, but still functioning, and may exhibit degraded performance."
            , "Power Save - Standby" + "/n" + "The device is not functioning, but could be brought to full power quickly."
            , "Power Cycle"
            , "Power Save - Warning" + "/n" + "The device is in a warning state, though also in a power save state." 
        };
    string[] errorArray = new string[] 
        { "Device is working properly."
            , "Device is not configured correctly."
            , "Windows cannot load the driver for this device."
            , "Driver for this device might be corrupted, or the system may be low on memory or other resources."
            , "Device is not working properly. One of its drivers or the registry might be corrupted."
            , "Driver for the device requires a resource that Windows cannot manage."
            , "Boot configuration for the device conflicts with other devices."
            , "Cannot filter.", "Driver loader for the device is missing."
            , "Device is not working properly. The controlling firmware is incorrectly reporting the resources for the device."
            , "Device cannot start.", "Device failed."
            , "Device cannot find enough free resources to use."
            , "Windows cannot verify the device's resources."
            , "Device cannot work properly until the computer is restarted."
            , "Device is not working properly due to a possible re-enumeration problem."
            , "Windows cannot identify all of the resources that the device uses."
            , "Device is requesting an unknown resource type."
            , "Device drivers must be reinstalled."
            , "Failure using the VxD loader."
            , "Registry might be corrupted."
            , "System failure. If changing the device driver is ineffective, see the hardware documentation. Windows is removing the device."
            , "Device is disabled.", "System failure. If changing the device driver is ineffective, see the hardware documentation."
            , "Device is not present, not working properly, or does not have all of its drivers installed."
            , "Windows is still setting up the device.", "Windows is still setting up the device."
            , "Device does not have valid log configuration.", "Device drivers are not installed"
            , "Device is disabled. The device firmware did not provide the required resources."
            , "Device is using an IRQ resource that another device is using."
            , "Device is not working properly. Windows cannot load the required device drivers."
        };
    private void comboBox1_SelectedIndexChanged(object sender, EventArgs e)
    {
        using (ManagementObjectSearcher intquery = new ManagementObjectSearcher("SELECT * FROM Win32_NetworkAdapter WHERE Description ='" + comboBox1.Items[comboBox1.SelectedIndex].ToString() + "'"))
        using (ManagementObjectCollection queryCollection = intquery.Get())
        {
            foreach (ManagementObject mo in queryCollection)
            {
                string Dev = (mo["DeviceID"].ToString());
                txtMacAdd.Text = (" " + mo["MacAddress"].ToString());
                TxtAdapter.Text = (" " + mo["AdapterType"].ToString());
                txtAvailability.Text = (" " + mo["Availability"].ToString());
                txtCaption.Text = (" " + mo["Caption"].ToString());
                txtDeviceID.Text = (" " + mo["DeviceID"].ToString());
                txtErrorCode.Text = (mo["ConfigManagerErrorCode"].ToString());
                using (ManagementObjectSearcher intquery1 = new ManagementObjectSearcher("ASSOCIATORS OF {Win32_NetworkAdapter.DeviceID='" + Dev + "'}WHERE ResultClass=Win32_NetworkAdapterConfiguration"))
                using (ManagementObjectCollection queryCollection1 = intquery1.Get())
                {
                    foreach (ManagementObject mo1 in queryCollection1)
                    {
                        string[] addresses = (string[])mo1["IPAddress"];
                        string[] gateways = (string[])mo1["DefaultIPGateway"];
                        string[] Subnets = (string[])mo1["IPSubnet"];
                        foreach (string ipaddress in addresses)
                        {
                            listIp.Items.Add(ipaddress);
                        }
                        foreach (string gateway in gateways)
                        {
                            TxtGateway.Text = (gateway);
                        }
                        foreach (string subnet in Subnets)
                        {
                            listSubnet.Items.Add(subnet);
                        }
                        /////// SHOW DCHP CONFIG/////////

                        checkBox1.Checked = mo1["DHCPEnabled"];

                        txtDHCPExpires.Text = System.Management.ManagementDateTimeConverter.ToDateTime(mo1["DHCPLeaseExpires"].ToString());
                        txtDHCPObtained.Text = System.Management.ManagementDateTimeConverter.ToDateTime(mo1["DHCPLeaseObtained"].ToString());
                        txtDHCPServer.Text = (mo1["DHCPServer"].ToString());
                        //Security
                        chkboxIPFilter.Checked = mo1["IPFilterSecurityEnabled"];

                        string[] IPSecProtocols = (string[])mo1["IPSecPermitIPProtocols"];
                        string[] IPSecPermitTCPPorts = (string[])mo1["IPSecPermitTCPPorts"];
                        string[] IPSecPermitUDPPorts = (string[])mo1["IPSecPermitUDPPorts"];
                        foreach (string IPSecProtocol in IPSecProtocols)
                        {
                            listPermitIP.Items.Add(IPSecProtocol);
                        }
                        foreach (string TCPPort in IPSecPermitTCPPorts)
                        {
                            listPermitTCP.Items.Add(TCPPort);
                        }
                        foreach (string UDPPort in IPSecPermitUDPPorts)
                        {
                            listPermitUDP.Items.Add(UDPPort);
                        }
                        //DNS
                        try
                        {
                            txtDNSDomain.Text = (mo1["DNSDomain"].ToString());
                            txtDNShostName.Text = (mo1["DNSHostName"].ToString());
                            string[] DNSDomainSuffSearch = (string[])mo1["DNSDomainSuffixSearchOrder"];
                            string[] DNSServerSearchOrder = (string[])mo1["DNSServerSearchOrder"];
                            foreach (string domainSuff in DNSDomainSuffSearch)
                            {
                                listDomainSuffSearchOrder.Items.Add(domainSuff);
                            }
                            foreach (string DNSServer in DNSServerSearchOrder)
                            {
                                listDNSServerSearchOrder.Items.Add(DNSServer);
                            }

                            chkDNSWinsResolution.Checked = mo1["DNSEnabledForWINSResolution"];
                            chkDNSRegEnabled.Checked = mo1["DomainDNSRegistrationEnabled"];
                        }
                        catch (Exception ex3)
                        { }
                    }
                }
            }
        }
    }
    private void button2_Click(object sender, EventArgs e)
    {
        try
        {
            using (ManagementObjectSearcer intquery = new ManagementObjectSearcer("SELECT * FROM Win32_NetworkAdapter WHERE Description ='" + comboBox1.Items[comboBox1.SelectedIndex].ToString() + "'"))
            using (ManagementObjectCollection queryCollection = intquery.Get())
            {
                foreach (ManagementObject mo in queryCollection)
                {
                    string thisstring = (mo["ConfigManagerErrorCode"].ToString());
                    int error;
                    int.TryParse(thisstring, out error);
                    MessageBox.Show(errorArray[error]);
                }
            }
        }
        catch (Exception ex1)
        {
            MessageBox.Show("Please Select a Device");
        }
    }
    private void BtnAvailability_Click(object sender, EventArgs e)
    {
        try
        {
            using (ManagementObjectSearcher intquery = new ManagementObjectSearcher("SELECT * FROM Win32_NetworkAdapter WHERE Description ='" + comboBox1.Items[comboBox1.SelectedIndex].ToString() + "'"))
            using (ManagementObjectCollection queryCollection = intquery.Get())
            {
                foreach (ManagementObject mo in queryCollection)
                {
                    string thisstring = (mo["Availability"].ToString());
                    int Avail;
                    int.TryParse(thisstring, out Avail);
                    MessageBox.Show(availabilityArray[Avail]);
                }
            }
        }
        catch (Exception ex2)
        {
            MessageBox.Show("Please Select a Device");
        }
    }
    private void btnPing_Click(object sender, EventArgs e)
    {
        // Reset the number of pings
        pingsSent = 0;
        // Clear the textbox of any previous content
        txtResponse.Clear();
        txtResponse.Text += "Pinging " + txtIP.Text + " with 32 bytes of data:\r\n\r\n";
        // Send the ping
        SendPing();
    }
    private void SendPing()
    {
        System.Net.NetworkInformation.Ping pingSender = new System.Net.NetworkInformation.Ping();
        // Create an event handler for ping complete
        pingSender.PingCompleted += new PingCompletedEventHandler(pingSender_Complete);
        // Create a buffer of 32 bytes of data to be transmitted.
        byte[] packetData = Encoding.ASCII.GetBytes("................................");
        // Jump though 50 routing nodes tops, and don't fragment the packet
        PingOptions packetOptions = new PingOptions(50, true);
        // Send the ping asynchronously
        pingSender.SendAsync(txtIP.Text, 5000, packetData, packetOptions, resetEvent);
    }
    private void pingSender_Complete(object sender, PingCompletedEventArgs e)
    {
        // If the operation was canceled, display a message to the user.
        if (e.Cancelled)
        {
            txtResponse.Text += "Ping was canceled...\r\n";
            // The main thread can resume
            ((AutoResetEvent)e.UserState).Set();
        }
        else if (e.Error != null)
        {
            txtResponse.Text += "An error occured: " + e.Error + "\r\n";
            // The main thread can resume
            ((AutoResetEvent)e.UserState).Set();
        }
        else
        {
            PingReply pingResponse = e.Reply;
            // Call the method that displays the ping results, and pass the information with it
            ShowPingResults(pingResponse);
        }
    }
    public void ShowPingResults(PingReply pingResponse)
    {
        if (pingResponse == null)
        {
            // We got no response
            txtResponse.Text += "There was no response.\r\n\r\n";
            return;
        }
        else if (pingResponse.Status == IPStatus.Success)
        {
            // We got a response, let's see the statistics
            txtResponse.Text += "Reply from " + pingResponse.Address.ToString() + ": bytes=" + pingResponse.Buffer.Length + " time=" + pingResponse.RoundtripTime + " TTL=" + pingResponse.Options.Ttl + "\r\n";
        }
        else
        {
            // The packet didn't get back as expected, explain why
            txtResponse.Text += "Ping was unsuccessful: " + pingResponse.Status + "\r\n\r\n";
        }
        // Increase the counter so that we can keep track of the pings sent
        string strpings = txtNumPackets.Text;
        int numbpings = int.Parse(strpings);
        if (txtNumPackets.Text == null)
        {
            numbpings = 4;
        }
        pingsSent++;
        // Send 4 pings
        if (pingsSent < numbpings)
        {
            SendPing();
        }
    }
}

As you can see I left the pinger alone, I didn't want to get pinged.

\$\endgroup\$
9
\$\begingroup\$

Refactor your comboBox1_SelectedIndexChanged method to just call different functions for each different task it is doing. Passing the correct arguments to your functions as necessary.

Do NOT have empty catch clauses in your code. Shows bad exception handling. If you put code in a try catch you should at least make sure you handle your exceptions properly.

Name your controls properly. Controls with names as ComboBox1 is not proper naming. That doesn't tell me from looking at your code what this combobox is all about. Proper naming makes a lot of difference to readability of your code.

This:

catch (Exception ex1)
{
    MessageBox.Show("Please Select a Device");
}

does not fit in that catch clause, the code that is inside the accompanying try block will not always fail due to a non selected device. You should handle this with an if statement, as it really is not an exception, just a flow in your program, informing the user what should be done. It does not signify an exceptional circumstance in your program.

\$\endgroup\$
6
\$\begingroup\$

Some thoughts:

  1. Comments would be nice. I see a few, very sparsely written, and it makes it hard to figure out what is going on from a quick glance. Furthermore, in your comments, add something like this:

    /*
    * NIC-WMI Query
    * Version 1.0, Created on 1/2/10 by Dylan Jackson
    *
    * Description of program...
    *
    * Audit Trail:
    * ------------
    *
    * 01/01/2010 DJ v0.9 Added a,b,c. Fixed d,e.
    * ...
    */
    

    A comment block like this lets me know what the program is about, what version it is, who wrote it (and when), what changes have been made (and by whom). You'll come up with your own style and what you do and don't want there (or it may be dictated to you), but something at the top is never a bad thing to help other readers know what is going on.

  2. Don't use generic object/class names. Things like WindowsFormApplication3 or button2 are bad as I can't tell what they do, especially when you're writing event code for them. button2_click tells me zip.

  3. Whitespace can be bad (as in too much between lines), but whitespace in long lists is always good. Ideally avoid going beyond 132 chars, and even then, if you have a long list break it up nicely so it is easily readable. For example:

    string[] errorArray = new string[] 
    { 
        "Device is working properly.", 
        "Device is not configured correctly.", 
        "Windows cannot load the driver for this device.", 
        "Driver for this device might be corrupted, or the system may be low on memory or other resources.", 
        "Device is not working properly. One of its drivers or the registry might be corrupted.",
    };
    
  4. I know you can't put the actual appearance of the form in the code, but hey, it'd be nice to have some idea of what the form looks like and what objects are what. Something like this wouldn't be objectionable (as an example):

    frmWhatIsYourName looks like this:

    +---------------------------------------------------+
    |               What is your name?                  |
    |---------------------------------------------------|
    |                                                   |
    |   Please enter your name: [txtName              ] |
    |                                                   |
    |                            [   Ok   ]  [ Cancel ] |
    |                             (btnOk)    (btnCancel)|
    +---------------------------------------------------+
    

    You don't have to get particularly fancy; a verbal description would work as well:

    /*
     * The form frmWhatIsYourName has a single text box named txtName for storing
     * the person's name, and two buttons, btnOk (OK) and btnCancel (CANCEL).
     */
    

That would make a good start, I think, and there have been good suggestions from everyone else here as well.

\$\endgroup\$
5
\$\begingroup\$

@Tony The Tiger's answer is perfect so I only want to ask a favor of you.

Please replace

if ((bool)mo1["DNSEnabledForWINSResolution"] != true)
{
    chkDNSWinsResolution.Checked = !true;
}

blocks with something more readable, like

chkDNSWinsResolution.Checked = (bool)mo1["DNSEnabledForWINSResolution"];

Also, !true is called false.

\$\endgroup\$
  • 4
    \$\begingroup\$ Your replacement code is not logically equivalent to the original code. If the value of mo1["DNSEnabledForWINSResolution"] is true, then the checkbox.Checked value remains unchanged. Your code would set it to false. \$\endgroup\$ – Mongus Pong Jun 7 '11 at 12:20
  • \$\begingroup\$ @Mongus Pong: thanks for the note. I thought he relied on default values being set in the form designer, and in case they didn't match default mo1 settings, he would see it either way first time the form is shown after the changes. However you're right it is worth mentioning. \$\endgroup\$ – Dan Jun 7 '11 at 12:54
  • \$\begingroup\$ it is certainly possible that your code might be what he meant! \$\endgroup\$ – Mongus Pong Jun 7 '11 at 12:58

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.