C# StringNotNullOrEmpty - am I wtf'ing?



  • I am just about to implement said class.

    And I don't feel that comfortable about it.

    Thing is that we need to write several hundred methods throughout an application, most of which contain one or more string arguments that should never be null.

    Following the principles of defensive programming and particularly fail-first, I've then begun writing a number of if(string.IsNullOrEmpty(myParm)) throw new ArgumentException() - type of parameter validation, which need to be done in both implementing classes and mocking classes, through at least two tiers - and since we're following a rigorous unit testing approach, we should also write test cases for most of these bad parameters.

    All in all, implementing the not null, not empty constraint in a class, we could avoid at least 1000 lines of superflous code, and I can't think of a rational argument of why not to do this.

    Still, I cannot help feeling that I am missing something obvious, as this just seems.. I don't know.. wrong?

    Any comments would be helpful.



  • I'm afraid you will not be able to make what you want. How do you think you can implement the "not null" constraint in a class in a way that variables of that class respect it? Of course you could make a class that contains a not null, not empty string (and enforces that constraint), but the variable pointing to the containt could still be null, so you have to check that one instead.



  • I was thinking about doing something along the lines of implementing the object so that consumers would use it as an actual string.

    Of course, that means implementing a lot of forwarding code, since string is sealed. Example code:

    [Serializable]

    public class StringNotNull

    {

     readonly string _value;

     public StringNotNull(string value) { if (value == null) throw new ArgumentException("StringNotNull cannot have a null reference constructor parameter"); _value = value; }

     public int IndexOf(string substring) { return _value.IndexOf(substring); }

     public int IndexOf(string substring, int startIndex) { return _value.IndexOf(substring, startIndex); }

     public static implicit operator string(StringNotNull o) { return o._value; }

     public static StringNotNull operator + (StringNotNull a, StringNotNull b) { return new StringNotNull(a._value + b._value); }

     public static StringNotNull operator +(string a, StringNotNull b) { return new StringNotNull(a + b._value); }

     public static StringNotNull operator +(StringNotNull a, string b) { return new StringNotNull(a._value + b); }

    }



  • In that case, all you need is a method - let's assume you implement the adopter anti-pattern and put it there - like that:

    class Adopter { 

    ... lots of orphaned methods... 

    public static string StringIsNotNullOrEmpty(string value) {
        if (value == null) throw new ArgumentException("StringNotNull cannot have a null reference constructor parameter");
        return value;
    }

    ... more lots of orphaned methods...

    }

    This way, whenever you need it, you just call

     ... foo.Bar(Adopter.StringIsNotNullOrEmpty(myString));



  •  You do know you still don't have any null checks.

     StringNotNull nullString = null;

    nullString.IndexOf(1); // w00t crash



  •  Stupid edit time-out, I would really look into PostSharp, it allows post compilation additions to your code using attributes (eg implementing INotifyPropertyChanged, logging,...)



  • Are you by chance using C# 3.0? If you are, you could write an extension method for the string class:

    <pre>

     public static bool IsNullOrEmpty(this string s)

    {

      return s == null || s == "";

    }

    </pre>

     I'm not sure if it will actually work for a null string, but it's worth a try, seeing as the string is "really" a parameter to a static method, so you're not invoking any method of the string itself...



  • IsNullOrEmpty() is already a framework method on String.  He's already using it.  He thinks it's WTFy and wants something else.



  • @XIU said:

     You do know you still don't have any null checks.

     StringNotNull nullString = null;

    nullString.IndexOf(1); // w00t crash

     Yes, that's embarassingly correct. I got that in a giant case of "DOH" while in the middle of coding it :-/

    To make a long and painful story short, I tried implementing it as a struct instead (to avoid the null check issue - otoh meaning I now have a value type instead of a reference type.. gaaargh..), but in the still I ran into all sorts of trouble that are probably too much to go into here.

     AmmoQ -> Thanks for the thought, and that would be the easiest route to follow, however I feel that the callee - and not the caller - should verify the correct state of the methods being called.



  • @nixen said:

    I feel that the callee - and not the caller - should verify the correct state of the methods being called.
    You're the only one.



  • @Welbog said:

    @nixen said:

    I feel that the callee - and not the caller - should verify the correct state of the methods being called.
    You're the only one.

     Thank you for that inspired and delightfully insightful comment - whatever you're smoking, let me have some as well.



  • @nixen said:

    AmmoQ -> Thanks for the thought, and that would be the easiest route to follow, however I feel that the callee - and not the caller - should verify the correct state of the methods being called.

     

    The callee can just as well use the StringIsNotNullOrEmpty method whenever it actually does something with the strings it received as parameters.


  • Can't you solve your problem with an AOP framework?



  •  A.  The idea you have is indeed a GIANT WTF.  You are now requiring that the users of your code use your WTF string class instead of regular strings. 

    B.  Aspects can do what you want, but I don't really like aspects very much, so I don't suggest them.

    C.  What's wrong with 1000 lines of code?  Although, you should have them all call a single method to check the parameters and throw the exception.  That way if you change the message of the exception (what if you want to sell this in other countries, and you want it internationalized?), or change the exception itself it's just 1 line of code instead of 1000. 


Log in to reply