Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
4  
Why would I pay someone to fix something that isn't broken? –  James Nov 29 '12 at 0:17
    
I might argue that it will increase the maintainability of the system in the long run and decrease the development time needed in the future. –  Julius Nov 29 '12 at 0:20
2  
Did the client pay for that? Does the client want to pay an extra X for that? No? Then suck it up and do your job. –  Telastyn Nov 29 '12 at 0:23
2  
What's wrong with extensive use of anonymous functions, eval(), or the well known short-circuiting properties of && and ||? Putting everything in one line like that is bad, but it isn't a brick wall. After spending about 30 seconds in a text editor adding reasonable indentation I had something pretty readable -- and I don't know PHP at all. –  Michael Shaw Nov 29 '12 at 1:35
2  
@Michael Shaw: extensive use of eval, anonymous functions and the misuse of logical operators for different purposes make code harder to read, harder to debug and harder to maintain. 'Code that generates code' is also inherently less self documenting, so I usually stay away from it. –  Julius Nov 29 '12 at 2:49
show 2 more comments

4 Answers 4

up vote 9 down vote accepted

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.

share|improve this answer
    
It's a CMS. It's broken. –  Erik Reppen Nov 29 '12 at 3:12
    
+1. But consider: if you have to add some functionality to a line of code you did not fully understand, it is also yours, and you will have to fix all the bugs you most probably introduced. Careful (!) refactoring the code beforehand can help one to get a better understanding and reduce the risk of introducing such bugs. –  Doc Brown Nov 29 '12 at 17:00
    
@Doc - Great point. If by refactoring you reduce your risk with better understanding and more readable code, then it should be done, as you say - carefully. I have in one instances, re factored, understood, modified, and re-factored the rubbish back in - unfortunately for commercial reasons is was the right thing to do. –  mattnz Nov 29 '12 at 19:22
add comment

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!

share|improve this answer
add comment

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.

share|improve this answer
    
+1 This. Without a good set of tests and understanding of requirements in front of you, you can't easily verify the new behavior matches the old and that no new bugs were introduced. –  Peter Smith Nov 29 '12 at 2:40
add comment

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.

share|improve this answer
    
B is hardly ever smaller than A. Refactoring code is an extra investment that should pay of in the long run when done properly. –  Julius Nov 29 '12 at 0:26
1  
@Julius - True. Which is why you almost never refactor without some compensation. –  System Down Nov 29 '12 at 0:28
    
If this is something like Drupal, I'd say it's better to swallow your pride and forego short-term memory in regards to code-awful. You'll never run out of little things to fix. –  Erik Reppen Nov 29 '12 at 3:16
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.