-3

I'm drawing a blank on what should be a simple question so any help is appreciated.

Is there a better way to handle all these 'if' statements rather than having them copied into each function?

This isn't the whole code but it should show my issue. Since they are all the same 'if' statements, can I just create a dedicated function for them then call them into each function or?

def btn_add_printer(self):

    printer = str(self.ui.comboBox.currentText())
    if printer == printer1_name_short:
        network_printer = printer1_name_long

    if printer == printer2_name_short:
        network_printer = printer2_name_long

    if printer == printer3_name_short:
        network_printer = printer3_name_long

def btn_remove_printer(self):

    printer = str(self.ui.comboBox.currentText())
    if printer == printer1_name_short:
        network_printer = printer1_name_long

    if printer == printer2_name_short:
        network_printer = printer2_name_long

    if printer == printer3_name_short:
        network_printer = printer3_name_long
khelwood
  • 55,782
  • 14
  • 81
  • 108
Alex
  • 5
  • 2
    Yes, if you have a bunch of duplicated code, you can put it in a function and call that function each time you need to invoke the code. – khelwood Dec 13 '19 at 22:37
  • 1
    "Since they are all the same 'if' statements, can I just create a dedicated function for them then call them into each function" Yeah, that makes sense. note, you should use `if elif else` to make it more efficient – juanpa.arrivillaga Dec 13 '19 at 22:37
  • Thank you guys. That makes sense, I'll also use if elif else. – Alex Dec 13 '19 at 22:59

3 Answers3

2

You can use a dictionary to assign the printerX_name_long.

def get_network_printer(name_short):
    return {
    printer1_name_short: printer1_name_long,
    printer2_name_short: printer2_name_long
    }.get(name_short, "default")

def btn_add_printer(self):
    network_printer = get_network_printer(self.ui.comboBox.currentText())

This is assuming you want to avoid multiple if-else statements. Otherwise, the answer by Chris suits better.

More possible relevant answers here

S.Au.Ra.B.H
  • 457
  • 5
  • 9
  • 1
    Note: The larger the `dict` gets, the more important it will be to define it *outside* the function; as written, you're rebuilding it from scratch on every call, so you don't get the benefit of reducing work in any meaningful way. – ShadowRanger Dec 14 '19 at 04:45
0

So since both functions use exactly the same code you could create a function that does this code, and then call that function from the other two. something like

class my_class:
    def btn_add_printer(self):
        self.printer_func()

    def btn_remove_printer(self):
        self.printer_func()

    def printer_func(self):
        printer = str(self.ui.comboBox.currentText())
        if printer == printer1_name_short:
            network_printer = printer1_name_long
        elif printer == printer2_name_short:
            network_printer = printer2_name_long
        elif printer == printer3_name_short:
            network_printer = printer3_name_long

        #not sure what you want to do with network_printer but can do it here
Chris Doyle
  • 10,703
  • 2
  • 23
  • 42
  • I appreciate the response. That makes perfect sense. One last question, I am looking to perform a different action with each function. How could I make that call? Just for example, could I put print('add') in the btn_add_printer function and print('remove') in btn_remove_printer function? – Alex Dec 13 '19 at 22:59
  • 1
    Yeah you can do what ever you want in each function. For example your printer function could return the netowkr printer. When you call that function in the add or remove you can store the netowkr printer then do what ever you like with it in the add or remove function. They can do different things with the network printer. The key is we now have the common code in its own function – Chris Doyle Dec 13 '19 at 23:01
0

Alternatively you can define a dict outside the function, much more compact.

def btn_add_printer(self):
    printer = str(self.ui.comboBox.currentText())
    if printer in self.printers: network_printer = printers[printer]

def btn_remove_printer(self):
    printer = str(self.ui.comboBox.currentText())
    if printer in self.printers: network_printer = printers[printer]

self.printers = {
        'printer1_name_short':'printer1_name_long',
        'printer2_name_short':'printer2_name_long',
        'printer3_name_short':'printer3_name_long',
}

Or pass the dict:

def btn_add_printer(self, printers):
    printer = str(self.ui.comboBox.currentText())
    if printer in printers: network_printer = printers[printer]

def btn_remove_printer(self, printers):
    printer = str(self.ui.comboBox.currentText())
    if printer in printers: network_printer = printers[printer]

printers = {
        'printer1_name_short':'printer1_name_long',
        'printer2_name_short':'printer2_name_long',
        'printer3_name_short':'printer3_name_long',
}

self.btn_add_printer(printers)

Or use a helper function:

def printerLong(self, printerShort):
    printers = {
        'printer1_name_short':'printer1_name_long',
        'printer2_name_short':'printer2_name_long',
        'printer3_name_short':'printer3_name_long',
    }

    if printerShort in printers:
        return printers[printerShort]


def btn_add_printer(self, printers):
    printer = str(self.ui.comboBox.currentText())
    network_printer = self.printerLong(printer)


def btn_remove_printer(self, printers):
    printer = str(self.ui.comboBox.currentText())
    network_printer = self.printerLong(printer)
misantroop
  • 2,276
  • 1
  • 16
  • 24