HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Showing various network information from a NIC card

Submitted by: @import:stackexchange-codereview··
0
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)

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 ///////////////////////////////////// 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.