Skip to content

Conversation

@RaiBnod
Copy link
Member

@RaiBnod RaiBnod commented Jan 25, 2019

Ticket: #43

Basic idea of the validation base is extracted from joi.

Our validation base should able to do validation of Java primitives, objects as well as the JSON. And also it should able to assign some default values.

@RaiBnod RaiBnod requested a review from zero88 January 25, 2019 03:45
Copy link
Contributor

@zero88 zero88 left a comment

Choose a reason for hiding this comment

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

  • Introduce DataTypeValidation with abstract classType method, then all kinds such as StringValidation, DoubleValidation extends on this class.
  • Test no need logger, use system.out. If you need logger, pls adapt to use TestHelper for setupClass. Remove TestBase, it doesn't existed anymore
  • Each Test method must validate one case

Copy link
Contributor

@zero88 zero88 left a comment

Choose a reason for hiding this comment

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

Request changes

@RaiBnod
Copy link
Member Author

RaiBnod commented Jan 27, 2019

@Zero-88 please proceed for reviewing...

@RaiBnod RaiBnod force-pushed the feature/validation branch 2 times, most recently from d252fe2 to 3022540 Compare January 28, 2019 16:17
@zero88
Copy link
Contributor

zero88 commented Jan 31, 2019

@RaiBnod
Actually, after more reading your code, I feel it is too complex to understandable.
You made:

  • Mixing validate plain java type and JSonObject, all in one. A boundary between them is very ambiguous.
  • Use Java generic is not useful and effective.
  • Why validate need Single.
  • Naming classes is confusing

I suggest an another solution in package com.nubeiot.core.validator.proposal, it is just interface and some mock steps with comments.
I'd like to expect that you can review and re-think to design core API more intuitive

Thanks

@RaiBnod
Copy link
Member Author

RaiBnod commented Feb 10, 2019

Refactoring the ValidationBases are done. @Zero-88, could you please review the changes.

@zero88 zero88 force-pushed the master branch 2 times, most recently from f2047b4 to 68d3fa8 Compare February 6, 2021 21:57
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.

3 participants