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

Code cleanup: Use number formatting utilities from the standard library #1743

Merged
merged 3 commits into from
May 30, 2022

Conversation

mcooley
Copy link
Member

@mcooley mcooley commented Nov 15, 2021

We should avoid using the NumberFormattingUtils class in CalcManager. The standard libraries in both C# and C++ offer number-formatting functionality, and we should use that where possible.

This PR removes all use of NumberFormattingUtils from Graphing Calculator code, so the only remaining code that uses it is in unit converter. Unit converter has many other problems (see #379), so I didn't attempt to refactor it in this PR.

There are three areas that changed:

  • Remove "fixed point with trimmed trailing zeros" formatting from the Graphing Calculator share output. Instead, use C#'s double.ToString(), which defaults to the general ("G") format. The notable change in behavior vs. the previous behavior is that scientific notation might be used if it enables a shorter output, although this should be rare since numbers of this magnitude are hard to input in the UI.
  • Remove "fixed point with trimmed trailing zeros" formatting from the Graph Settings code. Instead, use the default format of wostringstream, which is also a "general"-type format. Again, this changes behavior so scientific notation could be used, so we also need to change the code where these numbers are parsed back to doubles to use wistringstream, which can handle the scientific notation format.
  • Finally, move NumberFormattingUtils to the UnitConversionManager namespace. This indicates that it's intended for use only in the existing UnitConversionManager code and can be deleted if that code is refactored or removed.

@mcooley
Copy link
Member Author

mcooley commented Dec 13, 2021

This is a low-priority cleanup PR which is not urgent, but I'd also like to get it merged before I forget all the details about how it works :) @tian-lt @hanzhang54 @guominrui can one of you review this sometime soon?

@grochocki grochocki added the codebase quality Issues that are not bugs, but still might be worth improving (eg, code hygiene or maintainability) label May 19, 2022
Copy link
Contributor

@hanzhang54 hanzhang54 left a comment

Choose a reason for hiding this comment

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

Thanks Matt! It looks good to me. 😊

@hanzhang54
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@hanzhang54 hanzhang54 merged commit 8fab2cb into microsoft:master May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codebase quality Issues that are not bugs, but still might be worth improving (eg, code hygiene or maintainability)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants