2

Only loosely related to the title.

In my code I often encounter functions such as this one (e.g. from a pet project – a Discord bot written in Python – but seemingly a ubiquitous and reoccurring issue), which I'm struggling to refactor in an attempt to make it more readable/maintainable (ignore the async/await, treat it as a normal function):

async def execute(self, ctx: CommandContext):       # 1
    try:                                            # 2
        result = self.get_calculation_result(ctx)   # 3
        self.store_success_calc_result(ctx, result) # 4
        await ctx.msg.channel.send(str(result))     # 5
    except (ArithmeticError, ValueError):           # 6
        await ctx.msg.channel.send('bruh')          # 7

This function is called (eventually) when a certain command (calculate) is invoked by the user.

What I'm trying to accomplish is for my functions not to do conceptually multiple things, or things that aren't immediately clear from their names, and I'd like to avoid leaving the level of abstraction set by the function's purpose.

The main problem is line 4, store_success_calc_result, which has a side effect, and which I'm therefore struggling to include into any of the other functions (here there's only one, get_calculation_result). If I put it in get_calculation_result (or calculate, if we rename it to be less specific for some reason), will the reader know that's what's going on inside when they look at its name (get_calculation_result/calculate)? They won't. If I do get_calculation_result_and_store then that's ugly and unmaintainable (e.g. what if I later wanted to send the result to an external API within that function? I'd have to add that to the name, right?).

The above code is seemingly completely fine because it's so short, but not only does the setter function come out of the blue, on line 5 we can immediately see a problem, which is that we need to additionally process the result in order to send it, i.e. convert the result to a string. That means execute is already doing two things where it really shouldn't. (It's not that I'm trying to stick to some rules for their own sake, I just predict it might be a problem in the future if we apply more logic to it.) It'd be great if I could just do this:

async def execute(self, ctx: CommandContext):         # 1
    try:                                              # 2
        result = self.get_calculation_result_msg(ctx) # 3
        await ctx.msg.channel.send(result)            # 4
    except (ArithmeticError, ValueError):             # 5
        await ctx.msg.channel.send('bruh')            # 6

and within get_calculation_result_msg or whatever, I format the calculation result (done by get_calculation_result, or such). But I can't, precisely because I'd be hiding store_success_calc_result somewhere in the calculation functions. And note that store_success_calc_result is fine, but imagine if this function were a locking/unlocking mechanism, i.e. something quite low-level that shouldn't even concern the calculation process or message sending (or the reader, who's just trying to comprehend what execute is doing, and why), i.e. it's something that breaks the current level of abstraction (which it already is doing). Imagine there were 3 more function calls which do seemingly random things from the reader's POV, and now we need to include some conditional logic in execute (e.g. if some different-abstraction/semantic-level condition is satisfied, do something with the current result) – suddenly we've spread a lot of business logic around instead of keeping it where it should be. Note that store_success_calc_result requires a plain result, while send requires a string, which is potentially already conditional logic (assuming another use case or, concretely, command). As the application grows, there might be more conditions that need to be satisfied, and more functions which have side-effects, which need to be called in execute.

There probably is no silver bullet, but what's the standard practice on how to deal with these things, or maybe a pattern to ensure separation of concerns and maintainability in cases like these?


My idea here would be to call the storing function inside another, but to make up for it by using a name that suggests there's more to the calculation; perhaps execute_calculation_process, which returns the calculation result (or raises an exception), but also does more things inside. In that process there might be pre- and post-processing hooks which would be indicative of side-effects, but which could be "stacked" and separated from the main logic.

async def execute(self, ctx: CommandContext):       
    try:
        process_result = self.execute_calculation_process(ctx)
        await ctx.msg.channel.send(process_result.msg_value)
    except (ArithmeticError, ValueError):
        await ctx.msg.channel.send('bruh')

def execute_calculation_process(self, ctx: CommandContext):
    
    for hook in get_pre_processing_hooks():
        hook.execute(ctx)

    
    result_object = self.get_calculation_result(ctx) # not just result value

    for hook in get_post_processing_hooks(): # one of these is store_success_calc_result
        hook.execute(ctx, result_object)
    
    return result_object

Now this code would hardly ever require any modifications, and it follows the basic principles, in my opinion. The calculation function could be extracted into separate strategies, etc., the hooks could simply be included/excluded, containing all kinds of side effects. Is this over-engineered? Is there a simpler way? Thanks.


Addressing comments:

The reason the main function here is called "execute" is because the class it's in extends a base command one, which contains that function. Therefore, this code overrides it. I iterate over instances of the base command class, find the first applicable, and execute it.

By "command" in general I'm referring to a Discord bot command. Users enter these commands, and the bot executes them. The command could be implicit like 1+1 (which is recognized as a calculation command, i.e. the one at issue), or yt pop songs, which searches for "pop songs" on YouTube and returns the first few results. Renaming execute would solve nothing.

The relevant context here is the calculate command, which in this case can also be invoked like ans + 1, which would result in 1 added to the last calculation result for the user that issued the command. But we can easily imagine all sorts of extensions of this command in the future. For example, maybe we want to support "variables", where for instance var a 1 + 1 would require other store operations. Or we could let users define functions, in which case the pre-processing hook would have to fetch them before the calculation happens. Etc.

2

1 Answer 1

3

Some commands have side-effects, some do not and its their business. In context of a general purpose chatbot, there is no particular guarantees that apply to all commands - they can do anything.

If a specific command wants to store the calculation results, it that very command's implementation detail. (Other commands could omit this step). Therefore, given context, store_success_calc_result is located at the appropriate abstraction level.

However, input parsing, error handling and result transmission are not!

Consider instead:

async def execute(self, input: String):
    try:
        result = self.get_calculation_result(input)
        self.store_success_calc_result(ctx, result)
        return str(result)
    except (ArithmeticError, ValueError):
        raise UserError('bruh')

async def handle_command(command, ctx: CommandContext):
    try:
        result = await command.execute(ctx.input)
        await ctx.msg.channel.send(result)
    except UserError as e:
         await ctx.msg.channel.send(e.message)

Here we have transmission layer, that receives, parses, dispatches and transmits responses. And it is distinct and separate from command layer, which only concerns itself with business logic of commands.

4
  • And what if the command shouldn't send something to that channel, or should send something to another one? This solution is unfortunately short-sighted, or perhaps I wasn't clear. The current flow is as follows: (1) a generic on_message event handler is called, which (2) iterates over all command-handling strategies, finding an applicable one, and (3) executing it (hence execute), as you can see in the question. Commented Mar 23 at 16:11
  • The whole point of my question was what you've now moved into a separate method, therefore delaying the inevitable maintenance issue: imagine in the future we want to update something else in the DB depending on the result or the surrounding context - suddenly you're adding a line to (your) execute, and maybe an if, threby complicating the logic. Why does execute do 15 things? It should do one thing, conceptually, otherwise you're not sure what you're doing in it. It's a mess. Commented Mar 23 at 16:24
  • Absence of response is perceived as a bug in chatbots. Null Object can be used if absolutely necessary. If your business logic changes, ask another question. There is no universal approach in this industry yet.
    – Basilevs
    Commented Mar 24 at 13:35
  • Given context, the is no further need for abstraction of result persistence. Any kind of abstraction just "delays the maintenance issue". It is not inevitable though. YAGNI
    – Basilevs
    Commented Mar 24 at 13:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.