# I Made a Classic Refactoring Mistake

## Метаданные

- **Канал:** ArjanCodes
- **YouTube:** https://www.youtube.com/watch?v=b6RnBKvQ40o
- **Просмотры:** 24,072

## Описание

💡 Learn how to design great software in 7 steps: https://arjan.codes/designguide.

In my previous video on refactoring complicated business logic, I made several mistakes and you spotted them. In this video, I go through what went wrong, why these kinds of errors are so easy to introduce during refactoring, and the traps developers fall into when they assume they understand the rules encoded in existing code. We’ll look at how tests, coverage, and “cleaner” code can still hide logical mistakes, and why there often is no single ground truth when dealing with business logic.

Original video where I covered the refactoring: https://youtu.be/mH7e7fs9gaE

🎓 ArjanCodes Courses: https://www.arjancodes.com/courses.

💬 Join my Discord server: https://discord.arjan.codes.

⌨️ Keyboard I’m using: https://amzn.to/49YM97v.

🔖 Chapters:
0:00 Intro
0:27 Context
1:35 Three mistakes
4:27 A few other issues and remarks
9:13 Traps / what is the ground truth?
14:40 Final thoughts


#arjancodes #softwaredesign #python

## Содержание

### [0:00](https://www.youtube.com/watch?v=b6RnBKvQ40o) Intro

In my video covering refactoring complicated business logic, I made some mistakes which you noticed in the comments section. Well spotted, by the way. I want to go over them in this video and show you exactly what I did wrong, why these types of mistakes happen, and some of the traps that you can easily fall into. And I've seen some of you do that in a comment section actually. Now, if you haven't watched that video yet, I've put a link in the description. In short, the idea of that

### [0:27](https://www.youtube.com/watch?v=b6RnBKvQ40o&t=27s) Context

video was to refactor a piece of code and clean it up. This is what the original version of the code looks like. There is this huge approve order function that's very messy, lots of complicated if else statements. And I ultimately refactored it into this version which is much shorter. It has rejection rules which is a list of lambda functions and then I've split out the various checks and conditions into separate functions. Now before I did any major refactoring, I wrote tests and I showed you that those tests actually worked. The problem is that those tests were not complete and actually I introduced a few errors in particular three that I want to address today and I can prove that the new version that I created is indeed broken because if I run piest on the original version of the code then this is what you get and I've added a few tests for these three errors that you noticed in the comment section. So when I run those same tests on the new version, you see that I now get three failed tests. So it's clear that the refactored version of the code actually changes the behavior with respect to the original version. Now

### [1:35](https://www.youtube.com/watch?v=b6RnBKvQ40o&t=95s) Three mistakes

which three errors were these exactly? The first one is that a test for an admin is run only if the user is not premium. So here you can see that so if the user is premium then we have all of this logic else and then there is the admin check. Now in the new version of the code actually there is a uh privilege override check. So if the user is admin it's always approved but clearly that's not always the case. And I wrote this test for that. So I created a user a premium user that is also an admin. And then I create some order with an amount. There's no discount. And the EU uh and the region and the currency are mismatched. So when you try to do that then actually this order is rejected in the original version of the code but in the refactored version of the code this is actually automatically approved which is a mismatch in behavior. The second problem is that if the user is in the region EU in the original version of the code there is no check for negative prices in the items. As you can see here we have this piece of logic. So if the region is not in the EU then we check for negative prices. Otherwise, we check if the currency matches. Whereas in the updated code, if you take a look here, you see that there is a lambda function that checks whether any of the item prices is less than zero. And it does this for basically all cases unless you fail earlier. So also there is a mismatch in behavior. I also wrote a test for that. So that's this third one actually. So create a user, I create an order. Uh the region currency matches and then I have an item with a negative price and I'm going to check that indeed this order is approved which is the case in the original version. There you see that all the test pass. The third issue is that if the user is a premium user and not a trial user and then places a bulk order that's under or equal to thousands, they would be approved in the original version. So that's what you see here. So if there's a premium user, if the order amount is less than a thousand uh and the order type is bulk and not a trial, then it returns approved. And that happens even if the order had a discount. But in the final version actually you can see that uh there is a rejection rule that checks whether the order has a discount. So if there is no discount then it's going to be rejected. So also wrote a test for that. So that's this one. So I create a user. I create an order with a certain amount type bulk. Uh the user is not on a trial. I added a discount and then basically approve order is going to return approved. Whereas in my updated version, it's going to reject it because the has discount is true in the order object and that's this rejection rule right here. So those are the three mismatches. A few

