COMpatible+ Google+ Integrations+ Scoping+

Question

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 & GooglePermissionScopes.Email) == GooglePermissionScopes.Email)
 {
 if (endpoint.Length > 6)
 {
 endpoint += "%20";
 }
 endpoint += "email";
 }
 if ((googlePermissionScopes & GooglePermissionScopes.PlusLogin) == GooglePermissionScopes.PlusLogin)
 {
 if (endpoint.Length > 6)
 {
 endpoint += "%20";
 }
 endpoint += "https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fplus.login";
 }
 }
 if (state != "")
 {
 endpoint = endpoint + "&state=" + state;
 }
 endpoint = endpoint + "&redirect_uri=" + CallbackUrl + "&response_type=code" + "&client_id=" + ClientId + "&access_type=offline";
 if (force)
 {
 endpoint += "&approval_prompt=force";
 }
 return endpoint;
}

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

Answer 1

COM_GetLoginStatus()

This method is doing to much IMO and the naming isn't good either. It doesn't return a LoginStatus but it checks if a call to GetLoginStatus() returns LoginStatus.Success. So maybe naming this method COM_CheckLoginSuccess() would be better.

IMO you should extract the parsing of the string scopes argument to GooglePermissionScopes to a separate method.

If the optional parameter scopes is to be meant that it always should represent the all enum value and there won't be passed "" or string.Empty it would be better to just set the default value to all.

By doing all this the former COM_GetLoginStatus() would look like so

public bool COM_CheckLoginSuccess(ref GoogleIdentity identity, WebBrowserSettings settings, bool force, string scopes = "all")
{
 GooglePermissionScopes googlePermissionScopes = ParsePermissionScope(scopes);
 return GetLoginStatus(ref identity, settings, force, googlePermissionScopes) == LoginStatus.Success;
} 

The to be introduced method ParsePermissionScope() should then take advantage of the Enum.TryParse() method like so

private GooglePermissionScopes ParsePermissionScope(string scopes)
{
 GooglePermissionScopes googlePermissionScopes;
 if (!Enum.TryParse(scopes, true, out googlePermissionScopes))
 {
 googlePermissionScopes = GooglePermissionScopes.None;
 }
 return googlePermissionScopes;
}

---

GetLoginUrl()

I second @janos opinion about beeing string endpoint = "https://accounts.google.com/o/oauth2/auth?"; to be a constant or to make the class a little bit more configurable a property. If this url some time will change it would be a good idea to use a property.

If the GooglePermissionScopes enum has the [Flags] attribute a more obvious way would be to use the HasFlags() method to determine if a specific enum value is contained in the enum.

These checks if (endpoint.Length > 6) are senseless and can be removed because endpoint is initialized to a string with a Length> 6.

Because we are getting older each day and our eyes aren't getting better I prefer to not compare a varaible to "" but to string.Empty.

About the StringBuilder or +, well you say it is no loop, but in the worst case it acts like it is a loop from 0 to 8 because in the worst case you are doing endpoint += someString 9 times.

Personally I would go with the StringBuilder which would lead (assuming the Flags attribute) to

public string LoginUrl { get; private set; } = "https://accounts.google.com/o/oauth2/auth?";
public string GetLoginUrl(GooglePermissionScopes googlePermissionScopes = GooglePermissionScopes.None, bool force = false, string state = "")
{
 StringBuilder urlBuilder = new StringBuilder(LoginUrl, 1024);
 if (googlePermissionScopes != GooglePermissionScopes.None)
 {
 urlBuilder.Append("scope=");
 if ((googlePermissionScopes.HasFlag(GooglePermissionScopes.OpenID)))
 {
 urlBuilder.Append("openid");
 }
 if ((googlePermissionScopes.HasFlag(GooglePermissionScopes.Email)))
 {
 urlBuilder.Append("%20email");
 }
 if ((googlePermissionScopes.HasFlag(GooglePermissionScopes.PlusLogin)))
 {
 urlBuilder.Append("%20https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fplus.login");
 }
 }
 if (state != string.Empty)
 {
 urlBuilder.Append("&state=")
 .Append(state);
 }
 urlBuilder.Append("&redirect_uri=")
 .Append(CallbackUrl)
 .Append("&response_type=code&client_id=")
 .Append(ClientId)
 .Append("&access_type=offline");
 if (force)
 {
 urlBuilder.Append("&approval_prompt=force");
 }
 return urlBuilder.ToString(); ;
}

• Der Kommissar: You and janos made me feel like an idiot about that endpoint.Length > 6 bit...I don't know how or why it's there but that's all wrong. :)

Answer 2

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.

• Der Kommissar: I'm sure you didn't mean to, but you made me feel like an idiot about that endpoint.Leng > 6 bit...I'm not sure why or how that got there, but it makes no sense in my code. :)
• janos: I didn't mean to, of course :) But now having seen Heslacher's point, I feel like an idiot for not noticing that the statement doesn't need an explanation, but in fact it needs to go away completely...

Answer 3

In addition to what has been said by the others, and noticing 3 distinct blocks of code in the original method, I'd suggest to split GetLoginUrl into distinct methods, and also use string.Format where possible (for readability's sake, being that it changes very little from the performance point of view, unless - of course - this method is called a lot of times). So, the original method becomes something like the following (this is not tested):

public string GetLoginUrl(GooglePermissionScopes googlePermissionScopes = GooglePermissionScopes.None, bool force = false, string state = "")
{
 // this should be defined somewhere else IMO
 return string.Format("https://accounts.google.com/o/oauth2/auth?{0}{1}{2}",
 GetPermissionScopesURIAddition(googlePermissionScopes),
 GetStateURIAddition(state),
 GetForceURIAddition(force))
}
private string GetPermissionScopesURIAddition(GooglePermissionScopes googlePermissionScopes)
{
 string result = string.Empty;
 if (googlePermissionScopes != GooglePermissionScopes.None)
 {
 result += "scope=";
 if ((googlePermissionScopes & GooglePermissionScopes.OpenID) == GooglePermissionScopes.OpenID)
 {
 result += "openid";
 }
 if ((googlePermissionScopes & GooglePermissionScopes.Email) == GooglePermissionScopes.Email)
 {
 result += "%20";
 result += "email";
 }
 if ((googlePermissionScopes & GooglePermissionScopes.PlusLogin) == GooglePermissionScopes.PlusLogin)
 {
 result += "%20";
 result += "https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fplus.login";
 }
 }
 return result;
}
private string GetStateURIAddition(string state)
{
 string result = string.Empty;
 if (state != "")
 {
 result += "&state=" + state;
 }
 // format this using string.Format
 return result + "&redirect_uri=" + CallbackUrl + "&response_type=code" + "&client_id=" + ClientId + "&access_type=offline";
}
private string GetForceURIAddition(bool force)
{
 if (force)
 {
 return "&approval_prompt=force";
 }
 return string.Empty;
}

As a side note: I'd use StringBuilder anyway, even if performance does not improve much on small numbers, but this can be seen as a personal preference.