Notifying if a plane has been found

Question

The object is to notify if a plane was found or not. The aircraft_identifier variable is a must. cpl_id and target_no are optional, only either of them can be applied at time not both, they are used for further identification if aircraft_identifier is not unique.

qo = 'squawk' or 'callsign'  # only these variants are possible
qo_squawk = '12345'
qo_callsign = 'AMAN3'
qo_cplId = '11'
qo_targetNo = '22'

def decision_making(aircraft_identifier, cpl_id=None, target_no=None):
    found_target = False

    if qo == 'callsign' or qo == 'squawk':
        if aircraft_identifier.isdigit():
            found_target = (aircraft_identifier == qo_squawk)
        else:
            found_target = (aircraft_identifier == qo_callsign)

        if found_target:
            if cpl_id:
                found_target = True if qo_cplId == cpl_id else False
            if target_no:
                found_target = True if qo_targetNo == target_no else False

            if found_target:
                print('Found target!')
                return

    print('plane not found in list!')


decision_making('AMAN3', '22')

Can this code be made simpler?

Answer 1

A few suggestions:

• “only either can be used at a time not both” – nothing in the function enforces that at most one of these input parameters has been set. Something like this:

  if (cpl_id is not None) and (target_no is not None):
       raise ValueError("At most one of cpl_id and target_no should be supplied")
  

  would ensure that at most one of them can be specified by the user.

• You have several lines of the form

  found_target = foo == bar
  

  I’d put some parentheses around the condition, because I think it makes it easier to read:

  found_target = (foo == bar)
  

  Later in the function you have two lines of the form

  found_target = True if foo == bar else False
  

  which can be reduced to the construction above.

• You have one function that does both business logic (checking whether the plane is correct) and doing some work (printing to screen). You should really separate these two – this function should return True/False, and then the caller should handle printing an appropriate message.

• This line:

  if qo == 'callsign' or qo == 'squawk':
  

  is a bit unwieldy because you have to add an extra or statement every time you add a new possibility for qo. Better to do something like:

  if qo in ['callsign', 'squawk']:
  

  which is easy to extend with more values.

  (Although it's not clear why you're doing this check at all, given that qo will always be squawk in the function as defined.)

• There are no comments or docstrings. I don’t know anything about planes, so I just have to trust that the conditionals are correct. It would be good to provide some explanation of what this all means – for example, what’s a CPL ID, or what is the qo_ prefix for?