3
\$\begingroup\$

I have some URLs saved in my database, to download pictures from network cameras. Because each camera has it's own URL and parameters, we have to store the entire URL in a field in the database.

My code gets this URL from the database, creates a WebClient to make the call and downloads the byte array from it.

Some cameras use security credentials that we save as parameters in the URL, they are saved like this because it was determined it would be this way, I would have preferred to have fields in the database with username and password, but they decided to go this way, so my code got more messy.

Here is the part of the code that deals with the credentials:

var uri = new Uri(url);
if (uri.Query.Length > 0)
{
    var querys = uri.Query;
    if (querys.Contains("username") && querys.Contains("password"))
    {
        var userName = querys.Split('&').SkipWhile(x => !x.StartsWith("username")).First().Split('=').Last();
        var password = querys.Split('&').SkipWhile(x => !x.StartsWith("password")).First().Split('=').Last();
        var type = querys.Contains("type") ? querys.Split('&').SkipWhile(x => !x.StartsWith("type")).First().Split('=').Last() : "Digest";

        cache = new CredentialCache { { uri, type, new NetworkCredential(userName, password) } };
    }
}

Later on the code I will use the cache variable in the WebClient to make the call with it.

I used a lot of LINQ here to get the username and password from the URL.

My questions here are:

  • Is my code readable enough?
  • Is it the best approach? If not, how could I have made this better?
  • Should I be worried about performance in this implementation?

Here is an example of the URL:

http://192.168.2.33/cgi-bin/video.cgi?msubmenu=jpg&username=admin&password=4321&type=Digest
\$\endgroup\$
  • \$\begingroup\$ Comments are not for extended discussion; this conversation has been moved to chat. \$\endgroup\$ – Mathieu Guindon Dec 8 '17 at 12:46
4
\$\begingroup\$

Is my code readable enough?

Unfortunatelly there is no objective answer to this question. Whether code is readable is mainly a personal opinion. I think you code isn't readable because it calls a lot of methods one after anthoer in a single line. I find it's hard to tell where one ends and the other starts.

So I suggest adding a few line breaks to clearly separate all calls:

var userName = 
    query
        .Split('&')
        .SkipWhile(x => !x.StartsWith("username"))
        .First()
        .Split('=')
        .Last();

If you wrote it like this from the beginning it would also make your code much longer because you use very similar code in several places. This would probaly make you think does it have to be this way? and make you look for a different solution. This new solution is called encapsulation.

You don't want to have anything more the once. But more about this under the next question.


Is it the best approach? if not how could i have made this better?

You need to define what best means to you.

  • Is the code best when it's short or when it's testable?
  • Is it best when it is quick&dirty written or is it best when it's maintainable?
  • Is it best when it's everything in one place or is is it best if you have dedicated modules for everything and use a lot of dependency injection?

My No. 1 definition of best (production) code is: testable + maintainable (this does not necessarily mean short and/or simple). To achieve this two goals you code needs to be at least DRY and it should satisfy the Single Responsibility Principle.

This will make sure you can test each algorithm only once and if you find a bug you need to fix it in only one place.

Currently your code is not DRY. It repeats the same logic several times (parsing). It's also does not satisfy the SRP because it parses the query string and fills the cache by the same method.

You should encapsulate the parsing either by a helper method if this is the only place you need it or write a dedicated class and use it as a service with Dependency Incjection.

If you have everything already properly encapsulated you can always upgrade the code to a service if you need it later somewhere else. Why should you implement it again? I wouldn't, I'm lazy :-)


Your code could e.g. parse the query string into a dictionary which is much easier to use then always run through the entire string (worst case scenario) with Contains. Parse it once and reuse the results. I would, I'm lazy - but lazy is a good thing - it makes you write DRY code.

var query = 
    uri
        .Query
        .TrimStart('?')
        .Split('&')
        .Select(x => x.Split('='))
        .ToDictionary(
            x => x[0], 
            x => x.ElementAtOrDefault(1)
        );

I don't know if you always have pairs but in case you don't, the ElementAtOrDefault will make this not crash if a value is missing.


Should i be worried with performance in this implementation?

This question can be answered only by you. You need to know how often it's called and whether it might become a potential bottleneck. But I'm pretty sure you won't see it in any benchmarks. There are parts in your code that are with certainty much slower. The best way to tell whether your code needs optimization and where, is to use a profiler.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Comments are not for extended discussion; this conversation has been moved to chat. \$\endgroup\$ – Mathieu Guindon Dec 8 '17 at 18:23
0
\$\begingroup\$

Update: Ok based on the previous answer i made this adjustment.

var pairs = querys.TrimStart('?').Split('&');
var userName = pairs.SkipWhile(x => !x.StartsWith("username")).First().Split('=').Last();
var password = pairs.SkipWhile(x => !x.StartsWith("password")).First().Split('=').Last();
var type = querys.Contains("type") ? pairs.SkipWhile(x => !x.StartsWith("type")).First().Split('=').Last() : "Digest";

Now i am not parsing the entire string 3 times.

Thank you for the feedback.

|improve this answer|||||
\$\endgroup\$
0
\$\begingroup\$

Funny thing, yesterday I rewrote someome else's URL parser to use an object-based paradigm.

Is my code readable enough?

The repetitions in the parsing could be simplified

Is it the best approach? If not, how could I have made this better?

When I'm working in C# I make an effort to stick with the object paradigm, i.e. "Let objects do the work." The original code could be considered more of a procedural approach to solving the problem. The code below shows my interpretation of an object-based approach, and I welcome your feedback.

Should I be worried about performance in this implementation?

The call out to the camera and downloading of the pictures will take orders of magnitude longer than parsing the string. Unless you're doing "high volume" URL parsing, the performance for this particular section of the code is of little concern.

Below is the code. Please note that I left in the method ValueAsInt to show that we can give the Parameter object primary responsibility for providing its data in different formats.

To run it:

var app = new StackExchange.App_UrlParser("http://192.168.2.33/cgi-bin/video.cgi?msubmenu=jpg&username=admin&password=4321&type=Digest");

app.Run();

using System;
using System.Linq;

namespace StackExchange
{
    //https://codereview.stackexchange.com/questions/182254/query-url-parameters-in-downloading-pictures-from-network-cameras/182263#182264
    public class App_UrlParser
    {
        private string url;

        public App_UrlParser(string url)
        {
            this.url = url;
        }

        public void Run()
        {
            var u = new Url(url);
            var user = u.UserName;
            var pass = u.Password;
        }
    }

    public class Url
    {
        private string url;

        private string[] _parameters;
        private string[] parameters
        {
            get
            {
                _parameters = _parameters ?? new Uri(Uri.UnescapeDataString(url)).Query.TrimStart('?').Split('&');
                return _parameters;
            }
        }

        public string UserName
        {
            get
            {
                return getParameterByName("username");
            }
        }

        public string Password
        {
            get
            {
                return getParameterByName("password");                
            }
        }

        public Url(string url)
        {
            this.url = url;
        }

        private string getParameterByName(string name)
        {
            return parameters.Select(p => new Parameter(p)).Where(pp => pp.Key == name).First().Value;
        }
    }

    public class Parameter
    {
        private string param;

        private string[] _tokens;
        private string[] tokens
        {
            get
            {
                _tokens = _tokens ?? param.Split('=');
                return _tokens;
            }
        }

        public string Key { get { return tokens[0]; } }

        public string Value { get { return tokens[1]; } }

        public Parameter(string param)
        {
            this.param = param;
        }
    }
}
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Nice approach, i like it very much. Looks very clean to me and reusable, thank you Aron! \$\endgroup\$ – Zorkind Dec 7 '17 at 21:37
  • \$\begingroup\$ You're welcome. Glad you like it. I wonder who downvoted it... ;-) \$\endgroup\$ – Aron Dec 7 '17 at 21:42
  • \$\begingroup\$ I don't think this is THE solution (because it repeates the same mistakes as the OP code (-1)) but it makes a few valid points in text (+1) so I don't think it actually deserves being downvoted (0)... @Aron, it wasn't me. \$\endgroup\$ – t3chb0t Dec 7 '17 at 21:44
  • \$\begingroup\$ I don't see any down votes, i can't vote up tho :-\ \$\endgroup\$ – Zorkind Dec 7 '17 at 21:44
  • \$\begingroup\$ Thanks @t3chb0t. What mistakes do you see? \$\endgroup\$ – Aron Dec 7 '17 at 21:45
0
\$\begingroup\$

Turns out Microsoft have it's own Parser, so i am going to use it.

System.Web.HttpUtility.ParseQueryString

var querys = uri.Query;
var pairs = System.Web.HttpUtility.ParseQueryString(querys);

var userName = pairs.Get("username");
var password = pairs.Get("password");
var type = pairs.Get("type");

if (type == null) type = "Digest";

if (userName != null && password != null)
    client.Credentials = new CredentialCache { { uri, type, new NetworkCredential(userName, password) } };
else
    client.Credentials = CredentialCache.DefaultCredentials;

It is very much safer because internally it checks for Encodings in the URL.

In my tests it's a bit slower than just splitting everything, but then again, it is a very much more complex implementation and it's not fair to compare it against a simple split.

Here is the official documentation for it.

Official Documentation

|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.