Skip to content
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.

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

Remove unecessary try..catch constructs #377

Merged
merged 16 commits into from Oct 23, 2018

Conversation

Mackiovello
Copy link
Contributor

@Mackiovello
Copy link
Contributor Author

I decided to not do anything about the wonky spacing and indenting in many of these files. That should make it easier to review.

@Mackiovello
Copy link
Contributor Author

I decided to always remove the try..catch constructs since they always just swallowed the exception and wrote to the console. There are very few cases where that's a good practice.

@Mackiovello Mackiovello changed the title [WIP] Remove unecessary try..catch constructs Remove unecessary try..catch constructs Oct 15, 2018
@BillWagner
Copy link
Member

@Mackiovello I know this is aging a bit.

I'm having some internal discussions about what's the best plan here, because our sample code is used as a learning tool. Sometimes that means we use different practices than we would recommend for a production code base. We'll comment / review formally soon.

Copy link

@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.

Thanks for submitting another PR, @Mackiovello, and for eliminating some of the inappropriate exception handling from our examples. There are a few problems with simply removing exception handling, though. Our examples should handle exceptions that are likely to be thrown in the context of the example; they just shouldn't use Exception or SystemException to do that. In the case of the first example, exception handling should be used where there's user input. In the case of the last exception, one of the Add method calls is expected to fail; that should be handled as well. (I also can't get one of the examples to not throw an exception except by retrieving a union of identical URLs. But since the example isn't used in dotnet/docs or dotnet/dotnet-api-docs, that method can be removed.)

I've left comments for changes to be made in one C++ example and two C# examples; the comments apply, though, to all three sets of examples.

Again, thanks for doing this. We really appreciate it.

String^ remoteMachineName = Console::ReadLine();

// Get all notepad processess into Process array.
array<Process^>^myProcesses = Process::GetProcessesByName( "notepad", remoteMachineName );

Choose a reason for hiding this comment

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

The problem here, especially since Process.GetProcessesByName is passed user input, is that the string is unpredictable and remoteMachineName can cause a number of exceptions to be thrown. You can eliminate the ArgumentNullException by testing for a null or empty string before calling GetProcessesByName, but this as well as the C# and VB examples should still handle ArgumentException for an invalid remote computer name, and InvalidOperationException (for failure to connect to a remote computer -- an exception condition that isn't documented) at a minimum.

In general, while we certainly don't want to handle Exception and SystemException, we do want to handle exceptions that can realistically be thrown by an example.

@@ -51,29 +51,17 @@ private static void UnionDemo()
UrlIdentityPermission permIdPerm1 = new UrlIdentityPermission("http://www.fourthcoffee.com/process/");
UrlIdentityPermission permIdPerm2 = new UrlIdentityPermission("http://www.fourthcoffee.com/*");
UrlIdentityPermission p3 = (UrlIdentityPermission)permIdPerm1.Union(permIdPerm2);

Choose a reason for hiding this comment

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

This is throwing a NotSupportedException. Despite the documentation, this seems to only succeed if given two equal URLs. At the same time, the functionality is not supported in .NET Framework 4 and later. But the example is not used. The UnionDemo method can be removed from the source code (and the call to UnionDemo on line 12 should also be removed).

{
Console.WriteLine("Error description: " + e.Message);
}
BookComponent book1 = new BookComponent("Wizard's First Rule", "Terry Gooodkind");

Choose a reason for hiding this comment

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

Since the container doesn't support duplicates, we can't simply not handle exceptions. The problem here is that line 184 throws a SystemException, which is inappropriate. You could change it to ArgumentException, then handle the ArgumentException in a try/catch block around all of the BookComponent instantiates and calls to the Add method.

@Mackiovello
Copy link
Contributor Author

Thank you so much for the detailed feedback @rpetrusha!

I'll go through the examples over the next couple of days and ping you when I'm done.

I'm not quite sure how the project files are structured. For some examples, there's a project file in the same directory, but for others, there doesn't seem to be one. For example, for the GetProcessByName, I had to add my own project file in order to build and run. How is it supposed to work?

Also, I'm not super familiar with VB and C++, so please be extra observant that I'm following best practices with those languages.

@Mackiovello
Copy link
Contributor Author

Mackiovello commented Oct 18, 2018

Checklist for myself:

GetProcessByName

  • VB
  • C#
  • C++

UrlIdentity

  • VB
  • C#
  • C++

LibraryContainer

  • VB
  • C#
  • C++

@rpetrusha
Copy link

Thanks for responding, @Mackiovello. It's nice to be working with you again.

Since most of this is legacy code (almost all of the examples that handle Exception, and all of the example that handle SystemException, were written for .NET Framework 1.0 or 1.1), the examples were tested and run using the command-line compiler only. (In fact, when the examples were created, MSBuild didn't exist.) They can still be run using csc.exe.

Since our C++/CLI coverage tends to be spotty, we can simply plan on removing the C++ examples from the documentation if you don't feel comfortable modifying them. If you have difficulty with any of the VB examples, I can do them.

@Mackiovello
Copy link
Contributor Author

Since our C++/CLI coverage tends to be spotty, we can simply plan on removing the C++ examples from the documentation if you don't feel comfortable modifying them.

I could modify the C++ examples, but I don't have an environment setup for it so it would take some extra time. Should I create an issue to remove them or remove them in this PR?

@rpetrusha
Copy link

You can just remove them in this PR, @Mackiovello. We want to leave existing C++ examples that don't require modification.

@Mackiovello
Copy link
Contributor Author

@rpetrusha I've taken care of all the things you mentioned. Can you take another look?

@BillWagner BillWagner added the changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review label Oct 23, 2018
@rpetrusha
Copy link

Thank you so much, @Mackiovello, both for submitting the PR and for all of the additional changes you've made. This looks really good, so I'll merge your PR now.

@rpetrusha rpetrusha merged commit 032cf18 into dotnet:master Oct 23, 2018
@Mackiovello Mackiovello deleted the issue/6271 branch October 24, 2018 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants