Nested foreach is a bad practice?(please read)
-
You declare your variables
runningProcessesandprogramsToBeClosedbut you never initialize them. So they will be null in your foreach loops. Moreover, are you sure aQueueis a list of strings ? Did you try to debug your piece of code to see what's going on exactly ?No memory stick has been harmed during establishment of this signature.
my variables are already initialized and code is already working and doing its job, I just wanted to ask whether it's good practice.Because nested foreach seemed to much code to me. Thanks
-
my variables are already initialized and code is already working and doing its job, I just wanted to ask whether it's good practice.Because nested foreach seemed to much code to me. Thanks
I would avoid the use of one of the foreach loops. This way :
Process[] runningProcesses; //contains all running processes
List programsToBeClosed; //contains only string objectspublic void closePrograms(List programsToBeClosed)
{
foreach (Process item in runningProcesses)
{
if ((programsToBeClosed.Contains(item.ProcessName)) || (programsToBeClosed.Contains(item.MainWindowTitle)))
{
try //not to break loop when item couldn't be killed
{
item.Kill();
}
catch (Exception)
{
}
}
}
}Hope this helps.
No memory stick has been harmed during establishment of this signature.
-
I would avoid the use of one of the foreach loops. This way :
Process[] runningProcesses; //contains all running processes
List programsToBeClosed; //contains only string objectspublic void closePrograms(List programsToBeClosed)
{
foreach (Process item in runningProcesses)
{
if ((programsToBeClosed.Contains(item.ProcessName)) || (programsToBeClosed.Contains(item.MainWindowTitle)))
{
try //not to break loop when item couldn't be killed
{
item.Kill();
}
catch (Exception)
{
}
}
}
}Hope this helps.
No memory stick has been harmed during establishment of this signature.
this is it! Thank you very much for your help
-
hi, I'm using the below code to close some programs.But it seems like it's not the best practice. would you recommend me anything better? Thanks.
Process[] runningProcesses; //contains all running processes
Queue programsToBeClosed; //contains only string objects
public void closePrograms(Queue programsToBeClosed)
{
foreach (string program in programsToBeClosed)
{
foreach (Process item in runningProcesses)
{
try //not to break loop when item couldn't be killed
{
if (item.ProcessName == program || item.MainWindowTitle == program)
{
item.Kill();
}
}
catch (Exception)
{
}
}
}}
It depends on what the loops are doing. In this case it's unnecessary and phil.o's response is good. But it may still perform similar to what you presented because the Contains may simply do what your inner foreach is doing. I also assume that the lists will be fairly short so it won't matter too much. If you have long lists, and they are sorted (or can be), then I recommend a different tack, based on Ye Olde Master File Update*... 0) While both lists have items to process: 0.1) Compare the two current items 0.1.1) If they match, process the item, and advance both lists 0.1.2) If the first list's item is less than the second list's item, advance the first list 0.1.3) Otherwise, advance the second list 1) Do stuff with any items left in the first list (you won't need this) 2) Do stuff with any items left in the second list (you won't need this) This way you only iterate each list at most once. * The only thing I learned on COBOL class :sigh: , but I have needed it many times in my career, including within the last month.
-
It depends on what the loops are doing. In this case it's unnecessary and phil.o's response is good. But it may still perform similar to what you presented because the Contains may simply do what your inner foreach is doing. I also assume that the lists will be fairly short so it won't matter too much. If you have long lists, and they are sorted (or can be), then I recommend a different tack, based on Ye Olde Master File Update*... 0) While both lists have items to process: 0.1) Compare the two current items 0.1.1) If they match, process the item, and advance both lists 0.1.2) If the first list's item is less than the second list's item, advance the first list 0.1.3) Otherwise, advance the second list 1) Do stuff with any items left in the first list (you won't need this) 2) Do stuff with any items left in the second list (you won't need this) This way you only iterate each list at most once. * The only thing I learned on COBOL class :sigh: , but I have needed it many times in my career, including within the last month.
thanks for the detailed explanation.I solved the problem already but you gave me an idea about future problems. Regards...
-
hi, I'm using the below code to close some programs.But it seems like it's not the best practice. would you recommend me anything better? Thanks.
Process[] runningProcesses; //contains all running processes
Queue programsToBeClosed; //contains only string objects
public void closePrograms(Queue programsToBeClosed)
{
foreach (string program in programsToBeClosed)
{
foreach (Process item in runningProcesses)
{
try //not to break loop when item couldn't be killed
{
if (item.ProcessName == program || item.MainWindowTitle == program)
{
item.Kill();
}
}
catch (Exception)
{
}
}
}}
This response is an "aside:" it is not an answer to the OP's question, but a comment on the scenario, and a corollary question the question and answers raised ... for me. Looks like you got what you need from the answers here, and, after all, your original code works. But, I am surprised that this code did not run into the issue of a collection which is self-modified during a foreach, or for, loop, throwing a compile error. A problem which I've worked around in the past by using Linq to cast an existing collection to a List<whatever>, and then enumerating that generated collection which can be modified/deleted/whatever without causing an error, a trick I learned somewhere on StackOverFlow. But, I guess the 'simpler' structure of Array, by definition, cannot have this problem. best, Bill
"Last year I went fishing with Salvador Dali. He was using a dotted line. He caught every other fish." Steven Wright
-
It depends on what the loops are doing. In this case it's unnecessary and phil.o's response is good. But it may still perform similar to what you presented because the Contains may simply do what your inner foreach is doing. I also assume that the lists will be fairly short so it won't matter too much. If you have long lists, and they are sorted (or can be), then I recommend a different tack, based on Ye Olde Master File Update*... 0) While both lists have items to process: 0.1) Compare the two current items 0.1.1) If they match, process the item, and advance both lists 0.1.2) If the first list's item is less than the second list's item, advance the first list 0.1.3) Otherwise, advance the second list 1) Do stuff with any items left in the first list (you won't need this) 2) Do stuff with any items left in the second list (you won't need this) This way you only iterate each list at most once. * The only thing I learned on COBOL class :sigh: , but I have needed it many times in my career, including within the last month.
PIEBALDconsult wrote:
0.1.1) If they match, process the item, and advance both lists
0.1.2) If the first list's item is less than the second list's item, advance the first list0.1.1 assumes a sorting (which you state is required) ... of the two lists ... but also assumes matching of items in each list so that you can be assured for each ith. entry in List 1, the ith. entry in List2 is what you need to compare. If List2, or List1 has 'intermediate,' or 'extra' values, this technique will clearly fail. 0.1.2 implies that the lists' items have some possible form of quantitative comparison possible (in this case using less-than) : this is not relevant to the scenario implied in this thread by the OP. 1, and 2. imply a possible difference in the length of the lists, which you say can be ignored here: but what happens if in the the part of list 2 (which is longer than list1) is an item that matches an item in list1 which you did not find a match for as you progressed sequentially ? I'm not commenting on this from a 'picky-picky' perspective: I am generally curious as to why you think this 'schema' is broadly useful, or relevant to the OP's scenario, and I'd be even more curious as to how you actually used it recently. Now that we have Linq, selecting items that match from Lists is a whole lot easier (if more difficult to master ... at least for me). best, Bill
"Last year I went fishing with Salvador Dali. He was using a dotted line. He caught every other fish." Steven Wright
-
I would avoid the use of one of the foreach loops. This way :
Process[] runningProcesses; //contains all running processes
List programsToBeClosed; //contains only string objectspublic void closePrograms(List programsToBeClosed)
{
foreach (Process item in runningProcesses)
{
if ((programsToBeClosed.Contains(item.ProcessName)) || (programsToBeClosed.Contains(item.MainWindowTitle)))
{
try //not to break loop when item couldn't be killed
{
item.Kill();
}
catch (Exception)
{
}
}
}
}Hope this helps.
No memory stick has been harmed during establishment of this signature.
-
This might actually be worse as Contains probably iterates over the collection and you're calling it twice.
This might, even if I didn't test it. Thanks for pointing it out. Maybe using two
HashSet<string>would help ; first, lookups for hash should be quicker ; second, it could be taken profit ofUnion()andIntersect()methods to build a list of process names to be closed. But it's just a guess ; and I won't have time to measure gains or losses. Maybe the OP will.No memory stick has been harmed during establishment of this signature.
-
PIEBALDconsult wrote:
0.1.1) If they match, process the item, and advance both lists
0.1.2) If the first list's item is less than the second list's item, advance the first list0.1.1 assumes a sorting (which you state is required) ... of the two lists ... but also assumes matching of items in each list so that you can be assured for each ith. entry in List 1, the ith. entry in List2 is what you need to compare. If List2, or List1 has 'intermediate,' or 'extra' values, this technique will clearly fail. 0.1.2 implies that the lists' items have some possible form of quantitative comparison possible (in this case using less-than) : this is not relevant to the scenario implied in this thread by the OP. 1, and 2. imply a possible difference in the length of the lists, which you say can be ignored here: but what happens if in the the part of list 2 (which is longer than list1) is an item that matches an item in list1 which you did not find a match for as you progressed sequentially ? I'm not commenting on this from a 'picky-picky' perspective: I am generally curious as to why you think this 'schema' is broadly useful, or relevant to the OP's scenario, and I'd be even more curious as to how you actually used it recently. Now that we have Linq, selecting items that match from Lists is a whole lot easier (if more difficult to master ... at least for me). best, Bill
"Last year I went fishing with Salvador Dali. He was using a dotted line. He caught every other fish." Steven Wright
BillWoodruff wrote:
assumes a sorting
Yes, both lists must be sorted.
BillWoodruff wrote:
for each ith. entry in List 1, the ith. entry in List2 is what you need to compare
No, when there is no match, only the list with the lower value is advanced.
BillWoodruff wrote:
how you actually used it recently.
Given lists of database items (creation scripts of tables, views, procedures, etc.) between dev and prod (for instance), find those which exist in only one environment or whose scripts are not identical. (I may write an article on it.) See also: http://cs.uni.edu/~east/teaching/cobol/topics/seq_update/algorithms.html[^]
-
It depends on what the loops are doing. In this case it's unnecessary and phil.o's response is good. But it may still perform similar to what you presented because the Contains may simply do what your inner foreach is doing. I also assume that the lists will be fairly short so it won't matter too much. If you have long lists, and they are sorted (or can be), then I recommend a different tack, based on Ye Olde Master File Update*... 0) While both lists have items to process: 0.1) Compare the two current items 0.1.1) If they match, process the item, and advance both lists 0.1.2) If the first list's item is less than the second list's item, advance the first list 0.1.3) Otherwise, advance the second list 1) Do stuff with any items left in the first list (you won't need this) 2) Do stuff with any items left in the second list (you won't need this) This way you only iterate each list at most once. * The only thing I learned on COBOL class :sigh: , but I have needed it many times in my career, including within the last month.
I agree. Now if you have an elegant implementation, that would make an interesting Tip/Trick IMO. BTW: IMO what Bill hinted at is correct, the lists need to be fully sorted; e.g. sorting people on last name only would not guarantee a unique sorting order and then your approach would fail. :)
Luc Pattyn [My Articles] Nil Volentibus Arduum
-
I agree. Now if you have an elegant implementation, that would make an interesting Tip/Trick IMO. BTW: IMO what Bill hinted at is correct, the lists need to be fully sorted; e.g. sorting people on last name only would not guarantee a unique sorting order and then your approach would fail. :)
Luc Pattyn [My Articles] Nil Volentibus Arduum
I've been giving that some thought, but, while a generic solution may be devised, I'm not sure how elegant it would be because of the variety of possible data sources (my latest usage uses
DataReaders). I can imagine a MasterFileUpdate<T,U> that requires not only references to two data sources (which may not be the same type), but also a delegate to a Comparer<T,U>, and delegates for advancing the sources. Plus, as withDataReaders (but not withLists), some data sources will need an initial advance before they can be read. My concept then, once the Updater is set up and put in motion, would involve the Updater raising events to report its findings as it goes. The calling code would only attach handlers for the events it needs. ... on further thought I think there would need to be four generic parameters, not two :sigh: . It's still an interesting exercise and perhaps I'll get right on it. Edit: I have it roughed in, but not tested. It may not be too bad after all. Edit: Tested it withList<int>s -- it seems to work. I need to refactor it into a recent app and test it further. -
This response is an "aside:" it is not an answer to the OP's question, but a comment on the scenario, and a corollary question the question and answers raised ... for me. Looks like you got what you need from the answers here, and, after all, your original code works. But, I am surprised that this code did not run into the issue of a collection which is self-modified during a foreach, or for, loop, throwing a compile error. A problem which I've worked around in the past by using Linq to cast an existing collection to a List<whatever>, and then enumerating that generated collection which can be modified/deleted/whatever without causing an error, a trick I learned somewhere on StackOverFlow. But, I guess the 'simpler' structure of Array, by definition, cannot have this problem. best, Bill
"Last year I went fishing with Salvador Dali. He was using a dotted line. He caught every other fish." Steven Wright
both the array and the queue contain snapshots, i.e. they are views on the recent past; they don't get affected by changing the present, so there is no issue using them in a foreach. :)
Luc Pattyn [My Articles] Nil Volentibus Arduum
-
This response is an "aside:" it is not an answer to the OP's question, but a comment on the scenario, and a corollary question the question and answers raised ... for me. Looks like you got what you need from the answers here, and, after all, your original code works. But, I am surprised that this code did not run into the issue of a collection which is self-modified during a foreach, or for, loop, throwing a compile error. A problem which I've worked around in the past by using Linq to cast an existing collection to a List<whatever>, and then enumerating that generated collection which can be modified/deleted/whatever without causing an error, a trick I learned somewhere on StackOverFlow. But, I guess the 'simpler' structure of Array, by definition, cannot have this problem. best, Bill
"Last year I went fishing with Salvador Dali. He was using a dotted line. He caught every other fish." Steven Wright
BillWoodruff wrote:
But, I am surprised that this code did not run into the issue of a collection which is self-modified during a foreach, or for, loop, throwing a compile error.
There's no reason it would. The OP is not actually attempting to modify the list, rather he is acting on an item of the list which represents a snapshot. If he attempted to remove the item from the list, he would hit exactly the issue you describe.
Forgive your enemies - it messes with their heads
"Mind bleach! Send me mind bleach!" - Nagy Vilmos
My blog | My articles | MoXAML PowerToys | Mole 2010 - debugging made easier - my favourite utility