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

Remove multiply and divide methods accepting double from ComplexNumbe… #2852

Merged
merged 3 commits into from
Nov 2, 2024

Conversation

jagdish-15
Copy link
Contributor

pull request

Description

This pull request removes the multiply and divide methods that accept a double parameter from the ComplexNumber class in both the reference solution and the main.java file. These methods were deemed unnecessary since the primary focus of the exercise is to operate on complex numbers, and the existing tests do not validate these double overloads.

Changes Made:

  • Removed multiply(double factor) and divide(double divisor) methods from the ComplexNumber class.
  • Updated the reference solution accordingly to ensure consistency.

Rationale

The double overloads for multiplication and division were not accompanied by corresponding test cases in the canonical data, which could lead to situations where these methods are not adequately tested. By removing these methods, we streamline the class and enhance clarity, ensuring that the exercise focuses on complex number operations as intended.

Next Steps

Please review the changes and let me know if further modifications are needed.

Reviewer Resources:

Track Policies

Copy link
Member

@kahgoh kahgoh left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the compile error @jagdish-15! However, there is a Checkstyle error.

When fixing the issues from this review, feel free to put them in a commit on top of your current one and push to Github when ready. This should save you from having to close and opening PRs.

}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you make check that there is an empty line at the end? Without it, Checkstyle fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll do it right now!

@jagdish-15
Copy link
Contributor Author

"This commit passes all tests successfully. I appreciate any suggestions for further improvement!

Copy link
Member

@kahgoh kahgoh left a comment

Choose a reason for hiding this comment

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

Thanks! It looks good to me!

@kahgoh kahgoh merged commit f37d6a8 into exercism:main Nov 2, 2024
4 checks passed
@jagdish-15
Copy link
Contributor Author

Hi Kahgoh,

I realized that I submitted my GitHub handle as @jagdish-15 instead of jagdish-15 on Exercism, not knowing that the @ symbol shouldn't be included as this was my first PR ever, not just for Exercism but in general. I changed it to the correct handle after the PR was merged, but I didn’t receive the reputation points for my recent contributions. Could you please assist me in resolving this? Thank you for your help!

Best Regards,
Jagdish

@kahgoh
Copy link
Member

kahgoh commented Nov 2, 2024

I thought Exercism would pick it up after fixing your GitHub handle. You can check if it has by seeing if the PR appears on your contributions. If it hasn't, try removing the GitHub handle from Exercism and then putting it back again. If that hasn't worked, you might need to open up another thread on the forums.

@jagdish-15
Copy link
Contributor Author

I tried everything, but it still didn’t work. The contribution isn’t showing up in the contributions section. I even tried removing and re-adding my GitHub handle, and temporarily changed my Exercism handle to match GitHub’s, but no luck. I'll open another thread on forums then.

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