A "dry" principle case

Question

there is a need to check the data entered by the user and display a notification on the screen in case of incorrect input. I used the following method, but it seems to me that it don't quite fit into the principle of "don't repeat yourself". Does anyone have any ideas for simplification?

int productid = 0;
string errorMessage = "Неправильный формат данных:\n",
       productName = "", productGroup = "", productType = "";
if (!int.TryParse(ProductIdTB.Text, out productId))
{
    errorMessage += "+ Номер продукта\n";
}
if (string.IsNullOrEmpty(ProductNameTB.Text))
{
    errorMessage += "+ Название продукта\n";
}
else
{
    productName = ProductNameTB.Text;
}
if (string.IsNullOrEmpty(ProductGroupTB.Text))
{
    errorMessage += "+ Группа продукта\n";
}
else
{
    productGroup = ProductGroupTB.Text;
}
if (string.IsNullOrEmpty(ProductType.Text))
{
    errorMessage += "+ Вид продукта";
}
else
{
    productType = ProductType.Text;
}
if (errorMessage.Split(' ').Length > 1)
{
    MessageBox.Show(errorMessage);
    return;
}

An opinion base question is not for stackoverflow.com, nonetheless you are overwriting your variable with every if else condition, so if you have two errors you will be able to read just one at a time, maybe you can group your errors in some different way, or maybe throw an exception — DonMiguelSanchez, Jun 12 '23 at 08:18
You might want to consider removing all the `else` bits and simply populate the values only after the last condition, when you know there are no errors. — Zohar Peled, Jun 12 '23 at 08:21
@DonMiguelSanchez No, OP is using `+=` thus appending to the errormessage variable not overwriting it ... — derpirscher, Jun 12 '23 at 08:29
In the ASP.NET world we have model binders and validation attributes that perform all of the basic input validations so we don't have to. — phuzi, Jun 12 '23 at 08:29
Instead of splitting by `' '` you proably want to split by `'\n'` instead. And also the check with `> 1` won't work, as you already append `\n` to the header, thus the split will always give you two elements at minimum. I'd furthermore suggest not displaying such a big amount of data in a textbox. There are better means of validating and displaying errors in Windows Forms ... — derpirscher, Jun 12 '23 at 08:34
What happens if there are no errors? Then `errormessage` is still `"Неправильный формат данных:\n"` ie it contains 2 spaces and `errormessage.Split(' ')` will result in an array of length `3`. Thus, your messagebox will *always* be displayed — derpirscher, Jun 12 '23 at 08:38
WPF already covers this through [data validation](https://learn.microsoft.com/en-us/dotnet/desktop/wpf/data/data-binding-overview?view=netframeworkdesktop-4.8#data-validation), offering ways to specify validation rules on the form or object, trigger them automatically when a value changes, and display error indicators — Panagiotis Kanavos, Jun 12 '23 at 08:39

Thomas Weller · Accepted Answer · 2023-06-12T08:25:23.323

I can imagine building a class that does the checks for you and collects all errors, either in a string or a list of strings.

class ErrorMessageBuilder
{
    string totalmessage = "";

    void AppendErrorIfEmpty(TextBox t, string textboxname)
    {
        if (t.Text.IsNullOrEmpty())
        {
            totalmessage += textboxname + " can't be empty" + Environment.NewLine;
        }    
    }

    void AppendErrorIfNotInt(TextBox t, string textboxname)
    {
         int value;
         if (!int.TryParse(t.Text, out value))
         {
             totalmessage += textboxname + " must be an integer number"  + Environment.NewLine;
         }
    }
}

This would reduce the code to

var emb = ErrorMessageBuilder();
emb.AppendErrorIfNotInt(ProductIdTB, "Product ID");
emb.AppendErrorIfEmpty(ProductNameTB, "Product Name");
...

Since this looks like WinForms development to me, you can also have a look at the ErrorProvider class. Like with tooltips, it allows one error message per control. Perhaps that would be even more user-friendly.

Good idea, i'll use it – programMUSTer Jun 12 '23 at 08:39 — programMUSTer, Jun 12 '23 at 08:39

score 0 · Answer 2 · answered Jun 12 '23 at 13:31

0

Depending on how much this is used, it might not scale out very well considering the immutability of strings. For every += you are creating a new string instance, the bigger the string the more memory and garbage...

Consider StringBuilder and it's Append/AppendFormat methods.

+=/concat, string performance

answered Jun 12 '23 at 13:31

itsAllGravy_

1
2

yep, but i use `+=` isn't it appending a string instead of creating a new one? – programMUSTer Jun 12 '23 at 14:15
1

@programMUSTer: no, it doesn't it still creates a new string and gets rid of the old one. As long as you don't have kilobytes of strings, the normal approach with += is totally fine. – Thomas Weller Jun 12 '23 at 19:06

A "dry" principle case

2 Answers2