### [4:27](https://www.youtube.com/watch?v=b6RnBKvQ40o&t=267s) A few other issues and remarks

other things I noticed in your comments. So one is you noticed that while I was refactoring this whole thing I had some issues with and or combinations of conditions in particular uh this type of thing where we have a combination of and or and has precedence over or but overall it just shows how hard it is to have these kind of complicated and or statement. It's really easy to mess that up. Uh of course if you want to avoid that maybe you can try to split things up. Perhaps we could somehow split this function into two separate function. One checking uh the order amount and another checking the order type. Or you can use parenthesis like I did here to clarify what the supposed order is. There was also a question about the data structure. So what I did in the final version in the first version actually there is no data structure whatsoever. That's just to check for uh region EU. In the new version, I created a dictionary where I had these tupils, EU to euro, US to US dollar, etc. Uh that map to true and then I can check whether or not uh the uh region has a valid currency. And this is of course way easier to extend than the first version. The question is why did you use dictionary? Why not something else like a set or something or a list? Now there's no particular reason I did this. I think a set would actually also be just fine. And then you can actually simply do a check whether a certain tupole is in the set. So I think if I were to refactor this again, I would probably turn this into a slightly simpler data structure. Uh the most important point though is that it helps to come up with these type of data structures because they help you organize the code better. That was the main thing that I wanted to communicate with that. Another thing you mentioned is that well this returns a string, right? it returns approved or rejected like right here. And I didn't change that. Now, the reason I didn't change that is that I didn't want to change the outside behavior of the function. I still wanted to be able to run the same tests on it. If I change this to a boolean, well, then that wouldn't work. If I change it to an enum, uh maybe if the enum values happen to be these string values, but uh that was the reason why I didn't change that in my refactor. Now obviously I think it's better to use something else than a string. I think you should use a boolean or an enum if you have multiple different approval outcomes. So if I were to further improve this function after the refactor and it's used in only a couple of places, I would probably change that as well. Another thing some of you are wondering about is why is this a list of lambda functions and why not just the boolean expressions? You could simply compute all of these boolean expressions and then there's no need to call the function here. We can just check with any whether the order should be rejected or proved. Now there's actually two reasons why you might want to set it up this way. The first one is that the lambda function is executed only when we call it here. So for performance reasons that can be an interesting solution because for example we don't have to run this code at all if the amount is not eligible or if the order has a discount then we can reject early. So there's a performance gain there. Now probably in this example it's not a huge thing. So may not be a good reason to do it. But a second reason why I did this is that I like the idea of separating the specification of the rules from the execution of the rules. That's essentially what's happening here. Right? What you see here is the specification. We don't uh run any of this code. We simply specify what these rules are. And here we actually execute the rules and we check whether the order adheres to that specification. And in terms of design, that is actually a nice thing to do because then this rejection rules list is an object that you can also pass around and you can uh run it somewhere else applying it to some other type of object for example. And that's for me the main reason that I did this and it's actually a bit of foreshadowing because in a couple of weeks I'm going to post a video covering the specification pattern which is exactly this. Now in that video I'm going to do some blatant overengineering. This is child's play related to that. So, [clears throat] make sure you're subscribed to the channel. I noticed that like 2/3 of you who are watching my videos are not subscribed. Now, if you subscribe, this actually helps me out a ton because it helps uh the YouTube algorithm promote my content more and more people can see the videos and learn from them. So, I really appreciate if you can give me some love and subscribe to the channel and give this video a like while you're at it as well. Now

### [9:13](https://www.youtube.com/watch?v=b6RnBKvQ40o&t=553s) Traps / what is the ground truth?

