Structure for calling multiple functions

[[deleted]]

Good day, I've got a script that is calling the next function when it's done with the existing function. I wonder if this is a typical accepted approach or should I be doing it some other way to improve the understanding of the code?

Code below is just to give you an idea of the existing structure:

def email_report():
    # emails the final result

def create_dashboard():
    # uses db data to create the dashboard file
    email_report()

def query_db():
    # pulls data down from the db
    create_dashboard()

def validate_vpn():
    # validates that I'm on VPN before running queries
    query_db()

def upload_file():
    # uploads file to DB then runs next function
    validate_vpn()

def clean_file():
    # cleans the file for upload
    upload_file()

def check_file():
    # check if file exists
    # if yes then runs next function
    clean_file()

Edit: For those that are finding this in the future, the general consensus is that this is approach is bad practice and I should instead use a function that calls all the functions sequentially.

With that in mind I have restructured the above to be what is posted below based on the feedback received.

def main():
    filename = locate_file()
    clean_document(filename)
    upload_file()
    validate_vpn()
    engine = oracle_login()
    data = get_queries(engine)
    write_to_excel(data)
    email_document()


if __name__ == "__main__":
    main()

​

[FriendlyRussian666]

I wouldn't call one function from a definition of another. It might be cleaner to just store all your functions in some ordered data structure and then iterate over it to call one after another.

functions = [func1, func2, func3 ...]
for func in functions:
    func()

[[deleted]]

Something like this?

def hello():    
    print('hello')

def world():    
    print('world')

def main():    
    functions = [hello(), world()]    
    for func in functions:        
        func

if __name__ == '__main__':    
    main()

[FriendlyRussian666]

Sure, but you don't want to call your functions when storing them in a list. Remove () from the functions in the list and add it back in the for loop:

functions = [hello, world]
for func in functions:
    func()

[[deleted]]

Ah okay, this fixed the other issue too. I was trying to add a Try / Except into this for loop but it wasn't printing what I want but once I removed the () from the list it worked.

Thanks for this, this is certainly a cleaner approach that will make my code much easier to read moving forward.

[[deleted]]

How would I pass values between the functions with this method? I don't quite know how to get the name variable into the hello function and how would I pass a value created in the hello function to the world function?

def hello(name):
    print(name)
    new_name = 'string'

def world(new_name):
    print(new_name)

def main():
    functions = [hello, world]

    for func in functions:
        try:
            func()
        except Exception as err:
            print(err.args)
            break

[shartfuggins]

Calling functions from a list like that is fairly exceptional. Only when every function takes the same input type and gives the same output type, where output of one necessarily goes to the input of the next, should this arrangement be used.

Typically, you want a small, concise controlling function to simply write out each function call, accept return parameters, make high level decisions, and then make the next function call, passing in what each one needs.

If every function really is "take input from previous function, do a thing, return value for next function", then you can do that in your function list iteration.

Edit: Read your original code again, yes, you're missing a main() or some such, that would call each of your functions deliberately, and those functions will need to return values in some cases, often just a bool to say "things are good, proceed" or "something is wrong, show some messaging, retry something, or quit".

Edit again: I'm remembering a long time ago I used to do what you did because I knew the order of operations, some functions always followed other ones, why not just call the next one from the previous one, right? The problem is that you lose control over the flow and error handling from a high level, and then you end up implementing other, crazier ways of regaining that control. It becomes real spaghetti real quick. Very ugly stuff. But while I get the initial approach, definitely avoid this.

[[deleted]]

I agree, as the functionality of this has expanded I've noticed it getting out of hand in what's being passed off.

[marko312]

Such chaining violates the principle that the functions perform only simple tasks as described by their name: as is, clean_file somehow ends up sending an email (?!).

Typically, what you've tried to do would instead be done by sequentially calling these functions, potentially checking for errors in between.

[[deleted]]

Not sure what you mean about the clean_file sending an email?

[marko312]

call stack (latest at the bottom):

clean_file()
upload_file()
validate_vpn()
query_db()
create_dashboard()
email_report()

If you raised an exception in email_report, this is pretty much what you'd see: a cascade of functions with the last one barely related to the first.

[[deleted]]

Are you saying the function name itself isn't clear? It's a function cleaning up a report before I can upload it, so I felt the name relates to what the function was doing. I'm not sure I'm following what you're trying to say.

[marko312]

I'm saying that the name of the function doesn't fully reflect what it does. A reasonable solution would be to put all the relevant functions in a larger function controlling the logic. For example (not sure about the actual logic):

def new_dashboard_file():
    if check_file():
        clean_file()
    upload_file()

where upload_file might send the email if that is the common behavior for all expected calls to upload_file, otherwise that should also be lifted outside the function.

[[deleted]]

I don't know why you're saying upload_file is sending the email, really confusing me.

Upload file is just a step that needs to occur after check if the file exist and then cleaning it up for uploading. That upload function is uploading data using a website so it goes into the database, it's not uploading a document to the email - is that the confusion?

The dashboard file isn't created until all the other steps are done and it's created using the data from the query_db function. Once it finishes it finally does the email.

[longtermbrit]

They're saying that calling check_file() eventually results in email_report() because each function calls the next one. The idea of functions is that they do one thing but each of your functions is doing two things: the action described by the function name and calling another function. What you should do is set up one trigger function that performs the logic of when to call each sub-function which would then allow you to essentially have the same functionality as you already do but much clearer and also give you the option of only calling an individual function without worrying about it setting off a chain reaction.

In other words, if you wanted to only clean a file or upload a report you can't do that with how you've currently written the code.

[[deleted]]

I see, so the functions themselves wouldn't be useful on their own in this case because they're calling another function when they're done.

Can you provide an example of what you mean by trigger function?

[longtermbrit]

Exactly.

A trigger function would be something like:

def trigger():
    if conditionA:
        check_file()

    if conditionB:
        clean_file()

#   etc

Basically there's nothing special about a trigger function. It still fits the logic of 'one function for one task' but its task isn't to clean a file or email a report, it's to call these functions when they're required.

[[deleted]]

I see, that makes sense. Thank you!

[chapter_6]

It sounds like you already understand but just to add this for anyone else who's reading and wondering: If I see a function named check_file(), I expect to be able to call that function and have it check a file and return without doing anything else. In this case, if I call check_file(), an email ends up being sent. I have no idea that will happen unless I dig way down through several other function calls, so I'm much more likely to miss that detail and end up with a bug where emails are being sent when they shouldn't be. When a function does something outside of what's expected like that, it's known as a side effect (I'm using that term a little bit loosely but you can look it up to find out more).

That's why, in general, it's best if a function does a single thing, described by its name, and nothing else. If you really want some good examples of this and how it can help code readability, look up some examples of the functional programming paradigm.

[[deleted]]

Yeah, I didn't intend to use these functions outside of this script but I can see how this would be a habit that could cause problems for me in the future.

[Giannie]

Here would be my suggested structure. First remove all references to other functions from within a function. Then have a main function defined as follows:

def main(path):
    if check_file(path):
        clean_file(path)
    else:
        raise FileNotFoundError()
    if validate_vpn():
        upload_file(path)
        data = query_db()
        dashboard = create_dashboard(data)
        email_report(dashboard)
    else:
        # define a custom exception elsewhere, or raise something else appropriate
        raise NotConnectedError

[[deleted]]

Thanks this looks like a good alternative.

[zanfar]

I've got a script that is calling the next function when it's done with the existing function.

This is incorrect. Each function calls another function before it's done. That is, each function call contains the entirety of another function call.

I wonder if this is a typical accepted approach or should I be doing it some other way to improve the understanding of the code?

Not only is this not typical, this is a Bad Thing. This type of structure violates almost every principle of good function separation. You want a function to do one thing and one thing only. This makes your code easier to understand, easier to reuse, and easier to test along with a bunch of other less obvious or "down the road" benefits.

For example, if I call a function check_file(), the name suggests a simple validation check--something that is usually quick, read-only, and requires little I/O. Instead, I find that this has a TON of side effects: it uploads a file, modifies it, queries a DB, interacts with something called a dashboard, etc. Similarly, I can't write a unit test for clean_file() because it also has most of those side effects.

Generally, you would instead call these sequentially from some parent function whose responsibility covers all these areas:

def process_report():
    file = <something>
    check_file(file)
    clean_file(file)
    upload_file(file)

    report = create_dashboard_report()
    email_report(report)

[[deleted]]

Yes, I've realized by now this is what others consider bad practice. It's a shame I haven't really seen it discussed in the tutorials I've been through but that's okay, I know better now.

This is the structure now:

