5
\$\begingroup\$

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?

\$\endgroup\$
1
  • \$\begingroup\$ Some answers have suggested simplifications to the logic in this function. However, based on the wishy-washy name decision_making(), as well as the fact that the function doesn't return any useful value, I suspect that real gains would be made by changing the design of your entire program. I encourage you to ask another question containing more of your code. \$\endgroup\$ – 200_success Sep 20 '15 at 18:51
1
\$\begingroup\$

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?

\$\endgroup\$
3
  • \$\begingroup\$ This is a simplified version of production code, I should not have hardcoded the variables above found_target, it was added there to give an example for input. Of course in reality the function would return something! \$\endgroup\$ – JonB Sep 20 '15 at 19:04
  • \$\begingroup\$ Thank you for your suggestions, I modified my code partly according to your comments. \$\endgroup\$ – JonB Sep 20 '15 at 20:52
  • \$\begingroup\$ My concern is that I'm probably missing a check if both cpl_id and target_no var are set, that should not be allowed. Also I'm concerned with my last if found_target: branch, if I'm missing something. \$\endgroup\$ – JonB Sep 20 '15 at 20:58

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.