2
\$\begingroup\$

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 \r\n");
                }
                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 + "\r\n");
                    PingReply reply2 = ping.Send(hostname, 100);
                    txtConsole.AppendText("Pinged " + hostname + " at " + reply2.Address + " Successfully. \t Time: " + reply2.RoundtripTime + " ms \r\n");
                }
                else
                {
                    txtConsole.AppendText("Couldn't ping " + hostname + "; Error: " + reply.Status + ".\r\n");
                    playAlertSound();
                    Console.WriteLine(reply.Status);
                }
            }
            catch
            {
                txtConsole.AppendText("Error in hostname.\r\n");
            }
        }
    }
    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:\Windows\Media\chimes.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!\r\n");
                    file.Close();
                    updateListWithAddresses();
                    txtNewAddress.Text = "";
                }
                else
                {
                    txtConsole.AppendText("You have to write something in the address field. Try again.\r\n");
                }
            }
        }
        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!\r\n");
                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!\r\n");

        }
    }
    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();

        }
    }



   }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Run StyleCop on it, learn about StringBuilder, String.Format, about static, readonly modifiers. codinghorror.com/blog/2006/01/flattening-arrow-code.html. Run StyleCop on it. If you have ReSharper, use that too ;) but it does cost money. Path.Combine is useful too. Some things are better off marked as static and initialized at the top of the class. \$\endgroup\$ – Leonid Jun 9 '12 at 15:41
  • \$\begingroup\$ Please include the design level information like Class Diagram, Use cases, etc. \$\endgroup\$ – Kamran Amini Jun 9 '12 at 22:49
  • \$\begingroup\$ There is a reason why you are not getting replies ... \$\endgroup\$ – Leonid Jun 12 '12 at 4:57
4
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

I don't code C#, so just two general notes:

  1. If I'm right you are storing configuration in a dll file (ping.dll). DLL extension is usually used for executable code, not for config files.

  2. I guess there are better ways to format a datetime than concatenating strings: (Custom Date and Time Format Strings)

\$\endgroup\$

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.