let's talk about some traps. So from a purely technical perspective, the before version that we have here and the after here, they behave differently, but there's no way around it and I've proven it with the test. Now that is always a danger obviously when you're refactoring code. You may always change behavior. Um it also raises the question but what is actually correct? Which version is correct? Which version should we consider as sort of the ground truth? Is that the original one? I think some of you in the comments were making that assumption. Well, you could say that well the original code is what you're refactoring, so that is the ground truth. But of course, there is a reason why you're refactoring it. Uh some things in my opinion make more logical sense in the improved version like admin orders always being accepted or does an EU order also shouldn't have items with a negative price. The point is that when you're doing refactoring, you're always doing interpretation as well and you're making assumptions. So does that then mean that the new version is the correct one? [snorts] Uh I mean I did make quite a few assumptions in the refactoring process and not even explicitly. Uh I did not check that this was actually correct. So uh I don't think we can consider the updated version that I created as the new correct version of the code. I mean maybe there are other mistakes that we haven't uncovered yet. I mean who knows right? So in my opinion neither the original version nor the updated version of the code we can really consider that as sort of the truth. Well then maybe the test represent the truth right because in the test we basically capture the behavior. And now that I've added these extra tests to check these three uh logic changes. Well now maybe these represent the truth. But that's also not really the case because uh the tests are incomplete. If I run the original set of tests I ended up with in the video and I also print a coverage report and you see that these tests together, these are 13 tests. They cover 86% of the code. However, we know that there are logic errors in the updated version of the main. Right? So added these three tests which identifies these logical errors. But if I run the test again with these three added test which should be a more complete set of test, you see that actually the coverage is still at 86%. So if you say well in order to avoid these types of errors we need to aim for 100% coverage. That's only part of the story. Obviously, it can be a good thing that you have a high coverage percentage, but coverage only says something about how much of the code is run as part of your test. It doesn't say anything about test correctness. So, no matter what you do, there's no absolute guarantee that your tests are going to completely and accurately reflect the truth. And we may need to look even wider than just the code or the test. Perhaps uh the code and the test are not the truth, but the design document and the domain model that we made is the truth, right? But that's also not really the case because well, that's going to contain vague language, our own interpretation of things. Maybe it's based on interviews with the customer or user that are incomplete. So that's not something that we can use as ground truth either. But then maybe the truth is then what the user wants. Well, not really because typically users don't know what they want. Uh it's the whole reason agile exists, right? We build something quickly with incomplete information, show it to the user, and then we're going to iterate multiple times to get something that is uh what the user actually wants. So, what does it all mean? Do we then just do random things? Well, of course not. I mean, we can follow and we should follow best practices in designing and testing our software. uh we can follow an agile process but we just have to accept that we don't live in a 100% deterministic world that's guided by perfect logic. In fact, your job as a software engineer is to be the bridge between that imperfect vague real world and the simple deterministic logic world of software and programming. And to me that shows actually another trap. I there is actually no ground truth when you think about it. And even if there is one, it will be there only for a really short time until the requirements change. So we have to be careful as developers to not get stuck in too technical thinking. In the end, we're part of a process of trying to understand and specify a domain, interpreting it and codifying it, and then going back to the user with the result. And because of that result, the user changes requirements or we change our interpretation which leads to a new version and on it goes. In that sense, software design is not about code at all. Principles like abstraction, coupling, cohesion, keeping things simple. They're just tools to help us understand and specify a problem in a continuously changing domain. And the simpler we make things, the better we and our users can understand them and the better we will be in coming up with a good solution in the next iteration. The videos that I put here on YouTube

### [14:40](https://www.youtube.com/watch?v=b6RnBKvQ40o&t=880s) Final thoughts

they're educational videos and as you can see, I make mistakes. In fact, I make many of them. I always do my best to make less mistakes, but if there's one thing that I learn is that no matter what I do, I'm going to make mistakes. I do hope that this shows you that you don't have to be some sort of special genius to be a software developer. If I can do it with all the mistakes I make, you can definitely as well. And you know, the best way to learn is to be critical and ask questions. That's what the comment section is for. Don't worry about me. I can take it. Don't hold back. But be nice. Some of you can be a bit nicer, but don't ever hold back. All right. Now, your comments help everybody learn, including me. I don't post these videos just to hear that they're great or something. They're my perspective, and I can be opinionated, and sometimes I'm wrong about things. They are a starting point for discussion. A video like this doesn't stand on its own. It needs your opinion, your criticism, your thoughts, and all of those different views put together is what turns this into a valuable learning experience for everyone. Anyway, since this is the new year, I am wishing you a great 2026 with lots of learning. And next week, I'm going to do that again because I didn't plan this video. I wanted to skip a week due to the holidays, but after reading your comments, I still wanted to post this video. So, that's what I did. Now, I hope you had a great holiday. Stay critical but nice.

---
*Источник: https://ekstraktznaniy.ru/video/20762*