[Last Call] Learn how to a build a cloud-first strategyRegister Now

x
  • Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 179
  • Last Modified:

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

0
John Bolter
Asked:
John Bolter
2 Solutions
 
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
 
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

Featured Post

Upgrade your Question Security!

Add Premium security features to your question to ensure its privacy or anonymity. Learn more about your ability to control Question Security today.

Tackle projects and never again get stuck behind a technical roadblock.
Join Now