Extracting parameters from request URL path components using regexes

Question

The code is not efficient, let's say the url matched in the first regex case but I'm still evaluating second regex for the second case while knowing it's mutually exclusive and only one of them will be true. The efficiency will further lower if I add more urls to match against.

Also is there a way without introducing another boolean variable to eloquently handle these cases?

public void service(ServiceRequest serviceRequest, ServiceProxy serviceProxy) {
    HttpRequest request = serviceRequest.HttpRequest;
    HttpResponse response = serviceRequest.HttpResponse;

    Match match;
    bool found = false;

    match = Regex.Match(request.Path.Value, @"/articles/(\d+)/thumbnail"); // case "/articles/:id/thumbnail":
    if(match.Success) {
        found = match.Success;
        if(request.Method.Equals(HttpMethod.Get.ToString())) {
            serviceRequest.Params.Add("id", match.Groups[1].Value);
            serviceRequest.ResultData = serviceProxy.GetArticleThumbnail(serviceRequest);
        } else {
            throw new HttpException(HttpStatusCode.MethodNotAllowed, "Method Not Allowed");
        }
    }

    match = Regex.Match(request.Path.Value, @"/leads/(\d+)/collections/(.*)"); // case "/leads/:id/collections/:entityName": // share
    if(match.Success) { 
        found = match.Success;
        if(request.Method.Equals(HttpMethod.Put.ToString())) {
            serviceRequest.Params.Add("id", match.Groups[1].Value);
            serviceRequest.Params.Add("entityName", match.Groups[2].Value);
            serviceRequest.ResultData = serviceProxy.ShareCollection(serviceRequest);
        } else {
            throw new HttpException(HttpStatusCode.MethodNotAllowed, "Method Not Allowed");
        }   
    }

    if(!found) throw new HttpException(HttpStatusCode.NotFound, "Not Found"); // default case
}

Answer 1

Added if else branching structure using to avoid repeating regex's.

Match match = Regex.Match(request.Path.Value, @"/articles/(\d+)/thumbnail"); // case "/articles/:id/thumbnail":
if(match.Success) {
    if(request.Method.Equals(HttpMethod.Get.ToString())) {
        serviceRequest.Params.Add("id", match.Groups[1].Value);
        serviceRequest.ResultData = serviceProxy.GetArticleThumbnail(serviceRequest);
    } else {
        throw new HttpException(HttpStatusCode.MethodNotAllowed, "Method Not Allowed");
    }
} else if ((match = Regex.Match(request.Path.Value, @"/leads/(\d+)/collections/(.*)")) != null && match.Success) {
    if(request.Method.Equals(HttpMethod.Put.ToString())) {
        serviceRequest.Params.Add("id", match.Groups[1].Value);
        serviceRequest.Params.Add("entityName", match.Groups[2].Value);
        serviceRequest.ResultData = serviceProxy.ShareCollection(serviceRequest);
    } else {
        throw new HttpException(HttpStatusCode.MethodNotAllowed, "Method Not Allowed");
    }
} else {
    throw new HttpException(HttpStatusCode.NotFound, "Not Found"); // none of the cases above
}

Answer 2

In your own answer you already suggested the use of if - else if - else, but I find the way you wrote the conditions quite hard to read. Instead I suggest to save the matches in different variables, rather than overwriting the same variable.

Match matchThumbnail = Regex.Match(request.Path.Value, @"/articles/(\d+)/thumbnail");
Match matchCollections = Regex.Match(request.Path.Value, @"/leads/(\d+)/collections/(.*)");

This way, you can use them in the if-conditions without the risk of confusing the values due to them sharing the same variable name:

if (matchThumbnail.Success)
{
    // ...
}
else if (matchCollections.Success)
{
    // ...
}
else
{
    throw new HttpException(HttpStatusCode.NotFound, "Not Found");
}

For the exception condition, you can now use both in one condition. In your own answer, I think you might have a bug since when the first match is successful, the second one is not checked due to the else-if. (Edit: You commented that the regular expressions are mutually exclusive, so I changed the above to an if - else if - else.

---

I recommend using the C#-typical bracing style:

if (something)
{
    // Code
}

instead of

if (something) {
    // Code
}

Also I suggest using braces even for oneliners, as in the last line of your code. Even then I would use the bracing style shown above, but if you want to make a one-liner, putting braces within the same line makes it still more readable:

if (something) { // Code }

instead of

if (something) // Code

---

In C# the preferred way of checking string equality is using == instead of myString.Equals("");.

if (request.Method == HttpMethod.Get.ToString())

is usually considered more readable than

if(request.Method.Equals(HttpMethod.Get.ToString()))

---

// case "/articles/:id/thumbnail":

// default case

These comments do not add much information that is not already in the code, so they are redundant.

// case "/leads/:id/collections/:entityName": // share

The same applies here, however that // share is even confusing, as it is not obvious to me what it is supposed to tell about the code, and the comment in a comment is also weird. Maybe it was on a different line before.

In general I recommend writing comments in plain english on their own line before the code they are refering to.