HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Using C# properties for simple operations, where should I draw the line?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simpleoperationsthelinepropertiesdrawwhereforusingshould

Problem

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

Solution

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

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

Code Snippets

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;

    }
}
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));
if (HasIssues) {
    HasWarnings = Issues.Where(x => x.name == "Warnings").Count() > 0;
} else {
    HasWarnings = false;
}

Context

StackExchange Code Review Q#58667, answer score: 15

Revisions (0)

No revisions yet.