1
\$\begingroup\$

I recently created a license manager for my publicly released project and I just wondered how I could improve it.

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

namespace Kiwi.Application.Base.License
{
    sealed class LicenseManager
    {
        private string licUsername;
        private string licPassword;
        private string licApi;

        public LicenseManager()
        {
            licApi = "http://example.com/licenseCheck.php?u={0}&p={1}";
        }

        public void assignLicenseDetails(string licenseUsername, string licensePassword)
        {
            licUsername = licenseUsername;
            licPassword = licensePassword;
        }

        public WebClient generateNewClient()
        {
            return new WebClient();
        }

        public bool verifyLicense()
        {
            return generateNewClient().DownloadString(string.Format(licApi, licUsername, licPassword)).Equals("license_okay");
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ This feels very hypothetical, and to be honest, there's not much here for us to review... Can you give us more information on the transaction at example.com/licenseCheck.php? \$\endgroup\$ – Quill Dec 2 '15 at 1:32
5
\$\begingroup\$

Naming.

"Manager" smells. It's like "Helper": it doesn't tell anything about the purpose of the type, and before you know it it's stuffed with "helper" methods that have nothing to do with each other - and that decreases cohesion. Naming matters.

And naming conventions too: there's a reason every single type and method in the .NET framework is in PascalCase. The camelCase of the public methods clashes with what our eyes are used to see, and it's like we don't see sharp anymore, and start needing glasses (you do know why Java programmers need glasses right?).


Interface.

Classes describe types; instances of a type are objects, and objects expose an interface. I mean that kind of interface - not to be confused with interface. So, the interface then.

LicenseManager()
void assignLicenseDetails(string licenseUsername, string licensePassword)
WebClient generateNewClient()
bool verifyLicense()

By looking at the implementation, we can see that there's temporal coupling here: there's only a specific order these methods can be called in for the thing to make sense.

The private fields are only ever used in verifyLicense() - why are they scoped at class level? Why does generateNewClient() need to be public?


Implementation.

WebClient implements IDisposable, so if you're not wrapping it in a using block, you need to call its Dispose method explicitly.

Injecting the query string parameters in the url like this, with string.Format, feels wrong at worst and clumsy at best.

This would be a more idiomatic way (see this SO answer):

string AddQueryStringParameters(string url, IDictionary<string,string> parameters)
{
    var builder = new UriBuilder(url);
    builder.Port = -1;
    var query = HttpUtility.ParseQueryString(builder.Query);

    foreach(var parameter in parameters)
    {
        query[parameter.Key] = parameter.Value;
    }

    builder.Query = query.ToString();
    return builder.ToString();
}

KISS. Keep it stupid simple. What's the task at hand here? Pass 2 strings into a url querystring and determine the return value by validating the returned string.

public static bool VerifyLicense(string userName, string password)
{
    var parameters = new Dictionary<string,string>
        {
            { "u", userName },
            { "p", password }
        };

    var url = AddQueryStringParameters("http://example.com/licenseCheck.php", parameters);

    using(var client = new WebClient())
    {
        return client.DownloadString(url) == "license_okay";
    }
}

Well look at that. Now we make the class static and then the public interface looks like this:

static bool VerifyLicense(string userName, string password)

Yes, that's all. No more temporal coupling, and knowing which method to call for doing what, is a no-brainer. And since there's no state involved, the method is static and thread-safe.

Your implementation could run into problems if two or more threads called methods on the same instance: there's no telling whether another thread changed the value of the private fields between the moment a thread set them and the moment that thread reads them before making the web request.


Security.

A web API that requires its clients to pass credentials into the querystring like that is completely irresponsible. These things belong in a HTTP POST request, and your code sends them over a HTTP GET, 1996-style. If you own the web API, it needs a serious look-into. If you don't, ...seriously consider switching to a more secure API.

| improve this answer | |
\$\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.