2
\$\begingroup\$

Is there any way to improve this simple addition to the textbox control: appending a new line?

class CustomTextBox : TextBox
{
    public void AppendLine(string textData)
    {
        if (this.Text.EndsWith(Environment.NewLine) || this.Text == "")
        {
            this.AppendText(textData);
        }
        else
        {
            this.AppendText(Environment.NewLine + textData);
        }
    }  
}
\$\endgroup\$
  • \$\begingroup\$ Why are you creating a custom class, instead of writing this as an extension method? \$\endgroup\$ – svick Sep 11 '14 at 18:25
  • \$\begingroup\$ Extention Methods sound like something I ought to know about, please expand or point me somewhere, thanks. \$\endgroup\$ – chazjn Sep 11 '14 at 21:48
  • \$\begingroup\$ You could start with the official documentation: “Extension methods enable you to "add" methods to existing types without creating a new derived type, recompiling, or otherwise modifying the original type.” \$\endgroup\$ – svick Sep 11 '14 at 22:46
2
\$\begingroup\$

When I am thinking about changing / enhancing some code, I have usually two things (regarding the code for review) in mind:

  1. Whenever there is some method that gets called in every if-else / switch-case, think about using some variable to make it single call. (This includes same method on different objects of common type - not aplicable to this case, but for the record)
  2. Check all conditions (and variables), if they are really required. (This includes joined conditions, when one renders to be redundant because of another already met. Sometimes I see variables that are used to avoid goto, but it turns out it can be done without both by reorganizing it - again not aplicable to this code review, but for the check-list)

My final solution would look like this:

class CustomTextBox : TextBox {
    public void AppendLine(string textToAppend) {
        string current = this.Text;
        if (current.Length > 0
        && !current.EndsWith(Environment.NewLine))
            textToAppend = Environment.NewLine + textToAppend;
        this.AppendText(textToAppend);
    }
}

EDIT: I have deleted all redundant / wrong parts. The check for empty text is needed, and TextLength is a property of WinForms TextBox, not the one in WPF.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer. I need to check for empty text, otherwise the first line will only be a new line and the text will start on line 2. \$\endgroup\$ – chazjn Sep 5 '14 at 9:55
  • \$\begingroup\$ Also we have to check the text length using this.Text.Length - I cannot see a this.TextLength ? \$\endgroup\$ – chazjn Sep 5 '14 at 10:06
  • \$\begingroup\$ You are right, it must be there. And TextLength is on WinForms TextBox, probably not on WPF version. \$\endgroup\$ – user52292 Sep 5 '14 at 11:06
2
\$\begingroup\$

I don't care for empty quotes designating an empty string. It should be replaced with String.Empty. It simply clarifies your intention and leaves no ambiguity that you perhaps forgot to put something in the quotes, or maybe even meant to put a space in there. Otherwise, I feel this is very simple and solid.

\$\endgroup\$
  • 2
    \$\begingroup\$ I just checked and Java does not have String.EMPTY. I'm a bit sad. ( I know it's a C# question, I just verified for Java ) \$\endgroup\$ – Marc-Andre Sep 3 '14 at 16:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.