[Solved] What is risky about the following and how could you rewrite it to make it safer? [closed]

by


  1. First off, you haven’t set an explicit modifier on the class, so your are going to end up with whatever the default happens to be (if it’s in the namespace then internal or private if contained in a class). This can cause minor inconveniences at compile-time. A bigger issue might be if the default exposes the class beyond what was intended, in which case you may unwittingly allowing greater access to the class than what was expected. In any case, it’s difficult for beginner or moderate C# programmers to glance at this and know what to expect, which can be a maintainability issue.

  2. The biggest issue, though, is that all of your class fields are declared public. Typically you’d want to use properties to expose fields: to restrict read/write ability, to make the fields more discoverable via reflection, to perform validation, etc. Which leads to…

  3. You can see that the class is named “Line” and exposes both it’s points and it’s length. A caller can currently easily change any of those values, but the other values are not updated. Length should probably be calculated property (read-only) given that changing the length would have unpredictable results (which point would change?).

    So I would rewrite this class as:

    public class Line
{
    public Point P1 {get;set;}
    public Point P2 {get;set;}
    public double Length
    {
        get
        {
            return Math.Sqrt( 
                Math.Pow( P2.X - P1.X, 2 ) + 
                Math.Pow( P2.Y - P1.Y, 2 ) 
            );
        }
    }
}

  4. The mutability of this class can cause issues if there are multiple threads working together, or even in a simple case where two objects are referencing the same Line instance and one object might not aware of another’s changes (thus causing problems). If mutability is an issue, then all of the properties should be read-only:

    public class Line
{
    private Point _p1;
    public Point P1
    {
        get
        {
            return _p1;
        }
    }
    private Point _p2;
    public Point P2
    {
        get
        {
            return _p2;
        }
    }
    private double _length;
    public double Length
    {
        get
        {
            return _length;
        }
    }

    public Line(Point p1, Point p2)
    {
        _p1 = p1;
        _p2 = p2;
        _length = Math.Sqrt( 
                Math.Pow( _p2.X - _p1.X, 2 ) + 
                Math.Pow( _p2.Y - _p1.Y, 2 ) 
            );
    }

}

2

solved What is risky about the following and how could you rewrite it to make it safer? [closed]