Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A HasAttribute<T> function in DomNodeAdapter.cs #15

Closed
JelleVM opened this issue Aug 28, 2014 · 5 comments
Closed

A HasAttribute<T> function in DomNodeAdapter.cs #15

JelleVM opened this issue Aug 28, 2014 · 5 comments

Comments

@JelleVM
Copy link

JelleVM commented Aug 28, 2014

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
Copy link
Member

Ron2 commented Aug 28, 2014

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

@JelleVM
Copy link
Author

JelleVM commented Sep 1, 2014

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
Copy link
Member

Ron2 commented Sep 2, 2014

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

@JelleVM
Copy link
Author

JelleVM commented Sep 2, 2014

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
Copy link
Member

Ron2 commented Sep 13, 2014

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;
}

@Ron2 Ron2 closed this as completed in 13bc703 Sep 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants