Codereview#2 of 9 (Valente) from 6 (Furlan)#136
Codereview#2 of 9 (Valente) from 6 (Furlan)#136AlbVal97 wants to merge 1 commit intoPrandiniUniPD:CommonDemofrom
Conversation
| * *************************** | ||
| */ | ||
|
|
||
| private OnFragmentInteractionListener mListener; |
There was a problem hiding this comment.
This variable is never initialized and use, i think u can delete it
There was a problem hiding this comment.
You're right: I think I forgot to delete it after the recent refactoring, so I'll get it done.
| { | ||
| // TODO: Update argument type and name | ||
| void onFragmentInteraction(Uri uri); | ||
| } |
There was a problem hiding this comment.
I dont understand the meaning of this interface. Is that used somewhere? Does it needs to be completed? Otherwise i suggest u to delete it or at least complete the TODO.
There was a problem hiding this comment.
Due to the fact that it is linked to the first variable and it is never used (nor in @leopardo-rossi-developer class, which I used as an example) I think that I can safely delete it too.
| // TODO: Update argument type and name | ||
| void onFragmentInteraction(Uri uri); | ||
| } | ||
| } |
There was a problem hiding this comment.
I see some hardcoding, i suggest u to use the file strings.xml in resources to save your strings and then retrieve it when need. I see some case in line 41-46-51-56-169
There was a problem hiding this comment.
Thank you so much for pointing them out! I definitely forgot to use resources for line 169 and I didn't even recognized as hardcoding the other four.
|
|
||
| //The username and password inserted are gathered from the EditText objects | ||
| userName = usernameEditText.getText().toString(); | ||
| userPwd = pwdEditText.getText().toString(); |
There was a problem hiding this comment.
I think that could be better to not let the password as a normal string in the code for security reason. i suggest u to cript it before use and send it to Login_VM. i know this is a base implementation so this is an advice for future improvement.
There was a problem hiding this comment.
You're totally right: I'm going to implement this as soon as possible because, as you said, is a fairly basic security issue.
| /** | ||
| * String used to identify the error given by incorrect credentials | ||
| */ | ||
| private final String LOG_INCORRECT_CREDENTIALS = "Connection established. Credentials refused."; |
There was a problem hiding this comment.
Variables in line 62 and 67 is never used, i think u forgot to delete them
There was a problem hiding this comment.
That's true, I'm going to fix them. Thank you!
|
|
||
| //Sends a netword request to check if the provided credentials are correct | ||
| DatabaseManager.loginUser(context, username, password, listeners); | ||
| } |
There was a problem hiding this comment.
I see there are a lot of listeners, i suggest you to use different names to differentiate them. I had some difficulties to recognize them.
There was a problem hiding this comment.
You're right: I have replaced them with "forumLoginListeners" instead.
2 participants
Hello @GiovanniFurlan ,
I kindly ask you to review part of the code I have sent to you.
To be specific, I need the classes ForumLogin.java and Login_VM.java to be reviewed, with the exception of the brief parts of Login_VM.java that only belong to @TaulantBullaku .
They are responsible for performing the login request to the database and then launch the fragments useful to access the forum content.
Thanks a lot for your time!
Alberto