Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Are "One Shot" Property Getters considered an acceptable coding standard in .NET (or in general) [closed]

Recently while doing some work with C# & ActiveMQ (via the Apache.NMS libraryies) I came across the following property on the ActiveMQBytesMessage

public new byte[] Content
{
  get
  {
    byte[] buffer = (byte[]) null;
    this.InitializeReading();
    if (this.length != 0)
    {
      buffer = new byte[this.length];
      this.dataIn.Read(buffer, 0, buffer.Length);
    }
    return buffer;
  }
  ..(setter omitted)
}

The InitialiseReading method handled the connection & streaming of the data from active MQ into the .dataIn field. The problem though was that IT DID THIS EVERYTIME. And once that data was read, it could never be read again and the dataIn field was zero'd and reset. So simply by observing the property and observing it again, you lost the data. This made for some very strange bugs such as:

byte [] myBytes = new byte[msg.Content.Length]; 
//Touched the property. Data read in.

msg.Content.CopyTo(myBytes,0); 
//Uh oh! touched it again, copying a zero'd array.

or when you were debugging and you stuck a watch variable on the property or accidentally hovered your mouse over the property.

Is this kind of mechanism an accepted or prevalent way of using properties for streamed data.

like image 259
Eoin Campbell Avatar asked Aug 09 '12 10:08

Eoin Campbell


2 Answers

Very very poor code.

The general wisdom is that properties should not affect the inner state of an object. If you call set, then get you should always get back the value you've just set. If you call get twicwe, you should get the same result twice.

This should have been a method called GetContent at very least, but id still expect to be able to call that method repeatedly and get the same result.

like image 93
Jamiec Avatar answered Oct 20 '22 12:10

Jamiec


This is very poor code. Properties should contain very little logic, and by no means cause side effects.

This property would be better of as a method named getNextContent.

like image 39
Vitaliy Avatar answered Oct 20 '22 14:10

Vitaliy