List with items returns empty

Question

I have created a simple List function but if I Loop through the List it's empty. It should not be!

// List function 
    public class process_hook
    {
        public static List<String> pro_hook = new List<String>
                                              (new String[] { list_all_pocesses() });
        protected static string list_all_pocesses()
        {
            StringBuilder _list = new StringBuilder();
            foreach (Process i in Process.GetProcesses("."))
            {
                try
                {
                    foreach (ProcessModule pm in i.Modules)
                    {
                        pro_hook.Add(pm.FileName.ToString());
                    }
                }
                catch { }
            }
            return _list.ToString();
        }
    }


        // call 
        private void button1_Click(object sender, EventArgs e)
        {
            foreach (String _list in process_hook.pro_hook)
            {
                Console.WriteLine(_list);
            }
        }

Please revert your code back to its broken state as if you correct it in your question then all the answers to your original question are invalid. @Jon Skeet's answer is by far the most detailed and explained why your original code was not working rather than just giving you a completed solution without explanation. — Piers Myers, Jan 14 '11 at 13:57

Jon Skeet · Answer 1 · 2011-01-14T13:39:04.723

55

Well this is a problem to start with:

catch { }

If anything goes wrong, you'll just silently abort.

Maybe that's what's happening? (EDIT: It is. See later.)

The next problem is that your "list" will only ever contain a single string... is that really what you intended? I doubt that the list you're seeing is actually empty - but it will contain a single empty string.

(As a side note, I would strongly suggest that you start following .NET naming conventions and avoid global variables like this.)

EDIT: Aargh - I've just realized what you've done. You're probably actually getting a NullReferenceException in list_all_pocesses, which you've caught and ignored.

Your call to pro_hook.Add is made before you've assigned a value to pro_hook. Basically you've got a variable initializer which uses a method which in turn uses the variable. Don't do that. If you step through your code in the debugger, you may get more of an idea of what's going on, but basically you've created a big ball of spaghetti for yourself.

Why doesn't list_all_pocesses just return a List<string>? Why are you using a StringBuilder at all?

edited Jan 14 '11 at 13:39

answered Jan 14 '11 at 13:33

Jon Skeet

1,421,763
867
9,128
9,194

$exception {"Access is denied"} System.Exception {System.ComponentModel.Win32Exception} – honibis Jan 14 '11 at 13:38
@honibis: Even without that, he'll get a NullReferenceException. – Jon Skeet Jan 14 '11 at 13:39
It will definitely throw a `NullReferenceException`, which ends up being swallowed. The way the `pro_hook` variable is initialized is wrong. @bniwredyc fixed it in his solution. – Thorarin Jan 14 '11 at 13:48
@Thorarin: It will only throw a NullReferenceException if nothing else throws an exception first :) – Jon Skeet Jan 14 '11 at 14:02
Well, the `foreach` is not likely to cause a problem. `Process.GetProcesses` is not inside a try-catch ;) – Thorarin Jan 15 '11 at 15:28

score 5 · Answer 2 · answered Jan 14 '11 at 13:35

5

Well... you're returning an empty string builder. That's your problem. Your code is doing what you're telling it to do. :)

 return _list.ToString();

answered Jan 14 '11 at 13:35

Leniel Maccaferri

100,159
46
371
480

bniwredyc · Answer 3 · 2011-01-14T14:09:54.847

3

public class process_hook
{
    public static List<string> pro_hook = list_all_pocesses();
    protected static List<string> list_all_pocesses()
    {
        List<string> list = new List<string>();

        foreach (Process i in Process.GetProcesses("."))
        {
            foreach (ProcessModule pm in i.Modules)
            {
                list.Add(pm.FileName.ToString());
            }
        }
        return list;
    }
}

edited Jan 14 '11 at 14:09

answered Jan 14 '11 at 13:36

bniwredyc

8,649
1
39
52

`pro_hook` is `List` but `list_all_processes` has return type of `string` --> this won't compile. – Richard Jan 14 '11 at 13:47
1

