Skip to content

Conversation

@ScrubN
Copy link

@ScrubN ScrubN commented May 9, 2025

The previous code was using a configuration property to check if the file exists, which would always pass because it defaults to true.

@ScrubN ScrubN reopened this May 9, 2025
@ScrubN
Copy link
Author

ScrubN commented May 9, 2025

Force pushed the wrong branch, whoops.

Copy link

@intbeam intbeam left a comment

Choose a reason for hiding this comment

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

Checking if a file exists using File.Exists may cause unintended behavior because you can't know if the file gets immediately deleted between File.Exists and new FileStream (no matter how unlikely)

Use a try-catch for this specific type of issue, and only catch the specific exception (and not anything and especially not everything else)

try
{
}
catch(System.IO.FileNotFoundException ex)
{
}

CheckIfFileExists is a property on that class that affects the behavior of ShowDialog(), it doesn't do anything on its own. It prevents the user from entering a filename into the file text box that does not exist, so that should probably be set to true

@ScrubN
Copy link
Author

ScrubN commented May 9, 2025

If the user deletes the file in the 50 microseconds between the File.Exists check and the new FileStream and get a crash, then they deserve a medal. I see your point, but I disagree that it's relevant in this specific scenario, especially because I want to minimize diff changes to make it easier for TheCod3r to review the PR, and to avoid merge conflicts with #12.

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