8
\$\begingroup\$

I was wondering if I could get some pointers on how I can improve my library, whether it be design pattern/naming convention/or any ideas on a better implementation.

GitHub

As it stands, I am using a builder pattern, so you'd go:

var sum = new Summary();
sum.ApiKey(_apikey).Url(_url);

Then you would get the JSON which includes the summary of the webpage like so sum.GetJSON()

Here is the code:

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

namespace SummaryLib
{
public class Parameters
{
    public string ApiKey { get; set; }
    public string Url { get; set; }
    public int? SentenceCount { get; set; }
    public int? KeywordCount { get; set; }
    public bool? IncludeQuotes { get; set; }
    public bool? IncludeBreaks { get; set; }     
}

public class Summary
{
    private Parameters _parameters = new Parameters();

    #region Builder
    public Summary ApiKey(string _apikey)
    {
        _parameters.ApiKey = _apikey;
        return this;
    }
    public Summary Url(string _url)
    {
        _parameters.Url = _url;
        return this;
    }
    public Summary SentenceCount(int _sentencecount)
    {
        _parameters.SentenceCount = _sentencecount;
        return this;
    }
    public Summary KeywordCount(int _keywordcount)
    {
        _parameters.KeywordCount = _keywordcount;
        return this;
    }
    public Summary IncludeQuotes(bool _includequotes)
    {
        _parameters.IncludeQuotes = _includequotes;
        return this;
    }
    public Summary IncludeBreaks(bool _includebreaks)
    {
        _parameters.IncludeBreaks = _includebreaks;
        return this;
    }
    #endregion


    public async Task<string> GetJSON()
    {
        StringBuilder url = new StringBuilder($@"http://api.smmry.com");
        url.Append(UrlBuilder(UrlParameters()));

        using (var client = new HttpClient())
        using (var responsemessage = await client.GetAsync(url.ToString()))
        using (var content = responsemessage.Content)
        {
            return await content.ReadAsStringAsync();
        }
    }

    public List<string> UrlParameters()
    {
        var parameterList = new List<string>();

        foreach (var item in _parameters.GetType().GetProperties())
        {
            if (item.GetValue(_parameters) != null) parameterList.Add(item.Name);
        }

        var translator = new Dictionary<string, string>()
        {
            {"ApiKey","SM_API_KEY" },
            {"Url", "SM_URL" },
            {"SentenceCount", "SM_LENGTH"},
            {"KeywordCount", "SM_KEYWORD_COUNT"},
            {"IncludeQuotes","SM_QUOTE_AVOID" },
            {"IncludeBreaks","SM_WITH_BREAK" }
        };

        var parsedUrlParameters = new List<string>();

        foreach (var item in parameterList)
        {
            parsedUrlParameters.Add(translator.FirstOrDefault(x => x.Key == item).Value);
        }

        return parsedUrlParameters;

    }
    public string UrlBuilder(List<string> urlParameters)
    {
        var urlDictionary = new Dictionary<string, object>()
        {
            {"SM_API_KEY", _parameters.ApiKey},
            { "SM_URL", _parameters.Url },
            {"SM_LENGTH", _parameters.SentenceCount},
            {"SM_KEYWORD_COUNT", _parameters.KeywordCount},
            {"SM_QUOTE_AVOID", _parameters.IncludeQuotes},
            {"SM_WITH_BREAK", _parameters.IncludeBreaks }
        };

        StringBuilder builder = new StringBuilder($"?");
        foreach (var item in urlDictionary)
        {
            if(item.Value != null) builder.Append($"{item.Key}={item.Value}&");
        }           
        return builder.ToString();
    }

}
}

Explanation of the code:

The UrlParameters method uses reflection to get a list of which parameters were provided and assign the proper url variables (example:"SM_API_KEY","SM_URL" ect...).

The UrlBuilder method will create the actual url by including the variables from the UrlParameters function and the variables inputted by the user.

Example query

My rubber duck told me that I don't need the UrlParameters method to make this work. Regardless, I still feel like I shouldn't use a dictionary like I have in UrlBuilder.

\$\endgroup\$
1
  • \$\begingroup\$ Congratulations on fixing your question :-) \$\endgroup\$
    – Mast
    Commented Mar 21, 2017 at 7:47

2 Answers 2

2
\$\begingroup\$

Improvables

  • Parameters - the class name is too general, it should be SmmryParameters
  • Summary - this class is doing way to much. It not only builds the query string but also downloads the data. You need to separate it.
  • Parameter names and query string names should be closer together, I mean use attributes to specify query string names. You also repeat each property name as a dictionary keys as a string. This is a really bad idea. You should at least use the nameof.

Example

Here's how it could look like and let's with the attribute that will store the query string name of one parameter:

[AttributeUsage(AttributeTargets.Property)]
class SmmryParameterAttribute : Attribute
{
    private readonly string _name;
    public SmmryParameterAttribute(string name) { _name = name; }
    public override string ToString() => _name;
}

Then you decorate each property with this attribute:

public class SmmryParameters : Dictionary<string, object>
{
    [SmmryParameter("SM_API_KEY")]
    public string ApiKey
    {
        get => (string)this[GetSmmryParameterName()];
        set => this[GetSmmryParameterName()] = value;
    }

    [SmmryParameter("SM_URL")]
    public string Url
    {
        get => (string)this[GetSmmryParameterName()];
        set => this[GetSmmryParameterName()] = value;
    }

    [SmmryParameter("SM_LENGTH")]
    public int? SentenceCount
    {
        get => (int?)this[GetSmmryParameterName()];
        set => this[GetSmmryParameterName()] = value;
    }

    [SmmryParameter("SM_KEYWORD_COUNT")]
    public int? KeywordCount
    {
        get => (int?)this[GetSmmryParameterName()];
        set => this[GetSmmryParameterName()] = value;
    }

    [SmmryParameter("SM_QUOTE_AVOID")]
    public bool? IncludeQuotes
    {
        get => (bool?)this[GetSmmryParameterName()];
        set => this[GetSmmryParameterName()] = value;
    }

    [SmmryParameter("SM_WITH_BREAK")]
    public bool? IncludeBreaks
    {
        get => (bool?)this[GetSmmryParameterName()];
        set => this[GetSmmryParameterName()] = value;
    }

    private string GetSmmryParameterName([CallerMemberName] string propertyName = null)
    {
        return
            GetType()
                .GetProperty(propertyName)
                .GetCustomAttribute<SmmryParameterAttribute>()
                .ToString();
    }

    public override string ToString() => $"?{string.Join("&", this.Select(item => $"{item.Key}={item.Value}&"))}";
}

At the same time you use a little compile trick with the CallerMemberName to get the property name and based on this the actual query string name that will be the key of the dictionary that the SmmryParameters class is derived from.

It also overrides the ToString method to generate the query string.

Lastly you need a downloader to get the data:

public class SmmryDownloader
{
    public async Task<string> GetJsonAsync(SmmryParameters smmryParameters)
    {
        StringBuilder url = new StringBuilder($@"http://api.smmry.com");
        url.Append(smmryParameters);

        using (var client = new HttpClient())
        using (var responsemessage = await client.GetAsync(url.ToString()))
        using (var content = responsemessage.Content)
        {
            return await content.ReadAsStringAsync();
        }
    }
}

Notice that the method's name is now GetJsonAsync because by convention we add the Async suffix to async methods.

This is not a bullet-proof solution if you want to access the properties manually because it will throw if a key does not exist. You might want to use TryGetValue if you need to but it'll do for the current scenario otherwise you should implement the getters this way:

get => TryGetValue(GetSmmryParameterName(), out object value) ? (string)value : default(string);

Usage

var smmryParams = new SmmryParameters
{
    ApiKey = "123",
    Url = "https://en.wikipedia.org/wiki/Augustus"
};

var smmryDownloader = new SmmryDownloader();
json = smmryDownloader.GetJsonAsync(smmryParams);

Testing

In order to be able to test it you can create an interface for the downloader and use it to create another mock-downloader.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you I'll try to use that! Time to learn attributes :D \$\endgroup\$
    – ghostiek
    Commented Mar 24, 2017 at 7:46
0
\$\begingroup\$

Url Builder is may be like this..

    public string UrlBuilder()
    {
        var translator = new Dictionary<string, string>()
        {
            {"ApiKey","SM_API_KEY" },
            {"Url", "SM_URL" },
            {"SentenceCount", "SM_LENGTH"},
            {"KeywordCount", "SM_KEYWORD_COUNT"},
            {"IncludeQuotes","SM_QUOTE_AVOID" },
            {"IncludeBreaks","SM_WITH_BREAK" }
        };
        var  parsedUrlParameters = _parameters.GetType()
            .GetProperties()
            .Where(p => p.GetValue(_parameters) != null)
            .Select(p => string.Format("{0}={1}", translator[p.Name], p.GetValue(_parameters)))
            .ToArray();
        return "?"+ string.Join("&", parsedUrlParameters);

    }

update:

You do not really need both UrlParameters() and Parameters class. You can handle with Dictionar

public class Summary
{
//private Parameters _parameters = new Parameters();
private Dictionary<string, object> _parameters = new Dictionary<string, object>();

#region Builder
public Summary ApiKey(string _apikey)
{
    _parameters["SM_API_KEY"] = _apikey;
    return this;
}
public Summary Url(string _url)
{
    _parameters["SM_URL"] = _url;
    return this;
}
public Summary SentenceCount(int _sentencecount)
{
    _parameters["SM_LENGTH"] = _sentencecount;
    return this;
}
public Summary KeywordCount(int _keywordcount)
{
    _parameters["SM_KEYWORD_COUNT"] = _keywordcount;
    return this;
}
public Summary IncludeQuotes(bool _includequotes)
{
    _parameters["SM_QUOTE_AVOID"] = _includequotes;
    return this;
}
public Summary IncludeBreaks(bool _includebreaks)
{
    _parameters["SM_WITH_BREAK"] = _includebreaks;
    return this;
}
#endregion


public async Task<string> GetJSON()
{
    StringBuilder url = new StringBuilder("http://api.smmry.com");
    url.Append(UrlBuilder());

    using (var client = new HttpClient())
    using (var responsemessage = await client.GetAsync(url.ToString()))
    using (var content = responsemessage.Content)
    {
        return await content.ReadAsStringAsync();
    }
}


public string UrlBuilder()
{
    var parsedUrlParameters = _parameters
            .Select(p => string.Format("{0}={1}", p.Key, p.Value))
            .ToArray();
    return "?" + string.Join("&", parsedUrlParameters);
}
}
\$\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.