Property with "get" only, or a separate method?
-
So I was discussing this subject with one of my coworkers after he asked me to review some of his code. Our software breaks apart data files, and feeds them to a separate service to be stored in a SQL Server database. His "Record" class has several properties for each of the fields in the client's data file. This particular client's module uses the FileHelpers utility library to read a CSV format, so we're not parsing it manually. Three of the fields are: City, State, ZIP. Ultimately, we only really care to access the City/State/ZIP as a whole, a formatted string (e.g. Small-Town, USA 12345). With that being said, does it make more sense to loop over a collection of records and call a method like so:
private void SetCityStateZip(Record myRecord)
{
myRecord.CityStateZip = string.Format("{0}, {1} {2}", myRecord.City, myRecord.State, myRecord.Zip);
}"CityStateZip" is a property in the Record class, referencing a field "_cityStateZip". Or does it make more sense, instead of creating a whole new field/property, to use a property with a getter that contains the logic as follows:
public string CityStateZip
{
get
{
return string.Format("{0}, {1} {2}", _city, _state, _zip);
}
}I ask because to me, it makes sense to use the getter with simple logic, as opposed to a separate method. Why? Because CityStateZip is only accessed once for each record when the processing service takes place. The individual City/State/ZIP properties are not used elsewhere. I figured, for the sake of example, if a data file has 10,000 records, and you call the "SetCityStateZip" method for each, you're setting 10,000 "CityStateZip" properties well before it's even used. There are actually TWO properties like this, so that's 20,000 fields/properties being set when they're each only referenced once in processing. Why assign extra "string" values? Hope this makes sense. Kind of typed it in a hurry. Just wanting some feedback, as a coworker stated that he believes it's a bad idea to use logic in getters. If he is correct, so be it. But I sometimes enjoy a second/third/fourth/nth. opinion. ;)
djj55: Nice but may have a permission problem Pete O'Hanlon: He has my permission to run it.
-
So I was discussing this subject with one of my coworkers after he asked me to review some of his code. Our software breaks apart data files, and feeds them to a separate service to be stored in a SQL Server database. His "Record" class has several properties for each of the fields in the client's data file. This particular client's module uses the FileHelpers utility library to read a CSV format, so we're not parsing it manually. Three of the fields are: City, State, ZIP. Ultimately, we only really care to access the City/State/ZIP as a whole, a formatted string (e.g. Small-Town, USA 12345). With that being said, does it make more sense to loop over a collection of records and call a method like so:
private void SetCityStateZip(Record myRecord)
{
myRecord.CityStateZip = string.Format("{0}, {1} {2}", myRecord.City, myRecord.State, myRecord.Zip);
}"CityStateZip" is a property in the Record class, referencing a field "_cityStateZip". Or does it make more sense, instead of creating a whole new field/property, to use a property with a getter that contains the logic as follows:
public string CityStateZip
{
get
{
return string.Format("{0}, {1} {2}", _city, _state, _zip);
}
}I ask because to me, it makes sense to use the getter with simple logic, as opposed to a separate method. Why? Because CityStateZip is only accessed once for each record when the processing service takes place. The individual City/State/ZIP properties are not used elsewhere. I figured, for the sake of example, if a data file has 10,000 records, and you call the "SetCityStateZip" method for each, you're setting 10,000 "CityStateZip" properties well before it's even used. There are actually TWO properties like this, so that's 20,000 fields/properties being set when they're each only referenced once in processing. Why assign extra "string" values? Hope this makes sense. Kind of typed it in a hurry. Just wanting some feedback, as a coworker stated that he believes it's a bad idea to use logic in getters. If he is correct, so be it. But I sometimes enjoy a second/third/fourth/nth. opinion. ;)
djj55: Nice but may have a permission problem Pete O'Hanlon: He has my permission to run it.
Personally, I'd probably go for the property, but...there is a third option which I'd probably prefer: override ToString to return the string version:
public override string ToString() { return string.Format("{0}, {1} {2}", \_city, \_state, \_zip); }That way, controls such as DataGridView , ComboBox and so forth can also use it.
Those who fail to learn history are doomed to repeat it. --- George Santayana (December 16, 1863 – September 26, 1952) Those who fail to clear history are doomed to explain it. --- OriginalGriff (February 24, 1959 – ∞)
-
So I was discussing this subject with one of my coworkers after he asked me to review some of his code. Our software breaks apart data files, and feeds them to a separate service to be stored in a SQL Server database. His "Record" class has several properties for each of the fields in the client's data file. This particular client's module uses the FileHelpers utility library to read a CSV format, so we're not parsing it manually. Three of the fields are: City, State, ZIP. Ultimately, we only really care to access the City/State/ZIP as a whole, a formatted string (e.g. Small-Town, USA 12345). With that being said, does it make more sense to loop over a collection of records and call a method like so:
private void SetCityStateZip(Record myRecord)
{
myRecord.CityStateZip = string.Format("{0}, {1} {2}", myRecord.City, myRecord.State, myRecord.Zip);
}"CityStateZip" is a property in the Record class, referencing a field "_cityStateZip". Or does it make more sense, instead of creating a whole new field/property, to use a property with a getter that contains the logic as follows:
public string CityStateZip
{
get
{
return string.Format("{0}, {1} {2}", _city, _state, _zip);
}
}I ask because to me, it makes sense to use the getter with simple logic, as opposed to a separate method. Why? Because CityStateZip is only accessed once for each record when the processing service takes place. The individual City/State/ZIP properties are not used elsewhere. I figured, for the sake of example, if a data file has 10,000 records, and you call the "SetCityStateZip" method for each, you're setting 10,000 "CityStateZip" properties well before it's even used. There are actually TWO properties like this, so that's 20,000 fields/properties being set when they're each only referenced once in processing. Why assign extra "string" values? Hope this makes sense. Kind of typed it in a hurry. Just wanting some feedback, as a coworker stated that he believes it's a bad idea to use logic in getters. If he is correct, so be it. But I sometimes enjoy a second/third/fourth/nth. opinion. ;)
djj55: Nice but may have a permission problem Pete O'Hanlon: He has my permission to run it.
IMHO, the 2nd option makes much more sense - a
getonly property that returns the composite information. If the individual components (city,stateandzip) are not read by consumers ofRecord, they should only be private fields and not exposed as properties. /raviMy new year resolution: 2048 x 1536 Home | Articles | My .NET bits | Freeware ravib(at)ravib(dot)com
-
Personally, I'd probably go for the property, but...there is a third option which I'd probably prefer: override ToString to return the string version:
public override string ToString() { return string.Format("{0}, {1} {2}", \_city, \_state, \_zip); }That way, controls such as DataGridView , ComboBox and so forth can also use it.
Those who fail to learn history are doomed to repeat it. --- George Santayana (December 16, 1863 – September 26, 1952) Those who fail to clear history are doomed to explain it. --- OriginalGriff (February 24, 1959 – ∞)
-
IMHO, the 2nd option makes much more sense - a
getonly property that returns the composite information. If the individual components (city,stateandzip) are not read by consumers ofRecord, they should only be private fields and not exposed as properties. /raviMy new year resolution: 2048 x 1536 Home | Articles | My .NET bits | Freeware ravib(at)ravib(dot)com
-
Which object overrides the ToString() method? I understand the purpose, but the Record class holds far more than just the fields I listed.
djj55: Nice but may have a permission problem Pete O'Hanlon: He has my permission to run it.
In that case, just stick with the property - it should be more performant since it won't be creating unnecessary strings, and the string will be up-to-date with any changes to the underlying data.
Those who fail to learn history are doomed to repeat it. --- George Santayana (December 16, 1863 – September 26, 1952) Those who fail to clear history are doomed to explain it. --- OriginalGriff (February 24, 1959 – ∞)
-
Are they any proven advantages, so to speak, in using that over the first option? I mean, in terms of maintainability, readability, etc.?
djj55: Nice but may have a permission problem Pete O'Hanlon: He has my permission to run it.
Yes, in terms of program verification and maintenance. The first method imposes a requirement that
SetCityStateZip()be invoked prior to using the composite value. The latter imposes no such requirement, and further guarantees that the composite will always be correct if its constituent parts are correct. Going further, I suggestRecordhave a constructor that consumes a CSV string (or better yet a bag of properties), instead of requiring one or moreSet...methods or private property setters to be invoked. This dependency injection ensures aRecordinstance can never exist in an invalid state (i.e. constructed but not having all its properties set) before it's used. This also allows aRecordto be easily unit tested by mocking a set of parsed property values. /raviMy new year resolution: 2048 x 1536 Home | Articles | My .NET bits | Freeware ravib(at)ravib(dot)com
