Applying TrimSideSpaces() method to multiple string properties

Question

I am looking for generic ideas and guidelines of how to improve the style of my coding and make it more readable and robust.

public class TemplatePart : ITemplatePart
{
 #region Fields
 private readonly List<ITemplatePart> _options = new List<ITemplatePart>();
 private readonly List<ITemplatePart> _cables = new List<ITemplatePart>();
 #endregion // Fields
 #region Constructor
 #endregion // Constructor
 #region Creator
 public static TemplatePart CreateTemplatePart(string partName, string basePart)
 {
 return new TemplatePart
 {
 PartName = partName,
 BasePart = basePart
 };
 }
 public static TemplatePart CreateTemplatePart(DataTable table, DataRow row, int rowIndex)
 {
 return new TemplatePart
 {
 // as you can see, TrimSideSpaces() get's applied for every single content
 // being read from the sheet for each property.
 Key = rowIndex,
 PartName = TrimSideSpaces(row.Field<string>(Strings.TemplateSpreadSheet_Column_PartName)),
 BasePart = TrimSideSpaces(row.Field<string>(Strings.TemplateSpreadSheet_Column_BasePart)),
 Category = TrimSideSpaces(ExtractVendor(table.TableName))
 };
 }
 private static string TrimSideSpaces(string p)
 {
 var rgx = new Regex(@"\S.*\S");
 return rgx.Match(p).Value;
 }
 #endregion // Creator
 #region Public Properties
 public int Key { get; set; }
 public string PartName { get; set; }
 public string BasePart { get; set; }
 // This captures the tab the item is in.
 public string Category { get; set; }
 public double? PriceDom { get; set; }
 public double? PriceInt { get; set; }
 public IEnumerable<ITemplatePart> Options { get { return this._options; } }
 public IEnumerable<ITemplatePart> Cables { get { return this._cables; } }
 #endregion // Public Properties
 #region Private Helpers
 ....
 #endregion // Private Helpers
}

I have a similar concrete class with about 20 properties and I am thinking it might not be a good choice to apply methods on each property such as TrimSideSpaces() in this example.

I apologize if this is not a whole code which could compile.

Answer 1

• single letter variables shouldn't be used except for loop variables
• shortening variables names removes readability of the code
• if you don't need List<T> specific methods consider to change List<ITemplatePart> _options to IList<ITemplatePart>. You should if possible code against interfaces instead of implementations.
• instead of calling the TrimSideSpaces() method, which in reality does only return an empty string, you should just use the built in Trim() method of the String class which removes any leading and trailing space

Refactoring

By using the overloaded CreateTemplatePart() method

public static TemplatePart CreateTemplatePart(string partName, string basePart)
{
 return new TemplatePart
 {
 PartName = partName.Trim(),
 BasePart = basePart.Trim()
 };
}
public static TemplatePart CreateTemplatePart(DataTable table, DataRow row, int rowIndex)
{
 TemplatePart templatePart = CreateTemplatePart(row.Field<string>(Strings.TemplateSpreadSheet_Column_PartName),
 row.Field<string>(Strings.TemplateSpreadSheet_Column_BasePart));
 templatePart.Key = rowIndex;
 templatePart.Category = ExtractVendor(table.TableName).Trim();
 return templatePart;
} 

Update based on the comments

In regards to your 4th point, TrimSideSPaces() returns the content of the property without it's leading and trailing spaces. "which in reality does only return an empty string" how?

Using the method in your code the first test fails while the second one passes.

[TestMethod()]
public void TrimSideSpacesTest_WillFail()
{
 SnippetsCodeReview target = new SnippetsCodeReview();
 string value = " g ";
 string expected = "g";
 string actual = target.TrimSideSpaces(value);
 Assert.AreEqual(expected, actual);
}
[TestMethod()]
public void TrimSideSpacesTest_WillPass()
{
 SnippetsCodeReview target = new SnippetsCodeReview();
 string value = " g ";
 string expected = String.Empty;
 string actual = target.TrimSideSpaces(value);
 Assert.AreEqual(expected, actual);
} 

The suggested overloaded CreateTemplatePart() improves the code. how?

• Improvement could be the less repetitions because we use the overloaded CreateTemplatePart() method in which we call the Trim().
• Also, this "improvement" comes obviously with a lack of readability. This is partly because the Strings constants have some really long names.

An improved refactoring after renaming the overloaded methods to Create.

public static TemplatePart Create(DataTable table, DataRow row, int rowIndex)
{
 TemplatePart templatePart = Create(row);
 templatePart.Key = rowIndex;
 templatePart.Category = ExtractVendor(table.TableName).Trim();
 return templatePart;
}
private static TemplatePart Create(DataRow row)
{
 String partName = row.Field<string>(Strings.TemplateSpreadSheet_Column_PartName);
 String basePart = row.Field<string>(Strings.TemplateSpreadSheet_Column_BasePart);
 return Create(partName, basePart);
}
public static TemplatePart Create(string partName, string basePart)
{
 return new TemplatePart
 {
 PartName = partName.Trim(),
 BasePart = basePart.Trim()
 };
} 

• Mehrad: Couple of questions: In regards to your 4th point, TrimSideSPaces() returns the content of the property without it's leading and trailing spaces. "which in reality does only return an empty string" how? However, very good point on using Trim() instead. The suggested overloaded CreateTemplatePart() improves the code. how? Want to know the advantage of rearranging the code that way cuz the original code looks more readable and tidier (imo). (not discussing about relocating Trim() from overloaded to original part of it though).
• Mehrad: Good point. My Regex expression should be modified from "\S.*\S" to "\S(.*\S)?" to work on single letter strings. Since very unusual I didn't pick up on that one. but I should say suggested trim() is a better way to go. and also thanks for the updated answer.
• CodesInChaos: I don't have a problem with giving a semantically meaningless string parameter like TrimSideSpaces has a single character name s.

Answer 2

The two variables include:

1. Multiple properties
2. Multiple operations

For condition (1), having a class will encapsulate everything there itself.

For condition (2), you can have a ConverterUtil class with static methods having multiple operations and also one String-related-function that calls all the related operations.

Two requirements:

1. You promise to use only the operated value for future operations
2. You may want to use the actual value as well as operated value. Have the ConverterUtil static method called from the setters of the variables of your class for (1) or have two getters wherein one uses the ConverterUtil function for option (2). This will limit the proceedings to the class itself.

Another option includes having ConverterUtil operate on the class itself. Thus, your class will be untouched. But the overhead would increase for any new property or new method. ConverterUtil on property will abstract the operations from the class.

This Java snippet provides clarity for the above solution:

Class Input{
 private String name;
 public void setName(String name){
 this.name = ConverterUtil.getTransformedString(name);
 }
}
final Class ConverterUtil{
 public static String getTransformedString(String name){
 name.trim();
 name.toUpperCase()
 // and many more.
 }
 public static int getTransformedInteger(int value){
 //etc
 }
}

If required, have the transformation classes and inject it to a Factory class to operate on your class with properties to avoid having a utility class.

• Heslacher: In c# there is no final keyword. See: http://stackoverflow.com/a/1327549/2655508
• thepace: Sorry, its for a generic solution.