Skip to content

Conversation

@mairaw
Copy link
Contributor

@mairaw mairaw commented Jun 25, 2018

Fixes #6118 - found a few more topics with the same issue

Hide whitespace changes for a better diff

Copy link
Contributor

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

I left a couple of comments, @mairaw, about removing 2 using statements and the sections on compiling the code. You can then merge when you're ready.

}
```

## Compiling the code
Copy link
Contributor

Choose a reason for hiding this comment

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

This section can also be deleted, References are to assemblies, not namespaces; importing the System,.Net namespace is handled by the using statement. No types in the System.Net assembly are used.

}
```

## Compiling the code
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as previously: this section can be deleted.

}
```

## Compiling the code
Copy link
Contributor

Choose a reason for hiding this comment

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

This section can be removed.

using System;
using System.IO;
using System.Net;
using System.Text;
Copy link
Contributor

Choose a reason for hiding this comment

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

the using System.Text; statement is not needed and can be deleted.

using System;
using System.IO;
using System.Net;
using System.Text;
Copy link
Contributor

Choose a reason for hiding this comment

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

the using System.Text; statement is not needed and can be deleted.

@mairaw
Copy link
Contributor Author

mairaw commented Jun 25, 2018

@rpetrusha thanks for your thorough review. I wasn't much looking at the content inside the sections.

Since we were improving the topics, I also added the VB version of those. Do you wanna double check?

Copy link
Contributor

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

Left a number of additional suggestions, @mairaw. Other than removing Imports System, which I'd recommend strongly, you can decide whether you want to do them or not.

```

```vb
Imports System
Copy link
Contributor

Choose a reason for hiding this comment

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

Imports System isn't necessary; it's automatically imported by the compiler.

Imports System.Net

Namespace Examples.System.Net
Public Class WebRequestGetExample
Copy link
Contributor

Choose a reason for hiding this comment

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

It's typical to do this as a module (Public Module WebRequestGetExample)


Namespace Examples.System.Net
Public Class WebRequestGetExample
Public Shared Sub Main()
Copy link
Contributor

Choose a reason for hiding this comment

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

If this becomes a module, the signature becomes Public Sub Main()

reader.Close()
response.Close()
End Sub
End Class
Copy link
Contributor

Choose a reason for hiding this comment

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

This becomes End Module

}
```

```vb
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments as previously: System does not need to be imported, and a Module is better Irequiring a change to the Main method signature as well).

```

```vb
Imports System
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments here as for previous examples.

StreamReader reader = new StreamReader(responseStream);
Console.WriteLine(reader.ReadToEnd());

Console.WriteLine("Download Complete, status {0}", response.StatusDescription);
Copy link
Contributor

Choose a reason for hiding this comment

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

These Console.WriteLine statements with composite formatting strings could be done as interpolated strings in C# and VB if you'd like to make the changes. Here, `Console.WriteLine($"Download Complete, status {response.StatusDescription}");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are we adding notes about C# version needed when adding interpolated strings?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, @mairaw. I missed your question. No, we haven't been, other than in the C# and VB interpolated string docs themselves.

@mairaw
Copy link
Contributor Author

mairaw commented Jun 26, 2018

Appreciate your comments @rpetrusha. I believe all the feedback is now addressed.

@mairaw mairaw merged commit 85f4f1d into dotnet:master Jun 26, 2018
@mairaw mairaw deleted the empty-sections branch June 26, 2018 21:13
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.

2 participants