Optimize/ refactor program that highlights differences between two text collections

Question

I made a simple comparison window in WPF that has two RichTextBoxes highlighting the differences (in red) of two StringCollections.

Text Difference

I knocked the code up in about 5 minutes and having gone back to look at it it appears pretty shoddy. I've included comments to show my intentions.

private static void DifferentiateText(RichTextBox richTextBox1, RichTextBox richTextBox2, StringCollection text1,
 StringCollection text2, SolidColorBrush brushColour)
{
 var text1Length = text1.Count;
 var text2Length = text2.Count;
 //Loop through text1 array
 for (var i = 0; i < text1Length; i++)
 {
 var text1Line = text1[i] + "\r\n";
 //If we're within text1 and text2 array boundaries, then compare their values
 if (i < text2Length)
 {
 var text2Line = text2[i] + "\r\n";
 //If lines aren't the same, apply brush
 if (text1Line != text2Line)
 {
 AssignTextToTextBox(richTextBox1, text1Line, brushColour);
 AssignTextToTextBox(richTextBox2, text2Line, brushColour);
 }
 else //Output with default brush
 {
 AssignTextToTextBox(richTextBox1, text1Line);
 AssignTextToTextBox(richTextBox2, text2Line);
 }
 }
 else //At the end of text2 array, so just output the rest of text1
 {
 AssignTextToTextBox(richTextBox1, text1Line, brushColour);
 }
 //If we're at the end of text1 array but not at the end of text2 array, then just output the rest of text2
 if (i == text1Length - 1 && text1Length < text2Length)
 {
 for (; i < text2Length; i++)
 {
 var text2Line = text2[i] + "\r\n";
 AssignTextToTextBox(richTextBox2, text2Line, brushColour);
 }
 }
 }
}
private static void AssignTextToTextBox(RichTextBox richTextBox, string text, SolidColorBrush brushColour = null)
{
 var textRange = new TextRange(richTextBox.Document.ContentEnd, richTextBox.Document.ContentEnd)
 {
 Text = text
 };
 if(brushColour != null)
 textRange.ApplyPropertyValue(TextElement.ForegroundProperty, brushColour);
}

I don't particularly like how AssignToTextBox is used 6 times in the first for loop when really it will only be called a maximum of two times.

Is there any way to optimize/ refactor/ make-less-shoddy this code?

• Shelby115: I'm not sure if others would agree but that last for-loop is making me cringe just looking at it. Replace that with a while-loop.
• Johan Larsson: What about using a library?

Answer 1

Since you're using WPF, you should really consider implementing MVVM.

---

Wouldn't Environment.Newline be better than "\r\n"?

Also, why do you add the new line at those times? That way you're intorducing several points where you could forget to do so. Wouldn't it be better to apply this only in AssignTextToTextBox(), e.g. Text = text + Environment.Newline?

---

Comments should explain why something was implemented in that way, not what the code is doing. For example: //If lines aren't the same, apply brush doesn't tell me anything the code isn't showing me already. If you need comments to explain what your code does, it's a sign you need to re-write the code.

---

I don't particularly see the point of text1Length and text2Length, just use text1.Count and text2.Count directly. But more importantly, why are you using StringCollection when List<string> is available?

---

If you apply these changes, I think your logic can be reduced to this:

var maximum = (list1.Count > list2.Count) ? list1.Count : list2.Count;
for (var i = 0; i < maximum; i++)
{
 var text1Line = (i >= list1.Count) ? string.Empty : list1[i];
 var text2Line = (i >= list2.Count) ? string.Empty : list2[i];
 var applicableBbrushColour = (text1Line == text2Line) ? null : brushColour;
 AssignTextToTextBox(richTextBox1, text1Line, applicableBbrushColour);
 AssignTextToTextBox(richTextBox2, text2Line, applicableBbrushColour);
}

You could even move (i >= list1.Count) ? string.Empty : list1[i]; to a separate method, considering this logic is repeated; perhaps even to an extension method?

• Jack: Very helpful BCdotWEB, so thanks. Regarding MVVM, I have implemented this pattern but haven't as of yet for this window because I have not found a succinct way to bind to a RichTextBox. Regarding Environment.NewLine, I specifically require the new line to be composed of a \r and an \n but having had a quick Google I see Environment.NewLine includes both. Regarding comments- I only added them to SO to show my intentions! I am using StringCollection because I am working with SQL Server Management Objects which by default uses StringCollection. Is there any point to convert to a list?
• BCdotWEB: @JakkyD No point in converting AFAIK, no.