ShouldSerialize* functions should refer to an existing property

Clean Code New rules / language support
, ,
1

The .NET framework’s XmlSerializer class allows to control the conditional serialization of individual properties based on a specific (but not well-documented) convention: When deciding whether to serialize a property, the serializer will check if a no-args function with a boolean return value and the name ShouldSerialize{PropertyName} exists. If it does exist, then the return value of that function determines whether the property will be serialized. If it does not exist, the property will be serialized by default (unless other annotations on the property prevent this).

This is of course pretty fragile. If you decide to refactor your code at a later point and change the name of a property from Foo to Bar, you also need to change the name of the function to ShouldSerializeBar. It’s also easy to get a typo into the function name for more complex property names. In either case will the functionality silently break.

A possible Sonar rule could raise an issue if a class contains a function that

  1. is public
  2. is parameterless
  3. has a boolean return value
  4. has a name that starts with ShouldSerialize (case-sensitive)
  5. does not end with the name of any of the public properties or fields in that class.

While there is of course the possibility of identifying false-positives, it seems a bit unlikely that you would have a function with these very specific requirements without intending to use this pattern.

Non-complian code:

Public Class A

    Public Property Foo As Integer
    Public Property Bar As New List(Of B)
    
    Public Function ShouldSerializefoo() As Boolean ' NONCOMPLIANT - case mismatch
        Return Foo <> 0
    End Function

    Public Function ShouldSerializeBaz() As Boolean ' NONCOMPLIANT - typo/unknown property
        Return Bar IsNot Nothing AndAlso Bar.Count() > 0
    End Function

End Class

Compliant code:

Public Class A

    Public Property Foo As Integer
    Public Property Bar As New List(Of B)

    Public Function ShouldSerializeFoo() As Boolean ' COMPLIANT
        Return Foo <> 0
    End Function
    
    Public Function ShouldSerializeBar() As Boolean ' COMPLIANT
        Return Bar IsNot Nothing AndAlso Bar.Count() > 0
    End Function

End Class

External references:

This ShouldSerialize{PropertyName} convention is not only used by the XmlSerializer, but also with Windows Forms controls (and is probably where it originated before the serializer adopted it):

The convention has also been adapted by other serialization frameworks, such as json.net and protobuf.net:

(I couldn’t find any official documentation regarding the behaviour in the XmlSerializer. It’s a semi-hidden feature. It’s been around for a while and “just works” this way.)

While there are other naming-convention based functions, like {PropertyName}Specified and Reset{PropertyName}, those don’t seem as universally used and more prone to false-positives.

Type:

  • Code Smell
2

thanks @CrushaKRool , this looks like a good rule!

3

I’m willing to give this a try. I came with the some test cases (only specified in C# yet). For me the main question is: what to do when the contract (public instance method without parameters, return type boolean) is partly met. Should we ignore those, are report issues? I would suggest the latter.

public class Casing
{
    public string SomeProperty { get; set; }

    public bool shouldSerializeSomeProperty() => true; // Noncompliant {{Conditional XML serialization method prefix has invalid casing}}
    //          ^^^^^^^^^^^^^^^
    public bool ShouldSerializeSomePROPERTY() => true; // Noncompliant {{Property has different casing}}
    //                         ^^^^^^^^^^^^
    public bool ShouldSerializeSomeProperty() => true; // Compliant
}
public class Accessibility
{
    public string PublicProperty { get; set; }
    public string InternalProperty { get; set; }
    public string ProtectedProperty { get; set; }
    public string PivateProperty { get; set; }
    public string MixedProperty { get; set; }

    internal bool ShouldSerializeInternalProperty() => true; // Noncompliant {{Accessibility should be public}}
//  ^^^^^^^^
    protected bool ShouldSerializeProtectedProperty() => true; // Noncompliant {{Accessibility should be public}}
    private bool ShouldSerializeProtectedPivateProperty() => true; // Noncompliant {{Accessibility should be public}}
    internal protected bool ShouldSerializeProtectedMixedProperty() => true; // Noncompliant {{Accessibility should be public}}

    public bool ShouldSerializePublicProperty() => true; // Compliant
}
public class ReturnType
{
    public string BooleanProperty { get; set; }
    public string NonBooleanProperty { get; set; }
    public string VoidProperty { get; set; }

    public object ShouldSerializeNonBooleanProperty() => null; // Noncompliant {{Conditional XML serialization should return a boolean }}
    //     ^^^^^^
    public void ShouldSerializeVoidProperty() { } // Noncompliant {{Conditional XML serialization should return a boolean }}
    //     ^^^^
    public bool ShouldSerializeBooleanProperty() => true; // Compliant
}
public class NonStatic
{
    public string NonStaticProperty { get; set; }
    public string StaticProperty { get; set; }

    public static bool ShouldSerializeStaticProperty() { } // Noncompliant {{Conditional XML serialization should return a boolean }}
    //     ^^^^^^
    public bool ShouldSerializeNonStaticProperty() => true; // Compliant
}
public class Arguments
{
    public string NoArgumentsProperty { get; set; }
    public string AnyArgumentsProperty { get; set; }

    public bool ShouldSerializeAnyArgumentsProperty(object argument) { } // Noncompliant {{Conditional XML serialization methods should not have arguments}}
    //                                              ^^^^^^^^^^^^^^^
    public bool ShouldSerializeNoArgumentsProperty() => true; // Compliant
}
public class Members
{
    public string KnownField;
    public string KnownProperty { get; set; }

    public bool ShouldSerializeUnknownField() => true; // Noncompliant {{'UnknownField' is unknown member}}
    //                         ^^^^^^^^^^^^
    public bool ShouldSerializeUnknownProperty() => true; // Noncompliant {{'UnknownProperty' is unknown member}}
    //                         ^^^^^^^^^^^^^^^
    public bool ShouldSerializeKnownField() => true; // Compliant
    public bool ShouldSerializeKnownProperty() => true; // Compliant
}
4

Perhaps each fulfilled condition of the contract could be scored with an individual weight. The rule could then only report an issue if a specific threshold was met, i.e. if there is sufficient evidence that the programmer was in fact intending to fulfill this contract and not just happened to name a function like this.

Having it start with “ShouldSerialize” (case insensitive) should be the minimum requirement for the rule to kick in. Then it gets scored
+1 if it’s public
+1 if it has a boolean return value
+1 if it is parameterless
+1 if it’s non-static
+2 if the end of the name matches a property in the class (case-insensitive)

If it then does not fulfill the contract entirely but reaches of a total score of (for example) at least 3 points, it will report a finding.

5

All these issues would lead to issues when applying them on conditional serialization. From that perspective they all should raise an issue. The question is: do we expect false positives on any of these, as a FP is something you want to avoid.