Skip to content

Fix logic error in logging that logs errors in normal cases#97

Closed
tmenninger wants to merge 2 commits intoAzure:masterfrom
tmenninger:fix-logging-error-in-tasks
Closed

Fix logic error in logging that logs errors in normal cases#97
tmenninger wants to merge 2 commits intoAzure:masterfrom
tmenninger:fix-logging-error-in-tasks

Conversation

@tmenninger
Copy link
Copy Markdown

There are coding mistakes in the constructers of both IotHubSendTask and IotHubReceiveTask that log errors under all normal conditions

Checklist

  • I have read the [contribution guidelines] (https://github.com/Azure/azure-iot-sdk-java/blob/master/.github/CONTRIBUTING.md).
  • I added or modified the existing tests to cover the change (we do not allow our test coverage to go down). Should not need more coverage if tests already cover the null/not null cases.
  • If this is a modification that impacts the behavior of a public API
    • I edited the corresponding document in the devdoc folder and added or modified requirements. No API changes.
  • I submitted this PR against the correct branch:
    • This pull-request is submitted against the master branch.

Description of the problem

Please correctly use braces

Description of the solution

 public IotHubReceiveTask(IotHubTransport transport)
    {
        if (transport == null)
            throw new IllegalArgumentException("Parameter 'transport' must not be null");

        // Codes_SRS_IOTHUBRECEIVETASK_11_001: [The constructor shall save the transport.]
        this.transport = transport;

        logger.LogError("IotHubReceiveTask constructor called with null value for parameter transport");
    }

should become

 public IotHubReceiveTask(IotHubTransport transport)
    {
        if (transport == null) {

             logger.LogError("IotHubReceiveTask constructor called with null value for parameter transport");
            throw new IllegalArgumentException("Parameter 'transport' must not be null");
       }
        
        // Codes_SRS_IOTHUBRECEIVETASK_11_001: [The constructor shall save the transport.]
        this.transport = transport;
    }

@msftclas
Copy link
Copy Markdown

@tmenninger,
Thanks for having already signed the Contribution License Agreement. Your agreement has not been validated yet. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@prmathur-microsoft
Copy link
Copy Markdown
Member

Thanks for providing this change. I'll merge this in.

{

if (transport == null) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed

{
if (transport == null)
if (transport == null) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed

@prmathur-microsoft
Copy link
Copy Markdown
Member

I have two minor comments - if you can address them, this can easily go in. Thanks!

@prmathur-microsoft
Copy link
Copy Markdown
Member

Thanks for addressing comments. Your changes have been squashed to a single commit and merged at 29a3bcd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants