Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

9-feature-1-user-registration #17

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Conversation

parmism
Copy link
Contributor

@parmism parmism commented Nov 11, 2022

No description provided.

@parmism parmism linked an issue Nov 11, 2022 that may be closed by this pull request
@Madhavan7 Madhavan7 requested review from a user and sogolsahebi November 13, 2022 04:57
Copy link
Contributor

@sogolsahebi sogolsahebi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I review These classes/interfaces UserVerifier, UserVerificationUI, UserRegUseCase and UserRegistrationUI. I haven't found any issues, and they are following the clean architecture. Nice job!

If I want to add a recommendation, I suggest using different packages to organize user classes. (i.e. for example, a package for tests, interfaces and entities)it makes it easier to follow your codes.

…e" into an object that would either send the code via phone number or email
…possible to add multiple users to the file without issues
@Madhavan7 Madhavan7 requested a review from emma0925 November 17, 2022 22:22
@sogolsahebi sogolsahebi self-requested a review November 20, 2022 17:15
sogolsahebi
sogolsahebi previously approved these changes Nov 20, 2022
Copy link
Contributor

@sogolsahebi sogolsahebi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work , no issues found , ready to merge to main .

@sogolsahebi sogolsahebi self-requested a review November 21, 2022 17:12
sogolsahebi
sogolsahebi previously approved these changes Nov 21, 2022
ghost
ghost previously approved these changes Nov 22, 2022
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the developer choose clear names for variables, classese, method. And the code is well-designed and appropiate for our application

emma0925
emma0925 previously approved these changes Nov 22, 2022
Copy link
Contributor

@emma0925 emma0925 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code is well implemented with clean architectures. You implemented Entities, Usecases and UIs. I think it is also great that you have implemented a lot of test cases to make sure your code work as expected. Good Job! One thing that you could consider is maybe to add some more edge cases to your tests, such as cases with empty strings.

@@ -89,14 +90,6 @@ public List<User> getList() {
ObjectInputStream in = new ObjectInputStream(fileIn)) {

users = (ArrayList<User>) in.readObject();
/*
while(true){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is correct to delete this while loop since it is not necessary

UserDatabase accountDatabase = new UserDatabase(accounts);
accountDatabase.createUser("MeenakshiGopakumar", "123", "[email protected]", "Basic");
List<User> lst = accountDatabase.getList();
String email = lst.get(0).getEmail();
System.out.println(email);
Assertions.assertTrue(email.equals("[email protected]"));
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can do another test with edge cases that contains empty informations (e.g. "" for email)

@sogolsahebi sogolsahebi self-requested a review November 22, 2022 01:32
sogolsahebi
sogolsahebi previously approved these changes Nov 22, 2022
@Madhavan7 Madhavan7 dismissed stale reviews from sogolsahebi, emma0925, and ghost via a6d0b75 November 30, 2022 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature 1] User Registration
4 participants