SonyWWS/ATF

A HasAttribute<T> function in DomNodeAdapter.cs

Closed this issue · 5 comments

I have a Dom Node that has optional attributes, like:

<xs:attribute name="someAttribute" type="xs:float" use="optional" />

I would like to be able to check if my node has this attribute at some given time. However, the GetAttribute<T> function returns default(T) when the attribute doesn't exist. Personally I'd just return null, but I guess that's a design decision. In any case, I think a HasAttribute<T> function would be a worthwhile addition to the framework.

I just implemented it as follows in my classes inheriting it:

protected bool HasAttribute<T>(AttributeInfo attributeInfo)
{
    object value = DomNode.GetAttribute(attributeInfo);

    // If value is not null, attempt the cast; an invalid type will then cause
    // an IllegalCastException.
    if (value != null && (T) value != null)
        return true;
     else
        return false;
}
Ron2 commented

Hello, JelleVM,

Good question. You're talking about the DomNodeAdapter's GetAttribute<T>. If you go to the DomNodeAdapter's DomNode, then you can call the DomNode's IsAttributeDefault(AttributeInfo) which I think will do what you want.

        /// <summary>
        /// Gets whether or not the attribute's value (like by calling GetAttribute) is equal to
        /// the default value.</summary>
        /// <param name="attributeInfo">Attribute metadata</param>
        /// <returns>True if the attribute's value is equal to the default value</returns>
        public bool IsAttributeDefault(AttributeInfo attributeInfo)

So, your code would change from:

if (myDomNodeAdapter.HasAttribute<float>(attributeInfo))

to:

if (myDomNodeAdapter.DomNode.IsAttributeDefault(attributeInfo))

Right?

We could add another convenience method to DomNodeAdapter, to save the client from having to call (and discover) the DomNode.IsAttributeDefault() method. I don't think I would call this convenience method "HasAttribute" though, because conceptually, a DomNode "has" all the attributes that are defined for it.

--Ron

Hi Ron, thanks for your reply. Thanks for pointing me to the IsAttributeDefault method, I hadn't thought of that. However, I think it doesn't fully solve the problem. (Though I understand what you're saying that "conceptually, a DomNode 'has' all the attributes that are defined.").

What would you do in the following scenario: your DomNode has an optional variable, say of type int. If this variable is not there, you want your programme to compute it. If it is there already, you don't do anything, to avoid unnecessary computation. This would be expressed by the following code:

int m_someVariable; if (HasAttribute(assets.SomeType.myOptionalTypeAttribute)) m_someVariable = GetAttribute(assets.SomeType.myOptionalTypeAttribute); else m_someVariable = ComputeValueOfOptionalAttribute(); // heavy computation

Now the int value of the attribute could be 0. So if we'd use DomNode.IsAttributeDefault() instead and the variable value happens to be equal to the default value (i.e. 0), we will be doing heave extra work for nothing, I think. Am I right?

Could maybe DomNode use a new method:

public bool HasAttribute(AttributeInfo attributeInfo) /\* or "IsAttributeNull" or "HasOptionalAttribute" */ { return GetLocalAttribute(attributeInfo) == null; }
Ron2 commented

Hi JelleVM,

I see the problem; thank you for carefully explaining it. I think you're right, that this would be a useful method. Client code can call GetLocalAttribute() and check the result, but that's not very readable.

I need to think about the name for a bit, write unit tests, and ask coworkers before I check this in. (I'm in Japan for a conference this week, announcing that LevelEditor is open source!) The problem I see with HasAttribute(AttributeInfo) is that it might imply that the method is checking if the AttributeInfo has been defined for that DomNode's DomNodeType. How about AttributeWasSet()? Or AttributeHasBeenSet()?

/// <summary>
/// Gets whether or not the attribute has ever been set, even if it was set to its
/// default value.</summary>
/// <param name="attributeInfo">Attribute metadata</param>
/// <returns>True if the attribute has even been set and false otherwise.</returns>
public bool AttributeWasSet(AttributeInfo attributeInfo)
{
    return GetLocalAttribute(attributeInfo) != null;
}

--Ron

Hi Ron,

Thanks for considering it! It's indeed quite an uncommon user case, but that's why I brought it up.
The name for the function is entirely up to you and your team of course, but I agree that your suggestions are probably clearer for a generic user case.
Have fun in Japan -- more open source ATF stuff, yeay! :-)

Ron2 commented

I'll check this new method on DomNode shortly. Thanks again for the good suggestion!

/// <summary>
/// Gets whether or not the attribute has been set, even if it was set to its default value.</summary>
/// <param name="attributeInfo">Attribute metadata</param>
/// <returns>True iff the attribute has been set to anything other than null</returns>
/// <remarks>Setting an attribute to null is special, and makes the attribute look like it
/// was never set.</remarks>
public bool IsAttributeSet(AttributeInfo attributeInfo)
{
    return GetLocalAttribute(attributeInfo) != null;
}