Optimize/ refactor program that highlights differences between two text collections I made a simple comparison window in WPF that has two RichTextBoxes highlighting the differences (in red) of two StringCollections.



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 &lt; text1Length; i++)
    {
        var text1Line = text1[i] + "\r\n";

        //If we're within text1 and text2 array boundaries, then compare their values
        if (i &lt; 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 &amp;&amp; text1Length &lt; text2Length)
        {
            for (; i &lt; 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?
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&lt;string&gt; is available?



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

var maximum = (list1.Count &gt; list2.Count) ? list1.Count : list2.Count;

for (var i = 0; i &lt; maximum; i++)
{
    var text1Line = (i &gt;= list1.Count) ? string.Empty : list1[i];
    var text2Line = (i &gt;= 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 &gt;= list1.Count) ? string.Empty : list1[i]; to a separate method, considering this logic is repeated; perhaps even to an extension method?