Why You Shouldn't Mix Direct Returns with Callbacks

hello and welcome to senior code review buddy my name is Chris and today I'm going to talk about why I think you shouldn't have functions that mix returning values directly with returning values through callbacks this idea was suggested to me by a friend who ran into code similar to one of today's examples while working on an open- source project my friend found this pattern made it fairly frustrating for them to try and do the work they needed to and if you stick around to that example I'll let you know which open source project it was and I think you've probably heard of this project before thanks for the idea Fabio now most of the time when calling a function if there is a value to return it'll be returned right away just as part of the float and if you're dealing with a single-threaded program this is the main way to return a value but once we start getting threads involved a few more options open up like having the function take a call back to return the results later and these different options can help smooth out how program operates an IO heavy function can be offloaded to another thread only calling back to the main thread when it's done instead of blocking the main thread the whole time it can be harder to follow the flow of code when you've got multiple threads but the speed gains can be huge but you can have things get even more complicated if your functions sometimes return directly and sometimes use callbacks instead of just sticking with one I've got two examp examples today showing how this could go wrong and I'll share my suggestions for how to clean them up all right so for our first example we've got some initialization code we've got initialize here which calls into initialize database which either returns synchronously right away true or false or it will take some time and call into ASN initialization on another thread which will wait some amount of time and then set the event to signal that it's completed um these functions aren't doing anything other than communicating with each other right now just because it can be really complex already to work with threads so I figured we would just focus on sort of the interactions between each other so now when we look at initialize it needs to sort of handle the two cases I talked about we could either have value set right away or we could get the event telling us that it we need sort of wait on this event to know if it's initialized so so if we get event we've got the little code here that we'll handle and we'll just wait for 2 seconds and if the event is triggered in 2 seconds we'll assume value equals true and everything is good um if it doesn't we're actually just going to assume that the database has failed to initialize we don't in this flow communicate if there was a failure um if you actually look at the code it's just it's going to wait for 5 seconds so there aren't real failures here it's just it took too long but we'll just assume the way this is written that's kind of what happens and and yeah then here we've got the value yeah the initialize is completed or it wasn't and I have added a few tests below here um that that I can run just to make sure that things work and that yeah they're all good and that if I make any changes that things still work as expected now in the real world we probably wouldn't have this code waiting on the event right after we'd probably run some other code first um but I've left it here since there isn't other code to run and it makes our function easier to understand but I think we can agree that overall the result here is complicated like all we're doing is trying to initialize the database and this is a fair amount of code just to figure out if it was initialized or not so how can we improve this and can we improve it in such a way that we could have other code you know inserted here but between starting the initialized database and actually blocking for the result I think we can and I think the first step to make things clear is to stop returning to items callers should just be given a single value to worry about so let's create a new class initialization State we'll go up and to create initialization statation state so we'll start by taking what state could be um and we'll also take an optional q that we'll call Q um that's how we'll communicate across the threads instead of just waiting on an event so okay and then we need to um we'll then have two functions say get state which will return the current value of the state and wait for state which will block until we get the state and then return it um let's start by writing wait for State first all right so for wait for State we'll actually we want a time out in case people don't want to wait and so if we have a que and our state hasn't been set yet oops isone we will try fetching state from the cube and setting it to the state so you time out um and we'll also want to catch if the cube is empty CU that just means we still want to keep waiting let's see normal um yeah because maybe we called it with a 2C timeout and just wasn't ready yet and then as I mentioned we return our state and yeah that's all we need here it may still be none at that point or not okay and now that we've got wait for State done we'll do get state which actually is going to be quite easy CU we're going to be a little sneaky we're actually just going to call wait for State um but we'll set time out to none um because then it's not going to block anymore so it will do the fetching of the Q if anything is in it but will return right away oh I misspelled that so this way we're only sort of writing this function and this one kind of benefits from it for free all right so now that we've got initialization State done let's update initialize database to use it so the two synchronous cases are easy we just pass in the state we don't have to worry about setting a q because there is no other thread that is not needed okay and then yeah down here we are going to use a q as i' mentioned instead of an event so we get rid of that we pass in the Q thing there and then instead of returning this we want it to be the initialization state where the current state is none um and then we pass in Q and then we just have to update this I do q.p put and we'll only return true cuz we'll assume this kind of always works if it happens yeah so now initialized database is good now that we've completed initialize database to return the initialization State let's update initialize to work with that so instead of returning two values we just return State and then we will just say value equals state do wait for state and we'll still have it wait 2 seconds since that's what we did before and we don't need any of this code then and that's it and we run we make sure that everything still works yeah got expected well yeah so it's all good so I think this is much cleaner than before and by returning this initialized state callers have a much simpler interface to deal with if we wanted to add new code it's really easy to add stuff in here before we query state so yeah there's only sort of limited stuff have to worry about okay um with that done let's jump over to the second example so this example is a bit more complicated we've got a function query database that can do one of three things when we call it it can return a value right away this could be viewed as sort of finding a value in a cache um it can return a value later by calling a call back this can be viewed as you know it's not in the cache so have to do a slow database query or in the third case you query the database don't find the value and you don't actually call the Callback so you don't tell the user and as I said before I'll run them again but I do have some tests yep some tests for this one to make sure that if anything breaks as I mess around with it I find out so I'll start off by saying that I I really don't like this third case because as the caller of this function if the call back hasn't been called I don't know if it's I haven't waited long enough or because it's not going to get called back and there's no way to figure that out and it makes it a lot trickier to work with this function now this wasn't something I came up with on my own either this was the sort of situation that my friend found thems in when they were working on the open- source chromium project yep chromium which is used to develop the Chrome browser the edge browser and a bunch of other browsers I think so despite having all of these excellent Engineers on this project issues like this can still sneak in no one is going to have perfect code all of the time now did this enter this way because there was time pressure someone new to the team or did it just evolve over time I don't know but I think it's a reminder that bad code can kind of sneak in anywhere so we won't worry about the chromium example for now but how can we fix the example here well similar to last time I think we should adjust the interface to only return in one way we should get rid of returning directly here and return through the call back so the callers only need to worry about getting the information through the call back another change that I'm going to make is I'm going to actually return something here um I want users to know that okay the call back has been called I don't need to wait any longer it is done so we'll start by handling the synchronous case n are so we won't return directly instead we will have an else so if we don't hit that exception above do a create a thread which we'll call the call back with oops with a value none cuz we don't need to continue anymore so let's make sure everything still passes for that yep they all still passed that's good and now we'll clean up the other section which is just call back down here now I will point out that one of the challenges with using the Q here is if these values could be none we wouldn't be able to tell the difference between we didn't find anything versus we found something and it's none um but at least for this test case I've got here I'm assuming that the valid values to find our only integers so we won't worry about that uh so yeah so that's all we need and this function is updated can run make sure everything is still good yep and then when we go down here we actually can get rid of result now because we're never returning directly so want set results here and run again and make sure it's all good yep so we've got this I think it's cleaner than before um now this example was a bit easier to clean up than our first example it also was a lot easier than the chromium example since I have seen that so how how did my friend approach cleaning up that chromium example they did what I consider to be one of the hardest things to do they looked at it and decided it wasn't worth the effort of fixing while the code was challenging for them to work with and really could use a cleanup the value of cleaning it up just wasn't there this code isn't expected to be modified often so the time gained in Faster future changes would be minimal this code is also very important for chromium so if something accidentally broke during the cleanup and it wasn't caught by a test the cost here could be very high so after looking at the pros and the cons it was decided that at least for now this was one issue that just wasn't worth fixing thanks for walking through that with me today hopefully the next time you are working on a function that could either return a value directly or through a call back you'll commit to Just One path I think the callers of that function will thank you and that could even be you and thanks again to fabul for suggesting this great idea if you enjoyed this video please consider hitting the like button below and subscribing if you have any other comments code you'd like me to review or ideas you'd like me to talk about please leave a comment below or reach me directly at Chris senior codeview buddy.com thanks and have a great day [Music]

Channel: Senior Code Review Buddy

Views: 447

Keywords:

Id: VF-Kfn4zCuA

Length: 14min 51sec (891 seconds)

Published: Thu Jun 27 2024