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

GetInfo use case implemented #45

Merged
merged 3 commits into from
Nov 20, 2023
Merged

GetInfo use case implemented #45

merged 3 commits into from
Nov 20, 2023

Conversation

quiz3
Copy link
Collaborator

@quiz3 quiz3 commented Nov 17, 2023

I implemented the GetInfo use_case but I also modified a bunch of files cuz the switch to Maven format messed with all the imports so I needed to change a whole bunch of stuff.

Very important to thoroughly review this one though.

This is linked to Issue #29.

Copy link
Owner

@awesominat awesominat left a comment

Choose a reason for hiding this comment

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

I really like this, the public methods are documented, no CA engine violations, and a good choice of what to show the user for the GetInfo case.

I think this might go in the Buy Menu after a user searches for a specific stock so they can make sure this is the stock they wanted to buy

@awesominat
Copy link
Owner

I will merge this as soon as we have the new JSON data loader merged

Copy link
Owner

@awesominat awesominat 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 documentation can be improved. this should help clear up any confusion about the doc in the future. very good start though

Comment on lines 17 to 24
/**
* The getInfoInputData parameter should follow the specifications laid out in that class.
* <p>
* This method implements the bulk of the GetInfo use case.
* Info is fetched using the Finnhub driver.
*
* @param getInfoInputData an InputData object following the relevant CA Engine rules
*/
Copy link
Owner

Choose a reason for hiding this comment

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

I feel that the documentation needs to mention what the use case does, or rather, what the execute method does. currently, it just says it does the bulk of the use case, what does that mean?

Also, I don't think you need to mention it follows CA engine rules, the client doesn't really care about that. Instead, mention that the InputData contains the ticker

@quiz3
Copy link
Collaborator Author

quiz3 commented Nov 20, 2023

I have been informed that it is not bad that this is failing. It is likely due to the fact that this branch was created prior to some of the newer Maven architecture of our project being set up.

The merge process for this will be non-trivial but not very difficult, since all I modified most recently was the interaction.

@awesominat awesominat self-requested a review November 20, 2023 22:55
Copy link
Owner

@awesominat awesominat left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the documentation changes

@awesominat awesominat merged commit 37c2436 into main Nov 20, 2023
1 check failed
@awesominat awesominat deleted the get_company_info branch November 20, 2023 22:55
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.

2 participants