Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Refactoring: How do I even start?

socalledprogrammer.com

Post reply on HN
Print view View original

21–30 of 41 posts

Re: Refactoring: How do I even start?

#21

Post by philbarr »

pmr_ wrote: post #3

if( masterList[z].list2 != NULL && masterList[z].list2.length() > 0 ) { for( Integer y = 0; y The if-statement is a good summary of what is wrong with Java. The author doesn't even notice that the second argument of && is redundant and keeps it in the "refactored" version as well...

I actually don't think he should remove the second argument until the entire thing has been verified in tests. What if it was actually:

  masterList[a].list2.length()
..but he misread it as

  masterList[z].list2.length()
and so removed it..?

Re: Refactoring: How do I even start?

#22

Post by zo1 »

pmr_ wrote: post #3

if( masterList[z].list2 != NULL && masterList[z].list2.length() > 0 ) { for( Integer y = 0; y The if-statement is a good summary of what is wrong with Java. The author doesn't even notice that the second argument of && is redundant and keeps it in the "refactored" version as well...

You can't always rely on for-loop mechanics to do branching.

Also, having an implied "conditional" that must be extracted by reading the head-portion of a loop is very very unreadable. I'd reject your submission on a code-review.

Instead, you should do something like below. I assume that your example is contrived, so for the sake of argument, assume I have all sorts of business cruft around mine. I.e. should_process exists because there are business rules for processing/not processing the master_list.

    def should_process(master_list):
        return master_list is not None and len(master_list) > 0

    def main_doing_of_stuff_foo():
        if not should_process(masterList):
            return  # Yes, this thing should be in its own method so you can do an early exit.

        for item in masterList:  # Yes, you should be using an iterator as well, not indexing.
            print("Body goes here".)

Re: Refactoring: How do I even start?

#23

Post by QuercusMax »

pnathan wrote: post #11

Worth noting: some shops will summarily reject on code review any refactor that isn't part of the ticket/issue.

Along the same lines: it's considered good practice to perform a refactoring in a separate commit from code which changes behavior. This makes it much easier to review the code, as you can look at the changes in isolation. If you refactor and then change behavior in a single commit, it can result in a very confusing change.

Re: Refactoring: How do I even start?

#24

Post by zerohp »

I know this code is just an example, but it seems like it's entirely backwards. The "bad" code at the beginning is immediately clear in what it's doing, but the refactored code is significantly longer and hard to understand.

Re: Refactoring: How do I even start?

#26

Post by pmr_ »

zo1 wrote: post #22
pmr_ wrote: post #3

if( masterList[z].list2 != NULL && masterList[z].list2.length() > 0 ) { for( Integer y = 0; y The if-statement is a good summary of what is wrong with Java. The author doesn't even notice that the second argument of && is redundant and keeps it in the "refactored" version as well...

You can't always rely on for-loop mechanics to do branching. Also, having an implied "conditional" that must be extracted by reading the head-portion of a loop is very very unreadable. I'd reject your submission on a code-review. Instead, you should do something like below. I assume that your example is contrived, so for the sake of argument, assume I have all sorts of business cruft around mine. I.e. should_process…

I can't follow. The code I showed is already processing masterlist and decides if it should process masterlist[z].list2 for every z: 0..masterlist.length.

I fully agree that the code in my post is very bad, I took it from the article.

Re: Refactoring: How do I even start?

#27

Post by pmr_ »

philbarr wrote: post #21
pmr_ wrote: post #3

if( masterList[z].list2 != NULL && masterList[z].list2.length() > 0 ) { for( Integer y = 0; y The if-statement is a good summary of what is wrong with Java. The author doesn't even notice that the second argument of && is redundant and keeps it in the "refactored" version as well...

I actually don't think he should remove the second argument until the entire thing has been verified in tests. What if it was actually: masterList[a].list2.length() ..but he misread it as masterList[z].list2.length() and so removed it..?

You are saying that you actually need to read and understand code you are refactoring. Hard to disagree.

Re: Refactoring: How do I even start?

#29

Post by gedrap »

crdoconnor wrote: post #28

He missed step #1: write some tests that will provide feedback if you broke something, or assurance that you didn't.

This. The problem is that in some cases, the legacy system is really messed up and making it unit testable is a big refactoring task on it's own.

In these cases, I try to write some functional tests first (such as calling restful endpoints and checking the response, or using a headless browser). Not great but much better than nothing. Any ideas on how to do it better?

Re: Refactoring: How do I even start?

#30

Post by blt »

gedrap wrote: post #29
crdoconnor wrote: post #28

He missed step #1: write some tests that will provide feedback if you broke something, or assurance that you didn't.

This. The problem is that in some cases, the legacy system is really messed up and making it unit testable is a big refactoring task on it's own. In these cases, I try to write some functional tests first (such as calling restful endpoints and checking the response, or using a headless browser). Not great but much better than nothing. Any ideas on how to do it better?

The closest thing I've seen to an actual definition of "legacy code" is "code that's very difficult to unit test." It's a shitty catch-22.
Post reply on HN