How do I refactor a code like this?

[tangara goh]

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.

        public void ProcessWordDoc(WordprocessingDocument doc, Assessment newAssessment, string targetName, bool reupload) //target name is for Annex A search
        {

 private List<ClauseRow> ProcessTable(Body body, MainDocumentPart wDoc, string operatorname, string year, string cop, string TargetName, bool reupload)
        {
            List<ClauseRow> clauseList = new List<ClauseRow>();
            bool targetFound = false;
            int tableCount = 0;
            int clauseCount = 0;
            int C = 0;
            int CAFI = 0;
            int PC = 0;
            int NC = 0;

            foreach (var child in body.ChildElements)
            {
                if (child is Paragraph && !targetFound)
                {
                    Paragraph paragraph = (Paragraph)child;
                    string text = paragraph.InnerText;
                  
                    if (paragraph.ParagraphProperties != null && text.Length >= TargetName.Length)// refactored
                    {
                        if (IsHeader(paragraph) &&// refactored
                            text.ToLower().Trim.Contains(TargetName.ToLower().Trim());// refactored
                        {
                            targetFound = true;//refactored
                        }
                    }
                }
                else if (child is Paragraph && targetFound)   //stopping procedure
                {
                    Paragraph paragraph = (Paragraph)child;
                    string text = paragraph.InnerText;                  
                  
                    if (paragraph.ParagraphProperties != null && text.Length >= TargetName.Length)
                    {
                        if (IsHeader(paragraph) &&
                            text.Substring(0, 5).ToLower().Trim() == TargetName.Substring(0, 5).ToLower().Trim())
                        {
                            //Console.Out.WriteLine(TargetName.Substring(0, 5).ToLower().Trim());
                           
                            targetFound = false;
                        }
                    }
                }
                else if (child is Table && targetFound)
                {
                    Console.Out.WriteLine("Target Table found");
                    Table table = (Table)child;

                    ScoreCount tablescore = ClauseDocx.TableScoreCounter(table);
                    C += tablescore.C;
                    CAFI += tablescore.CAFI;
                    PC += tablescore.PC;
                    NC += tablescore.NC;

                    List<ClauseRow> clauseSublist = ClauseDocx.TableExtraction(body, wDoc, table);
                    clauseList.AddRange(clauseSublist);
                    clauseCount += clauseSublist.Count();
                    tableCount++;
                    Console.Out.WriteLine("Total number of clauses extracted: " + clauseCount);
                    Console.Out.WriteLine("Total number of tables extracted: " + tableCount);
                }
            }
            if (clauseList.Count == 0)
            {
                Console.Out.WriteLine("No table extracted");
            }

The refactored method is

private bool MatchedTraget(string TargetName, Paragraph paragraph, string text) {
bool targetFound = false;
if(IsHeader(paragraph)){
text.toLower().Trim().Contains(TargetName.ToLower().Trim());
targetFound = true;
}
return targetFound;
}

private bool IsHeader(Paragraph paragraph) {
return paragraph.ParagraphProperties.ParagraphStyleId != null &&
paragraph.ParagraphProperties.ParagraphStyleId.Val.ToString().ToLower().StartsWith("heading");
}
}

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

 if (child is Paragraph && !targetFound)
                {
                    Paragraph paragraph = (Paragraph)child;
                    string text = paragraph.InnerText;
                  
                    if (paragraph.ParagraphProperties != null && text.Length >= TargetName.Length)// refactored
                    {
                        if (IsHeader(paragraph) &&// refactored
                            text.ToLower().Trim.Contains(TargetName.ToLower().Trim());// refactored
                        {
                            targetFound = true;//refactored
                        }
                    }
                }
                else if (child is Paragraph && targetFound)   //stopping procedure
                {
                    Paragraph paragraph = (Paragraph)child;
                    string text = paragraph.InnerText;                  
                  
                    if (paragraph.ParagraphProperties != null && text.Length >= TargetName.Length)
                    {
                        if (IsHeader(paragraph) &&
                            text.Substring(0, 5).ToLower().Trim() == TargetName.Substring(0, 5).ToLower().Trim())
                        {
                            //Console.Out.WriteLine(TargetName.Substring(0, 5).ToLower().Trim());
                           
                            targetFound = false;
                        }
                    }
                }

Thanks.

[Carey Brown]

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]

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?

[Junilu Lacar]

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]

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]

               if (child is Paragraph && !targetFound)
               {
                   Paragraph paragraph = (Paragraph)child;
                   string text = paragraph.InnerText;
                  
                   if (paragraph.ParagraphProperties != null && text.Length >= TargetName.Length)// refactored
                   {
                       if (IsHeader(paragraph) &&// refactored
                           text.ToLower().Trim.Contains(TargetName.ToLower().Trim());// refactored
                       {
                           targetFound = true;//refactored
                       }
                   }
               }
               else if (child is Paragraph && targetFound)   //stopping procedure
               {
                   Paragraph paragraph = (Paragraph)child;
                   string text = paragraph.InnerText;                  
                  
                   if (paragraph.ParagraphProperties != null && text.Length >= TargetName.Length)
                   {
                       if (IsHeader(paragraph) &&
                           text.Substring(0, 5).ToLower().Trim() == TargetName.Substring(0, 5).ToLower().Trim())
                       {
                           //Console.Out.WriteLine(TargetName.Substring(0, 5).ToLower().Trim());
                           
                           targetFound = false;
                       }
                   }
               }
               else if (child is Table && targetFound)
               {
                    //....
               }

Would look clearer to me if it was more like:

	private void ProcessTable( /* ... */ )
	{
		boolean targetFound = false;
		//....
		if( child is Paragraph )
		{
			if( IsParagraphTarget( (Paragraph)child, TargetName ) )
			{
				targetFound = ! targetFound; // toggle
			}
		}
		else if( child is Table && targetFound )
		{
			//....
		}
	}

	public static boolean IsParagraphTarget( Paragraph paragraph, String TargetName )
	{
		if( paragraph.ParagraphProperties == null || ! IsHeader(paragraph) )
			return false;
		
		String text = paragraph.InnerText;
		return text.length() >= TaragetName.length() &&
				text.substring(0, 5).toLower().trim() == TargetName.substring(0, 5).toLower().trim();
	}

[Junilu Lacar]

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

	private void ProcessTable( /* ... */ )
	{
		boolean targetFound = false;
		//....
		if( child is Paragraph )

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]

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]

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]

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]

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]

@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.