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

removed login and signup, remove old CSV dependency, switched to JSON #48

Merged

Conversation

awesominat
Copy link
Owner

@awesominat awesominat commented Nov 19, 2023

in this PR, I've

  1. removed login and signup
  2. removed the concept of "Users" and replaced it with "User" as the application will only house one user. that means all hash maps mapping username to user are now just a singular User entity that loads in data from user.json
  3. remove CSV as we have a lot of nested data structures, making the data file incredibly unreadable and especially unnecessary if it's only one user
  4. added JSON saving using GSON
  5. added LocalDate type adapters as LocalDate has private, inaccessible variables, messing up how GSON works
  6. added TransactionDeserializer as Transaction is an abstract class and it's unclear, from GSON's perspective, if the user's transaction is a Buy/Sell/Topup
  7. altered all test files and most app files to follow new structure

PR fixes and closes #47 and #6

@awesominat
Copy link
Owner Author

the tests fail because the new code references DashboardViewModel, a class not found in HEAD of main

Copy link
Collaborator

@rickygrosvenor-pramanick rickygrosvenor-pramanick 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 that the changes look sound and these can be tested further upon its integration to the main branch. I was looking at the logginInTesting file, and I see the test is commented out, so would we need to write a new test for it or is the commented out section valid. Furthermore, I was wondering as to why we switched to JSON from CSV? I understand from your comments that there were a lot of dependencies in the old CSV format however, it is not so clear as to how the implementation of the JSON format to store transaction history helps in this case.

@awesominat
Copy link
Owner Author

good point about the logging in app file. it seems I forgot to remove it.

to address your other concerns:

  1. We switched from CSV to JSON because we now only support 1 user. Previously, if we had multiple users, we could store them each on a new line alongside their history and portfolio. To do that, we would have had to encode the history and portfolio into JSON anyway. However, this is unreadable as the JSON will be squeezed onto one line without formatting.
  2. Since we now only support one user, we can get rid of the CSV + JSON hybrid storage and switch to just JSON, this also gives us the freedom to look at the JSON file without it all being on one line as it's by default prettified to be on multiple lines.
  3. There were not a lot of dependencies in the old CSV.
  4. The data structures we support are nested pretty deep. Take the history, its a hash map of strings mapping to TransactionHistory, which contains a Stock and a List of Transactions. Transactions contain PricePoint. This adds an unprecedented amount of depth to the data structures, which would have been a headache to read through the squeezed CSV file to debug and future issues.

I hope these addressed your concerns

Copy link
Collaborator

@rickygrosvenor-pramanick rickygrosvenor-pramanick left a comment

Choose a reason for hiding this comment

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

Yep thanks for the clarification, all looks and sounds good.

@rickygrosvenor-pramanick rickygrosvenor-pramanick merged commit 4281cfd into main Nov 20, 2023
1 check failed
@awesominat awesominat deleted the remove-loginsignup-save-json-alter-tests branch November 20, 2023 05:02
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.

remove Login for good and replace with JSON
2 participants