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

[Code Review] Vavinan #43

Closed
wwweert123 opened this issue Mar 31, 2024 · 4 comments · Fixed by #80
Closed

[Code Review] Vavinan #43

wwweert123 opened this issue Mar 31, 2024 · 4 comments · Fixed by #80

Comments

@wwweert123
Copy link

image

Try not to say that the error is unknown, because in the PE all errors should be known and accounted for

image

In here, I don't think there is a need to check whether the data is a integer. This is because the Integer.parseInt method will automatically throw the number format exception if it fails

image

You should add some error handling here because the user might access out-of-bounds for the splitted string "parts"

Overall code looks organized but there are some parts where the function serve almost the same purpose as another. For example parseTransactionType and parseTransaction are the same except for the separator. Perhaps, more descriptive names can be used to differentiate the two or you could abstract the repeated logic out into another function.

@Vavinan
Copy link

Vavinan commented Mar 31, 2024

  1. It's for our debugging purpose to catch some unhandled error cases, we will remove that in our final product

  2. We will consider this change in our future iterations

  3. For this one the user input is received in way that it will prompt the user to type Transaction type first and then followed by other data one by one, where each data will be received separately. I have included the relevant function below. Correct me if I took the question wrongly.

Screenshot 2024-03-31 164148

Thank you.

@wwweert123
Copy link
Author

Ok understood, then this should be fine!

@wwweert123
Copy link
Author

Oh and I missed out something, Is the category always an integer as well,? because you did parseInt() which might cause an uncaught exception to be thrown

@Vavinan
Copy link

Vavinan commented Mar 31, 2024

Yes, it will be an integer always. For now, it has been caught if other numbers or text is typed instead. I have attached one example below. We will make a check explicitly in our future iterations. Thank you for notifying this.

Screenshot 2024-03-31 171851

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 a pull request may close this issue.

2 participants