Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Should I be putting Linq statements inside of an objects properties? Is that a best practice or is that a no no? Also if that is ok, where do I draw the line with this? I assume db access is probably too far but is there a line, or just a gray area? Do you see any issues with my usage of Linq?

[XmlRoot("message")]
public class Message
{
    [XmlAttribute]
    public string type { get; set; }
    [XmlElement("segment")]
    public Segment[] segments { get;set; }
    [XmlElement("group")]
    public Group[] groups { get; set; }
    [XmlElement("field")]
    public Field[] Issues { get; set; }`public Field[] Issues { get; set; }

    public bool HasNoIssues
    {
        get
        {
            return Issues == null || Issues.Length == 0;
        }
    }

    public bool HasWarnings
    {
        get
        {
            if (!HasNoIssues)
                return Issues.Where(x => x.name == "Warnings").Count() > 0;
            return false;
        }
    }

    public bool HasErrors
    {
        get
        {
            if (!HasNoIssues)
                return Issues.Where(x => x.name == "Errors").Count() > 0;
            return false;
        }
    }

    public ResponseTags[] Warnings
    {
        get
        {
            if (HasWarnings)
                return Issues.Where(x => x.name == "Warnings").First().groups.First().warnings;
            return new ResponseTags[0];
        }
    }

    public ResponseTags[] Errors
    {
        get
        {
            if (HasErrors)
                return Issues.Where(x => x.name == "Errors").First().groups.First().warnings;
            return new ResponseTags[0];
        }
    }

    public string DisplayWarnings
    {
        get
        {
            var warningString = "";
            if (HasWarnings)
            {
                warningString += Issues.Where(x => x.name == "Warnings").First().groups.First().warnings.Aggregate("", (data, t) => data + t.value +"\n");                   
            }
            return warningString;

        }
    }

    public string DisplayErrors
    {
        get
        {
            var errorString = "";
            if (HasErrors)
            {
                errorString += Issues.Where(x => x.name == "Errors").First().groups.First().errors.Aggregate("", (data, t) => data + t.value+"\n");
            }
            return errorString;

        }
    }}

[Serializable]
public class Segment
{
    [XmlAttribute]
    public string type { get; set; }
    [XmlElement("field")]
    public Field[] fields { get; set; }
}

[Serializable]
public class Group
{
    [XmlAttribute]
    public string type { get; set; }
    [XmlElement("segment")]
    public Segment[] segments { get; set; }
    [XmlElement("e")]
    public ResponseTags[] errors { get; set; }
    [XmlElement("w")]
    public ResponseTags[] warnings { get; set; }
    [XmlElement("i")]
    public ResponseTags[] informations { get; set; }
}


[Serializable]
public class Field
{
    [XmlAttribute]
    public string name { get; set; }
    [XmlText]
    public string value { get; set; }
    [XmlElement("datatype")]
    public DataType datatype { get; set; }
    [XmlElement("group")]
    public Group[] groups { get; set; }
}


[Serializable]
public class DataType
{
    [XmlAttribute]
    public string type { get; set; }
    [XmlElement("field")]
    public Field[] fields { get; set; }
}

[Serializable]
public class ResponseTags
{
    [XmlAttribute]
    public string value { get; set; }
    [XmlAttribute]
    public string path { get; set; }
    [XmlAttribute]
    public string type { get; set; }
    [XmlAttribute]
    public string HL7ErrorCode { get; set; }
    [XmlAttribute]
    public string HL7ErrorCodeFullDescription { get; set; }
    [XmlAttribute]
    public string SegmentID { get; set; }
    [XmlAttribute]
    public string FieldPosition { get;set;}
    [XmlAttribute]
    public string FieldRepetition { get; set; }
    [XmlAttribute]
    public string ComponentNumber { get; set;}
    [XmlAttribute]
    public string SubComponentNumber { get; set;}
    [XmlAttribute]
    public string Severity { get; set; }
}

Then the object is being populated by some xml deserialization

private Message getMessage(string xml)
    {
        if (xml.Length == 0)
            return new Message();
        XmlSerializer serial = new XmlSerializer(typeof(Message));
        MemoryStream soapwriter = new MemoryStream(Encoding.UTF8.GetBytes(xml));
        return (Message)serial.Deserialize(soapwriter);
    }

And here is some test xml, there is no real data in here, I have scraped that out

<message xmlns="" type="VxuV04">
    <segment type="msh">
        <field name="MSH-1 (FieldSeperator)">
            <![CDATA[ | ]]>
        </field>
        <field name="MSH-2 (EncodingCharacters)">
            <![CDATA[ ^~\& ]]>
        </field>
        <field name="MSH-3 (SendingApplication)">
            <datatype type="hd">
                <field name="NamespaceID">OMG IMMUNIZATIO</field>
                <field name="UniversalID"/>
                <field name="UniversalIDType"/>
            </datatype>
        </field>
        <field name="MSH-4 (SendingFacility)">
            <datatype type="hd">
                <field name="NamespaceID">FF0000</field>
                <field name="UniversalID"/>
                <field name="UniversalIDType"/>
            </datatype>
        </field>
        <field name="MSH-5 (ReceivingApplication)">
            <datatype type="hd">
                <field name="NamespaceID">AK TEST FACILITY</field>
                <field name="UniversalID"/>
                <field name="UniversalIDType"/>
            </datatype>
        </field>
        <field name="MSH-6 (ReceivingFacility)">
            <datatype type="hd">
                <field name="NamespaceID">ML1001</field>
                <field name="UniversalID"/>
                <field name="UniversalIDType"/>
                <group type="ProblemCollection">
                    <e value="Invalid value: ML1001. Reason: No user assigned to this facility." path="MSH/ReceivingFacility/NamespaceID" type="problem" HL7ErrorCode="103" HL7ErrorCodeFullDescription="Table value not found" SegmentID="Msh" SegmentSequence="" FieldPosition="6" FieldRepetition="1" ComponentNumber="" SubComponentNumber="" Severity="E"/>
                </group>
            </datatype>
        </field>
        <field name="MSH-7 (DateTimeOfMessage)">3/28/2014</field>
        <field name="MSH-9 (MessageType)">
            <datatype type="msg">
                <field name="MessageCode">VXU</field>
                <field name="TriggerEvent">V04</field>
                <field name="MessageStructure">VXU_V04</field>
            </datatype>
        </field>
        <field name="MSH-10 (MessageControlID)">dsfg00541680</field>
        <field name="MSH-11 (ProcessingID)">
            <datatype type="processingtype">
                <field name="ProcessingID">P</field>
                <field name="ProcessingMode"/>
            </datatype>
        </field>
        <field name="MSH-12 (HL7Version)">2.5.1</field>
        <field name="MSH-15 (AcceptAcknowledgmentType)"/>
        <field name="MSH-16 (ApplicationAcknowledgmentType)"/>
        <field name="MSH-21 (MessageProfileIdentifier)">
            <datatype type="ei">
                <field name="EntityIdentifier"/>
                <field name="NamespaceID"/>
                <field name="UniversalID"/>
                <field name="UniversalIDType"/>
            </datatype>
        </field>
        <field name="MSH-22 (ResponsibleSendingOrganization)">
            <datatype type="xon">
                <field name="OrganizationName"/>
                <field name="OrganizationNameTypeCode"/>
                <field name="AssigningAuthority">
                    <datatype type="hd">
                        <field name="NamespaceID"/>
                        <field name="UniversalID"/>
                        <field name="UniversalIDType"/>
                    </datatype>
                </field>
                <field name="IdentifierTypeCode"/>
                <field name="AssigningFacility">
                    <datatype type="hd">
                        <field name="NamespaceID"/>
                        <field name="UniversalID"/>
                        <field name="UniversalIDType"/>
                    </datatype>
                </field>
                <field name="OrganizationIdentifier"/>
            </datatype>
        </field>
        <field name="MSH-23 (ResponsibleReceivingOrganization)">
            <datatype type="xon">
                <field name="OrganizationName"/>
                <field name="OrganizationNameTypeCode"/>
                <field name="AssigningAuthority">
                    <datatype type="hd">
                        <field name="NamespaceID"/>
                        <field name="UniversalID"/>
                        <field name="UniversalIDType"/>
                    </datatype>
                </field>
                <field name="IdentifierTypeCode"/>
                <field name="AssigningFacility">
                    <datatype type="hd">
                        <field name="NamespaceID"/>
                        <field name="UniversalID"/>
                        <field name="UniversalIDType"/>
                    </datatype>
                </field>
                <field name="OrganizationIdentifier"/>
            </datatype>
        </field>
        <field name="Errors">
            <group type="ProblemCollection">
                <e value="MSH-6 (ReceivingFacility) : Invalid value: zz1001. Reason: No user assigned to this facility." path="MSH/ReceivingFacility/NamespaceID" type="problem" HL7ErrorCode="z3" HL7ErrorCodeFullDescription="Table value not found" SegmentID="Msh" SegmentSequence="" FieldPosition="6" FieldRepetition="1" ComponentNumber="" SubComponentNumber="" Severity="E"/>
            </group>
        </field>
    </segment>  
    <field name="Errors">
        <group type="ProblemCollection">
            <e value="MSH-6 (ReceivingFacility) : Invalid value: ML1001. Reason: No user assigned to this facility." path="MSH/ReceivingFacility/NamespaceID" type="problem" HL7ErrorCode="zzz" HL7ErrorCodeFullDescription="Table value not found" SegmentID="Msh" SegmentSequence="" FieldPosition="6" FieldRepetition="1" ComponentNumber="" SubComponentNumber="" Severity="E"/>
        </group>
    </field>
    <field name="Warnings">
        <group type="ProblemCollection">
            <w value="PID-11 (PatientAddress)-Mailing : County Code  is not in the list of known county codes ." path="PID/PatientAddress" type="problem" HL7ErrorCode="zzz" HL7ErrorCodeFullDescription="Table value not found" SegmentID="Pid" SegmentSequence="" FieldPosition="11" FieldRepetition="" ComponentNumber="" SubComponentNumber="" Severity="W"/>
            <w value="PID-11 (PatientAddress)-Physical : County Code  is not in the list of known county codes ." path="PID/PatientAddress" type="problem" HL7ErrorCode="zzz" HL7ErrorCodeFullDescription="Table value not found" SegmentID="Pid" SegmentSequence="" FieldPosition="11" FieldRepetition="" ComponentNumber="" SubComponentNumber="" Severity="W"/>
        </group>
    </field>
    <field name="ErrorsAndWarningsCombined">
        <group type="ProblemCollection">
            <e value="MSH-6 (ReceivingFacility) : Invalid value: ML1001. Reason: No user assigned to this facility." path="MSH/ReceivingFacility/NamespaceID" type="problem" HL7ErrorCode="zz" HL7ErrorCodeFullDescription="Table value not found" SegmentID="Msh" SegmentSequence="" FieldPosition="6" FieldRepetition="1" ComponentNumber="" SubComponentNumber="" Severity="E"/>
            <w value="PID-11 (PatientAddress)-Mailing : County Code is not in the list of known county codes" path="PID/PatientAddress" type="problem" HL7ErrorCode="zz" HL7ErrorCodeFullDescription="Table value not found" SegmentID="Pid" SegmentSequence="" FieldPosition="11" FieldRepetition="" ComponentNumber="" SubComponentNumber="" Severity="W"/>
            <w value="PID-11 (PatientAddress)-Physical : County Code is not in the list of known county codes ." path="PID/PatientAddress" type="problem" HL7ErrorCode="zzz" HL7ErrorCodeFullDescription="Table value not found" SegmentID="Pid" SegmentSequence="" FieldPosition="z" FieldRepetition="" ComponentNumber="" SubComponentNumber="" Severity="W"/>
        </group>
    </field>
</message>

I promise the closing message tag is there

share|improve this question
1  
I don't know much about Linq, so I'll refrain from answering, but I don't see any issues with this kind of abstraction. In fact, I think it's a good thing. –  RubberDuck Jul 31 at 16:37
    
That is what I was thinking but there are some dev's that have been deving for years and they seem to break everything out into methods. I feel like this way is cleaner. –  DeadlyChambers Jul 31 at 16:39
8  
Though, consider replacing Issues.Where(x => x.name == "XXX").Count() > 0 with Issues.Any(x => x.name == "XXX"). Still LINQ, but much more concise. –  Jesse C. Slicer Jul 31 at 16:49
2  
The .Any() is more efficient as well as it stops after finding the first result whereas .Count() doesn't know what number it's being compared to and has to iterate the entire collection. Other than that, you look good. If you weren't using LINQ, you'd still have to perform the operations that LINQ does to find out if the object has any errors (for example). –  krillgar Jul 31 at 17:34
1  
Posting the rest of the code to get this to work –  DeadlyChambers Jul 31 at 18:57

3 Answers 3

up vote 13 down vote accepted

I think that most things are said, I recapitulate and add some of my own stuff:

  • As Jesse said, I'd use Any instead of Where + Count. It should have a better performance, I think.
  • Properties are for me: HasNoIssues, HasWarnings and HasErrors
  • In my Opinion the rest should be methods
  • Issues.Where(x => x.Name == "Warnings").First() is the same as Issues.First(x => x.Name == "Warnings")
  • I changed the names of the functions (but it may be a matter of taste).

So it would be in my approach like this:

    public Field[] Issues { get; set; }

    public bool HasErrors
    {
        get
        {
            if (HasIssues)
            {
                return Issues.Any(x => x.Name == "Errors");
            }
            return false;
        }
    }

    public bool HasIssues
    {
        get
        {
            return Issues != null && Issues.Length > 0;
        }
    }

    public bool HasWarnings
    {
        get
        {
            if (HasIssues)
            {
                return Issues.Any(x => x.Name == "Warnings");
            }
            return false;
        }
    }

    public ResponseTags[] GetErrors()
    {
        if (HasErrors)
        {
            return Issues.First(x => x.Name == "Errors").Groups.First().Warnings;
        }
        return new ResponseTags[0];
    }

    public ResponseTags[] GetWarnings()
    {
        if (HasWarnings)
        {
            return Issues.First(x => x.Name == "Warnings").Groups.First().Warnings;
        }
        return new ResponseTags[0];
    }

    // For the display functions:
    // I don't understand why you only usses the first
    // groups of errors and warnings, but that may be a requirement.

    public string GetErrorsDisplayData()
    {
        var errorString = "";
        if (HasErrors)
        {
            errorString += Issues.First(x => x.Name == "Errors")
                .Groups.First()
                .Errors.Aggregate("", (data, t) => data + t.Value + "\n");
        }
        return errorString;
    }

    public string GetWarningsDisplayData()
    {
        var warningString = "";
        if (HasWarnings)
        {
            warningString += Issues.First(x => x.Name == "Warnings")
                .Groups.First()
                .Warnings.Aggregate("", (data, t) => data + t.Value + "\n");
        }
        return warningString;
    }
share|improve this answer
8  
I would suggest HasIssues is better than HasNoIssues for consistency and to avoid double negatives in code like if (!HasNoIssues). –  mjolka Jul 31 at 22:17
    
Yes, you're right. I edit the code. –  Olorin71 Aug 1 at 8:51

Methods Set and Get Properties through their Get and Set methods, so it isn't a really good idea to perform operations on properties inside their get and set methods.

you should use the object methods to do this.

public string DisplayErrors
{
    get
    {
        var errorString = "";
        if (HasErrors)
        {
            errorString += Issues.Where(x => x.name == "Errors").First().groups.First().errors.Aggregate("", (data, t) => data + t.value+"\n");
        }
        return errorString;

    }
}

should be like this

public string DisplayErrors (bool inputBoolean)
{
    var errorString = "";
    if (inputBoolean)
    {
         errorString += Issues.Where(x => x.name == "Errors").First().groups.First().errors.Aggregate("", (data, t) => data + t.value+"\n");
    }
    return errorString;
}

Console.WriteLine(DisplayErrors(HasErrors));

DisplayErrors is an action to me, so it should be a method that the object performs not a property of the object,

  • Property = description
  • Method = Action

Look at what is in the HasWarnings property. A boolean property should be an object flag that is set to true or false, a method inside the object can get or set the flag (to either true or false) or it can be visible to something interacting with the object.

All of this makes me think over the whole piece of code, if I create a new object all of these properties are referring to other properties nothing is set.

Warnings property looks at HasWarnings property which in turn looks at HasNoIssues property (which really should be something like HasIssues) inside of a class called Issues , it has Issues.

This isn't a completed, working class, if you develop it farther you should see that this isn't a good design at all

HawWarnings should be a property, but it shouldn't look to another property for it's value.

There needs to be a constructor that sets the basic properties, this is where you say

if (HasIssues) {
    HasWarnings = Issues.Where(x => x.name == "Warnings").Count() > 0;
} else {
    HasWarnings = false;
}
share|improve this answer
    
Out of idle curiosity, how do you feel about HasWarnings? That seems like a property to me. I completely get what you're saying about DisplayErrors though. –  RubberDuck Jul 31 at 17:15
4  
I like what you have with HasWarnings as you're basically saying "Does this object have warnings?" whereas the DisplayErrors is telling it to do something (display the errors). If you named the property "Errors", then that'd drop it further into a gray area. –  krillgar Jul 31 at 17:34
    
we are talking about naming now @ckuhn203. look at what is in the property. A boolean property should be an object flag that is set to true or false, a method inside the object can get or set the flag (to either true or false) or it can be visible to something interacting with the object. –  Malachi Jul 31 at 18:07
    
Yeah this is just a snippet from the entire class. It is an object I have created to be created from some xml that is being parsed. –  DeadlyChambers Jul 31 at 18:52
    
you should have posted the whole code and pointed to what you wanted reviewed. as is, the code doesn't work and the question will be closed –  Malachi Jul 31 at 18:58

Seeing as Issues is a fixed size array, personally I'd be looking into caching the results (unless Issues or things within the Issues will be changing more than the properties are being called).

Though if your class is being populated with de-serialisation and it's not supposed to be changed (as I'd assume it is given the class' name), I'd be tempted to make it publicly immutable by setting all the setters to private/protected. This would have the additional benefit of meaning you could evaluate the properties at the point of de-serialisation and not have to query the arrays from within properties at all.

share|improve this answer
    
All of the arrays are now lists. –  DeadlyChambers Aug 4 at 16:31
    
@DeadlyChambers In which case I'd suggest using IList<T>/ICollection<T> for the external representation since it means you can swap out the class being used internally and it won't break the external interface. –  Pharap Aug 5 at 5:55

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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