patterncsharpModerate
Using C# properties for simple operations, where should I draw the line?
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?
``
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");
}
``
[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
you should use the object methods to do this.
should be like this
Look at what is in the
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.
This isn't a completed, working class, if you develop it farther you should see that this isn't a good design at all
There needs to be a constructor that sets the basic properties, this is where you say
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.