Skip to content

Fix list enumeration #11851

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
daxian-dbw merged 3 commits into from
Mar 26, 2020
Merged

Fix list enumeration #11851

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
013a95e
Fix list enumeration
nxtn Feb 13, 2020
a8c9b5e
Add a comment and a reference
nxtn Mar 12, 2020
6c324ff
Remove the reference
nxtn Mar 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 6 additions & 18 deletions src/Microsoft.PowerShell.Commands.Management/commands/management/Service.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1087,17 +1087,9 @@ internal List<ServiceController> DoStopService(ServiceController serviceControll
/// True if all dependent services are stopped
/// False if not all dependent services are stopped
/// </returns>
private bool HaveAllDependentServicesStopped(ICollection<ServiceController> dependentServices)
private bool HaveAllDependentServicesStopped(ServiceController[] dependentServices)
Copy link
Copy Markdown
Collaborator

@vexx32 vexx32 Feb 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the possible impact of changing the method signature here would be, but given it's a private method anyway, it's probably safe to do as long as the code calling the method is still OK to call it with this change.

Copy link
Copy Markdown
Contributor Author

@nxtn nxtn Feb 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is only called from one place, hence I believe it's separated to provide better readability.

{
foreach (ServiceController service in dependentServices)
{
if (service.Status != ServiceControllerStatus.Stopped)
{
return false;
}
}

return true;
return Array.TrueForAll(dependentServices, service => service.Status == ServiceControllerStatus.Stopped);
}

/// <summary>
Expand All @@ -1106,14 +1098,10 @@ private bool HaveAllDependentServicesStopped(ICollection<ServiceController> depe
/// <param name="services">A list of services.</param>
internal void RemoveNotStoppedServices(List<ServiceController> services)
{
foreach (ServiceController service in services)
{
if (service.Status != ServiceControllerStatus.Stopped &&
service.Status != ServiceControllerStatus.StopPending)
{
services.Remove(service);
}
}
// You shall not modify a collection during enumeration.
services.RemoveAll(service =>
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment why we have to use LINQ.

Copy link
Copy Markdown
Contributor Author

@nxtn nxtn Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not LINQ. It's a method of List<T>.

Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is just better. Please add a comment.

service.Status != ServiceControllerStatus.Stopped &&
service.Status != ServiceControllerStatus.StopPending);
}

/// <summary>
Expand Down