I have written some spaghetti code

Hi. I am still learning to write C#.

I am writing an ASP.Net application. I have some dynamically created controls and I need to update the values so they are retained on a post back.

I wrote the code below. I was showing it to a friend, and I was proud of my code, and he told me I could do better than that! He also said it was spaghetti code and I knew what he meant.

I have changed the hidden field to a String in my example code, but in ASP.Net it is a HiddenField. I have just converted it to a string and hard coded some of the ID's to make it easy to copy/paste and get working.

Can anyone help me simplify it.  I don't think I am wanting a one liner horrible perl type thing that I won't be able to understand tomorrow, but I don't want it to be spaghetti code.

Thank you





        private String hiddenField = "";


        private void updateHiddenFieldForASPNetDynamicallyCreatedControls(String id, Boolean fieldCheckbox)
        {
            List<String> fields = new List<String>(hiddenField.Split(new char[] { '\r' }, StringSplitOptions.RemoveEmptyEntries));
            Boolean foundField = false;
            for (int fieldNumber = 0; fieldNumber < fields.Count; fieldNumber++)
            {
                //posn 0 = id, posn 1 = boolean true or false, posn 2 ... n = lots of other stuff 
                String[] values = fields[fieldNumber].Split(new char[] { '\t' });
                if (id == values[0])
                {
                    values[1] = fieldCheckbox.ToString();
                    foundField = true;
                    StringBuilder replacement = new StringBuilder();
                    foreach(var value in values)
                    {
                        replacement.Append(value);
                        replacement.Append('\t');
                    }
                    fields[fieldNumber] = replacement.ToString().Trim(new char[]{'\t'});
                }
            }

            if (!foundField)
                fields.Add(String.Format("{0}\t{1}", id, fieldCheckbox));

            hiddenField = "";
            foreach (var field in fields)
                hiddenField += String.Format("{0}\r", field);
        }



        private void button1_Click(object sender, EventArgs e)
        {
            hiddenField = "123\tTrue\t1\t5\tyes\tyes\tundefined\r";
            updateHiddenFieldForASPNetDynamicallyCreatedControls("123", true);
            updateHiddenFieldForASPNetDynamicallyCreatedControls("123", false);
            updateHiddenFieldForASPNetDynamicallyCreatedControls("456", true);
            updateHiddenFieldForASPNetDynamicallyCreatedControls("789", true);
            updateHiddenFieldForASPNetDynamicallyCreatedControls("456", false);
        }

Open in new window

John BolterAsked:
Who is Participating?
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

käµfm³d 👽Commented:
I don't think I am wanting a one liner horrible perl type thing that I won't be able to understand tomorrow...
And that right there defeats your friend's argument, I think. Listen, today's machines are ridiculously powerful. We don't have to pay as much attention to keeping code small and tight. In the old days you had to because you didn't have as much RAM to play with, and processors weren't multi-cored. But now we do have those things. The important thing to do today is to make your code readable and understandable. As you said, 6 months from now, when you haven't looked at the code in the interim, you (or someone else) may need to come back to this code. Will it make sense 6 months from now? If it doesn't, then you've written bad code.

The only real suggestions I would make are that even though there are constructs that can take a single statement (like if and foreach), I prefer to include the braces. It's very easy for someone coming behind you to have a need to include an additional statement in an if block, and if you've written a single-line if sans braces, they may not see that you did not include the braces. Then what they think was an if block with two statements turns out to be an if block with one statement followed by a 2nd statement (outside of the if). The same would hold true for the foreach in your code above. In other words, instead of this:

if (!foundField)
    fields.Add(String.Format("{0}\t{1}", id, fieldCheckbox));

...

foreach (var field in fields)
    hiddenField += String.Format("{0}\r", field);

Open in new window

   
...I would put this:

if (!foundField)
{
    fields.Add(String.Format("{0}\t{1}", id, fieldCheckbox));
}

...

foreach (var field in fields)
{
    hiddenField += String.Format("{0}\r", field);
}

Open in new window


It's a bit longer to see on the screen, but you avoid a subtle bug that will not result in any kind of compile-time or run-time error (i.e. it's a logic error). The only exception I occasionally make is I would put the if body on one line so that it's clear there is only one line to the if body:

e.g.

if (!foundField) fields.Add(String.Format("{0}\t{1}", id, fieldCheckbox));

Open in new window


...but I usually only do that in rare cases, and where there are multiple if blocks in succession (so that it's evident I'm using single-line ifs).

Next, keep in mind that string concatenation, at a low level, is an expensive operation. You typically won't see any performance hit when you concatenate strings, but in general if you have more than four or five string concatenations going on it's usually better to use a StringBuilder to build your string. StringBuilders avoid the overhead of creating new strings with each concatenation--like what happens in a standard concatenation operation. You're already using a StringBuilder, but depending on how many times the foreach that appends to hiddenField runs, you may want to use a 2nd one.
0

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
Carl TawnSystems and Integration DeveloperCommented:
I think your friend may be misunderstanding what "spaghetti code" means :)

There is nothing "spaghetti code" about what you have written. Making code verbose isn't a bad thing, especially if it makes your intentions more obvious. Reducing the number of lines simply for the sake of trying to look clever by reducing the number of lines is idiotic.
0
John BolterAuthor Commented:
Thank you. You are both right, of course, so obviously right.
0
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
C#

From novice to tech pro — start learning today.