I am working on a freelance job at home. The client wants me to write some new functionality for his CMS, but it is taking me a lot of time to figure out what the code is doing, because it is written in a very unreadable style.
Below is just an example of what I mean. The previous programmer made extensive use of anonymous functions, of eval(), he uses deeply nested ternary operators, he didn’t indent code, didn’t use comments, and he uses funny constructions like misusing the behaviour of logical operators || and && for creating if/else conditions (the second condition of && only gets tested if the first one is true, opening the possibility to use && as an if/else construction). All in all it’s insane code and it’s costing me a lot of time to find out how the current code works.
return ($this->main->context != "ajax" || in_array($this->type, $this->definition->ajax)) ? eval('return method_exists($this,"Show'.ucfirst($this->type).'") ? $this->Show'.ucfirst($this->type).'('.(count($args) ? join(",",array_map(create_function('$a','return (is_numeric($a) || preg_match("/^array/",$a)) ? $a : """.$a.""";'),$args)) : "").') : null;') : '';
Would you refactor this code and how would you handle this sort of thing with your client, I mean financially?
7
You “own” what you change with respect to liability for fixing defects and undesirable behavior…. maybe you can get out of it sometimes, but the customer will say “You changed it, it’s broken, you fix it. Too many of these, and he won’t be a happy customer.
If you refactor, and break it, it’s yours.
If you refactor, and it was broken before, and is afterwards, it’s yours.
If you refactor and fix a “defect”, but change the behavior, when the “broken” behavior was being used, it’s yours.
Unless you have great unit tests, refactoring will change behavior – and you now own the responsibility for fixing it if it’s broken.
Up to you if you are prepared to take on that risk for free. I don’t. So you need to discuss this with your client, and ask them how much they are prepared to pay for it. Ensure they understand the risk that you break something, and have an agreement on how that is paid for.
I have purposely not addressed the issue of direct time/costs to refactor, as already commented, most clients will say “What value do I get for paying you that much money”, that is the easy part of the discussion.
3
I might do some cleanup to maintain my own sanity, but I wouldn’t include my cleanup in the final changeset I test and submit to the client. The other responses are right on that point. You don’t want to risk breaking something because you did something that wasn’t requested.
If you believe that the code in its current form will be difficult to modify and maintain, let the client know as soon as possible. If I take my car to a mechanic to have the oil changed, I don’t expect him to pretend not to see that my brake lines are damaged just because it’s not what I hired him to fix. I expect him to suggest reasonable repairs and maintenance.
It may be very useful for your client to know the code is bad. They may decide they don’t want work with the author again. They may decide it’s not worth fixing. Or they might have you fix it. They are paying you for your expertise. Supply it!
Refactoring tricksy code is always dangerous. The risk of missing some assumption that was present in the original is just way too high, and you may well end up doing more harm than good, however well-intentioned you may be at the outset.
If you don’t need to do it as part of this job, then don’t touch it with a 10-foot pole. However awful this may sound, once you’ve delivered the requirement you can walk away when done and the problem is not yours anymore.
On the other hand, if you do need to do it, then maybe it’s time to renegotiate terms with the client. After all, you’ve been dumped with a lot of crap that wasn’t in the original agreement, so it’s unrealistic to expect you to deliver within the originally agreed terms given the circumstances (which you couldn’t be reasonably expected to have been aware of when you took the job). The client may also be in a position to get hold of the original developer and at least clarify things a little better than you could figure out on your own, so it’s in your own best interest.
1
If it’s something easy (in my definition takes less than an hour), then I would just do it to preserve my sanity.
However, anything larger than that needs a bit of thought.
How long will it take you to finish your job (the one you’re being paid for) without refactoring? Let’s call this A.
How long will it take you to finish your job after refactoring, plus the amount of time of the actual refactoring and testing (to make sure you didn’t break anything). Let’s call this B.
I would only do it if B is significantly less than A. Otherwise I would ask for compensation from the client.
3