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

(fix) Reworked logic for gravida calculation #337

Merged
merged 3 commits into from
Jul 10, 2024

Conversation

arodidev
Copy link
Contributor

@arodidev arodidev commented Jul 2, 2024

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

This PR corrects the gravida calculation logic within the form engine.

Screenshots

Related Issue

Other

@arodidev arodidev requested review from CynthiaKamau and samuelmale and removed request for CynthiaKamau July 2, 2024 11:20
parityAbortion + parityTerm + 1;
}
return gravida;
return parseInt(parityTerm) + parseInt(parityAbortion);
Copy link
Member

Choose a reason for hiding this comment

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

So in other words the Gravida is the summation of the parityTerm with parityAbortion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Affirmative.

Copy link
Member

Choose a reason for hiding this comment

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

Per our conversation from Coffee Break call, you might need to consider the current pregnancy in your calculation @arodidev.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@denniskigen I did a bit of consultation, and it seems the convention should be to do a summation of the two.

@arodidev arodidev marked this pull request as draft July 3, 2024 04:31
@arodidev arodidev marked this pull request as ready for review July 3, 2024 04:41
Copy link
Member

@denniskigen denniskigen left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. I've left a suggestion related to using a different function for the number validation and a JSDoc comment string.

@@ -260,17 +260,14 @@ export class CommonExpressionHelpers {
};

calcGravida = (parityTerm, parityAbortion) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
calcGravida = (parityTerm, parityAbortion) => {
/**
* Calculates the gravida (total number of pregnancies) based on term pregnancies and abortions/miscarriages.
*
* @param {number|string} parityTerm - The number of term pregnancies.
* @param {number|string} parityAbortion - The number of abortions (including miscarriages).
* @returns {number} The total number of pregnancies (gravida).
* @throws {Error} If either input is not a valid number.
*
* @example
* const gravida = calcGravida(2, 1);
* console.log(gravida); // Output: 3
*/
calcGravida = (parityTerm, parityAbortion) => {

const term = parseInt(parityTerm, 10);
const abortion = parseInt(parityAbortion, 10);

if (isNaN(term) || isNaN(abortion)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (isNaN(term) || isNaN(abortion)) {
if (!Number.isInteger(term) || !Number.isInteger(abortion))

Perhaps Number.isInteger is a better check here since we're specifically dealing with whole numbers. Unlike isNaN, which allows any valid number (including decimals), Number.isInteger will reject decimals. Additionally, isNaN attempts to coerce the input into a number before checking, which can lead to unintended behavior, while Number.isInteger will return false for any non-number types. If we decide to consider decimal inputs in the future, reverting to isNaN would be more appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @denniskigen, this has been resolved.

@denniskigen denniskigen changed the title (fix): Reworked logic for gravida calculation (fix) Reworked logic for gravida calculation Jul 3, 2024
Copy link
Member

@samuelmale samuelmale left a comment

Choose a reason for hiding this comment

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

I'm naive when it comes to the business aspects but the code LGTM

@arodidev arodidev merged commit 61a2829 into openmrs:main Jul 10, 2024
4 checks passed
Art-Ndiema pushed a commit to Art-Ndiema/openmrs-form-engine-lib that referenced this pull request Aug 8, 2024
* redo gravida logic

* handle non integer values

* Use Number.isInteger() over isNaN() plus JSDoc comments
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.

4 participants