search

jurassix / react-validation-mixin

Simple validation mixin (HoC) for React.
MIT License
283 stars 38 forks source link

Refactoring #6

Closed ivan-kleshnin closed 9 years ago

ivan-kleshnin commented 9 years ago

1) Check if (validations) which you use is incorrect (useless), as JS objects are always truthy

> Boolean({})
true

2) Those naked reduces look cumbersome. I recommend to utilize lodash library to greatly simplify logic checks (see diff). For now the code seems to be quite complicated (which it isn't). I see you're into functional approach (which I am too). So I refactored ValidationStrategy a bit to demonstrate how it may look like.

3) The potential for simplification is not yet reached. Arrow functions may be added (afterwhile) and flatMap is not yet provided by lodash.

4) I'm just starting with mocha / chai. Therefore I didn't add or edited tests. Well, at least it passes current and it was simple algo replacements.

5) I prefer to name lodash as Ld. (underscore) is already used as trash variable operator in destructuring `var , a = b`. Whether to capitalize module variable names or not... question is still open in community. Though I see newer frameworks & libraries (hapi, react) prefer to capitalize them. So I'm just following this new trend.

6) My IDE (Idea) highlights some jsDoc syntax errors. So I fixed them. Firstly: js types should be capitalized string -> String. Secondly: argument name should come right after declared type 

@param {?string} state field name...` -> `@param {?String} field State key...
*/
handleValidation: function(field, preventDefault) // arg names have to match 
...
ivan-kleshnin commented 9 years ago

lodash requirement is probably too heavy to include it on frontend as a whole. It is possible to inline dependencies or utilize standalone functions (lodash.isempty, lodash.flatten). In any case it will improve readability comparing to current stage. Your opinion?

jurassix commented 9 years ago

Refacts looks good. I think lodash is pretty heavy to drop into this mixin though. We should create a utility class and implement isEmpty and flatten or find lightweight npm modules that do each of these independently. I'll do some research today.

jurassix commented 9 years ago

This looks good: https://www.npmjs.com/package/lodash.isempty

jurassix commented 9 years ago

this too: https://www.npmjs.com/package/lodash.flatten

jurassix commented 9 years ago

If you refactor your pull request to depend on these two modules I'll merge it in. Seems like the right level of clean code to third party dependencies.

ivan-kleshnin commented 9 years ago

Done, thanks!

jurassix commented 9 years ago

released as version 3.0.3