1
private readonly float[] _location;

public Player()
{
    _location = new float[3];
}

public float[] Location
{
    get
    {
        _location[0] = Worker.Elements["lbXCoordinate"].Value;
        _location[1]= Worker.Elements["lbYCoordinate"].Value;
        _location[2] = Worker.Elements["lbZCoordinate"].Value;

        return _location;
    }
}

Two questions:

  1. I often find myself setting the value of a Property in the get Accessor (as seen above), as opposed to using a set Accessor. This way I ensure that whenever someone asks for the value, they get an updated version. Is that generally OK to do?

  2. I realize that returning an Array in a Property is not good practice (link), but I am concerned that if I return a ReadOnlyCollection, then I have to make a new instance of both that and a float every time someone asks for an updated value (see below). It will be asked for several times per second in my program, so that seems to be unwise with regards to performance. Is it okay to just ignore this rule in my case, as the value is actually set every time a user asks for it (so we won't have an issue in the case someone happens to overwrite the array)?

Using ReadOnlyCollection:

private ReadOnlyCollection<float> _location;

public Player()
{
}

public ReadOnlyCollection<float> Location
{
    get
    {
        _location = new ReadOnlyCollection<float>(new float[] {Worker.Elements["lbXCoordinate"].Value, Worker.Elements["lbXCoordinate"].Value, Worker.Elements["lbXCoordinate"].Value});

        return _location;
    }
}
Magnus
  • 6,791
  • 8
  • 53
  • 84

1 Answers1

1

You can return a IReadOnlyCollection<float> instead, because the array implements this interface.

This is valid code:

private  _location[] = new float[3];

public IReadOnlyCollection<float> Location
{
    get
    {
        return _location;
    }
}

Regarding the setting of the value in the getter, this seems ok to me, as the overhead is likely minimal.

You just need to ensure some synchronization in case of different threads accessing Worker.Elements.

To ensure consistent values returned, you can sync on Worker.Elements whenever you try to read or write this variable:

get
{
   lock(Worker.Elements)
   {
      _location[0] = Worker.Elements["lbXCoordinate"].Value;
      _location[1]= Worker.Elements["lbYCoordinate"].Value;
      _location[2] = Worker.Elements["lbZCoordinate"].Value;

      return _location;
   }       
}
thumbmunkeys
  • 20,606
  • 8
  • 62
  • 110
  • `ReadOnlyCollection` is not an interface. it is a class. – Sriram Sakthivel Aug 26 '14 at 13:08
  • Thanks! I do have different threads accessing it. What kind of synchronization would that be? Is it enough if I make _location volatile? I don't really care if different threads end up having different values for location, as the idea is just that whoever asks should have the most recent value. Apologies if I am missing a key point on synching. – Magnus Aug 26 '14 at 13:36
  • Thank you! I will go and read up on thread synching. Just briefly, why do I need to synch if I do not care that different threads get different values? I mean, after all they are supposed to do that. When the character moves, his location changes, so a thread asking a few sec after another will get a diff value. – Magnus Aug 26 '14 at 13:55
  • 1
    I am not sure if this is a problem for your app, but consider this: the writing thread has written X and Y. Then it gets interrupted by a reading thread, that reads X,Y,Z. In that case Z is an outdated value. My guess is that it's not a problem, as you update (write) very often. – thumbmunkeys Aug 26 '14 at 13:58
  • 1
    Ah got it. This particular code seems safe without lock though, right? As if someone calls instance.Location, then it writes directly before it reads (as seen in code above). So if it gets interrupted, then the read simply never completes. Therefore it either gets all X, Y, Z coordinates, in the IReadOnlyCollection, or it gets nothing at all (if it is interrupted). Right? – Magnus Aug 26 '14 at 14:23
  • locking doesn't cause the read to not complete in case of a lock from the writer, it means that the reader will be blocked until the writer has finished (and vice versa). So I would go with the lock in any case. If you don't have locking, you might get that slight inconsistency in values, that I described earlier. – thumbmunkeys Aug 26 '14 at 14:26
  • Understood, thank you. Last question: Would it not be necessary to return Array.AsReadOnly(_location)? Or does the return type of the method somehow convert the array to IReadOnlyCollection? PS, I could not find in the MSDN documentation that Array Class implements IReadOnlyCollection: http://msdn.microsoft.com/en-us/library/system.array(v=vs.110).aspx – Magnus Aug 26 '14 at 16:29
  • it's fine to return `IReadOnlyCollection` as the c# arrays seem to implement that interface, see here http://stackoverflow.com/a/18042379/451540. It has no runtime overhead if you do that. – thumbmunkeys Aug 26 '14 at 16:39
  • Thumbmunkeys, As the IReadOnlyCollection is still referencing the original Array (just up-casted), I realized that the caller can just cast it back to Array and subsequently modify it. So we have not really solved the issue, right? – Magnus Aug 26 '14 at 18:58
  • If you make an API that is used by a lot of people that might be a consideration, otherwise I don't really see the point. – thumbmunkeys Aug 26 '14 at 19:57