5
\$\begingroup\$

(I've improved the code from my last question)

My program....

  1. Downloads a file (web tracker)
  2. Update if version is lower
  3. Connects to a server using the tracker information

If one of these operations were to fail, the program stalls for a minute and goes back to step one.

A few wrong things I can notice for myself:

  1. responseStr: this doesn't look a good variable name, any ideas?
  2. I've to replace Thread.Sleep, use Timer instead.

public const double APP_VERSION = 0.76;

public const string INFO_HOST = "http://*********.appspot.com";
public const string INFO_PATH = "/tracker.txt";

[STAThread]
static void Main()
{
    Thread connection = new Thread(WebFile.Download);
    connection.Start();

    // ...
}

public class WebFile
{
    public static void Download()
    {
        System.Globalization.CultureInfo customCulture = (System.Globalization.CultureInfo)System.Threading.Thread.CurrentThread.CurrentCulture.Clone();
        customCulture.NumberFormat.NumberDecimalSeparator = ".";

        System.Threading.Thread.CurrentThread.CurrentCulture = customCulture;

        while (true)
        {
            try
            {
                WebRequest request = WebRequest.Create(INFO_HOST + INFO_PATH + "?v=" + APP_VERSION);
                WebResponse response = request.GetResponse();

                string responseStr = streamResponse(response);
                parseResponse(responseStr);
            }
            catch
            {
                Thread.Sleep(60000);
                continue;
            }
        }
    }

    private static string streamResponse(WebResponse response)
    {
        string content;
        using (var streamReader = new StreamReader(response.GetResponseStream()))
        {
            content = streamReader.ReadToEnd();
        }

        return content;
    }

    private static void parseResponse(string response)
    {
        if (response.Length > 1 && response.Substring(0, 1) != "#")
        {
            Thread.Sleep(60000);
            return;
        }

        string[] contentArray = response.Substring(1).Split(',');

        if (contentArray.Length >= 2)
        {
            if (contentArray.Length == 4)
            {
                String updateVersionStr = contentArray[2];
                String downloadURL = contentArray[3];

                double updateVersion;
                var updateVersionResult = double.TryParse(updateVersionStr, out updateVersion);

                if (updateVersionResult && updateVersion > APP_VERSION)
                {
                    String dataFolder = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData),
                                            "Client");

                    using (WebClient client = new WebClient())
                    {
                        try
                        {
                            client.DownloadFile(downloadURL, dataFolder + "/update.exe");
                            Process.Start(dataFolder + "/update.exe");
                            Environment.Exit(0);
                        }
                        catch { }
                    }
                }
            }

            string serverIp = contentArray[0];
            string serverPortStr = contentArray[1];

            int serverPort;
            var serverPortResult = int.TryParse(serverPortStr, out serverPort);

            if (serverPortResult == false)
            {
                serverPort = 5000;
            }

            ConnectionInfo connectionInfo = new ConnectionInfo();

            while (connectionInfo.SockConnectionCount == 0)
            {
                if (connectionInfo.SockConnectionRetry == false)
                {
                    Connection.Connect(serverIp, serverPort, connectionInfo);
                }
                else
                {
                    connectionInfo.SockConnectionRetry = false;
                    break;
                }
            }

            Thread.Sleep(60000);
            return;
        }
        else
        {
            Thread.Sleep(60000);
            return;
        }
    }
}

public class ConnectionInfo
{
    public int SockConnectionCount { get; set; }
    public bool SockConnectionRetry { get; set; }
}

public class Connection
{
    public static void Connect(string serverIp, int serverPort, ConnectionInfo connectionInfo)
    {
        connectionInfo.SockConnectionCount = 1;

        try
        {
            TcpClient client = new TcpClient(serverIp, serverPort);
            NetworkStream nwStream = client.GetStream();
            StreamReader reader = new StreamReader(nwStream);
            StreamWriter writer = new StreamWriter(nwStream);

            connectionInfo.SockConnectionCount = 2;

            writer.WriteLine("V:" + APP_VERSION);
            writer.Flush();

            writer.WriteLine("C:cmd");
            writer.Flush();

            writer.WriteLine("U:" + Environment.UserName + "," + System.Environment.MachineName);
            writer.Flush();

            string inputLine;

            while ((inputLine = reader.ReadLine()) != null)
            {
                parseMessage(writer, serverIp, inputLine);
            }
        }
        catch
        {
            connectionInfo.SockConnectionRetry = true;
            connectionInfo.SockConnectionCount = 0;
            return;
        }
    }

    private static void parseMessage(StreamWriter Writer, string serverIp, string msg)
    {
        // ...
    }
}
\$\endgroup\$
2
\$\begingroup\$

Some quick remarks:


Reduce indentation in methods if possible: apply the same as you did for if (response.Length > 1 && response.Substring(0, 1) != "#") to if (contentArray.Length >= 2):

string[] contentArray = response.Substring(1).Split(',');

if (contentArray.Length < 2)
{
    Thread.Sleep(60000);
    return;
}

if (contentArray.Length == 4)
{
   // etc.

That way a maintainer doesn't have to scroll 60+ lines just to find out that the else to that if is merely yet another sleep + return.


Why assign serverPortResult when you only use it once? Instead of:

int serverPort;
var serverPortResult = int.TryParse(serverPortStr, out serverPort);
if (serverPortResult == false)
{
    serverPort = 5000;
}

... do this:

int serverPort;
if (!int.TryParse(serverPortStr, out serverPort))
{
    serverPort = 5000;
}

Again you use two lines when one would be enough:

TcpClient client = new TcpClient(serverIp, serverPort);
NetworkStream nwStream = client.GetStream();

Could be just this:

NetworkStream nwStream = new TcpClient(serverIp, serverPort).GetStream();

And why not call it networkStream? No need for abbreviations.

\$\endgroup\$
  • \$\begingroup\$ Thank you so much. That's lot of issues I feels kinda upset! Anyways, I fixed all your points. Thanks for the useful links. \$\endgroup\$ – Kenny Feb 5 '15 at 21:57
  • \$\begingroup\$ Don't feel upset, this is how you learn the trade. It's how I learned the trade. ;-) We're all striving to write better code. \$\endgroup\$ – BCdotWEB Feb 6 '15 at 7:01

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.