this should be a separate PR!!

Recently while working on some task I had to write some hacky code, and by hacky, I mean code with lot if..else. There are a lot of places in the code base with such random if..else changing the flow of based on some environment variable. Hence I was initially okay with adding all these ifs but once I finished with the functionality and looked back to refactor so that I can send it for review, no matter how I refactored it, it turned out to be messy code which I just couldn't push for a review!

Then I took a decision to refactor whole functionality so that I can later remove all the ifs placed at multiple places in code. When I decided to refactor it was good from a long term perspective just that I decided to take such a decision without discussing it with anyone in the team. Before you get me wrong, this refactoring was already done to new parts of the code to solve this exact problem just that it was not applied to the old part of the codebase due to not having enough time or not the correct opportunity.

Once I pushed it for review, the first comment I got was "this should be put in separate PR!". Well, in all honesty, that was expected and also true! but in reality, due to 1 QA per 5 devs, my team is usually bottleneck for releases and hence we hardly get any time to work on old code or a new feature in our test framework. If I had left the refactoring for later stage I would have never got another opportunity to work on that part of the code but the part I really wanted to avoid was leaving the code with if..else which is really hard to understand after some time like for e.g. why was this different? why I am converting variable to int and sometimes to float? or some similar questions. Of course, I can add comments but how many such comments one can add?

From a regular development environment, I should have raised separate PR but while working in a QA, with stringent timelines I think going ahead with single PR with "unplanned" changes was the right call!

Just snippet of what code looked like (all are made up fields), and what I refactored it into...

Old Way

# checking request against response or some constants

assert_equal(request.name, response.name)
assert_equal(request.type, response.type)
assert_equal(response.checksum, get_checksum(request))

if env == "DEV":
    assert_true(respone.timestamp)
else:
    assert_ts_in_range(response.timestamp, tolerance_in_sec=2)

New way

assertor = get_msg_assertor(response, env)  # class based

assertor.name(request.name)
assertor.type(request.type)
assertor.checksum(get_checksum(request))
assertor.timestamp()  # DEV will just check for presence and UAT will check for tolerance