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 Answers 2

3
\$\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\$
2
  • \$\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
    Commented Feb 5, 2015 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
    Commented Feb 6, 2015 at 7:01
4
\$\begingroup\$

Just noticed this and noticed the one thing the other answer omitted was:

System.Globalization.CultureInfo customCulture = (System.Globalization.CultureInfo)System.Threading.Thread.CurrentThread.CurrentCulture.Clone();

It is acceptable to initialize variables using var keyword to avoid long redundant declarations. And I don't believe using the Clone() method is the best way to get a copy of the current culture. Try this instead:

var customCulture = new CultureInfo(CurrentCulture.Name);

You should be able to avoid the namespace identifiers (e.g. System.Globalization) if you put them at the top of the file in the using declarations.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.