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

Simplify some of the calc engine string logic #449

Merged
merged 8 commits into from
Apr 11, 2019

Conversation

joseartrivera
Copy link
Contributor

For #428 some new buttons/functions will need to be added. In preparation, this PR attempts to make that process easier.

Description of the changes:

Currently Calculator handles strings by defining integers for each type of function that can be performed, this integer will eventually correspond with an index in s_engineStrings which holds the corresponding display string for each function. Some functions such as Sin can have multiple strings (degrees, rads, grads, inverse). Functions like Sin are mapped to another array called "rgUfne" where a new integer is given depending on the output string which will then be given to s_engineStrings. The new integer returned by the "rgUfne" array runs the risk of overlapping with any new functions that may be added in CCommand.h. Furthermore, it is expected that the strings in s_engineStrings and rgUfne are defined in a particular order (not necessarily sequential), otherwise the logic will break. This makes adding new strings for new functions confusing and difficult, since a lot of the logic is not clearly defined.

This PR attempts to make this a bit simpler by changing the s_engineStrings and rgUfne arrays to be unordered_maps instead of arrays. For s_engineStrings the keys will now be strings, allowing the existing logic for indexing to be used by simply converting the number into a string to access the value. This will also allow us to create keys in the future that are not limited to integers but to strings that hold more meaning.

The rgUfne array will also be updated to be a map that will take in an integer and give you the corresponding string that can be passed to s_engineStrings. The UFNE object in the rgUfne array will also be updated to hold all the possible string keys for a function, instead of indexing them on other numbers that may overlap with existing definitions.

Now to add a new string for a new IDC_FOO function, we would just need to add the "FooString" resource keys to the g_sids array and use the updated rgUfne map to link the IDC_FOO value to the corresponding "FooString" resource key. This way the resource key can be a meaningful string, and not an integer that must be in any particular order.

How changes were validated:

Tested each function manually in standard, scientific, and programmer modes.

src/CalcManager/CEngine/scicomm.cpp Outdated Show resolved Hide resolved
src/CalcManager/CEngine/scicomm.cpp Outdated Show resolved Hide resolved
src/CalcManager/Header Files/EngineStrings.h Outdated Show resolved Hide resolved
src/CalcManager/Header Files/EngineStrings.h Outdated Show resolved Hide resolved
src/CalcManager/CEngine/scicomm.cpp Outdated Show resolved Hide resolved
src/CalcManager/CEngine/scicomm.cpp Outdated Show resolved Hide resolved
src/CalcManager/CEngine/scicomm.cpp Show resolved Hide resolved
src/CalcManager/CEngine/calc.cpp Outdated Show resolved Hide resolved
src/CalcViewModel/UnitConverterViewModel.cpp Outdated Show resolved Hide resolved
Copy link

@danbelcher-MSFT danbelcher-MSFT left a comment

Choose a reason for hiding this comment

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

Thanks!

@danbelcher-MSFT danbelcher-MSFT merged commit f6f1044 into microsoft:master Apr 11, 2019
joseartrivera added a commit to joseartrivera/calculator-1 that referenced this pull request Apr 11, 2019
Simplify some of the calc engine string logic (microsoft#449)
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