1
\$\begingroup\$

today I coded a simple API class in C# with just a WebClient instance in each void and just simple voids to do simple things, can anyone give me any support in how I can improove it? Thanks.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Net;
using System.Text;
using System.Threading.Tasks;

namespace uTune.Base.Api
{
    class API
    {
        /// <summary>
        /// Runs a website script vue a url.
        /// </summary>
        /// <param name="url">URL to run.</param>
        public void runScript(string url)
        {
            WebClient client = new WebClient();
            client.DownloadString(url);
        }

        /// <summary>
        /// Runs a website script with the option of multiple parameters
        /// </summary>
        /// <param name="url">URL to run.</param>
        /// <param name="parameters">A list of parameters to include.</param>
        public void runScriptWithParameters(string url, Dictionary<string, string> parameters)
        {
            string urlToRun = url;
            bool firstParam = true;

            if (parameters.Count > 0)
            {
                foreach (KeyValuePair<string, string> parameter in parameters)
                {
                    if (firstParam)
                    {
                        urlToRun += "?" + parameter.Key + "=" + parameter.Value;
                        firstParam = false;
                    }
                    else
                    {
                        urlToRun += "&" + parameter.Key + "=" + parameter.Value;
                    }
                }
            }

            runScript(urlToRun);
        }

        /// <summary>
        /// Returns the pages content after a script has been executed.
        /// </summary>
        /// <param name="url">URL to get the content from.</param>
        /// <returns>Webpages content.</returns>
        public string getScriptReturn(string url)
        {
            WebClient client = new WebClient();
            return client.DownloadString(url);
        }

        /// <summary>
        /// Returns the pages conect after a script has been executed.
        /// </summary>
        /// <param name="url">URL to get the content form.</param>
        /// <param name="parameters">Parameters included in the URL.</param>
        /// <returns>Webpages content</returns>
        public string getScriptReturnWithParameters(string url, Dictionary<string, string> parameters)
        {
            string urlToRun = url;
            bool firstParam = true;

            if (parameters.Count > 0)
            {
                foreach (KeyValuePair<string, string> parameter in parameters)
                {
                    if (firstParam)
                    {
                        urlToRun += "?" + parameter.Key + "=" + parameter.Value;
                        firstParam = false;
                    }
                    else
                    {
                        urlToRun += "&" + parameter.Key + "=" + parameter.Value;
                    }
                }
            }

            return getScriptReturn(urlToRun);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ today I coded a simple [...] class [...] to do simple things - do you mind telling us something about those simple things? what is it supposed to do? what results to you expect? \$\endgroup\$ – t3chb0t Jul 5 '16 at 7:35
  • \$\begingroup\$ Its used to report crashes and bug reports, and also it it used to retrieve certain settings about the project, I'll also probably implement a auto-update system in the future using them methods to check if a new version is even avalible (For example: providing the current version as a parameter and comparing it against the latest version). \$\endgroup\$ – Liam Hardy Jul 5 '16 at 7:46
1
\$\begingroup\$
  • both methods, runScriptWithParameters and getScriptReturnWithParameters have a lot of code in common which should be extracted to a separate method.
  • using string concatenation by using += will create each time a new string because strings are immutable. A better way would be to use a StringBuilder instead.
  • based on the .NET naming guidelines methods should be named using PascalCase casing.
  • the check for if (parameters.Count > 0) is not needed, because if you get the enumerator and there are no items to enumerate over, the loops is just ending.
  • you don't do any null validation on the methods parameters of your public methods. Omitting these will leak implementation details.
  • you should use the var type whenever the type is obvious form the right hand of the assignment.

Implementing some of the mentioned points will lead to

private string ComposeUrlParameters(Dictionary<string, string> parameters)
{
    var builder = new StringBuilder(1024);
    var firstParameter = true;
    foreach (var parameter in parameters)
    {
        if (firstParameter)
        {
            builder.Append("?");
            firstParameter = false;
        }
        else
        {
            builder.Append("&");
        }
        builder.AppendFormat("{0}={1}",parameter.Key, parameter.Value);
    }

    return builder.ToString();
}  

which then can be called like so

public void RunScriptWithParameters(string url, Dictionary<string, string> parameters)
{
    if (url == null) { throw new ArgumentNullException("url"); }
    if (parameters == null) { throw new ArgumentNullException("parameters"); }  

    var urlToRun = url + ComposeUrlParameters(parameters);
    RunScript(urlToRun);
}

You could compose the url parameters with linq as well like so

private string ComposeUrlParameters(Dictionary<string, string> parameters)
{
    if (parameters.Count == 0)
    {
        return string.Empty;
    }

    var builder = new StringBuilder(1024);
    var firstParameter = parameters.First();

    builder.AppendFormat("?{0}={1}",firstParameter.Key, firstParameter.Value);

    foreach (var parameter in parameters.Skip(1)
    {
        builder.AppendFormat("&{0}={1}",parameter.Key, parameter.Value);
    }

    return builder.ToString();
}  
\$\endgroup\$
  • 1
    \$\begingroup\$ parameters.First => parameters.First() \$\endgroup\$ – johnny 5 Jul 5 '16 at 14:11

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.