Problem
I know this may be very time consuming for you as the answerer of this question, but I have recently been looking into C# and I decided that I would try to develop a pinging application, so that I did. I have all the functionality. I just have a question regarding readability and how to set up the individual documents. As for now I only have one document and one class and several methods, but I was wondering whether I should have multiple? I am only using one global variable (path). Also I would like you to comment on the code, if you see somewhere where I could do something differently, optimizing my code.
Here’s the code:
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;
using System.Threading;
using System.Net.NetworkInformation;
using System.Media;
using System.IO;
using System.Text.RegularExpressions;
namespace PingComplete
{
public partial class Form1 : Form
{
public string pingFilePath = Application.StartupPath + "/ping.dll"; //the file where all the ip addresses are located
public Form1()
{
InitializeComponent();
}
private void Ping(string hostname) //ping function
{
using (Ping ping = new Ping())
{
try
{
PingReply reply = ping.Send(hostname, 100);
if (reply.Status == IPStatus.Success)
{
txtConsole.AppendText("Pinged " + hostname + " at " + reply.Address + " Successfully. t Time: " + reply.RoundtripTime + " ms rn");
}
else if (reply.Status == IPStatus.TimedOut) //Problem with the pings to be too frequently timed out, so a "fix" or "hack" around this.
{
txtConsole.AppendText("Connection time out. Connection retried for " + hostname + "rn");
PingReply reply2 = ping.Send(hostname, 100);
txtConsole.AppendText("Pinged " + hostname + " at " + reply2.Address + " Successfully. t Time: " + reply2.RoundtripTime + " ms rn");
}
else
{
txtConsole.AppendText("Couldn't ping " + hostname + "; Error: " + reply.Status + ".rn");
playAlertSound();
Console.WriteLine(reply.Status);
}
}
catch
{
txtConsole.AppendText("Error in hostname.rn");
}
}
}
public void updateListWithAddresses() //Reloads the list in the sidebar with newest addresses
{
if (File.Exists(pingFilePath))
{
listAddresses.Items.Clear();
string[] fileContents = File.ReadAllLines(pingFilePath);
listAddresses.Items.AddRange(fileContents);
}
}
public void playAlertSound() //Alert sound when there's an error, horrible sound - should be revised
{ //Alert sound
SoundPlayer alertSound = new SoundPlayer(@"c:WindowsMediachimes.wav");
alertSound.Play();
}
private void saveConsoleData() //logs the console to the file
{
if (txtConsole.Text != "")
{
string pathtohistory = Application.StartupPath + "/history-from-y-" + DateTime.Now.Year + "-m-" + DateTime.Now.Month + "-d-" + DateTime.Now.Day + "-h-" + DateTime.Now.TimeOfDay.Hours + "-m-" + DateTime.Now.TimeOfDay.Minutes + "-s-" + DateTime.Now.TimeOfDay.Seconds + "-ms-" + DateTime.Now.TimeOfDay.Milliseconds + ".txt";
//create file
StreamWriter MyStream = null;
MyStream = File.CreateText(pathtohistory);
MyStream.Close();
using (System.IO.StreamWriter file = new System.IO.StreamWriter(pathtohistory, true))
{
file.WriteLine(txtConsole.Text);
file.Close();
}
}
}
private void newAddress() //adding new address
{
if (File.Exists(pingFilePath))
{
using (System.IO.StreamWriter file = new System.IO.StreamWriter(pingFilePath, true))
{
if (txtNewAddress.Text != "")
{
file.WriteLine(txtNewAddress.Text);
txtConsole.AppendText(txtNewAddress.Text + " was added, now go ping it!rn");
file.Close();
updateListWithAddresses();
txtNewAddress.Text = "";
}
else
{
txtConsole.AppendText("You have to write something in the address field. Try again.rn");
}
}
}
else
{
//create file
StreamWriter MyStream = null;
MyStream = File.CreateText(pingFilePath);
MyStream.Close();
using (System.IO.StreamWriter file = new System.IO.StreamWriter(pingFilePath, true))
{
file.WriteLine(txtNewAddress.Text);
txtConsole.AppendText(txtNewAddress.Text + " was added, now go ping it!rn");
file.Close();
updateListWithAddresses();
}
}
}
private void btnPing_Click(object sender, EventArgs e) //When ping button is clicked
{
for (int i = 0; i < listAddresses.Items.Count; i++)
{
listAddresses.SetSelected(i, true);
Ping(listAddresses.Text);
listAddresses.SetSelected(i, false);
}
txtConsole.AppendText("Query completed.n");
}
private void btnNewAddress_Click(object sender, EventArgs e) //add new address
{
newAddress();
}
private void Form1_Load(object sender, EventArgs e)
{
llAbout.Links.Add(0, 24, "http://mikkeljuhl.com");
updateListWithAddresses();
}
private void btnDelete_Click(object sender, EventArgs e) //deletes an address from the file and sidebar
{
if (listAddresses.SelectedIndex >= 0)
{
listAddresses.Items.RemoveAt(listAddresses.SelectedIndex);
string[] contentOfFile = new string[listAddresses.Items.Count];
listAddresses.Items.CopyTo(contentOfFile, 0);
File.WriteAllLines(pingFilePath, contentOfFile);
txtConsole.AppendText(listAddresses.Text + " was succcessfully removed!rn");
}
}
private void btnClearConsole_Click(object sender, EventArgs e) //clears console
{
txtConsole.Clear();
}
private void llAbout_LinkClicked(object sender, LinkLabelLinkClickedEventArgs e)
{
string target = e.Link.LinkData as string;
System.Diagnostics.Process.Start(target);
}
private void exitToolStripMenuItem_Click(object sender, EventArgs e)
{
Application.Exit();
}
private void Form1_FormClosed(object sender, FormClosedEventArgs e)
{
saveConsoleData();
}
private void txtNewAddress_KeyDown(object sender, KeyEventArgs e)
{
if (e.KeyCode == Keys.Enter)
{
newAddress();
}
}
}
}
Solution
You have a 338 character line, find a way to split up your long lines.
You don’t need to fully qualify things that you have a using statement for: System.IO.StreamWriter could be StreamWriter because you have a using System.IO; at the start of the file.
These using statements aren’t needed:
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading;
using System.Text.RegularExpressions;
Default value of a variable is null, usually no need to set it explicity. In fact why not just combine these two lines.
StreamWriter MyStream = null;
MyStream = File.CreateText(pathtohistory);
succcessfully is spelled wrong.
I didn’t look at your logic, cleanup your code, including some of the things mentioned in the comments and someone might be able to look at it in more detail.
I don’t code C#, so just two general notes:
-
If I’m right you are storing configuration in a
dllfile (ping.dll). DLL extension is usually used for executable code, not for config files. -
I guess there are better ways to format a datetime than concatenating strings: (Custom Date and Time Format Strings)