armaan-n
commented
11 months ago SOLID / Design / CA
In FilePatientDataAccessObject.java, the process of instantiating a patient is very messy, and can likely be factored out. Instead, a patient builder should be used, and the parsing of string to other data types can be done within the build methods. This would help the code adhere more to the single responsibility principle, since if the file structure were to change, minimal effect would be needed to refactor the code, where it might become very tedious in its current state.
Also, if the edit profile method, the repeated use of the save method is redundant, instead, you can increment the changes variable for every detected change, and simply call save() and the end if there are more than 0 changes. The repeated checking for equality is also redundant unless it is necessary to extract the exact number of changes. Instead, if possible, I suggest that instead, you simply call patient.setX(X) outside of any control flow statements, call save() and return void.
Other than that, the design and adherence to the SOLID principles. There are no problems with the direction of your dependencies. Particularly, good job on reducing your dependence on the UI, and delegating most of the work to the state.
Correctness
After reviewing the use case interactor, the behaviour seems to be correct. However, you used an interesting method to edit the state from the view currentState.setEmail(emailInputField.getText() + e.getKeyChar()). I’m not sure the desired behaviour is achieved whenever the user inputs a backspace, or if they clear the text field (i.e. by highlighting and hitting backspace. Instead, you should just retrieve the contents of the text field and set the state variable accordingly. This error is made at multiple points in the view.
Also, nothing in the view is preventing me from entering an empty string into the ccv field, which yields an error message in the console.
Style
You did well when it comes to style. You respected the camelcase convention, curly brace location. Good use of access modifiers, helpers and instance variables have been made private, and public is used only when necessary.
Tests
There are no tests in the current PR. In the future, you should make a PR with tests that cover a variety of input data objects, and ensure the output data is as expected. Namely, you should test for failure (no changes made, unable to access data access object), success (editing one field, editing multiple fields, editing all fields), and data structure consistency (ensure the patient’s fields are changed appropriately, ensure the view manager model changes to the appropriate screen). Moreover, you should perform some integration tests that use the data access object you created, and if possible, some end to end tests.
Documentation
The documentation is lacking. If possible, write some documentation for large public methods whose behaviours are complex. Namely, the save method and constructor in the data access object, the create methods in the use case factory, and the execute method in the interactor. You should also use Javadoc conventions when writing documentation (i.e. using @param, @return and @throws tags).
This PR request encompasses the edit profile use case. A separate pull request for the test will follow suit.
Only patient users will be able to edit their profiles, who can only change their username, password, email address, phone number, and insurance details.
EditView and EditedView Details
When a user presses the edit profile button on the home page, they would be redirected to Edit View, which showcases the patient their profile (similar to what they saw when signing up, but with limited text fields). This state will illustrate each text field to be already pre-populated with the user's current details, instead of being blank. For example the username text field would like:
Username: kayumova
This implementation detail was made to make it easier to track for changes and allow users to change multiple profile details in one go. Hence, after pressing the 'edit profile' button, they will be able to see their new details and unchanged details populate in the Edited State.
The fail view will be prepared when the file patient data access object is unable to detect any changes made to the user's profile in Edit State.