Two circles intersect

Question

Do two circles in a 2D plane disjoint?

$$(r_1 + r_2) \cdot (r_1 + r_2) \gt \Delta{X} \cdot \Delta{X} + \Delta{Y} \cdot \Delta{Y}$$

Any comments / corrections on this code?

internal struct HoleInfo
{
 public float Diameter { get; internal set; }
 public float X { get; internal set; }
 public float Y { get; internal set; }
 public HoleInfo(float x, float y, float diameter)
 {
 X = x;
 Y = y;
 Diameter = diameter;
 }
}
static bool HoleOverlap(HoleInfo hole1, HoleInfo hole2)
{
 float holeSize = (hole1.Diameter + hole2.Diameter) / 2;
 float deltaX = hole1.X - hole2.X;
 float deltaY = hole1.Y - hole2.Y;
 return (holeSize * holeSize > deltaX * deltaX + deltaY * deltaY);
}

It is called HoleInfo because the app is concerned if holes drilled in a board will overlap.

• Maxim: Using internal set in properties of the HoleInfo do you suppose these properties can be changed outside of this type in other way than via constructor? It is not good to have mutable struct. Also there is no need to use parentheses in return statement.
• t3chb0t: is the math correct - Code Review is not about validating the math. And yes, this time the downvote comes from me.

Answer 1

You're breaking some rules for good struct design - see them here.

X DO NOT define mutable value types.

✓ DO implement IEquatable on value types.

I'm going to disagree with everyone else and say that calling your struct a Hole is fine as it's a domain specific concept.

I think your method naturally belongs on the type itself. I'd call it Intersects to avoid the disjoint/overlap debate.

// using static System.Math; // Easy access to Pow.
public struct Hole : IEquatable<Hole>
{
 public float Radius => Diameter / 2;
 public float Diameter { get; }
 public float X { get; }
 public float Y { get; }
 public Hole(float x, float y, float diameter)
 {
 X = x;
 Y = y;
 Diameter = diameter;
 }
 public bool Intersects(Hole other)
 {
 var sumR = Radius + other.Radius;
 var deltaX = X - other.X;
 var deltaY = Y - other.Y;
 return Pow(sumR, 2) > Pow(deltaX, 2) + Pow(deltaY, 2);
 }
 public bool Equals(Hole other)
 {
 return Diameter.Equals(other.Diameter) && X.Equals(other.X) && Y.Equals(other.Y);
 }
 public override bool Equals(object obj)
 {
 if (ReferenceEquals(null, obj)) return false;
 return obj is Hole && Equals((Hole)obj);
 }
 public override int GetHashCode()
 {
 unchecked
 {
 var hashCode = Diameter.GetHashCode();
 hashCode = (hashCode * 397) ^ X.GetHashCode();
 hashCode = (hashCode * 397) ^ Y.GetHashCode();
 return hashCode;
 }
 }
 public static bool operator ==(Hole left, Hole right)
 {
 return left.Equals(right);
 }
 public static bool operator !=(Hole left, Hole right)
 {
 return !left.Equals(right);
 }
}

• VisualMelon: Was just writing up an answer about the mutability... However, I would suggest making at an immutable (non-comparable) class. Encouraging the comparison of floats can end badly, and there is no given reason for it to be a struct in the first place. It is also not apparent that holes are conceptually comparable (decision the OP needs to make, explicit implementation as shown is good if so). Agree with the API naming suggestions (especially third-person singular on Intersects). Personally I wouldn't use Pow: it just feels heavy-handed, and I the explicit product is recognisable.
• RobH: @VisualMelon - all valid points. Struct vs class argument here could go either way.
• paparazzo: Multiply is a little faster than Pow here
• paparazzo: Why if (ReferenceEquals(null, obj)) return false; rather than obj == null
• VisualMelon: @d347hm4n it would be dividing 0 by 2, not 2 by 0.

Answer 2

Yes you got the math wrong. Your code computes whether the circles are disjoint. For a complete overlap (once circle containing another), it is \$(r_1 - r_2)^2 > \Delta_x^2 + \Delta_y^2\$. So you should

 float holeSize = (hole1.Diameter - hole2.Diameter)/2;

---

HoleSize is not a good name: the value it contains does not refer to any Hole. In line with your other names better call it deltaR.

• paparazzo: I am not looking for once circle containing another. Agree on naming.
• vnp: @Paparazzi then you shouldn't call it overlap. Call it disjoint perhaps.
• paparazzo: Maybe it is a language thing but one circle in another would be contained to me. Overlap is touching or crossing. It is called hole because the app is for holes drilled in a board to overlap. Good feedback.

Answer 3

Since your struct already is internal, I feel like you shouldn't make the setters internal. Make them private or public (That's up to you). Why? Because this way, when people read your code, they won't be concerned if your properties should be modified or not. Reading a class, you know that a public setter means you can do whatever you want with the property and it shouldn't break anything. If it's private, I know I can't touch it. Being internal brings confusion IMO. Your class already is internal, so why bother.

HoleInfo is weird as @vnp said. Though, if it's related to your business's domain I guess it could make sense, but that struct looks like a Circle to me!

Some might disagree with me, but I think the HoleOverlap method should be in your class, not a static method beside.

You wonder if you did the maths right? Well, you should test it! Write some unit tests for that method. It won't cost you much, and you'll be sure that every cases work. Like, if your circles touch in one point (both are tangent) does your code work or should you use >= instead of > (I didn't actually test it ahah).

internal struct Circle
{
 public float Diameter { get; set; }
 public float X { get; set; }
 public float Y { get; set; }
 public HoleInfo(float x, float y, float diameter)
 {
 X = x;
 Y = y;
 Diameter = diameter;
 }
 public bool Intersects(Circle other)
 {
 float holeSize = (this.Diameter + other.Diameter) / 2;
 float deltaX = this.X - other.X;
 float deltaY = this.Y - other.Y;
 return (holeSize * holeSize > deltaX * deltaX + deltaY * deltaY);
 }
}