Query URL parameters in downloading pictures from network cameras 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 &gt; 0)
{
    var querys = uri.Query;
    if (querys.Contains("username") &amp;&amp; querys.Contains("password"))
    {
        var userName = querys.Split('&amp;').SkipWhile(x =&gt; !x.StartsWith("username")).First().Split('=').Last();
        var password = querys.Split('&amp;').SkipWhile(x =&gt; !x.StartsWith("password")).First().Split('=').Last();
        var type = querys.Contains("type") ? querys.Split('&amp;').SkipWhile(x =&gt; !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&amp;username=admin&amp;password=4321&amp;type=Digest
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('&amp;')
        .SkipWhile(x =&gt; !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&amp;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('&amp;')
        .Select(x =&gt; x.Split('='))
        .ToDictionary(
            x =&gt; x[0], 
            x =&gt; 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.