issue #81 #3

Open
wants to merge 48 commits into
from

3 participants

@rgonzalez72

Can you please take a look at the changes I made? All your comments are welcome.

@ncjones
pytrainer member

Please do not put my name on your code.

@ncjones
pytrainer member

Hi Rodolfo,

I have three main issues with this code.

Firstly, your naming conventions are not so good. Python variable and method names should all be lower camel case and you should avoid using non-descriptive names. For example, "validateFields" should be "validate_fields" and "retVal" should be "is_valid".

The second issue is use of the "type code" anti-pattern. Instead of passing around type codes and looking up related properties in maps you should just have different objects for each type which contain the properties directly.

The third issue is that you have unsuccessfully tried to make a generic input validation framework but have only made the code more complicated in the process. "FieldValidator" sounds generic but it should actually be called "ProfileFieldValidator" because it has built-in knowledge of those specific fields.

I would expect a better generic approach would be to create custom widgets which implement some validation interface and fire "validation_state_changed" events to indicate when they become valid or invalid.

pytrainer member

Yes, a class hierachy is the OO alternative to a type code.

For each field that needs to be validated you can just instantiate the appropriate type of validator. The validator can probably be made capable of generating sensible localised error messages.

My thoughts on a custom widget were to create a new abstract subclass of gkt.Widget which wraps another widget and provides validation state and validation events. Then there will be very minimal wiring together for the UI - there just needs to be the same validation state change listener added to each widget which will add or remove an error message from the UI. As you point out though, either there will need to be multiple error messages displayed at a time or, while errors still exist, previous error messages need to be restored after errors are resolved so this approach requires a bit more thought. Maybe the validation state change handler needs to iterate over all known inputs and revalidate them when one of the inputs validation state returns to "valid". The Eclipse IDE presents input validation messages in this way (one message per screen, updated as you type, relevant to the earliest invalid field on the screen) so it could be worthwhile looking at the source code to see what pattern is used there.

Whether we use custom widgets or not though there will still need to be the string input validation type hierarchy so I suggest going ahead with that and seeing how it looks.

@ncjones
pytrainer member

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:
    1. open athlete preferences
    2. enter a non-numeric value into the weight field
    3. select sports tab
    4. click "add sport"
    5. close the preferences dialog
    6. 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 warning icon on the left.

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.
pytrainer member

Hmm, you are correct on both of these issues.

I disagree with the recommendation from the Gnome HIG though. I am not sure if we should follow this particular guideline. I tested out a few Gnome UIs with regards to this behaviour and don't find them particularly useful. For example, if you open the properties dialog for a file in Nautilus and change the name of the file to an invalid value (the same as an existing file or "/" etc) then you will get an obtrusive error dialog appear after the field loses focus. IMO it provides a better user experience to have inline error message appear in real time as the invalid value is being typed.

For the error message remaining visible when switching between tabs I suggest moving the message above the tabs so that it is obvious that it applies not just to the current tab.

Fortunately these issues can be largely mitigated byusing gtk.SpinButton to prevent invalid numeric input being entered at all. I have tested the SpinButton widget and confirmed that this is suitable for this type of use. I see that you have created a somewhat generic EntryInputFieldValidator type to perform this function. You can replace its use with the SpinButton widget.

There is also another type of validation that we need to consider: sport and equipment names must be unique. Currently you cannot add a new equipment item that has a non-unique name but there is no explanation as to why clicking "add" has no effect.

@dgranda
pytrainer member

Would it be possible for you to send a pull request (http://help.github.com/send-pull-requests/) to review and merge your changes?

@dgranda dgranda was assigned Feb 23, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment