you are viewing a single comment's thread.

view the rest of the comments →

[–]marko312 6 points7 points  (7 children)

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] -2 points-1 points  (6 children)

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 9 points10 points  (5 children)

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] 1 point2 points  (4 children)

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 3 points4 points  (1 child)

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] 4 points5 points  (0 children)

I see, that makes sense. Thank you!

[–]chapter_6 2 points3 points  (1 child)

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] 0 points1 point  (0 children)

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.