3
\$\begingroup\$

My lead developer has commented that the code I write is hard to understand and that I "overcomplicate things". I'm willing to accept this and improve but I'm not sure if this is genuinely the case or if his personal preferences are seeping into his judgement.

A red flag for me was the following situation: We're receiving database entries in the form of an array of dictionaries called columns. We then make HTML templating decisions based on their contents. Note that "special" types such as files or map coordinates are really text fields with an additional type_[name] array field for validation.

columns = someDBData
for column in columns:
    column_type = column['data_type']
    isValid = Someclass.perform_some_validation(column_type) # returns a boolean
    isCalendar = Someclass.is_calendar(column_type) # returns true if it's a time, datetime or datetime-local data type
    isString = Someclass.is_string(column_type) # ditto but with text, mediumtext or varchar

    isHidden = True if column['hidden'] != 0 else False
    isReadOnly = True if (configuracion_open == "" and column['open_insert'] == 0) or (configuracion_open == "Editando_registro" and column['open_edit'] == 0) else False
    isFile = True if column_type == "text" and column['type_file'] is True else False
    isGeo = True if column_type == "text" and column['type_geo'] is True else False
    isGeneric = True if isCalendar or isString and not isFile else False

   # repeat for other validations and then do templating stuff

When reviewing my work, he claimed that using "long ternary operators such as these" would be confusing for other people maintaining the code in the future. It seems as if he'd prefer to use a more verbose if/elif/else structure for readability, even though giving the program unnecessary verticality will also result in a readability problem in the future, at least in my opinion. In summary, should I reserve ternary operators for short and simple validations?

\$\endgroup\$
  • \$\begingroup\$ We don't review stub code, and this is stub code. So this will get closed unless you can put some real code here. Still, I am no Python expert, should you not write isHidden = True if column['hidden'] != 0 else False as isHidden = (column['hidden'] != 0) ? Beyond that, yes, long ternaries are a pain to understand. \$\endgroup\$ – konijn Nov 4 at 12:36
4
\$\begingroup\$

I think there is nothing wrong with using boolean-valued expressions to compute boolean results, instead of if/elif/else statements. However, it is still important to write these expressions clearly. Most of your expressions can be simplified, and none of your expressions actually require using the ternary operator.

If you write a ternary operator where the true-branch has the value True and the false-branch has the value False, then the ternary operator is superfluous.

If the condition is a boolean already, such as True if column['hidden'] != 0 else False, then you can simply write column['hidden'] != 0 as this will already be either True or False as required.

If the condition is not necessarily a boolean, e.g. True if my_list else False (not from your example) where my_list is a list and the condition tests if it is non-empty, then you can simply write bool(my_list) to calculate the boolean result you want.

When you have a condition like isCalendar or isString and not isFile which has both and and or, I recommend always using brackets to clarify the order of operations. Not everyone knows that and takes precedence over or, and even if they do know this, they may not know that you knew, and might trust the code less. Write isCalendar or (isString and not isFile) to make clear what this code does; or if you wanted it to be (isCalendar or isString) and not isFile, write that.

The expression for isReadOnly is very long, so I suggest splitting this over multiple lines. If an expression is inside (parentheses), then you can space it across multiple lines however you wish.

By the way, column['type_file'] is True is redundant, assuming column['type_file'] is always a boolean. If it's True, then True is True results in True, while False is True results in False; either way, the result is the same as the original value of column['type_file']. Imagine writing x + 0 where x is a number; it won't change the result, so you might as well just write x.

\$\endgroup\$
2
\$\begingroup\$

You may proceed with ternary operators in your case, as the original intention might be applying "Replace nested conditional with guard classes" which is good and well-known refactoring technique.

As was mentioned, the condition itself is evaluated to boolean/logical value, no need to duplicate the "evidence" like var_name = True if True else False.

Additionally, the condition column_type == "text" is duplicated and can be consolidated/extracted into a separate flag isTextType:

for column in columns:
    column_type = column['data_type']
    isTextType = column_type == "text"
    isValid = Someclass.perform_some_validation(column_type) # returns a boolean
    isCalendar = Someclass.is_calendar(column_type) # returns true if it's a time, datetime or datetime-local data type
    isString = Someclass.is_string(column_type) # ditto but with text, mediumtext or varchar

    isHidden = column['hidden'] != 0
    isReadOnly = (configuracion_open == "" and not column['open_insert']) \
                  or (configuracion_open == "Editando_registro" and not column['open_edit'])
    isFile = isTextType and column['type_file']
    isGeo = isTextType and column['type_geo']
    isGeneric = isCalendar or (isString and not isFile)

Furthermore, your validation logic goes within a for loop, but what if the program would need to validate a single column/record in some other place within application?
To make it more unified and scalable it's better to extract the validation logic into a separate function applying Extract function technique.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.