rgonzalez72/pytrainer — comment on commit 4e9e597cea20dfba672a3c90584ba3edfce5999e
File:  line 0
By ncjones:

Hi Rodolfo,

I am providing my general feedback here for all of your input validation changes. I have listed them in order of importance and grouped into functional, structural and stylistic issues.

Functional / Usability Issues

* invalid values can still be submitted:
# open athlete preferences
# enter a non-numeric value into the weight field
# select sports tab
# click "add sport"
# close the preferences dialog
# open the athlete preferences
* validation should occur on value change, not on focus change
- When an invalid value is provided it is not apparent until focus is shifted out of the field
- When a validation error is resolved I cannot click the "ok" button without first changing focus out of the input field.
* presenting validation messages for numeric inputs may not be necessary if we prevent the user from typing invalid characters.
* Error messages are not very good "error with the weight field" would be better as "weight requires a valid number".
* Error message for previous tab still visible after switching tabs
* Error message styling is unattractive. My suggestion is left aligned normal text with a yellow warning icon on the left.
* Required fields should be indicated with a red asterisk so user knows in advance it is required

Structural Issues

* if we can use gtk.SpinButton to prevent invalid number input and checking for required values is the only type of validation required then maybe we don't even need the generic validation framework
* number one rule in writing maintainable software is DRY but there is a lot of duplicated code:
- many input validator types are identical except for the field name. This can be parameterised in a more generic validator type, eg decimal validator type for all fields of decimal type and. Sometimes validators differ on requiredness. This can also be parameterised on all input validators.
- iterating logic to check all validators is duplicated. This can be genericised
- log message is defined on all validators but this should be able to be genericised
- the "error with the x field" should only need to be translated once but this text is repeated many times with slight variations requiring extra translation work
* don't use tuple for data structures, use classes - referencing properties by index is unreadable.

Code Style Issues

* use BDD style test cases: "given, when, then" and use "should" in the test name.
* don't import modules, import types, eg "from foo.bar import Baz" not "from foo import bar".
* don't put white space between function name or class name and left parenthesis, eg should be: "my_function()" not "my_function ()".
* Use camel case for acronyms, eg MetFieldValidator not METFieldValidator.
* you have committed a lot of glade formatting changes. This makes the diff hard to read. Hopefully we can get rid of Glade some day but in the mean time we should avoid creating unnecessary diffs.