-2

I have a simple list of integers,

original = new List<int>() { 3, 6, 12, 7, 7, 7, 7, 8, 10, 5, 5, 4 };

and a temp list of integers,

temp = new List<int>();

and a list of lists,

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

I want the original list to be divided into many lists, I use the following algorithm to assign the looped values to the list temp and later push the assigned values to listOf, after that I clear the list temp so that I can assign new values.

for (int i = 0; i < orginal.Count; i++) {
    if (orginal[i] > orginal[i + 1]) {
        for (int i2 = i + 1; i2 < orginal.Count; i2++) {
            if (orginal[i2] <= orginal[i2 + 1]) {
                listin.Add(orginal[i2]);
            } else {
                listOf.Add(listin);
                listin.Clear(); 
            }
        }
    }
}

The wanted output would be something like:

listOf[0] = 7 7 7 7 8 10
listOf[1] = 5 5

What do I need to change to achieve this output?

Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
Max
  • 37
  • 6

2 Answers2

3

Here is your error:

listOf.Add(listin);
listin.Clear();

You added listin to listOf, and then immediately cleared it. Whatever is inside listOf is now deleted, which is not what you wanted to happen.

Instead, you should "transfer ownership" of listin to listOf, and get yourself a brand-new list:

listOf.Add(listin);
listin = new List<int>();
Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
  • Just in case someone needs explaining the same using different words - variables != objects. The `listin` variable remembers some list-object. `listOf.Add(listin)` gets it from `listin` variable and puts it into `listOf`. But `listin` does not change. After that line, that list-object is remembered both by the list-of-lists (as the last element) and by `listin` variable. Later, clearing it in the next line by `listin.Clear()`, clears **that object**. Now, since that's the same object remembered in two ways, whever you read it as-an-element-of-list-of-lists, or by `listin` variable, it's empty. – quetzalcoatl Dec 30 '17 at 15:57
-1

Try following. There is no need to clear :

       static void Main(string[] args)
        {
            List<int> original = new List<int>() { 3, 6, 12, 7, 7, 7, 7, 8, 10, 5, 5, 4 };
            List<int> temp = null;

            while (original.Count > 0)
            {
                int count = 1;
                for(; (count < original.Count) && (original[count] >= original[count - 1]); count++);
                temp = original.Take(count).ToList();
                original = original.Skip(count).ToList();
            }
        }
jdweng
  • 33,250
  • 2
  • 15
  • 20
  • 1
    Terrible idea. For example, that Skip+ToList can be wasting a total of N^2 time for copying original list again and again, where in fact we only should be using the input list in a forward-iterator fashion. If you wanted to show "the power of LINQ" i.e. to skip creating copies and work on the original input objects, then you should change List<> to IEnumerable<> and drop that .ToList completely. OTOH, if that was not your point, then why use LINQ at all? Get an IEnumerator from the source list, or run a simple foreach, and have it all done in one pass with reading all source items only once. – quetzalcoatl Dec 30 '17 at 16:22
  • 1
    Then of course, I have to admit that code like you presented is a bit more readable and easier to understand. I personally like LINQ ops as they reduce amount of comments I need to write to explain what happens. Certainly you could also compact that last FOR/COUNT loop into 'functional' find-index-that-not-sat-monotonicity-condition and therefore have the outer loop have simple & clear three stages: analyze, result, moveforward. But.. changing not-so-complex O(N) to more-readable O(N^2) is not a thing to advise lightly not knowing the size of the input. – quetzalcoatl Dec 30 '17 at 16:25
  • I'm just duplicating the OPs code which which requires N^2. The OP wants all incremental sequences in the input array. The user didn't know how to clear the TEMP variable. I eliminated having to clear TEMP with my code. You can't reduce the complexity of the algorithm. If you do not copy the elements each time through the loop then you have to index each time through loop to the last item from previous time through loop. By the time you handle the indexing it is almost equivalent to the copying. – jdweng Dec 30 '17 at 20:02
  • 1
    `I'm just duplicating the OPs code which which requires N^2` - yes, but the original code is easily correctable to be O(N) just by altering outer `i` to be in sync with inner loop. Your code doesn't have that. `I eliminated having to clear TEMP with my code.` - so did dasblinkenlight, with less code, less changes to OP code, less (..), check out his answer. `the time you handle the indexing it is almost equivalent to the copying` - this is true in your code (using Skip) but that's totally not true in general case. No-copy skip/index can be done as `i+=n` when using random-access List. – quetzalcoatl Dec 30 '17 at 21:30
  • 1
    Finally, `If you do not copy the elements each time through the loop then you have to index each time` that's also not true. Having the input in a random-access list, you can read it via forward-iterator like GetEnumerable, scan each single value in the input list ONE time and observe last min/max element values and corresponding indices, push them onto a Stack when break in monotonicity is detected. You will get a set of pairs that denote monotonic ranges in original list, and you will easily reading them back from there. One scan. Zero copying. Trivial changes to OP's original algo. – quetzalcoatl Dec 30 '17 at 21:35
  • 1
    But all of what I'm saying right now is irrelevant. I'm just in a mood for arguing with your arguments, sorry... My original intent was different. The code you provided may be seen as more-readable than original, that's great, but may perform worse than OP's code (when it is fixed and works) just due to unnecessary-use/over-use/uncareful-use of LINQ thingies. Such things should be made clear, or else every new C# programmer will use SKIP thinking it's light/optimized/etc [whereas it is exactly the opposite](https://stackoverflow.com/questions/6245172/why-isnt-skip-in-linq-to-objects-optimized) – quetzalcoatl Dec 30 '17 at 21:39
  • I've given up on optimizing for performance when posting answers.I use to avoid using list objects due the large overhead and and preferred using arrays with less overhead.But found out it was a lot harder to code using arrays than lists.So when teaching beginners is it more important to give simple answers that are understandable or complex answer that run efficiently?I blame Bill Gates who believes that any Microsoft Product can be used by beginners with only 5 minutes of instructions.I like Unix much better than Window.Unix developers were Scientist while Microsoft used Junior Programmers. – jdweng Dec 30 '17 at 23:10
  • Yes, it's the fault of Bill Gates that you wrote such shabby code – David Heffernan Dec 31 '17 at 16:28