13

I have the following code:

  class SearchCriteria
  {
        public string Name { get; set; }
        public string Email { get; set; }
        public string Company { get; set; }
        // ... around 20 fields follow

        public void Trim()
        {
            if( ! String.IsNullOrEmpty( Name ) )
            {
                 Name = Name.Trim();
            }

            if( ! String.IsNullOrEmpty( Email ) )
            {
                Email = Email.Trim();
            }

            // ... repeat for all 20 fields in the class.
        }
   }

I want to write one function that will properly trim the fields, something like:

public void Trim()
{
    Trim( Name );
    Trim( Email );
    // ...
}

private static void Trim( ref string field )
{
    if( ! String.IsNullOrEmpty( field ) )
    {
        field = field.Trim();
    } 
}

Of course, this is not permitted in C#. One option I have is to write a helper and use reflection. Is there another way I can achieve this (reflecting on so many properties will deffinitely have a performance hit on that particular scenario and I can't afford that)?

5
  • 7
    Maybe you could trim the strings in their respective property setters? Apr 23, 2015 at 8:54
  • 2
    I would probably redesign the whole class to use a HashMap for saving 20 different String values since they seem a List of Optional attributes. Then you have one PUT Method, which could do the trimming.
    – Falco
    Apr 23, 2015 at 10:02
  • @Falco: how do you know that all are strings? There could be an DateTime DayOfBirth. Using a List<String> would be too abstract since you cannot say: give me SearchCriteria.Email anymore. And if you use a Dictionary<string, string> you have to know the key. Apr 23, 2015 at 14:27
  • @TimSchmelter You can create static constants for the key-names of the Dictionary. Or an Enum and have a dictionary<enum,string> this will clean up the class a lot. -> And I'm sure there are only String fields, because he wants to call trim() on all of them. You cannot trim a date ;-)
    – Falco
    Apr 24, 2015 at 9:49
  • What i wanted to say is that it's not uncommon to have 20 properties and that it's normally not better to use a List<string> for them. You save few lines but you get worse code because all have to be strings, you cannot change it in future(f.e. change string Company to Company Company). You have to loop them or know the keys somehow, the class isn't readable or maintainable anymore for other developers. Basically are you replacing OOP concepts with strings. Apr 24, 2015 at 9:55

8 Answers 8

11

If you already have the code, what are you asking? It's readable and efficient. But maybe it would be better to let the properties already trim the passed value in the first place.

class SearchCriteria
{
    private string _Name;
    public string Name
    {
        get { return _Name; }
        set { _Name = value == null ? null : value.Trim(); }
    }

    private string _Email;
    public string Email
    {
        get { return _Email; }
        set { _Email = value == null ? null : value.Trim(); }

    }

    private string _Company;
    public string Company
    {
        get { return _Company; }
        set { _Company = value == null ? null : value.Trim(); }

    }

    // ... around 20 fields follow
}

Even if you could use a reflection approach. Consider that this code is always difficult to understand and to maintain. And it will silently trim properties even if they should not be trimmed. For example if another developer extends this class.

5
public void Trim()
{
    Name = Trim( Name );
    Email = Trim( Email );
    // ...
}

private string Trim(string field )
{
    if( ! String.IsNullOrEmpty( field ) )
        field = field.Trim();
    return field;
}

EDIT:

try also to apply Trim fuction in setters of properties

class SearchCriteria
{    
    private string Trim(string field)
    {
        if( ! String.IsNullOrEmpty( field ) )
            field = field.Trim();
        return field;
    }

    private string _name;
    public string Name
    {
        get { return _name; }
        set { _name = Trim(value); }
    }

    private string _email;
    public string Email
    {
        get { return _email; }
        set { _email = Trim(value); }

    }

    // ... other string properties
    // no public void Trim() method
}
0
3

Seems overkill .. saved the time in Trim(), wasted the time in field declaration

class SearchCriteria
{
    private Dictionary<string, string> _internalValues = new Dictionary<string, string>();
    public string Name { get { return _internalValues.ContainsKey("Name") ? _internalValues["Name"] : null; } set { _internalValues["Name"] = value; } }
    ....

    public void Trim()
    {
        foreach (var entry in _internalValues)
        {
            if (!string.IsNullOrEmpty(entry.Value)) _internalValues[entry.Key] = entry.Value.Trim();
        }
    }
}
1
  • If there are no legacy reasons why the public interface of the class has to stay the same, I would scrap the whole accessors and simply do SearchCriteria.get( SCEnum.Email );
    – Falco
    Apr 24, 2015 at 9:52
3

I prefer this style, the duplication is unavoidable to maintain readability, but this will save some screen space

class SearchCriteria
{
   public string Name { get; set; }
   public string Email { get; set; }
   public string Company { get; set; }


   public void Trim()
   {
        if(!String.IsNullOrEmpty(Name)) Name = Name.Trim();
        if(!String.IsNullOrEmpty(Email)) Email = Email.Trim();
        if(!String.IsNullOrEmpty(Company)) Company = Company.Trim();
   }
}

