Room.HotspotCount setter silently adding items to Room.Hotspots. about ags HOT 12 CLOSED

I mentioned this on forums before, the HotspotCount etc setters were made as a temporary solution (for the alpha version) to give user a way to set number of room regions.
I do consider changing this as soon as we have a proper UI in the editor to add/remove regions.

Regardless, I am not sure I understand what do you think is a bug?

 if you change HotspotCount then it's your responsibility to ensure that the list is updated also (by calling the method that does this, or adding them manually).

But that is what the setter should be doing now.
Can you show how do you use this property?

from ags.

I meant that it's the responsibility of the person changing the count to update the list, not the responsibility of the setter. I know these are a temporary solution, but I consider this a major flaw in the interface as it's totally unclear what's happening.

Reading a CRM file into the editor code I tried reading the area count into the Room.*Count property and then create the items in the associated list in a loop: for (int i = 0; i < Room.HotspotCount; ++i) Room.Hotspots.Add(new RoomHotspot(room)); Doing this creates an infinite loop, though it's not clear (until you realize that the *Count properties are just syntactic sugar for the underlying IList.Count which is already accessible properly).

from ags.

I have to disagree, and I am trying to find proper words here to explain this, because, I think, you are missing an important point about 'property' concept.

I may be saying a banal thing, but it is pretty natural for several properties to change same internal object. It would be a mistake to examine and use properties as variables. Properties are closer to functions.
It would be right thing to expect, that by writing a property you are changing whole state of an object so that it stays consistent. Likewise, by reading a property you should expect it to reflect current consistent state of an object.
This is what properties were introduced for at first place.
At this properties are different from variables, by changing which you change only single value, which can easily bring data to inconsistency: like having "item count" value = 10, but having zero real items in array.

Your statement "that it's the responsibility of the person changing the count to update the list" would make sense if referred to the variable, but it does not really make sense when referred to property.
As I said, the properties may be thought as being functions, and your code:

for (int i = 0; i < Room.HotspotCount; ++i) Room.Hotspots.Add(new RoomHotspot(room))
May be compared to this:
for (int i = 0; i < Room.GetHotspotCount(); ++i) Room.Hotspots.Add(new RoomHotspot(room))

Again: it should not be unclear to you that a property returns updated state of an object, you should EXPECT that, it is a natural thing. If it didn't, it would fail as a property.

I am guessing that you wrote that code by copying the one from C++ implementation, but it is not always right to do such a straight copy. It might have been right only if you would not use the property, but created a new (public?) variable instead (because in C++ code HotspotCount is a variable).
However, in that concrete example you do not need such class member at all, you need only a local variable to read the data from stream, and then use it to populate the list.

EDIT: Or, if all you do inside the loop is creating an object, you could just set Room.HotspotCount once and let the room class do the rest.

from ags.

I can see that we're kind of butting heads on this, but I still don't disagree. I understand how properties work. And the idea of validating the value that's being set makes sense. But what you've basically done is provided a property to set the List(T).Count property because you assumed that it was wrong in not having an accessible setter. I don't think that devising a way to counteract the .NET Framework is an ideal implementation for an interface. At the very minimum you should have documented the behavior, but I still disagree with this.

from ags.

The MSDN states that, "Properties should be stateless with respect to other properties." As the HotspotCount property is directly affecting the state of the Hotspots property, the two are not upholding this contract. It further adds, "In general, methods represent actions and properties represent data." Adding and removing items from a list is definitely an action. I've just read the MSDN's Property Usage Guidelines and I, for one, feel validated. You may want to at least look them over.

from ags.

One more related thing. SetHotspotCount is checking if the value is >= 256 in order to throw an error message that the value must be <= 256. Since the value is in the inclusive range starting from 1, 256 is a valid value.

from ags.

Monkey... let's make a thought experiment and imagine a situation where the HotspotCount does not change the real list's size and just sets some extra variable - just as you wanted to expect from this property. Now, what we have there - is two public properties (HotspotCount and Hotspots) that allow to set an object in a conflicting state: user can set HotspotCount to positive value and not add anything to Hotspots. Or, vice versa, one can add new items to the list and not set correct value to HotspotCount. Does that make any sense? How one can even expect something like this?

