1
\$\begingroup\$

This class is designed to decide if a ticket is qualified or not. A qualified tickets is one that is open and not assigned or if a ticket is a work in progress and assigned it can be taken over. It also validates if a ticket has proper ticket form is CSXXXXXXX. Any and all advise/ideas are wanted and appreciated!

class Ticket:
        def CSCheck(serviceNow:object):
            while True:
                CSNumber = input("CS Number: ")
                if CSNumber.startswitch("CS"):
                    if " " not in CSNumber:
                        if len(CSNumber) > 9 or len(CSNumber) < 9:
                            print("Ticket numbers are 9 characters long")
                        else:
                            return CSNumber
                    else:
                        print("Ticket cannot contain spaces")
                else:
                    print("Ticket's must start with CS")
    
        async def qualifyTicket(username:str,computerInfo:object):
            available = await checkState(computerInfo)
            assigned = await checkAssigned(username,computerInfo)
    
            if available = "open" and assigned != "":
                print("This ticket appears to be assigned by a supervisor")
            elif available = "open" and assigned == "":
                #Ask user if they would like to have this ticket
            elif available = "in progress" and assigned == "":
                # shouldn't happen later check last user to add notes
            elif available = "in progress" and assigned != "":
                while True:
                    takeover = input("{0} is working on this ticket, take it over (y/n)".format(assigned))
                    if takeover == "y":
                        # begin assigning process
                    elif takover == "n":
                        # later functionality to check another ticket
                        sys.exit(0)
    
        async  def checkState(computerInfo:object):
            current_state = computerInfo["CS"].state
            current_dispo = computerInfo["CS"].u_disposition
            if current_dispo == "No Longer Required":
                print("This ticket is no longer required")
                return False
            else:
                if current_state == 1: #Open
                    return "open"
                elif current_state == 2 or current_state == 11: #Work in Progress/On Hold
                    print("It appears this ticket is already being worked on.")
                    return "in Progress"
                    
                elif current_state == 3 or current_state == 7: #Closed/Cancelled
                    print("This ticket has been closed or cancelled.")
                    # later implement feature to check another ticket?
                    sys.exit(0)
                    
        async def checkAssigned(username:str,computerInfo:object):
            technician = computerInfo["technician"].user_name
                if computerInfo["CS"].assigned_to != None:
                        return technician
                else:
                    return
    
        async def userAccept(username:str, computerinfo:object):
            accept = input("Do you accept this ticket (y/n): ")
            while True:
                if accept == "y":
                    await computerInfo["CS"].updateCS("assigned_to",username)
                    await computerInfo["assigned"].__init__(computerInfo["CS"].serviceNow,computerInfo["CS"].assigned_to)
                    await computerInfo["CS"].updateCS("u_wi_primary_tech",username)
                    await computerInfo["technician"].__init__(computerInfo["CS"].serviceNow,computerInfo["CS"].u_wi_primary_tech)
                    await computerInfo["CS"].updateCS("state",2)
                    await computerInfo["CS"].updateCS("u_substate", " ")
                elif accept == "n":
                    # later implement feature to check another ticket?
\$\endgroup\$
1
\$\begingroup\$

Some comments:

  1. This is not really a class, but rather a namespace, and seemingly a pointless one, given Python code structure.

    None of your methods are instance methods, (non take or need the self parameter), nor does your class contain any data fields.

    It seems more like a collection of mostly unrelated utility functions, so there really is no advantage in grouping them under a class instead of under a file, which you can treat as a module.

  2. The CSCheck function, which seems more suited to be called getValidCS, does not check the 'XXXXXXX' part of the ticket id. Can any character go there other than a space? Like letters, or things like @ $ # etc?

    I would recommend using a regular expression, as tight as possible, and reducing the number of individual checks. You might not be able to tell the user precisely what part of his input is bad, but you will have proper control over the validity of the ticket identifier.

    This will also make your code shorter and more maintainable.

  3. The checkState method seems to be particularly problematic because its behavior varies greatly:

    It can return a string, return boolean, or just end the program. While Python's dynamic typing allows this, it is not a very good practice, as return values of such functions are harder to handle, especially handle correctly.

    Also, it is not quite clear what purpose converting numeric status values to sting status values serves, if you are not going to use those strings in UI?
    It seems, from your code structure, it would be better to leave the status as numbers, as that will reduce risk of bugs due to typos.

    It would also be better to assigned them to named constants, both to make them more readable and help the interpreter catch some of the bugs.

    Though Python does not have true constants, the use of all upper case class members or global (file level) variables is commonly considered to define constants.

    The math module even violates this rule and uses lower case constants like math.pi

  4. Similar thing with checkAssigned:
    In one branch it returns no value, not even None

    It also demonstrates a strange design feature of the computerInfo object: There seems to be no connection between the actual assigned_to field and the field holding the name of the person the ticket is actually assigned to.

    I would consider reworking that to make this function obsolete.

\$\endgroup\$
1
  • \$\begingroup\$ I completely agree with your first point concerning vaudating the number (I found out I could use the re module). As for the namespace mention, how could I reformat this to match that idea (Sorry still very new to truly pythonic design)? After for the computerInfo object, it’s a stored dictionary of 5-6 objects, that’s why it has a lot of computerInfo[key].variable. I’ll probably post that as a rework question later. I really appreciate your help! \$\endgroup\$ – Michael Canady Oct 3 '20 at 16:31

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.