CODEWITHSUNDEEP
c#
G Gr
G Gr

Reputation: 6090

loop adding more to listbox than expected

I am trying to achieve something that is starting to annoy me, I have a user defined integer that is inputed using a textbox. The number the user enters must be 7 digits long, I add each of the 7 numbers into a list. I want to, starting from the first digit of the user defined number multiply by 8 down to 2.

For instance if the user inputs 4565457 then the first number becomes 4 x 8:

4 x 8 
5 x 7
6 x 6
5 x 5
4 x 4
5 x 3
7 x 2

I have attempted this but the output I get in my listbox has multiple numbers, alot more than the 7 entrys:

    List<int> integerList = new List<int>();
    for (int a = 0; a < textBox1.Text.Length; a++)
    {
            integerList.Add(int.Parse(textBox1.Text[a].ToString())); 

            foreach (int item in integerList)
            {
                for (int b = 8; b > 1; b--)
                {
                    listBox1.Items.Add(item * b);
                }
            } 
    }

Upvotes: 0

Views: 198

Answers (4)

Austin Salonen
Austin Salonen

Reputation: 50235

Too many loops in my opinion.

// at this point, it's assumed that inputText characters are all digits.
// you should have code that confirms this (var shouldContinue = inputText.All(char.IsDigit);)
var inputText = "4565457";
var input = inputText.Select(c => c - 48);   // quick and dirty "ToInt" per char

var multipliers = Enumerable.Range(2, inputText.Length).Reverse();

var multiplied = input.Zip(multipliers, (a, b) => a*b);

listBox1.Items.AddRange(multiplied.ToArray());

This becomes pretty simple when you break it down.

I need the integer value of each of my character digits. Assuming ASCII, this is simply subtracting 48 from the character (you can intuit this from asciitable.com). Using .Select, we get the result lazily evaluated.

var toInt = givenText.Select(digit => digit - 48);

I need the values 2 thru 8 reversed.

// can really only be used once
var multipliers = new [] {8, 7, 6, 5, 4, 3, 2};

// can be used with any length string > 0
var multipliers = Enumerable.Range(2, givenText.Length).Reverse();

For each value, I need it multiplied by its complement in the multipliers array. Zip is a method that pairs array (more generally IEnumerable) items and quits as soon as one of the sources is exhausted. It takes function that returns whatever you do with the pair; in this case, multiplying them.

Func<int, int, int> multiply = (a,b) => a*b;
var multiplied = toInt.Zip(multipliers, multiply);

// ... and inlined.
var multiplied = toInt.Zip(multipliers, (a,b) => a*b);

I want to add those values to my ListBox. The code above actually hasn't actually evaluated anything yet. All the work gets executed when .ToArray() is called.

//listBox1.Items.Clear()  //maybe?
listBox1.Items.AddRange(multiplied.ToArray());

When you're done, you end up with the code above the break.

Upvotes: 1

Matthew Watson
Matthew Watson

Reputation: 109732

I think what's happening is that your inner loops are executing inside the outer loop, and they should be alongside it like this:

List<int> integerList = new List<int>();
for (int a = 0; a < textBox1.Text.Length; a++)
{
    integerList.Add(int.Parse(textBox1.Text[a].ToString())); 
}

foreach (int item in integerList)
{
    for (int b = 8; b > 1; b--)
    {
        listBox1.Items.Add(item * b);
    }
}

Look at your outermost loop. The first iteration, you'll have added one digit to integerList. Then the innermost loop loops 8 times using the one digit that's in integerList.

Next time your outer loop iterates, you add the next digit to integerList, so now it has two digits. And on we go into the inner loops, which loop 8 times again, but this time over an integerList that has two items.

And so on and so forth.

But there's still an error! (I missed this at first because I misread your spec in the OP)

You only want to loop once per input digit, and do one multiplication per loop.

So we really need to fix the code as follows:

List<int> integerList = new List<int>();

for (int a = 0; a < textBox1.Text.Length; a++)
{
    integerList.Add(int.Parse(textBox1.Text[a])); // Note: Didn't need the ToString()!
}

int b = 8; // Will be 8 for the first multiplication.

foreach (int item in integerList) // Loop once per input digit.
{
    listBox1.Items.Add(item * b);
    --b; // So now it will be correct for the next loop iteration.
}

We're only looping once per input digit now. And b starts at 8 and goes down by 1 for each loop iteration.

Upvotes: 2

Justin Pihony
Justin Pihony

Reputation: 67115

How about something like this

YourText.Select((character, index) => character + "x" + (YourText.Length - index)));

Now, you have your text remapped to have it with the indexer. Just wrap with an if to stop at a certain index.

All that is happening here is that I am taking the given text and telling it to map each character so that it is:

`character` 
plus the multiplier symbol 
plus the length of the original text minus the `index`

The final part is because the index will be the 0 based index of the string, so you seem to want this flipped.

So, in essence, this is just wrapping up what would normally be a for loop into a one liner. Another benefit is that it also will not execute until you request an actual value.

Upvotes: 2

computer10171
computer10171

Reputation: 445

You are getting more than 7 entries because of the nested for loop. Try putting the inner loop outside of the first loop.

for (int a = 0; a < textBox1.Text.Length; a++)
{
    ...
}

foreach (int item in integerList)
{
    ...               
}

Upvotes: 0

Related Questions