Refactoring methods to prevent double loading of data

Operational methods call the validation methods to validate data before they do their job.
The validation methods load the requisite data and validate them.
The operational methods then load the same data items and continue.

In order to reduce the re-querying the validation methods now load the data, validate the data and returns the data if valid.

Refactoring method `LoadKeychainInternal`/`ValidateKeychain` to `GetValidatedKeychainAdaper`
Refactoring method `GetCurrentUserInternal` to `GetValidatedGitHubUser`

Author: StanleyGoldman

src/GitHub.Api/Application/ApiClient.cs

@@ -86,7 +86,9 @@ public async Task ValidateCurrentUser(Action onSuccess, Action<Exception> onErro
             Guard.ArgumentNotNull(onSuccess, nameof(onSuccess));
             try
             {
-                await ValidateCurrentUserInternal();
+                var keychainConnection = keychain.Connections.First();
+                var keychainAdapter = await GetValidatedKeychainAdaper(keychainConnection);
+                await GetValidatedGitHubUser(keychainConnection, keychainAdapter);
                 onSuccess();
             }
             catch (Exception e)
@@ -95,13 +97,6 @@ public async Task ValidateCurrentUser(Action onSuccess, Action<Exception> onErro
             }
         }
 
-        public async Task GetCurrentUser(Action<GitHubUser> callback)
-        {
-            Guard.ArgumentNotNull(callback, "callback");
-            var user = await GetCurrentUserInternal();
-            callback(user);
-        }
-
         public async Task Login(string username, string password, Action<LoginResult> need2faCode, Action<bool, string> result)
         {
             Guard.ArgumentNotNull(need2faCode, "need2faCode");
@@ -204,11 +199,10 @@ private async Task<GitHubRepository> CreateRepositoryInternal(string repositoryN
             {
                 logger.Trace("Creating repository");
 
-                await ValidateKeychain();
-                await ValidateCurrentUserInternal();
-
-                var uriString = keychain.Connections.First().Host;
-                var keychainAdapter = await keychain.Load(uriString);
+                //TODO: ONE_USER_LOGIN This assumes only ever one user can login
+                var keychainConnection = keychain.Connections.First();
+                var keychainAdapter = await GetValidatedKeychainAdaper(keychainConnection);
+                await GetValidatedGitHubUser(keychainConnection, keychainAdapter);
 
                 var command = new StringBuilder("publish -r \"");
                 command.Append(repositoryName);
@@ -233,7 +227,7 @@ private async Task<GitHubRepository> CreateRepositoryInternal(string repositoryN
                     command.Append(" -p");
                 }
 
-                var octorunTask = new OctorunTask(taskManager.Token, nodeJsExecutablePath, octorunScriptPath, command.ToString(), 
+                var octorunTask = new OctorunTask(taskManager.Token, nodeJsExecutablePath, octorunScriptPath, command.ToString(),
                     user: keychainAdapter.Credential.Username, userToken: keychainAdapter.Credential.Token)
                     .Configure(processManager);
 
@@ -267,11 +261,10 @@ private async Task GetOrganizationInternal(Action<Organization[]> onSuccess, Act
             {
                 logger.Trace("Getting Organizations");
 
-                await ValidateKeychain();
-                await ValidateCurrentUserInternal();
-
-                var uriString = keychain.Connections.First().Host;
-                var keychainAdapter = await keychain.Load(uriString);
+                //TODO: ONE_USER_LOGIN This assumes only ever one user can login
+                var keychainConnection = keychain.Connections.First();
+                var keychainAdapter = await GetValidatedKeychainAdaper(keychainConnection);
+                await GetValidatedGitHubUser(keychainConnection, keychainAdapter);
 
                 var octorunTask = new OctorunTask(taskManager.Token, nodeJsExecutablePath, octorunScriptPath, "organizations",
                         user: keychainAdapter.Credential.Username, userToken: keychainAdapter.Credential.Token)
@@ -308,27 +301,57 @@ private async Task GetOrganizationInternal(Action<Organization[]> onSuccess, Act
             }
         }
 
-        private async Task<GitHubUser> GetCurrentUserInternal()
+        private async Task<IKeychainAdapter> GetValidatedKeychainAdaper(Connection keychainConnection)
         {
-            try
+            if (keychain.HasKeys)
             {
-                logger.Trace("Getting Current User");
-                await ValidateKeychain();
+                logger.Trace("LoadKeychainInternal: Loading");
 
-                var uriString = keychain.Connections.First().Host;
-                var keychainAdapter = await keychain.Load(uriString);
+                var keychainAdapter = await keychain.Load(keychainConnection.Host);
+                logger.Trace("LoadKeychainInternal: Loaded");
+
+                if (string.IsNullOrEmpty(keychainAdapter.Credential?.Username))
+                {
+                    logger.Trace("LoadKeychainInternal: Username is empty");
+                    throw new TokenUsernameMismatchException(keychainConnection.Username);
+                }
+
+                if (keychainAdapter.Credential.Username != keychainConnection.Username)
+                {
+                    logger.Trace("LoadKeychainInternal: Token username does not match");
+                    throw new TokenUsernameMismatchException(keychainConnection.Username, keychainAdapter.Credential.Username);
+                }
 
+                return keychainAdapter;
+            }
+
+            logger.Trace("LoadKeychainInternal: No keys to load");
+            throw new KeychainEmptyException();
+        }
+
+        private async Task<GitHubUser> GetValidatedGitHubUser(Connection keychainConnection, IKeychainAdapter keychainAdapter)
+        {
+            try
+            {
                 var octorunTask = new OctorunTask(taskManager.Token, nodeJsExecutablePath, octorunScriptPath, "validate",
-                    user: keychainAdapter.Credential.Username, userToken: keychainAdapter.Credential.Token)
+                        user: keychainAdapter.Credential.Username, userToken: keychainAdapter.Credential.Token)
                     .Configure(processManager);
 
                 var ret = await octorunTask.StartAsAsync();
                 if (ret.IsSuccess)
                 {
+                    var login = ret.Output[1];
+
+                    if (login != keychainConnection.Username)
+                    {
+                        logger.Trace("LoadKeychainInternal: Api username does not match");
+                        throw new TokenUsernameMismatchException(keychainConnection.Username, login);
+                    }
+
                     return new GitHubUser
                     {
                         Name = ret.Output[0],
-                        Login = ret.Output[1]
+                        Login = login
                     };
                 }
 
@@ -350,55 +373,6 @@ private async Task<GitHubUser> GetCurrentUserInternal()
                 throw;
             }
         }
-
-        private async Task ValidateCurrentUserInternal()
-        {
-            logger.Trace("Validating User");
-
-            var apiUser = await GetCurrentUserInternal();
-            var apiUsername = apiUser.Login;
-
-            var cachedUsername = keychain.Connections.First().Username;
-
-            if (apiUsername != cachedUsername)
-            {
-                throw new TokenUsernameMismatchException(cachedUsername, apiUsername);
-            }
-        }
-
-        private async Task<bool> LoadKeychainInternal()
-        {
-            if (keychain.HasKeys)
-            {
-                if (!keychain.NeedsLoad)
-                {
-                    logger.Trace("LoadKeychainInternal: Previously Loaded");
-                    return true;
-                }
-
-                logger.Trace("LoadKeychainInternal: Loading");
-
-                //TODO: ONE_USER_LOGIN This assumes only ever one user can login
-                var uriString = keychain.Connections.First().Host;
-
-                var keychainAdapter = await keychain.Load(uriString);
-                logger.Trace("LoadKeychainInternal: Loaded");
-
-                return keychainAdapter.Credential.Token != null;
-            }
-
-            logger.Trace("LoadKeychainInternal: No keys to load");
-
-            return false;
-        }
-
-        private async Task ValidateKeychain()
-        {
-            if (!await LoadKeychainInternal())
-            {
-                throw new KeychainEmptyException();
-            }
-        }
     }
 
     class GitHubUser
@@ -435,7 +409,7 @@ class TokenUsernameMismatchException : ApiClientException
         public string CachedUsername { get; }
         public string CurrentUsername { get; }
 
-        public TokenUsernameMismatchException(string cachedUsername, string currentUsername)
+        public TokenUsernameMismatchException(string cachedUsername, string currentUsername = null)
         {
             CachedUsername = cachedUsername;
             CurrentUsername = currentUsername;
@@ -448,12 +422,8 @@ protected TokenUsernameMismatchException(SerializationInfo info, StreamingContex
     class KeychainEmptyException : ApiClientException
     {
         public KeychainEmptyException()
-        { }
-        public KeychainEmptyException(string message) : base(message)
-        { }
-
-        public KeychainEmptyException(string message, Exception innerException) : base(message, innerException)
-        { }
+        {
+        }
 
         protected KeychainEmptyException(SerializationInfo info, StreamingContext context) : base(info, context)
         { }

src/GitHub.Api/Application/IApiClient.cs

@@ -14,7 +14,6 @@ Task CreateRepository(string name, string description, bool isPrivate,
         Task ContinueLogin(LoginResult loginResult, string code);
         Task<bool> LoginAsync(string username, string password, Func<LoginResult, string> need2faCode);
         Task Logout(UriString host);
-        Task GetCurrentUser(Action<GitHubUser> callback);
         Task ValidateCurrentUser(Action onSuccess, Action<Exception> onError = null);
     }
 }

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PopupWindow.cs

@@ -113,27 +113,27 @@ private void Open(PopupViewType popupViewType, Action<bool> onClose)
             OnClose.SafeInvoke(false);
             OnClose = null;
 
-            //Logger.Trace("OpenView: {0}", popupViewType.ToString());
+            Logger.Trace("OpenView: {0}", popupViewType.ToString());
 
             var viewNeedsAuthentication = popupViewType == PopupViewType.PublishView;
             if (viewNeedsAuthentication)
             {
-                //Logger.Trace("Validating to open view");
+                Logger.Trace("Validating to open view");
 
                 Client.ValidateCurrentUser(() => {
 
-                    //Logger.Trace("User validated opening view");
+                    Logger.Trace("User validated opening view");
 
                     OpenInternal(popupViewType, onClose);
                     shouldCloseOnFinish = true;
 
                 }, exception => {
-                    //Logger.Trace("User required validation opening AuthenticationView");
+                    Logger.Error(exception, "User required validation opening AuthenticationView");
                     authenticationView.Initialize(exception);
                     OpenInternal(PopupViewType.AuthenticationView, completedAuthentication => {
                         if (completedAuthentication)
                         {
-                            //Logger.Trace("User completed validation opening view: {0}", popupViewType.ToString());
+                            Logger.Trace("User completed validation opening view: {0}", popupViewType.ToString());
 
                             Open(popupViewType, onClose);
                         }