NPE check for Password and Account Name field #384
Conversation
| new NullPointerException("Password is null"); | ||
| } | ||
| } else { | ||
| new NullPointerException("Account Name does not exist"); |
There was a problem hiding this comment.
You forgot to throw the exception.
Use throw statement.
There was a problem hiding this comment.
And, it is not always meant to throw NullPointerException.
Go for throwing IllegalArgumentException with an informative message.
There was a problem hiding this comment.
Looking at the below Ln:69, it seemed that you don't want to throw the exception if the password is missing.
There was a problem hiding this comment.
This is what I'm thinking:
if accountName is null, you can throw illegal argument exception
but if password is null, I think, you can continue without throwing the exception.
Because, mAccount is set to null, it will be initialized at the Ln:70.
| mAccount = account; | ||
| }else { | ||
| // - case migration from old AccountManagerStoreDataSource | ||
| mAccount = null; | ||
| identifier = new SharedStorageIdentifier(); | ||
| } | ||
| } else { | ||
| throw new IllegalArgumentException("Invalid parameters, Account name cannot be null"); |
There was a problem hiding this comment.
[optional], "Account name cannot be null" is sufficient here.
| } | ||
| } | ||
|
|
||
| //Create the account if it wasn't retrieved, | ||
| if (mAccount == null) { | ||
| sb.append("Account Name:: "+accountName); | ||
| sb.append("Account Type:: "+accountName); |
There was a problem hiding this comment.
Account Type:: "+accountName , this should be accountType.
| for (Account account : accounts) { | ||
| if (accountName.equals(account.name)) { | ||
| if (accountName != null &&accountName.equals(account.name)) { | ||
| String password = mAccountManager.getPassword(account); |
There was a problem hiding this comment.
Do you want to put some logging in this block also.
As later if password does not match, we make mAccount to null, and which eventually will go to the if(mAccount==null) block.
| mAccount = account; | ||
| }else { | ||
| // - case migration from old AccountManagerStoreDataSource | ||
| mAccount = null; | ||
| identifier = new SharedStorageIdentifier(); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
We might be throwing the exception for name mismatch. It's a mistake.
We need to rewrite the logic like below:
accountName is invariant to the loop. Hence, a null check for the accountName should be done outside the for-loop.
if (acoountName == null) {
throw new IllegalArgumentException("Invalid parameters, Account name cannot be null");
}
for (Account account : accounts) {
if (accountName.equals(account.name)) {
}
}
| @@ -51,27 +53,35 @@ public AccountManagerUtil(Context context, String accountName, boolean sharedSto | |||
| mAccountManager = AccountManager.get(MAS.getContext()); | |||
| //Attempt to retrieve the account | |||
| Account[] accounts = mAccountManager.getAccountsByType(accountType); | |||
| sb.append("No of accounts:: "+accounts.length); | |||
There was a problem hiding this comment.
why don't you simply log the details wherever you want!
| @@ -51,27 +58,42 @@ public AccountManagerUtil(Context context, String accountName, boolean sharedSto | |||
| mAccountManager = AccountManager.get(MAS.getContext()); | |||
| //Attempt to retrieve the account | |||
| Account[] accounts = mAccountManager.getAccountsByType(accountType); | |||
There was a problem hiding this comment.
Apply synchronization to the block that consists of the below logic:
synchronized {
- get-accounts
- iterate
- add-account-explicitly
}
| mAccount = null; | ||
| identifier = new SharedStorageIdentifier(); | ||
| messageBuilder.append(" trying account:" + account.name); | ||
| synchronized (this) { |
There was a problem hiding this comment.
Use class-object for synchronization.
synchronization (AccountManagerUtil.class) {
}
There was a problem hiding this comment.
Can I use the object already created (mutex)?
We are adding a check for Null Pointer Exception for the Password and Account Name field.