Add users #190
Add users #190
Conversation
| { | ||
| using (var sha256 = SHA256.Create()) | ||
| { | ||
| var hashedBytes = sha256.ComputeHash(Encoding.UTF8.GetBytes(password)); |
There was a problem hiding this comment.
good job on password hashing!
| if (user.Role == "User") | ||
| { | ||
| await DisplayAlert("Notice", "Contact manager", "OK"); | ||
| } |
There was a problem hiding this comment.
using else if's would greatly reduce the nesting, which would make it way more readable
| if (txtName.Text != null) | ||
| { | ||
| if (txtEmail.Text != null) | ||
| { |
There was a problem hiding this comment.
same here, inverting the conditions would reduce nesting greatly.
would also help to refactor this into multiple methods
| private async void LoginButton_Clicked(object sender, EventArgs e) | ||
| { | ||
| string email = txtLoginEmail.Text; string password = txtLoginPassword.Text; | ||
| var user = users.FirstOrDefault(u => u.Email == email && PasswordHasher.VerifyPassword(password, u.Password)); | ||
|
|
||
| if (user != null) | ||
| { | ||
| if (user.Role == "User") | ||
| { | ||
| await DisplayAlert("Notice", "Contact manager", "OK"); | ||
| } | ||
| else | ||
| { | ||
| if (user.Role != "Admin") | ||
| { | ||
| userManagementSection.IsVisible = true; | ||
| } | ||
| else | ||
| { | ||
| if (email == "[email protected]") | ||
| { | ||
| userManagementSection.IsVisible = true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| else | ||
| { | ||
| if (email == "1" && password == "1") | ||
| { | ||
| userManagementSection.IsVisible = true; | ||
| } | ||
| else | ||
| { | ||
| await DisplayAlert("Login Failed", "Invalid credentials", "OK"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The code contains multiple nested if statements, making it difficult to read and understand. It could be simplified significantly by using guard clauses. Also, the code's formatting is inconsistent (e.g., the first two variable declarations are on the same line), and the method checks for the role and email in a way that could be simplified.
| private async void AddUserButton_Clicked(object sender, EventArgs e) | ||
| { | ||
| if (txtName.Text != null) | ||
| { | ||
| if (txtEmail.Text != null) | ||
| { | ||
| if (txtPassword.Text != null) | ||
| { | ||
| if (txtTeam.Text != null) | ||
| { | ||
| if (IsValidEmail(txtEmail.Text)) | ||
| { | ||
| var newUser = new User | ||
| { | ||
| Name = txtName.Text, | ||
| Email = txtEmail.Text, | ||
| Password = PasswordHasher.HashPassword(txtPassword.Text), | ||
| Team = txtTeam.Text | ||
| }; | ||
| if (pickerRole.SelectedItem != null) | ||
| { | ||
| newUser.Role = (string)pickerRole.SelectedItem; | ||
| if (pickerAccessLevel.SelectedItem != null) | ||
| { | ||
| newUser.AccessLevel = (string)pickerAccessLevel.SelectedItem; | ||
| await userService.Add(newUser); | ||
| users.Add(newUser); | ||
| ClearUserInputFields(); | ||
| await DisplayAlert("Success", $"{newUser.Name} added!", "OK"); | ||
| } | ||
| else await DisplayAlert("Error", "Access Level not selected", "OK"); | ||
| } | ||
| else await DisplayAlert("Error", "Role not selected", "OK"); | ||
| } | ||
| else await DisplayAlert("Error", "Invalid Email", "OK"); | ||
| } | ||
| else await DisplayAlert("Error", "Team is empty", "OK"); | ||
| } | ||
| else await DisplayAlert("Error", "Password is empty", "OK"); | ||
| } | ||
| else await DisplayAlert("Error", "Email is empty", "OK"); | ||
| } | ||
| else await DisplayAlert("Error", "Name is empty", "OK"); | ||
| } |
There was a problem hiding this comment.
the code has too many nested if statements, making it hard to follow, there are also unnecessary checks for null on text fields that are typically empty strings rather than null in UI contexts, and there's no need for multiple await DisplayAlert statements for each field check, they just lead to repetition and clutter. Try implementing this code using guard clauses
added for this week