I work @ Red Hat, Inc. but opinions are my own.
Lucian Maly wrote:I think the best way to approach this is to capture all potential improvements (use a keyword in the comments of your code that the entire team will agree on and use, e.g. #TODO:) during the "initial" development sprints. Scrum master/PM should then turn it into lower priority tickets as you go and then have at least one more "refactoring" sprint(s) only for these tickets.
This way, you will not go too far and start attacking "theoretical" problems, but identify and fix components that have actually bitten you along the way.
Junilu Lacar wrote:larger scale refactoring efforts that require their own "tickets" (ugh) should be reserved for legacy code that you have to maintain.
Junilu Lacar wrote:People with less design/development experience will have a less keen sense of "smell"... You won't refactor something that doesn't smell to you...
Jorge Ruiz-Aquino wrote:
Newbie folks tend to c&p existing blocks of code to replicate some logic ending with tons of code duplication.
Jorge Ruiz-Aquino wrote:
However, here is where the "pair reviews" come handy. Hoping that the experienced guy have in mind good practices and the sense of "code smell".
Junilu Lacar wrote:As a newbie on a real-world project, you should have been told that Copy/Paste programming is grounds for immediate termination of employment.
Seriously though, stop making this OK just because you're a newbie. This should be Lesson #1 on Day #1 : Copy/Paste coding, BAD! BAD PROGRAMMER!
Junilu Lacar wrote:I think you meant "peer reviews" but "pair reviews" works, too, if you were referring to the instant code reviews that pair programming affords you.
Junilu Lacar wrote:More than likely the average "experienced guy" in large corporate settings ... has practically little to no experience in refactoring or unit testing.
Junilu Lacar wrote:I'd bet that if you brought up this Five Lines of Code to your lead developer, there's a good chance you'll get a strange "WTF you talkin' about?" look back.
Junilu Lacar wrote:I think the best way to approach this is to capture all potential improvements (use a keyword in the comments of your code that the entire team will agree on and use, e.g. #TODO:) during the "initial" development sprints. Scrum master/PM should then turn it into lower priority tickets as you go and then have at least one more "refactoring" sprint(s) only for these tickets.
Ouch. You just described what many practitioners would call an anti-pattern. Refactoring is best done as soon as you get tests to pass, which should be done frequently and incrementally. This (what you described) is more like a way to ensure that refactoring gets pushed to "later when we have more time," which of course you only do when, as you say, you have already been bitten (usually by a P1 in production).
This kind of misunderstanding of what refactoring is is what I was afraid would result from the recommendation to wait until you've written all the code to refactor. In my experience, that "big bang refactoring in the end" approach just gets you in more trouble than it supposedly saves you; it's usually much more costly than incremental and in-the-moment refactoring.
In my opinion, larger scale refactoring efforts that require their own "tickets" (ugh) should be reserved for legacy code that you have to maintain. Certainly, spending some time refactoring and fencing in legacy code with characterization tests is a good and prudent practice.
This way, you will not go too far and start attacking "theoretical" problems, but identify and fix components that have actually bitten you along the way.
Sure, there's a chance that inexperienced folks get too carried away and get too "theoretical" about what should be refactored and how it should be done but in general, if you keep with the Four Rules of Simple Design, you really only need Rename, Extract, and Compose Method most of the time. Almost all other refactoring techniques are just one or a combination of these three applied to a specific set of conditions. I have found Rename, Extract, and Compose Method to be sufficient for 80% of the refactoring that I need to do "in the moment."
I work @ Red Hat, Inc. but opinions are my own.
Lucian Maly wrote:I worked in some fast-paced agile environments and immediate refactoring was always perceived as a "waste of the development time" and "not adding any features while there are deadlines to meet".
Lucian Maly wrote:Theoretically speaking you can go on refactoring indefinitely because there is NEVER a perfect code.
You mean you have never written a method with −1 lines in?Junilu Lacar wrote:. . . My absolute limit in terms of lines of code is zero. . . .
Campbell Ritchie wrote:You mean you have never written a method with −1 lines in?
How do you approach code refactoring in the real world?
First make the change easy, then make the easy change
there is NEVER a perfect code
"refactoring" sprint
there's a chance that inexperienced folks get too carried away and get too "theoretical"
Rename, Extract, and Compose Method
his recommendation to wait a while before refactoring
Copy/Paste programming is grounds for immediate termination of employment
I think you meant "peer reviews" but "pair reviews" works
the average "experienced guy" in large corporate settings [...] has practically little to no experience in refactoring or unit testing
Challenge accepted
capture all potential improvements
delay refactoring until you're ready to deliver is just not in line with the approach recommended by other experts
there may be valid business situations where the business can tell developers to forego refactoring
then sure, skip refactoring
accumulating debt becomes a liability
Christian Clausen wrote:
Please don't do these, they will be a nightmare, ..."refactoring" sprint
Christian Clausen wrote:Copying something you don't understand should be grounds for termination.
I love synchronous peer review (ie. pair or mob programming). However, I am not a fan of blocking peer review (ie. pull requests), because they prevent continuous integration, which causes bad merges, and longer lead time.
Christian Clausen wrote:
I just want to clarify that I only mean wait until you are ready to push your code to master, which I think you should do every day. This is because while we are writing the code it is often quite fluent which means we can change data structures or approaches easily. I don't want to refactor the code to support some algorithm, if I then change my mind and have to undo the refactoring. Only once I have the desired functionality mostly locked down I start refactoring.
Hahahahahahahahahahahaha! That is SO funny.Junilu Lacar wrote:. . . StackOverflow programming. . . . .
Christian Clausen wrote:Firstly, in the real world, I always treat the team as the only method of delivery, which means that the biggest refactoring effort comes from talking with the team, and deciding on a common level of quality, and then holding each other to it.
Campbell Ritchie wrote:
Hahahahahahahahahahahaha! That is SO funny.. . . StackOverflow programming. . . . .
Same here! We really do agree on quite a few things.