Validating Support Tickets

Question

Being given the following code I wonder how can I make it better, without using variable valid, yet check ALL the cases and if at least one of them matches set the returned value to False

  def validate_ticketer_issue(self, ticketer_issue):
      """
      Validate ticket system issue for common rules
      :param ticketer_issue: (str) ticket system issue key
      :return: (bool) True if issue is successfuly validated
      """
      valid = True
      issues_not_allowed = ('Pool', 'Document', 'Declaration', 'Restriction', 'Pre-Check')
      issues_not_allowed_inprod = ('Analysis', 'Test')

      try:
          issue = self.service.issue(ticketer_issue)
          if len(issue.fields.versions) == 0:  # Affects Version/s
              valid = False
              print('{}: Affects Version/s field is empty'.format(ticketer_issue))

          if issue.fields.issuetype.name in issues_not_allowed:
              valid = False
              print('{}: According to working model commit is not'                        
                    ' allowed to the ticket system issue types'
                    '{}'.format(issue['key'], issues_not_allowed))

          if issue.fields.issuetype.name in issues_not_allowed_inprod:
              valid = False                             
              print('{}: According to working model commit' 
                    ' to the ticket system {} issue types'
                    ' is not allowed in production code'.format(
                        issues_not_allowed_inprod, issue['key']))

          if issue.fields.issuetype.name == 'Fault':
              if not self.validate_fault_issue_summary(issue):
                  valid = False

      except ticketer.exceptions.TicketerError:
          valid = False
          print('{}: issue does NOT exist in the ticket system!'.format(ticketer_issue))

      if valid:
          print('{}: OK'.format(ticketer_issue))

      return valid

Answer 1

The code you show is explicit about what it does, easy to read and understand. I suppose you find the usage of the valid flag error-prone in case the logic gets deeper nested, or any sort of refactoring that might forget about that flag?

When you encounter an error, you are actually doing two things : setting the valid flag to False and printing an error. One simplification would be to make a list of error messages (no valid variable), and push error messages on it. You don't print anymore, and the list of errors becomes the valid flag.

def validation_function(item):
    errors = []

    try:
        if not test1:
            errors.append("Error on test 1 for item")
        if not test2:
            errors.append("Error on test 2 for item")
    except SomeException as e:
            errors.append("Exception on whatever for item")

    # change the return type to list of errors
    # reporting errors is responsability of caller.
    return errors

You could raise an exception from inside the validation_function but I see it as a low level validation routine which collects errors and returns them. The caller is responsible for handling (raise, print) the returned errors if any.

Also, your validate_ticketer_issue is actually harmonizing errors : it leads its own set of tests but also catches exceptions raised by functions it calls and translates that to a boolean + error messages. The above example preserves that behaviour which is all right as long as you are very specific about the exceptions you catch.

Finally you could see the function as a bunch of predicates with specific error messages run one after the other.

# List of predicates and messages. Predicates must have a 
# harmonized api. Note: I'm using lambdas here but regular 
# functions will work too.
PREDICATES = [
    (lambda item: some_logic_here, "Error on test 1 for item"),
    (lambda item: some_other_logic_here, "Error on test 2 for item")
]

def validation_function(item):
    errors = []

    try:
        for predicate, msg in PREDICATES:
            if not predicate(item):
                # here you have to harmonize you messages
                # so that they can be formatted using the
                # same argument.
                errors.append(msg.format(item=item))
    except SomeException as e:
            errors.append("Exception on whatever for item")

    # change the return type to list of errors
    # reporting errors is responsability of caller.
    return errors

This example might seem more scalable but for a small amount of predicates, it is probably overkill as understanding the code is not obvious, logic lives in other places, and some work has to be done to harmonize predicates and their messages.

These are the few ideas I have to improve the code so that it doesn't rely on the valid flag. The second step makes it perhaps more scalable at the expense of readability.