void Main()
{
    var criteria = new SearchCriteria();
    criteria.Email = "thing ";
    Console.WriteLine(criteria.Email.Length);
    criteria.Trim();
    Console.WriteLine(criteria.Email);
    Console.WriteLine(criteria.Email.Length);
}
3

If you weren't using auto properties you could just use a ref. I agree it is by no means optimal.

class SearchCriteria
{
    private string _name;
    public string Name { get { return _name; } set { _name = value; }}
    public string Email { get; set; }
    public string Company { get; set; }
    // ... around 20 fields follow

    void Trim(ref string str)
    {
        if (!String.IsNullOrEmpty(str))
        {
            str = str.Trim();
        }
    }

    public void Trim()
    {
        Trim(ref _name);

        // ... repeat for all 20 fields in the class.
    }
}
2

Just out of completion. I don't know if this is a good way. But you can use as Tim Schmelter said reflection. But as he also pointed out the code is harder to maintain and if someone extend it might be problems. But here is an example of how you could do it:

class SearchCriteria
{
   public string Name { get; set; }
   public string Email { get; set; }
   public string Company { get; set; }
   // ... around 20 fields follow

   public void Trim()
   {
       typeof(SearchCriteria).GetProperties()
             .Where (w =>w.PropertyType==typeof(string))
             .ToList().ForEach(f=>
             {
                 var value=f.GetValue(this);
                 if(value!=null && !string.IsNullOrEmpty(value.ToString()))
                 {
                    f.SetValue(this,value.ToString().Trim(),null);
                 }
             });
   }
}
2

You can amend your code as follows to

public void Trim()
{
    Name = Trim(Name);
    Email = Trim(Email);
    // ...
}

private static void Trim(string field)
{
    if( ! String.IsNullOrWhiteSpace( field ) )
    {
        field = field.Trim();
    }

    return field;
}

You cannot pass a property by reference, and the method String.IsNullOrEmpty() will consider white-space as non-empty, so I have used String.IsNullOrWhiteSpace().

2

Reflection will obviously be not the most performant solution, but with some modifications and caching it still can be used.

Here is the reflection helper that will allow you to create collection of mutator delegates and cache it inside of your class:

public static class ReflectionHelper
{
    public static IEnumerable<PropertyInfo> GetPropertiesOfType<THolder, TPropType>()
    {
        return typeof(THolder).GetPropertiesOfType(typeof(TPropType));
    }


    public static IEnumerable<PropertyInfo> GetPropertiesOfType(this Type holderType, Type propType)
    {
        if (holderType == null)
            throw new ArgumentNullException("holderType");
        if (propType == null)
            throw new ArgumentNullException("propType");

        return holderType
            .GetProperties()
            .Where(prop =>
                prop.PropertyType == propType); 
    }


    public static IEnumerable<Action<Func<TPropType, TPropType>>> CreateMutators<THolder, TPropType>(THolder holder)
    {
        if (holder == null)
            throw new ArgumentNullException("holder");

        return holder.GetType()
            .GetPropertiesOfType(typeof(TPropType))
            .Select(prop =>
                new
                {
                    getDelegate = (Func<TPropType>)Func.CreateDelegate(
                         typeof(Func<TPropType>), 
                         holder, 
                         prop.GetGetMethod()),
                    setDelegate = (Action<TPropType>)Action.CreateDelegate(
                         typeof(Action<TPropType>), 
                         holder, 
                         prop.GetSetMethod())
                })
            .Select(accessor =>
                (Action<Func<TPropType, TPropType>>)((mutate) =>
                {
                    var original = accessor.getDelegate();
                    var mutated = mutate(original);
                    accessor.setDelegate(mutated);
                }))
            .ToArray();
    }
}

Class code - you cache the mutators and use them inside the Trim method:

class SearchCriteria
{
    public SearchCriteria()
    {
        this.Name = "adsfasd     ";
        this.Email = "       adsfasd     ";
        this.Company = "  asdf   adsfasd     ";

        this.stringMutators = ReflectionHelper.CreateMutators<SearchCriteria, String>(this);
    }

    public string Name { get; set; }
    public string Email { get; set; }
    public string Company { get; set; }
    // ... around 20 fields follow

    private IEnumerable<Action<Func<String, String>>> stringMutators;


    private String TrimMutate(String value)
    {
        if (String.IsNullOrEmpty(value))
            return value;

        return value.Trim();
    }

    public void Trim()
    {
        foreach (var mutator in this.stringMutators)
        {
            mutator(this.TrimMutate);
        }
    }

    public override string ToString()
    {
        return String.Format("Name = |{0}|, Email = |{1}|, Company = |{2}|",
            this.Name,
            this.Email,
            this.Company);
    }
}

Main code:

            var criteria = new SearchCriteria();

            Console.WriteLine("Before trim:");
            Console.WriteLine(criteria);


            Console.WriteLine("After trim:");
            criteria.Trim();
            Console.WriteLine(criteria);

P.S.: However it is not very direct or clear solution, so I'd recommend to go with "smart" setter(getters) as it is described in other answers. Or, perhaps, you can try some Aspect Oriented Programming approach.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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