trjhtr

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:

How can the complexity be reduced?

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.

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'