Skip to content
This repository has been archived by the owner on Jul 26, 2023. It is now read-only.

Related with #71 #79

Merged
merged 4 commits into from
Nov 27, 2015
Merged

Related with #71 #79

merged 4 commits into from
Nov 27, 2015

Conversation

jmelosegui
Copy link
Collaborator

  • Complying with the conventions for helper methods
  • Added new helper method

throw new Win32Exception();
}

int bufferSize = buffer.ToInt32();
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's named bufferSize but it look like it's used as a current possition in the buffer


if (lpServiceName.Trim() == string.Empty)
{
throw new ArgumentException(nameof(lpServiceName));
Copy link
Collaborator

Choose a reason for hiding this comment

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

The single argument form of ArgumentException is taking a description (Where for ArgumentNullException it's a parameter name) :

throw new ArgumentException("Service name must not be empty", nameof(lpServiceName));

{
if (string.IsNullOrWhiteSpace(lpServiceName))
{
throw new ArgumentNullException(nameof(lpServiceName));
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're throwing not just in the null case but in the whitespace case. I suggest throwing ArgumentException then, which is less precise, to match the more general test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll go ahead and merge since you've done a lot of good work here and this is a net positive. But please consider following up with another short change here.

@AArnott
Copy link
Collaborator

AArnott commented Nov 27, 2015

Thanks, @jmelosegui. see inline comment, although I'll merge now.

AArnott added a commit that referenced this pull request Nov 27, 2015
Fix and add helper methods to AdvApi
@AArnott AArnott merged commit b23f38d into dotnet:master Nov 27, 2015
@jmelosegui jmelosegui deleted the AdvApi_Helpers branch November 27, 2015 13:23
AArnott added a commit that referenced this pull request Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants