5
\$\begingroup\$

Since reading Robert Martin's book "Clean Code" I have been inspired to revisit and refactor some of my code to break it down into succinct methods and small specialised classes.

Given the need to retrieve the NameAttribute off the Name2 property of the TestObject class below...

    public class TestObject
    {
        public string Name1 { get; set; }
        [Name("Address")]
        public string Name2 { get; set; }
    }

I have built a class called PropertyNameAttributeFinder to acheive this.

public class PropertyNameAttributeFinder
{
    #region Fields

    private readonly PropertyInfo _property;
    private NameAttribute _attributeFound;

    #endregion

    #region Construtors

    /// <summary>
    /// Initializes a new instance of the <see cref="PropertyNameAttributeFinder"/> class.
    /// </summary>
    /// <param name="property">The property.</param>
    /// <exception cref="System.ArgumentNullException">property</exception>
    public PropertyNameAttributeFinder(PropertyInfo property)
    {
        if (property == null) throw new ArgumentNullException("property");

        _property = property;
    }

    #endregion

    #region Public Members

    /// <summary>
    /// Checks for attribute.
    /// </summary>
    /// <returns>The current instance for fluid API</returns>
    public PropertyNameAttributeFinder CheckForAttribute()
    {
        SetAttributeIfExists();
        return this;
    }

    /// <summary>
    /// Gets a value indicating whether this instance has found an attribute.
    /// </summary>
    /// <value>
    /// <c>true</c> if this instance has found an attribute; otherwise, <c>false</c>.
    /// </value>
    public bool HasFoundAttribute
    {
        get { return _attributeFound != null; }
    }

    /// <summary>
    /// Gets the AttributeFound that was found.
    /// </summary>
    /// <value>
    /// The AttributeFound.
    /// </value>
    public NameAttribute AttributeFound
    {
        get
        {
            if (!HasFoundAttribute)
            {
                throw new InvalidOperationException("No attribute type was found so cannot be returned. Hint: Use HasFoundAttribute first.");
            }
            return _attributeFound;
        }
    }

    #endregion

    #region Private Members

    private void SetAttributeIfExists()
    {
        _attributeFound = _property.GetAttribute<NameAttribute>();
    }
    #endregion
}

The class will be used as follows...

    [TestMethod]
    public void AttributeFound_WhenCalledAfterCheckAttributeAndPropertyDoesHaveAtrribute_ReturnsInstanceOfAttribute()
    {
        // ARRANGE
        Type testType = typeof(TestObject);
        PropertyInfo property = testType.GetProperty("Name2");

        // ACT
        NameAttribute actual = new PropertyNameAttributeFinder(property)
            .CheckForAttribute()
            .AttributeFound;

        // ASSERT
        Assert.IsNotNull(actual);
    }

Can this class be written better to express it's intent in a clearer manner?

EDIT: Following request from @Heslacher, here is the code from the extension method called by _property.GetAttribute<NameAttribute>(); in SetAttributeIfExists.

    /// <summary>
    /// Get an attribute for a property
    /// </summary>
    /// <typeparam name="T"></typeparam>
    /// <param name="propertyInfo"></param>
    /// <returns></returns>
    public static T GetAttribute<T>(this PropertyInfo propertyInfo)
        where T : Attribute
    {
        var attribute = propertyInfo.GetCustomAttributes(typeof(T), false).FirstOrDefault();
        return (T)attribute;
    }

Please note: It is not the code in this extension which I want reviewed, but rather the PropertyNameAttributeFinder class and how calling it reads. I.E. doe the public members clearly reveal the intent of the class.

\$\endgroup\$
  • \$\begingroup\$ Is there a reason you're using attributes to name properties rather than the property names themselves? \$\endgroup\$ – Dan Lyons Dec 21 '15 at 18:49
  • \$\begingroup\$ @DanLyons - Yes it is part of a stored procedure framework I am working on where the the properties represent database fields and parameters. Most of the time the field/parameter name is taken from the property name and likewise the DbType is inferred from the property CLR type, but there are occasions when the field and parameter names and datatypes are "overridden" with attributes. \$\endgroup\$ – Dib Dec 21 '15 at 20:46
3
\$\begingroup\$

I think that Heslacher is right and you don't strictly need this class, but as you've written it I'd like to point out a couple of things:

  1. Many people, myself included, don't like to use regions. They just add noise - especially on such a small class
  2. SetAttributeIfExists is one step too far IMO. The method name isn't accurate either, you're setting the value if it exists or not.
  3. The exception message is really odd if you specify a property without the attribute:
[TestMethod]
public void AttributeFound_WhenCalledAfterCheckAttributeAndPropertyDoesntHaveAtrribute_ReturnsNull()
{
    // ARRANGE
    Type testType = typeof(TestObject);
    PropertyInfo property = testType.GetProperty("Name1");

    // ACT
    NameAttribute actual = new PropertyNameAttributeFinder(property)
        .CheckForAttribute()
        .AttributeFound;

    // ASSERT
    Assert.IsNull(actual);
}

I would get an exception saying this:

No attribute type was found so cannot be returned. Hint: Use HasFoundAttribute first.

That's not a good hint - I've already called HasFoundAttribute. The message could also be more specific as you didn't find a NameAttribute not any old attribute type. The point is you've extended 1 line of code to an entire class for no benefit.

\$\endgroup\$
  • \$\begingroup\$ 1 Regions, fair comment, There are other in my office that are not keen on them, and maybe as I work to making make my classes smaller and smaller I will not need the either. \$\endgroup\$ – Dib Dec 21 '15 at 18:01
  • \$\begingroup\$ 2 SetAttributeIfExists - maybe it would be better termed as SetAttributeOrNull ??? As it does set the attribute to null, if there is no attribute present. \$\endgroup\$ – Dib Dec 21 '15 at 18:02
  • \$\begingroup\$ 3 Exception Message. If you have already called HasFoundAttribute then you would have received a false value, and should therefore NOT (or be daft to be) be calling AttributeFound.... But I do agree the exception message should correctly identify it is the NameAttribute was not present. I'd assume that the developer who's code consumed this class would have units test which would fail if AttributeFound was called when no NameAttribute was present, and the failed test would highlight the need to check HasFoundAttribute first. \$\endgroup\$ – Dib Dec 21 '15 at 18:08
  • \$\begingroup\$ I can also see your point about SetAttributeIfExist not adding much value, except once it is finally "well named" it will better read nicer in CheckForAttribute... but in the same breath, I'm not overly precious about it so it could go! \$\endgroup\$ – Dib Dec 21 '15 at 18:12
  • 1
    \$\begingroup\$ @Dib - you could try it but I still think you're making a mountain out of a mole hill. I would check out Mark Seemann's course on SOLID on pluralsite (which you can get for free for 6 months here). He explains the right level of modularity very well. \$\endgroup\$ – RobH Dec 21 '15 at 18:38
2
\$\begingroup\$

This class could be generic which wouldn't restrict its usage to a NameAttribute only. You already have a generic extension method so it would be best to make the class generic as well.

If making this generic is an option for you, I would suggest to rename the class to PropertyAttributeFinder.

Changing the property's name HasFoundAttribute to HasAttribute or IsAttributeFound would be better, because it better states its purpose.

I usually like such fluent ways of calling methods of a class, but in this case I think it doesn't buy you anything and its over-engineering a simple task.

Throwing an exception if the attribute isn't found will result in some try..catch block of the caller code, which isn't good because it could be prevented. Just letting the property return null or default<T> if the attribute isn't found would be much better.


Although I have said a lot about the code in question, I would suggest to just skip the idea of having such a class. You already have this extension method which is exactly doing what you want to achieve and I don't see abny advantage of having that said class.

\$\endgroup\$
  • \$\begingroup\$ Generics - This class was born out of a generic extension method, but have been reading that it is "Cleaner" to have specialized class to deal with specific requirements rather than general generic classes. This allows the implementation in this class to be changed to be attribute specific without risk to any other code calling the class (or generic method) for another attribute to be affected. But I will bear it in mind. \$\endgroup\$ – Dib Dec 21 '15 at 11:43
  • \$\begingroup\$ Changing the HasFoundAttribute property back to HasAttribute (which I did originally have before posting here for review) seemed to indicate that the PropertyNameAttributeFinder has the attribute, which I deemed to be a little mislead, hence why the more specific HasFoundAttributewas chosen. I may have been mis reading "Uncle Bob's" recommendations but to me his book teaches about making the your code code read almost like a novel. maybe I have gotten the wrong end of the stick? \$\endgroup\$ – Dib Dec 21 '15 at 11:48
  • \$\begingroup\$ Fluid API - Yes I agree, very little value has been gained from adding that so I may remove it. \$\endgroup\$ – Dib Dec 21 '15 at 11:49
  • 1
    \$\begingroup\$ You are right about HasAttribute but thats the same like HasFoundAttribute. So IsAttributeFound seems to be the logical choice. \$\endgroup\$ – Heslacher Dec 21 '15 at 11:50
  • \$\begingroup\$ Throwing exceptions vs throwing null. As far as I am aware from reading the above mentioned book, throwing exceptions is preferred to returning null. Also the class has an API which allows the calling code to check the presence of the attribute first by asking for the boolean value from theHasFoundAttribute property. \$\endgroup\$ – Dib Dec 21 '15 at 11:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.