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

jc - Add parameters to Commons type. #30

Merged
merged 4 commits into from
Mar 7, 2022
Merged

jc - Add parameters to Commons type. #30

merged 4 commits into from
Mar 7, 2022

Conversation

jacksoncooper
Copy link
Contributor

Adds the fields specified in gamePlay.md to the Commons type, and modifies the controller to allow them to be passed in a GET request to /api/commons/new.

Internally, startingDate is a Date type, which Spring seems to know how to serialize. The Date is constructed from a GregorianCalendar using the 'default time zone' which I believe is the system's time zone, which may not be ideal if this game is running on a cluster in Vermont. GregorianCalendar has a constructor that accepts a TimeZone but my eyes kind of glazed over.

parameters

@codecov
Copy link

codecov bot commented Mar 4, 2022

Codecov Report

Merging #30 (ee6483f) into main (ae707b1) will increase coverage by 0.55%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main      #30      +/-   ##
============================================
+ Coverage     66.85%   67.40%   +0.55%     
  Complexity       42       42              
============================================
  Files            47       47              
  Lines           353      359       +6     
  Branches          9        9              
============================================
+ Hits            236      242       +6     
  Misses          116      116              
  Partials          1        1              
Impacted Files Coverage Δ
...156/happiercows/controllers/CommonsController.java 71.42% <100.00%> (+4.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae707b1...ee6483f. Read the comment docs.

@jacksoncooper

This comment was marked as outdated.

@jacksoncooper jacksoncooper requested a review from btk5h March 5, 2022 19:38
Comment on lines 22 to 24
private String cowPrice;
private String milkPrice;
private String startingBalance;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be numeric values instead of strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They have type String because of the @RequestBody annotation. My first try resulted in a type error. I thought Spring would coerce them like it does for query parameters but I guess not.

@ApiParam("request body") @RequestBody CreateCommonsParams params

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, what types did you try to use? RequestBody should use the same HttpMessageConverter used by other parts of Spring, so I'm surprised that it's not supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may have used primitives.

Copy link
Contributor

Choose a reason for hiding this comment

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

You may need @DateTimeFormat and @NumberFormat to get those to work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh my gosh Bryan thank you! My only concern now is that the Commons and CreateCommonsParams types only differ by an id field and their respective annotations.

Copy link
Contributor

@btk5h btk5h Mar 6, 2022

Choose a reason for hiding this comment

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

I think it's natural to be concerned about code duplication, but there's a case to be made about decoupling the database internals from the public-facing API. For simple use cases, the model and entity will usually look very similar, but that may change as new fields and relations are added. Having a separate model and entity also allows us to freely make changes to database persistence without requiring any changes on the frontend.

You might find better resources about this design pattern if you try reading up on Data Transfer Objects (DTOs). There are libraries that make this model/DTO <-> entity conversion much simpler, though we don't currently have that set up in these projects.

@jacksoncooper jacksoncooper requested a review from btk5h March 6, 2022 01:16
@jacksoncooper
Copy link
Contributor Author

Thanks Bryan. That took far too long.

Copy link
Contributor

@Sytarno Sytarno left a comment

Choose a reason for hiding this comment

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

Nice, ended up being a larger task than expected. Good job maintaining coverage.

@jacksoncooper
Copy link
Contributor Author

jacksoncooper commented Mar 6, 2022

May we merge? @btk5h

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants