patterncsharpModerate
Showing various network information from a NIC card
Viewed 0 times
showingnetworkcardnicfromvariousinformation
Problem
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)
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)
Solution
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
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
Your namespace
Catch generic exceptions sparingly, only do it if you really want to treat all exceptions the same and comment this accordingly, just
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
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.
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
Don't have empty event handlers.
Consider using FXCop or Resharper.
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.
Context
StackExchange Code Review Q#2847, answer score: 12
Revisions (0)
No revisions yet.