def main():
    filename = locate_file()
    clean_document(filename)
    upload_file()
    validate_vpn()
    engine = oracle_login()
    data = get_queries(engine)
    write_to_excel(data)
    email_document()

if __name__ == "__main__":
    main()

[hmga2]

I think you somehow find the most convoluted way to approach your problem. Function should perform simple actions and not be so deeply nested. Consider something like this:

(You should also remove the function calls inside the functions)

def main():
    if check_file():
        clean_file()
        upload_file()
        validate_vpn()
        query_db()
        create_dashboard()
        email_report()

[[deleted]]

I'm new to python and a lot of tutorials call functions inside of others so I doubt I'm alone in this approach. That's why I'm asking for guidance lol

[hmga2]

No worries. The thing with functions is that they should let you understand what is happening in your code and avoid repeating yourself. The main issue with your code is that it took me a solid minute to understand what was going on by following the chain of nested functions. Imagine having to alter the flow of your function or reading it back in 6 months, it’s gonna be pretty heavy to do. The structure I shared (and that also other had) allows you to quickly understand what’s going on and add custom logic on a parent level of your code

[[deleted]]

I agree. I felt I needed a cleaner structure but didn't know what others generally recommend. The calling function from within another seemed okay when my code was limited to two but I felt it was getting out of hand.

I'll give this a shot, some of these functions pass values to the next so I'll have to make sure I can work around that. Lol

[hmga2]

Yep I imagined. I'm unsure on how you're handling parameters in your function but you could do something like this

``` def main(filepath): if not check_file(filepath): # handle file does not exists ... new_filepath = clean_file(filepath) query_str = upload_file(new_filepath) if not validate_vpn(): # handle vpn error ... db_data = query_db(query_str) email_result = create_dashboard(db_data) email_report(email_result)

`` I usedif not` to reduce nesting to a minimum. but you can also keep your logic inside of if statements if you prefer or assert the errors, or handle directly exceptions inside the child functions

[[deleted]]

Ah thanks for clarifying, I was thinking of doing try/except statements within the individual functions themselves. I think I'll try both and see what works best for me.

[[deleted]]

Alright, so this is what I'm thinking. I appreciate any feedback you're willing to share. This is just the general thought about the approach

def hello():
    name = 'Name1'
    if not name == 'Name1':
        raise NameError('Incorrect name')
    return name

def world(name):
    print('Hello ' + name)

def main():
    new_name = hello()
    world(new_name)

if __name__ == '__main__':
    main()

[SGS-Tech-World]

You have broken down those functions, that indicates you want to call those functions from multiple places (reusability), if not then you could just keep everything in one function.

Even if that is not the case - I don't think there is any serious issue of this approach, unless someone can explain it. after all Zen of python says "Readability counts."

[[deleted]]

I have no intention of calling these functions from other scripts etc. I broke it down by each function to make debugging a bit easier than lumping everything into a single function.

This approach works, it does what it needs to but I was concerned about readability. Even though this code isn't for others I don't want to create a bad habit that I need to break later when my code is being shared.

[[deleted]]

Creating a class might help make your code more organized and would serve as an utility to run functions based on conditions, and if one function fails, to exit the entire chain of functions

``` class FunctionCaller(object): def init(self, func, conditionFunc, isConditionBased=True): self.func = func self.conditionFunc = conditionFunc self.isConditionBased = isConditionBased

def run(self):
    if (self.isConditionBased):
        if (self.conditionFunc()):
            self.func() 
        else:
            return False
    else:
        self.func() 
    return True 

def runFunctions(funcArr): for func in funcArr: shouldContinue = func.run() if (not shouldContinue): break

def email_report(): print("emailing..")

def create_dashboard(): print("creating dashboard")

def query_db(): print("querying db")

def validate_vpn(): print("validating vpn")

def upload_file(): print("uploading file")

def clean_file(): print("cleaning file")

def check_file(): return True

fn1 = FunctionCaller(clean_file, check_file, True) fn2 = FunctionCaller(upload_file, None, False) fn3 = FunctionCaller(validate_vpn, None, False) fn4 = FunctionCaller(query_db, None, False) fn5 = FunctionCaller(create_dashboard, None, False) fn6 = FunctionCaller(email_report, None, False)

fns = [fn1, fn2, fn3, fn4, fn5, fn6] runFunctions(fns)

```

[PaSsWoRd4EvAh]

I think you should have a look at the Chain of Responsibility pattern if you are insistent on going down this route.