5
\$\begingroup\$

I have some Field objects that need to be serialized. Their primary string identifier is called a Tag and starts with a number, so it's not suitable for use as a class name. Thus, for a field bearing tag 32A, I am using the name Field32A. However, during serialization, I need access to the literal string value "32A".

I don't think it's a good idea to convert the name of the class to a string and substring the 32A off the end (though I'm open to hearing argument). And, instead of hard-coding this string in the serialization logic somewhere deep in the class, I thought of putting the string in an attribute that can decorate the class itself, in close proximity to the class name in the source code, so future developers who (say) copy this class to a new one won't miss the need to change the class name and the string representing the tag value.

Note: the FieldTag attribute on the FieldBase class is just there to prove that we’re getting the right fieldtag (and in some cases I did incorrectly get the base class field tag, such as when it was omitted on the derived class, until I put the inherit: false in there).

The magic here is occurring inside the Field32A.ToString method where it refers to FieldTag, which goes to the property on the base class and gets the value from the attribute. Do you think this is a good idea?

Run the following code live in a DotNetFiddle

[FieldTag("Base")]
public abstract class FieldBase {
    public string FieldTag {
        get {
            FieldTagAttribute fieldTagAttribute = (FieldTagAttribute) (
                GetType().GetCustomAttribute(typeof(FieldTagAttribute), inherit: false)
            );

            if (fieldTagAttribute == null) {
            throw new InvalidOperationException(String.Format(
               "Class {0} has no attribute [FieldTag]. "
               + "Please add the attribute to this class.", GetType()));
            }

            return fieldTagAttribute.FieldTag;
        }
    }
}

public class FieldTagAttribute : Attribute {
   public FieldTagAttribute(string fieldTag) {
       FieldTag = fieldTag;
   }

   public string FieldTag { get; set; }
}

[FieldTag("32A")]
public class Field32A : FieldBase {
    public string Value{ get; set; }
    public override string ToString() {
        return ":" + FieldTag + ":" + Value;
    }
}

void Main() {
    Field32A field32A = new Field32A { Value = "12345" };
    field32A.ToString().Dump(); // :32A:12345
}
\$\endgroup\$
7
  • 1
    \$\begingroup\$ It's bad form for a property getter to throw an exception. I would do so in a protected constructor of your FieldBase class. At that point, you can cache the retrieved value in a class member and not have to do any reflection in the property getter at all. \$\endgroup\$ Jul 16, 2015 at 21:31
  • \$\begingroup\$ @JesseC.Slicer I don't think it's bad form to throw an exception in a property setter if something is invalid. Care to elaborate on why you feel that way? \$\endgroup\$
    – RubberDuck
    Jul 16, 2015 at 21:44
  • \$\begingroup\$ Ohhh. Getter. I read that as "setter" \$\endgroup\$
    – RubberDuck
    Jul 16, 2015 at 21:45
  • \$\begingroup\$ @JesseC.Slicer This is really helpful feedback. I completely agree with you about throwing exceptions in a getter (I just never thought about it before). And caching the value at instantiation time makes perfect sense. If you change your comment to an answer, I can at least vote it up, or even accept it if no one else gives more comprehensive feedback. \$\endgroup\$
    – ErikE
    Jul 16, 2015 at 22:21
  • 1
    \$\begingroup\$ That is understood. That kind of advice should've only been posted as an answer anyway. We just need one implementation in a question at a time to prevent confusion, and answers must be "mapped" to one. \$\endgroup\$
    – Jamal
    Jul 17, 2015 at 18:31

1 Answer 1

3
\$\begingroup\$

It's considered bad form for a property getter to throw an exception. I would do so in a protected constructor of your FieldBase class. At that point, you can cache the retrieved value in a class member and not have to do any reflection in the property getter at all.

I'm also a fan of immutable semantics where available and appropriate and have modified the attribute itself to be such:

[FieldTag("Base")]
public abstract class FieldBase {
    protected FieldBase() {
        FieldTagAttribute fieldTagAttribute = (FieldTagAttribute) (GetType().GetCustomAttribute(typeof(FieldTagAttribute), false));

        if (fieldTagAttribute == null) {
            throw new InvalidOperationException(String.Format("Class {0} has no attribute [FieldTag]. Please add the attribute to this class.", GetType()));
        }

        s_FieldTag = fieldTagAttribute.FieldTag;
    }

    private readonly string s_FieldTag;

    public string FieldTag { get { return s_FieldTag; } }
}

[System.AttributeUsageAttribute(System.AttributeTargets.Class, Inherited = false)]
public sealed class FieldTagAttribute : Attribute {
   public FieldTagAttribute(string fieldTag) {
       s_FieldTag = fieldTag;
   }

   private readonly string s_FieldTag;

   public string FieldTag { get { return s_FieldTag; } }
}

[FieldTag("32A")]
public class Field32A : FieldBase {
    public string Value { get; set; }
    public override string ToString() {
        return ":" + FieldTag + ":" + Value;
    }
}

public class Program {
    public static void Main() {
        Field32A field32A = new Field32A { Value = "12345" };
        field32A.ToString().Dump();
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Any objection to { get; private set; } rather than using the backing field? \$\endgroup\$
    – ErikE
    Jul 17, 2015 at 1:42
  • 2
    \$\begingroup\$ Because it's not truly immutable and can be changed by any other private or public method in the class. readonly makes it frozen after construction. C#6 (Roslyn) has nicer constructs for that: getter-only initialized properties. \$\endgroup\$ Jul 17, 2015 at 2:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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