Skip to content

Conversation

@jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Oct 29, 2025

Fixes #

Context

I was working on something else entirely and saw some easy improvements to make in WriteLinesToFile.

Changes Made

  1. File.ItemSpec is being accessed 14 times. I think the implementation will make a new string each time.
  2. It has two code paths, overwrite vs not and both code paths were creating the directory
  3. The contents of the file to write are stored in a StringBuilder with no initial capacity. I took a wild guess and set to lines * 64
  4. If no lines are specified, a StringBuilder is no longer created and instead it uses string.Empty
  5. Since the contents are now stored in a string on all code paths, the comparison of the current vs new is simplified since string.Equals() will check the length
  6. Reduced nesting by inverting the if (File != null) statement

Testing

From what I can tell, the existing tests are good.

Notes

@jeffkl jeffkl self-assigned this Oct 29, 2025
Copilot AI review requested due to automatic review settings October 29, 2025 22:42
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the WriteLinesToFile.Execute() method to improve code readability and maintainability through better control flow structure and reducing nesting levels.

  • Inverted the null check for File to use early return
  • Extracted filePath variable from File.ItemSpec using FileUtilities.NormalizePath at the beginning of the method for consistent reuse
  • Removed redundant null checks and improved code organization by flattening nested conditions

@jeffkl jeffkl requested a review from a team October 30, 2025 16:32
@rainersigwald
Copy link
Member

  1. I think the implementation will make a new string each time.

Only if it's got a % in it, by my read

// If there are no percent signs, just return the original string immediately.
// Don't even instantiate the StringBuilder.
int indexOfPercent = escapedString.IndexOf('%');
if (indexOfPercent == -1)
{
return trim ? escapedString.Trim() : escapedString;
}

(but still no need to check that every time)

@YuliiaKovalova YuliiaKovalova merged commit 27c7c81 into dotnet:main Nov 4, 2025
16 checks passed
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