Now, let's take your code as an example again. If I understood that correctly, you were accesing room object's properties from external method, i.e. you were using the object via its interface. And you were expecting, that to put room object into correct state you should 1) set number of items by HotspotCount 2) add real items, thus actually providing "number of items".
I would immediately say that such expectations go against concept of incapsulation.

I absolutely agree that the HotspotCount property is duplicate and redundant for the Room's interface in the presence of Hotspots property. This is a hack into interface design. That's true, and that should be changed eventually. The sole reason it is there - is to allow editor's Property Grid to have an editable field that would change number of items.

What I disagree and am arguing about - is your implications about what effects that property may have, I found them conceptually wrong (as I explained above). Since I added that property I wanted to make sure that its use won't let the user of this interface to put the object into inconsistent state, which would set different information on same issue (item count) depending on what property is used.

from ags.

Regarding the "Properties should be stateless with respect to other properties.". To tell truth, I am not sure it means precisely what you say (sorry, my english is far from perfect). I rather believe it is connected to the previous sentence: "Allow properties to be set in any order"; in which case this simply means that related properties should not require certain object's "intermediate" state (between changes of other properties) to be read or set, neither put an object into such "intermediate" state (it may be either "all on" or "all off", not "partially on").

For example, check the System.Windows.Rect type. You will see whole set of properties that change each other's "state".

from ags.

one can add new items to the list and not set correct value to HotspotCount. Does that make any sense? How one can even expect something like this?

You've done nothing to prevent this. There is absolutely no restriction on Room.Hotspots (exposed by the IList interface, the actual underlying type is List(T)) that would prevent or exclude me from adding 9000 items without ever seeing your method. Your implementation of HotspotCount does not keep the list in a consistent state, and the setter method for an int property adding items to a list is obscene.

I would immediately say that such expectations go against concept of incapsulation.

Encapsulation isn't always about keeping the entire structure in a consistent state (nor is that what you've accomplished). The idea of encapsulation is keeping properties themselves in a valid state. That is, your restriction on the value of HotspotCount is valid. HotspotCount modifying another property is not a valid encapsulation technique. If you want to have that level of control, it should be via a container type that explicitly declares that aspect as a part of its interface. Doing this via two separate properties is not correct.

The sole reason it is there - is to allow editor's Property Grid to have an editable field that would change number of items.

Don't get me wrong, I get this, but I'm just saying that you've gone about it the wrong way. Perhaps instead of doing this via a property you should let the method that is invoked when the text field in the editor is changed handle keeping the list in a consistent state. This would be a valid usage of the property, the methods, and encapsulation.

Since I added that property I wanted to make sure that its use won't let the user of this interface to put the object into inconsistent state, which would set different information on same issue (item count) depending on what property is used.

But again, your control in the int property's setter does nothing to deter putting the object into an inconsistent state via the list itself (by adding more than the allowed number of items).

"Properties should be stateless with respect to other properties."..this simply means that related properties should not require certain object's "intermediate" state.

If I call HotspotCount's setter, the way it's implemented now, then it is "flagging" Hotspots as being in an invalid state. This is not a valid usage of properties. HotspotCount should not be changing the state of other properties. That should be handled by a method. As a matter of fact, it's already handled by a method, you're just incorrectly calling that method from an int property's setter. Again, this isn't the way properties are supposed to work.

For example, check the System.Windows.Rect type. You will see whole set of properties that change each other's "state".

For the sake of being open minded, I'm willing to look and see what properties you're referencing. If you can tell me a single one of them. Because I looked through the entire class definition (source) and none of System.Windows.Rect's properties are being used this way. The properties validate that the values being set are valid for the object, but none of them change the state of other properties to keep the object in a consistent state. That is handled by the appropriate methods where necessary.

I'm really not trying to be inflammatory here, and I understand that this was just a temporary solution. But as it stands it's an exposed part of the editor's interface and it's implemented in a way that violates best practices. All I'm saying that needs to be done is to move the method call out of the property setter and into the proper location. Unless you actually are worried about always keeping the object in a consistent state, in which case you need to devise your own collection type for this purpose, because List(T) is insufficient for that (given that you have no access to the list's internal methods and setters).

from ags.

Alright, I finally see that your main criticism is that HotspotCount property introduces the constraint that Hotspot does not. (Is it?)
That's true, and that's not good. In fact, you are making me want redo this now.

Yet, in such case, was this Issue's original statement referring to exactly this problem?

Still I would like to add few comments, because I suspect we have different understanding of the meaning of certain expressions.

one can add new items to the list and not set correct value to HotspotCount. Does that make any sense? How > one can even expect something like this?

You've done nothing to prevent this. There is absolutely no restriction on Room.Hotspots (exposed by the IList interface, the actual underlying type is List(T)) that would prevent or exclude me from adding 9000 items without ever seeing your method. Your implementation of HotspotCount does not keep the list in a consistent state, and the setter method for an int property adding items to a list is obscene.

By saying "correct value" in this case I did not mean the value allowed by the editor (1-256). I meant the value that corresponds to actual number of items. You may add 9000 items to the list, but the HotspotCount will always return THAT value without ever being set on its own. You may set any value to HotspotCount, and the list will always have the number of items that HotspotCount allowed to set.

If you want to have that level of control, it should be via a container type that explicitly declares that aspect as a part of its interface. Doing this via two separate properties is not correct.
Perhaps instead of doing this via a property you should let the method that is invoked when the text field in the editor is changed handle keeping the list in a consistent state.

I think you don't know how Property Grid is working. It uses class reflection to get or set its properties. It scans the object passed to find properties that has "Browsable" attribute. Unless I missed some opportunity, it does not let me to bind methods to its fields.
I agree that the better thing would be to reimplement the container and expose it to the Property Grid, which would also required to create a new property editor class with UI to feed the list, like it is done for ListView columns, for example. But I am still not sure it is the way the editor should handle regions, that's why I did not go into that direction.

But again, your control in the int property's setter does nothing to deter putting the object into an inconsistent state via the list itself (by adding more than the allowed number of items).

By saying "inconsistent state" I mean the state of an object when two properties return different information on same issue (number of items). You could say that using Hotspots property to add 9000 items may put the Room into invalid state, which might be true; but then again, it was always like that, and not changed by introduction of HotspotCount.

Finally, the Rect example was given as I misunderstood what "state" you are talking about. If you mean "valid"/"invalid" state, then it does not do such things.
I was referring the simple case when changing one property makes other reflect that change from its own perspective.
For example, writing the Size property will make Width and Height properties reflect that change.

from ags.

You could say that using Hotspots property to add 9000 items may put the Room into invalid state, which might be true; but then again, it was always like that, and not changed by introduction of HotspotCount.

And I'm not blaming or criticizing you for that, but I will continue to maintain that the setter for HotspotCount modifying Hotspots is an incorrect usage of the properties.

For example, writing the Size property will make Width and Height properties reflect that change.

This is true, but this is a case where the Size property is of type System.Drawing.Size. The very existence of that type is for the purpose of setting the width and height of an object. Having separate Width and Height properties that are updated when the Size is changed is to be expected because that's what System.Drawing.Size (as a type) is for.

Having an int property setter change the size of a List(T) in this fashion is neither obvious, nor is it good programming. Unless it's an explicit part of the interface (e.g., for a specialized collection type with a settable Count member that grows the internal cache; for the System.Drawing.Size type it is an explicit part of its definition as a type that it sets the Width and Height at once), side effects like this should be avoided.

Bill Wagner also wrote about Properties vs. Methods

They should not have dependencies on each other. Note that this would include setting one property and having it affect another. (For example, setting the FirstName property would affect a read-only FullName property that composed the first name + last name properties implies such a dependency )

The behavior you've implemented introduces this type of dependency that Wagner says should (generally) be avoided. The reason why such dependencies are advised against is because they're not always clear or expected. If you devised a specialized collection container type, then this could be a stated feature/aspect/side effect of usage. Throwing an int property (even temporarily) into the Room class that mutates other properties like this is not clear, or expected.

from ags.

This issue is no longer relevant to the current development.

from ags.