Hope you don't mind, but I removed the underscore prefix for the local `list` variable. The .NET style guidelines weren't being followed anyway, but the underscore suggests a member variable. – Thorarin Jan 14 '11 at 13:53
@Thorarin, np, I've just followed op's initial style. – bniwredyc Jan 14 '11 at 14:02
edit: try/catch removed because now the only possible source of exceptions is the GetProcesses() method call and it was ouside try/catch block originally. – bniwredyc Jan 14 '11 at 14:14

score -3 · Accepted Answer · answered Jan 14 '11 at 13:37

-3

_list.ToString() is not going to return any meaningful value. Try something like this instead:

public static List<string> pro_hook = list_all_processes();

protected static List<string> list_all_processes()
{
    var list = new List<string>();

    foreach (Process i in Process.GetProcesses(".")) {
        try {
            foreach (ProcessModule pm in i.Modules) {
                list.Add(pm.FileName);
            }
        } catch { }
    }

    return list;
}

answered Jan 14 '11 at 13:37

cdhowie

158,093
24
286
300

2

Sorry, but I just had to down vote you for leaving the "catch { }" in there. – Bernhard Hofmann Jan 14 '11 at 13:57
7

Why just give corrected code without explanation? What will anyone learn from this? The empty catch block is a really bad thing to do - either remove it all together or ensure you do something with any exceptions caught. – Piers Myers Jan 14 '11 at 14:02
why is the catch {} is a bad idea? – User6996 Jan 14 '11 at 14:05
10

@Power-Mosfet: Because it swallows *all* exceptions *silently*: you can't tell when *anything's* gone wrong. a) it's almost always a bad idea to handle all exceptions uniformly; b) it's almost always a bad idea to swallow exceptions, taking no action (not event logging). Doing both at once is a *terrible* idea. – Jon Skeet Jan 14 '11 at 14:09
Jon, thank you for the explanation. to be honest i really don't care about this readnow, just experimenting in my spare time. Im just a beginner who is happy what a working application. thank you for the input. – User6996 Jan 14 '11 at 14:29
@Piers: You obviously did not actually read my answer. The main issue I saw was the bizarre attempt to build up an array in a string builder and then return the result of `List.ToString()`, which will not be meaningful. I pointed that out because it was the primary error I saw. There were others (the NRE, arguably the empty catch-all block) but they weren't as strong of an error to me. And no, I also don't consider `catch { }` to be inherently bad in every possible case. It's useful when experimenting or when you simply *don't care* about exceptions in a particular block. – cdhowie Jan 14 '11 at 14:40
@cdhowie: I did read your answer, several times in fact. Just saying "_list.ToString() is not going to return any meaningful value" does not explain why - the list does have a meaningful value, null, which means that it doesn't contain any values as none have been added in the loop. – Piers Myers Jan 14 '11 at 14:55
@Piers: `List.ToString()` will *never* return a meaningful value. It will return `"System.Collections.Generic.List`1[System.String]"`. – cdhowie Jan 14 '11 at 15:01
@cdhowie: The original code doesn't call `List.ToString()` as far as I can see. It calls `StringBuilder.ToString()` (because `_list` is a `StringBuilder`). Where do you see the call to `List.ToString()`? Maybe I've missed something. – Jon Skeet Jan 14 '11 at 15:04
@Jon: revision 5 had it with `List`, and has been correctly rollbacked to the previous unanswered version. I guess it would be more coherent if cdhowie deletes his answer, as it is not in line with what is asked. – Alex Bagnolini Jan 14 '11 at 16:12
2

@Alex: Perhaps. The question is kind of bizarre in and of itself. As this code worked for the OP, I'm hesitant to delete it outright. But the question could probably be closed and deleted as "too localized" -- this particular problem is unlikely to be encountered by anyone else as it is a unique symphony of diverse and egregious errors. – cdhowie Jan 14 '11 at 16:42

List with items returns empty

4 Answers4