Restructure sell to remove dependency on dashboard state #136
Conversation
gursi26
commented
Nov 27, 2023
gursi26
commented
- removed dependency to dashboardstate for updating sell page. now the sell use case can directly make api calls to get updated information
- Added more columns to sell table including current sell price for all units and a single unit.
- Added refresh button to update sell table based on updated stock prices.
…sell use case can directly make api calls to get updated information. Added more columns to sell table including current sell price for all units and a single unit. Added refresh button to update sell table based on updated stock prices.
| HashMap<String, String> ownedStocksAmounts = new HashMap<String, String>(); | ||
| List<Double> ownedAmounts = dashboardState.getOwnedAmounts(); | ||
| if (ownedStocks != null && ownedAmounts != null && !ownedAmounts.isEmpty() && !ownedStocks.isEmpty()) { | ||
| for (int i = 0; i<ownedStocks.size(); i++) { | ||
| ownedStocksAmounts.put(ownedStocks.get(i), String.valueOf(ownedAmounts.get(i))); | ||
| } | ||
| for (int i = 0; i < ownedStocks.size(); i++) { | ||
| String stockTicker = ownedStocks.get(i); | ||
| Double amountOwned = ownedAmounts.get(i); | ||
| Double sellPriceSingle = sellPrices.get(i); | ||
| Double sellPriceAll = sellPriceSingle * amountOwned; | ||
| tableModel.addRow(new Object[] {stockTicker, amountOwned, sellPriceSingle, sellPriceAll}); |
There was a problem hiding this comment.
its the same runtime/number of iterations as before, I dont see whats wrong with it.
There was a problem hiding this comment.
I think this is great as it removal of these dependencies is great for decoupling which aids our design's adherence of Clean Architecture and SOLID as it ensures that changes in one layer does not have cascading effects on the other layers. Furthermore, stylistically I think it passes all the naming conventions we have previously discussed i.e. CamelCase. I think something to work on in the future would be running unit testing to ensure that this works as required.
| @@ -57,6 +72,30 @@ public String getSellSuccess() { | |||
| return sellSuccess; | |||
| } | |||
|
|
|||
| public void setBalance(Double balance) { | |||
There was a problem hiding this comment.
- Your whole PR is an excellent example of good naming conventions, with attributes and methods being named using
camelCaseand classes named usingPascalCase. - The changes you made to
SellStatewere very crucial, as without them the view would have been much less functional. - The code's correctness is undeniable, as it is so neat that I could understand every addition it will make, but also it passes all of our Maven tests which would detect compile errors and failures of other pre-built tests we have made.
- The test coverage of the
Selluse case and interface adapters is not extensive, however this is not a problem specific to this use case but a general problem for our app. I just wanted to mention that we will need significantly more testing, including on the files that are modified here. - As with every class in our app, these changes reflect very good SOLID design principles and CA setup.
- Another thing to note with the
Selluse case here is that Javadoc is missing. Every constructor is public and many of the methods, including this one,SellState.setBalanceare also public, meaning all of these require Javadoc. - Your explanation/description of this PR was exemplary, as it laid out the changes you made and the reasons for making them.
Overall, this is a high-quality PR, and thus merits approval.
|
wow thanks guys. there is still one main problem with this approach, which is the fact that I need to use an API call to grab current stock prices, which slows the program down. Once #137 is fixed, I plan to use currentPrices from DashboardState in SellView. This means that the two are not entirely decoupled, but the reduced number of API calls makes it the most favourable option. |
