Skip to content
This repository was archived by the owner on Dec 19, 2023. It is now read-only.

Conversation

@jmisur
Copy link
Contributor

@jmisur jmisur commented May 30, 2016

Example of using Money type in response object.
I think serializing Money into 2 fields is hack anyway, which we should change. It contains hidden implicit field without possibility to change the name.
If anybody needs to serialize custom (3rd party) type into json with multiple fields, one can use custom class and map fields manually.

@fbenz
Copy link
Contributor

fbenz commented May 30, 2016

My opinion: If a serializer adds more than just a value, it should be nested, e.g.

{
  "balance": {
    "amount": 100.0,
    "currency": "EUR"
  }
}

This way it does not hurt that amount and currency have fixed names, because they are nested and the field can have a custom name (here: balance).

But for this PR it does not matter as only a value is written.

@jmisur
Copy link
Contributor Author

jmisur commented May 30, 2016

Yes in that case simple class with those fields is sufficient - for which existing facilities just work. I did a hacky serialization for Money because it's 3rd party, but I would rather change it in API than provide some hacky support in restdocs.
Can I merge?

}

private String fromGetter(String javaMethodName) {
int cut = javaMethodName.startsWith("get") ? 3 : 2;
Copy link
Contributor

@fbenz fbenz May 30, 2016

Choose a reason for hiding this comment

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

If you look at the method in isolation, it's impossible to see where the 2 is comming from. How about:
int cut = javaMethodName.startsWith("get") ? "get".length() : "is".length();

@fbenz
Copy link
Contributor

fbenz commented May 30, 2016

LGTM, just one potential readability improvement

@jmisur jmisur merged commit 6ac2d4c into master May 31, 2016
jmisur added a commit that referenced this pull request Mar 2, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants