Win a copy of Terraform in Action this week in the Cloud forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Tim Cooke
  • Campbell Ritchie
  • Paul Clapham
  • Ron McLeod
  • Liutauras Vilda
Sheriffs:
  • Jeanne Boyarsky
  • Rob Spoor
  • Bear Bibeault
Saloon Keepers:
  • Jesse Silverman
  • Tim Moores
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
Bartenders:
  • Piet Souris
  • Al Hobbs
  • salvin francis

How do I refactor a code like this?

 
Ranch Hand
Posts: 837
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I am in a temporary job where I am asked to do unit test but the code is not written in a 'clean' way so I am having difficult in performing unit test and something that I am still struggling.
The language used is not Java but C# but I am used to this forum so I hope someone can advise me how to go about refactoring because I really am lost and my skill is still very shaky.



The refactored method is





I would like to know what are the refactoring methods I can use in order to make the code more clean for unit test.
Especially the part in the



Thanks.
 
Saloon Keeper
Posts: 8779
71
Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I'm not a C# person but I see some potential issues:
  • Sometimes "Trim" is used and other times "Trim()" is used.
  • There's an if(expression); {...} // the semi-colon shouldn't be there.

  • Is this code you've already refactored a bit?
    Why do you think it's not clean?
     
    tangara goh
    Ranch Hand
    Posts: 837
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Carey Brown wrote:I'm not a C# person but I see some potential issues:

  • Sometimes "Trim" is used and other times "Trim()" is used.
  • There's an if(expression); {...} // the semi-colon shouldn't be there.

  • Is this code you've already refactored a bit?
    Why do you think it's not clean?



    Sorry there was probably an error there in the pre-refactored code.

    Actually, I don't know much about C# but what to do, my boss insisted Java == C#
    And really I need the money ...cos it is not much but better than working as a cashier where I can't even say I know I will be deemed ARROGANT!

    How would you go about further refactoring the code since there are quite alot of similar elements in the if else portion?
     
    Sheriff
    Posts: 16719
    278
    Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Here's my general approach to refactoring:

    First, understand what the code is doing.
    Second, identify different tasks. If possible, separate/isolate tasks from each other
    Third, eliminate duplication
    Fourth, look for patterns in the code and determine how to organize around those patterns

    Being able to test code relies mostly on the first two steps. If you don't understand what the code is doing in the first place, how can you test it properly? If parts of the code are intertwined with each other, then how can you test it in small units?

    The idea of "clean" code is very nebulous. What's clean to one person may be very dirty to another depending on a number of factors. I have started to prefer using "clear" as gauge instead. I think it's much easier to reason about and assess code clarity than it is to assess "cleanliness."

    I think we can all agree that there are a few things that inhibit the clarity of the code in question:
    1. It's a LONG method -- long methods tend to do a lot of things and this makes discerning what exactly it's doing more difficult
    2. There are many conditionals -- this suggests multiple things being done, which again makes it more difficult to understand exactly what the code is doing
    3. Poorly chosen names - many names are cryptic and don't help the reader understand their purpose and role in the code's logic.

    If you start by addressing the above concerns, you can at least start wrapping your head around what this code is doing. As it is, when I first look at the code, I already want to look away. You have to make code inviting to the reader, so that it captures and holds their attention.
     
    Junilu Lacar
    Sheriff
    Posts: 16719
    278
    Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    If you start with the method name, ProcessTable, this is a very generic name and doesn't really tell you what the method does. This is a poorly chosen name.

    The return value gives you a bit of a clue of what the method does: it returns a list of ClauseRow. What is a ClauseRow? What is the relationship between a ClauseRow and each of the parameters passed to the method?

    Can you give an example of specific values for these parameters and the expected list of ClauseRow objects that will be produced by the method ProcessTable?

    In a sentence or two, can you explain in more detail and clarity what exactly this method does?
     
    Carey Brown
    Saloon Keeper
    Posts: 8779
    71
    Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    Would look clearer to me if it was more like:

     
    Junilu Lacar
    Sheriff
    Posts: 16719
    278
    Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Carey Brown wrote:Would look clearer to me if it was more like:


    But are you clear on what the functionality of ProcessTable is in the first place?

    Remember, refactoring only changes the internal structure and organization of code to make it easier to understand and work with. By changing the signature of ProcessTable, you have crossed the line from refactoring to redesign.
     
    Junilu Lacar
    Sheriff
    Posts: 16719
    278
    Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
    • Likes 1
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    I would state the ultimate goals just to make sure you don't lose sight of them. You don't do refactoring just for refactoring's sake, so you can say "I have cleaned up the code." So what if you cleaned up the code?

    These are some possible goals to set, so you can do purposeful refactoring:

    1. I want to make the ProcessTable code more testable

    2. I want to be able to detect bugs in the ProcessTable code

    3. I want to be able to easily introduce changes to the ProcessTable functionality.

    If you keep one or more of these goals in mind and make sure that each refactoring you do will eventually lead to achieving the goal(s), then your refactoring will be more purposeful and directed at the right thing.
     
    Junilu Lacar
    Sheriff
    Posts: 16719
    278
    Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    tangara goh wrote:I am in a temporary job where I am asked to do unit test but the code is not written in a 'clean' way so I am having difficult in performing unit test and something that I am still struggling.


    Based on this, it seems OP's goal is to ultimately be able to put some unit tests around the code. In that case, it's critical to have a solid understanding of what the code does in the first place.

    @OP: can you give examples of inputs to the ProcessTable method and the corresponding output you expect? I think this should be your first step.

    Then I would write one or two tests to reflect those examples. It will be a good starting point for your refactoring efforts. Basically, what you're going to do first is write a characterization test or two that will reflect your current understanding of what the code does. If you can write a few tests that show your understanding of the code, then you can start the refactoring work needed to make the production code reflect the understanding that you have in the test code.
     
    Carey Brown
    Saloon Keeper
    Posts: 8779
    71
    Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Junilu Lacar wrote:
    Remember, refactoring only changes the internal structure and organization of code to make it easier to understand and work with. By changing the signature of ProcessTable, you have crossed the line from refactoring to redesign.

    Not my intent to change the signature, it was just a place holder so I could edit it in Eclipse. I do believe that the process of cleaning up code brings to light some of the purpose of the code which should be followed by applying clear naming and modularization.
     
    Junilu Lacar
    Sheriff
    Posts: 16719
    278
    Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
    • Likes 1
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator

    Carey Brown wrote:I do believe that the process of cleaning up code brings to light some of the purpose of the code which should be followed by applying clear naming and modularization.


    Yes, that's true but characterizing what a particular method does is usually done from an outside-in approach rather than an inside out approach. I would imagine you have some idea of what you expect code to do given some inputs. You can glean some of that understanding from the context(s) in which the method is called. This would be the same context that you'd create in your characterization tests. Doing characterization testing from the inside-out seems like it would be a last resort when you can't even decipher what the calling code is doing and you don't have any idea what kind of inputs are going to be sent to the method via its parameters or dependencies. As Andy Hunt wrote, "Always consider context." Context is where understanding begins.
     
    Junilu Lacar
    Sheriff
    Posts: 16719
    278
    Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
    • Mark post as helpful
    • send pies
      Number of slices to send:
      Optional 'thank-you' note:
    • Quote
    • Report post to moderator
    @OP - what progress have you made? I feel like you just abandoned this thread at this point. That's a bit disrespectful of the time and effort people put in to giving you answers.
     
    Don't get me started about those stupid light bulbs.
    reply
      Bookmark Topic Watch Topic
    • New Topic