4
\$\begingroup\$

We have project.task model, where each task has many lines and each line has progress field indicating how much work has been done and how much still remaining (from 0 to 100 percent). The task cannot be done until all lines' progress is 100%.

@api.onchange('task_line_ids','task_line_rider_ids')
def progress_on_change(self):
    for record in self:
        if len(record.task_line_ids) != 0 and record.state in ('open', 'done'):
            all_done = True
            for line in record.task_line_ids or record.task_line_rider_ids:
                if line.progress != 100:
                    all_done = False
                    break         
                if all_done == True:
                    record.state = 'done'
                else:
                    record.state = 'open'

However, SonarQube reported a code smell: SonarQube

How can the complexity be reduced?

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

There appears to be a bug in your code. The line:

if len(record.task_line_ids) != 0 and record.state in ('open', 'done')

Is followed by the line:

for line in record.task_line_ids or record.task_line_rider_ids

Since bool(record.task_line_ids) will always be True by the virtue of the list not being empty (as previously checked). This second line is equivalent to

for line in record.task_line_ids

This does not seem to be the intention of the code. Assuming you want to check all lines in either record.task_line_ids and record.task_line_rider_ids you may want to use the following code:

all_lines = record.task_line_ids + record.task_line_rider_ids
all_done = all(line.progress == 100 for line in all_lines)

Combined with the other answer's suggestion, this should result in a pretty clean function.

\$\endgroup\$
7
\$\begingroup\$

16 and 15 are pretty artificial values, you should aim for a better code quality rather than a higher/smaller metric in particular.

Let's see what can be improved...

  • Your boolean comparison if all_done == True: could (and should) be written in a more concise way: if all_done:.
  • Python has an optional else on for loops which can be understood as "this loop exited the usual way, not via a break". This removes the need for the all_done variable altogether:

    if len(record.task_line_ids) != 0 and record.state in ('open', 'done'):
        for line in record.task_line_ids or record.task_line_rider_ids:
            if line.progress != 100:
                record.state = 'open'
                break         
        else:
            record.state = 'done'
    
  • You could use the all or any builtin to rewrite this without the for... else logic:

    if len(record.task_line_ids) != 0 and record.state in ('open', 'done'):
        if all(line.progress == 100 for line in record.task_line_ids or record.task_line_rider_ids):
            record.state = 'done'
        else:
            record.state = 'open'
    
  • You could re-introduce the temporary variable and use ternary operator:

    if len(record.task_line_ids) != 0 and record.state in ('open', 'done'):
        all_done =  all(line.progress == 100 for line in record.task_line_ids or record.task_line_rider_ids)
        record.state = 'done' if all_done else 'open'
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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