COMpatible+ Google+ Integrations+ Scoping+

So I have part of a library which deals with scopes in Google+ integration, and I'm curious on the overall view of it.

Yes, things are stringly typed in one of the methods, that is because this has to be COMpatible, which does not support enum's.

I have (purposefully) omitted XML documentation, as most people think I go overboard and I'd rather not bloat.

public bool COM_GetLoginStatus(ref GoogleIdentity identity, WebBrowserSettings settings, bool force, string scopes = "")
{
    GooglePermissionScopes googlePermissionScopes = GooglePermissionScopes.None;

    scopes = scopes.ToLower();

    if (scopes == "")
    {
        scopes = "all";
    }

    foreach (string scope in scopes.Split(','))
    {
        switch (scope)
        {
            case "openid":
                googlePermissionScopes |= GooglePermissionScopes.OpenID;
                break;
            case "email":
                googlePermissionScopes |= GooglePermissionScopes.Email;
                break;
            case "plus.login":
            case "pluslogin":
                googlePermissionScopes |= GooglePermissionScopes.PlusLogin;
                break;
            case "all":
                googlePermissionScopes |= GooglePermissionScopes.All;
                break;
        }
    }

    return GetLoginStatus(ref identity, settings, force, googlePermissionScopes) == LoginStatus.Success;
}

The next method is responsible for building an endpoint to contact the API at.

```
public string GetLoginUrl(GooglePermissionScopes googlePermissionScopes = GooglePermissionScopes.None, bool force = false, string state = "")
{
string endpoint = "https://accounts.google.com/o/oauth2/auth?";

if (googlePermissionScopes != GooglePermissionScopes.None)
{
endpoint += "scope=";

if ((googlePermissionScopes & GooglePermissionScopes.OpenID) == GooglePermissionScopes.OpenID)
{
endpoint += "openid";
}

if ((googlePermissionScopes & GooglePer

In COM_GetLoginStatus,
you can get rid of the if (scopes == "") by incorporating it in case "all" in the switch:

case "":
        case "all":
            googlePermissionScopes |= GooglePermissionScopes.All;
            break;

This is probably good to move to a constant declared at the top of the file where it's easy to see:

string endpoint = "https://accounts.google.com/o/oauth2/auth?";

If the values in GooglePermissionScopes have distinct bits,
then instead of this:

if ((googlePermissionScopes & GooglePermissionScopes.OpenID) == GooglePermissionScopes.OpenID)

You could write simpler like this:

if ((googlePermissionScopes & GooglePermissionScopes.OpenID) > 0)

But then again, I'm not familiar with GooglePermissionScopes so I don't know if this is correct, or acceptable.

It's not clear what magic number 6 is about here:

if (endpoint.Length > 6)
        {
            endpoint += "%20";
        }

It would be good to make that clear somehow, at the minimum with a comment.

As @Heslacher pointed out, endpoint is initialized to a string longer (much longer) than 6 characters, so this condition is completely pointless.

This can be written simpler using +=:

endpoint = endpoint + "&state=" + state;

This can be similarly simplified,
and "&response_type=code" + "&client_id=" can be joined:

endpoint = endpoint + "&redirect_uri=" + CallbackUrl + "&response_type=code" + "&client_id=" + ClientId + "&access_type=offline";

As for your question:


I'm not sure if I should be using a StringBuilder here or not, since it's not a looped string concatenation.

Since it's not in a loop, I doubt the performance difference would be measurable. I think it doesn't matter. It's fine to keep it simple.

You wrote URL-encoded strings at multiple places, for example %20 and https%3A%2F%2F.
Writing this by hand is error prone, and a PITA.
It would be better to write in human-readable format,
and call a url-encoder method at the end.
Granted, the current approach is better in terms of "performance",
but again, it's probably not measurable and not worth it.

StackExchange Code Review Q#106987, answer score: 3