You can’t refactor yourself out of a bad design
source link: https://alcher.dev/python/software/2023/02/13/you-cant-refactor-out-of-a-bad-design.html
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
You can't refactor yourself out of a bad design
Feb 13, 2023 • John Alcher
Refactoring is a disciplined technique for restructuring an existing body of code, altering its internal structure without changing its external behavior.
A few weeks back, I was hosting an internal training session for automated testing techniques. Below is a snippet of “production code” that the participants are supposed to add tests for:
@api_view(["POST"])
@permission_classes([IsAuthenticated])
def loan_book(request, pk):
user: User = request.user
if user.credits < 5:
return Response({"error": "Insufficient credits"}, status=422)
book = get_object_or_404(Book, pk=pk)
if book.is_already_loaned:
return Response({"error": "Book is unavailable"}, status=422)
loan = Loan.objects.create(borrower=request.user, book=book)
message = f"Book <{book.title}> loaned to <{request.user.first_name}> with reference number #{loan.reference_number}"
data = {"message": message}
return Response(data, status=200)
The code itself is not that complex and could be a great starting point for the participants to practice their refactoring skills, which is the topic for the next week’s session. There’s a dozen of refactoring improvements that could be made here: moving to a CBV, moving the credit checking to its own model method, or perhaps even moving the precursory checks to their own custom view decorators.
But you can’t refactor yourself out of a bad design. The given code snippet operates the credit system using an integer:
if user.credits < 5:
return Response({"error": "Insufficient credits"}, status=422)
If you’ve worked with any software that deals with credits, balances, or running totals in general, you’ll know that this is a non-maintainable way to handle such system.
# Direct credit access is allowed
user.credits = 0
# How would you know why/who/how the change of credits is applied?
# ... some time later
user.credits == -1
# Something seems to decrement the credits twice.
# How would you know which business function did so?
# Suppose you implement a history system by creating a
#History record before modifying the credit field
def loan_book(request):
user = request.user
History.objects.create(type="loan", amount=2, user=user)
user.credits -= 2
user.save()
# Are you ever sure that the totality of the History objects
# would match the user's current credit value?
history_total = History.objects.filter(user=user).aggregate(Sum("count"))
history_total == user.credits # ???
The solution is to remove the credits model field and use the History object as the single source of truth.
class User:
@computed
def credits(self):
return History.objects.filter(user=self).aggregate(Sum("count"))
But obviously, on an already existing platform, you can’t refactor this way without making system-wide changes to your database structure. For starters, you’ll need an initial History record for each user to capture their current credit. This will probably live on a separate script that lives outside the scope of your refactoring session:
# Ran as a management command or a standalone script
def create_initial_history_object():
for user in User.objects.all():
History.objects.create(user=user, type="initial", amount=user.credit)
We all know that in software, you have to move fast and fail fast, and you’ll always have the option to refactor in the future. But taking the extra time to consider your initial design could save you a lot of headache down the line.
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK