vaadin / patient-portal-demo-flow

0 stars 0 forks source link

Populate data for patient-profile #47

Closed pekam closed 7 years ago

pekam commented 7 years ago

Add all missing properties for patient-profile view, convert Gender into a String.


This change is Reviewable

SomeoneToIgnore commented 7 years ago

Reviewed 2 of 2 files at r1. Review status: all files reviewed at latest revision, 3 unresolved discussions.


src/main/java/com/vaadin/flow/demo/patientportal/converters/GenderToStringConverter.java, line 29 at r1 (raw file):

    @Override
    public String toPresentation(Gender modelValue) {
        return modelValue.name().toLowerCase();

Just return name.


src/main/java/com/vaadin/flow/demo/patientportal/converters/GenderToStringConverter.java, line 35 at r1 (raw file):

    public Gender toModel(String presentationValue) {
        if (presentationValue != null) {
            for (Gender gender : Gender.values()) {

Use Gender.valueOf()


src/main/java/com/vaadin/flow/demo/patientportal/converters/GenderToStringConverter.java, line 41 at r1 (raw file):

            }
        }
        throw new IllegalArgumentException(

Better throw all the exceptions early, if possible. In this case, you can invert if clause and get a better readability.


Comments from Reviewable

SomeoneToIgnore commented 7 years ago

Reviewed 1 of 1 files at r2. Review status: all files reviewed at latest revision, 4 unresolved discussions.


src/main/java/com/vaadin/flow/demo/patientportal/converters/GenderToStringConverter.java, line 29 at r2 (raw file):

    @Override
    public String toPresentation(Gender modelValue) {
        return modelValue.name().toLowerCase();

So, why cannot you just return modelValue.name() here and not use presentationValue.toUpperCase() at the bottom?


Comments from Reviewable

pekam commented 7 years ago

Review status: all files reviewed at latest revision, 4 unresolved discussions.


src/main/java/com/vaadin/flow/demo/patientportal/converters/GenderToStringConverter.java, line 29 at r1 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…
Just return name.

I don't want to. It looks ugly to have capital letters like: "Gender: MALE" in the UI.


src/main/java/com/vaadin/flow/demo/patientportal/converters/GenderToStringConverter.java, line 35 at r1 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…
Use `Gender.valueOf()`

Done.


src/main/java/com/vaadin/flow/demo/patientportal/converters/GenderToStringConverter.java, line 41 at r1 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…
Better throw all the exceptions early, if possible. In this case, you can invert `if` clause and get a better readability.

I just let the Gender.valueOf() throw the exceptions


Comments from Reviewable

pekam commented 7 years ago

Review status: all files reviewed at latest revision, 4 unresolved discussions.


src/main/java/com/vaadin/flow/demo/patientportal/converters/GenderToStringConverter.java, line 29 at r2 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…
So, why cannot you just return `modelValue.name()` here and not use `presentationValue.toUpperCase()` at the bottom?

see above


Comments from Reviewable