Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Area of a triangle #399

Merged
merged 6 commits into from
Feb 4, 2023
Merged

Area of a triangle #399

merged 6 commits into from
Feb 4, 2023

Conversation

sonomirco
Copy link
Collaborator

What type of PR is this? (check all applicable)

  • ♻️ Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 👷 Optimization
  • 📝 Documentation Update
  • 🔖 Release
  • 🚩 Other

Description

This PR added the faction to calculate the area of a triangle.

Related Tickets & Documents

This PR closes #398

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help

Added to documentation?

  • 📓 docs
  • 🙅 no documentation needed

@sonomirco sonomirco added enhancement 📢 Issues describing an enhancement or pull requests adding an enhancement. minor Pull requests requiring a minor version update according to semantic versioning. labels Jan 5, 2023
@sonomirco sonomirco self-assigned this Jan 5, 2023
/// <returns>The area of the triangle.</returns>
public static double AreaOfTriangle(PolyLine polyLine)
{
var pts = polyLine.ControlPointLocations;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we do some check on the number of vertices for the polyline?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I assumed that if you calculate a triangle's area, you are passing a Polyline composed of 4 points.
We may not need this overload method.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think is still valid to keep it, as long as we introduce a check on the number of control points, and that first and last point are coincident (or polyline is closed)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This method should be more specific given the name. It should take 3 points as parameters only. Using the vector based method for the area it shouldn't require any collinearity or coplanarity checks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll remove this overload for now.

@sonomirco
Copy link
Collaborator Author

@d3ssy @cesarecaoduro ready to be merged.

@sonomirco sonomirco merged commit 121b6ac into master Feb 4, 2023
@sonomirco sonomirco deleted the mibi/dev/area-of-triangle branch February 4, 2023 02:21
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement 📢 Issues describing an enhancement or pull requests adding an enhancement. minor Pull requests requiring a minor version update according to semantic versioning.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Area of triangle
3 participants