5
\$\begingroup\$

My insight here is that it's usually not useful to distinguish between null and empty strings. Sometimes it is, but usually not. Usually I like to assign my strings as empty if they would have been null.

var lastName = Request.Params["lastName"] ?? ""

Thus I had the idea of a struct which would wrap a real string, internally coalesce it to "", prevent all manner of null references, provide "less dangerous" versions of common string operations, yet still be compatible with strings.

This is an incomplete listing--I just wanted to get feedback on the idea. Please let me know why this is awesome, mediocre, or just plain terrible!

//with normal string
public void WriteName(string name)
{
    if(name != null && name.Length > 30)
    { name = name.Substring(0,30); }

    Console.WriteLine(name);
}
//with safestring
public void WriteName(SafeString name)
{
    Console.WriteLine(name.ReduceTo(30));
}

Below is the SafeString struct:

/// <summary>
/// Struct wrapper for a string, to prevent null references when you 
///  don't care to distinguish between `null` and empty strings.
/// Has implicit conversions to & from string
/// </summary>
public struct SafeString : IEquatable<SafeString>, IEquatable<string>
{
    /*
      The wrapped string value. 
      This will be properly initialized only if the constructor is called.
      It's possible to create a struct with `default(SafeString)` 
        or by calling the parameterless ctor, `new SafeString()`
      C#6 will allow us to define a parameterless ctor, 
        but there is still the issue of `default(SafeString)` which does not call any ctor.
      Thus when we wish to use the value of `_theString`, 
        we need to either to check for null or funnel access through a helper        
    */
    private string _theString;

    public SafeString(string s)
    {
        _theString = s ?? "";
    }

    public override string ToString()
    {
        //perform initialization in case this was created without using the ctor
        return _theString ?? (_theString = "");
    }

    public int Length { get { return ToString().Length; } }


    //safe substring method -- so you don't get an error when asking for a substring 
    //   longer than the original
    public SafeString SubString(int start, int length)
    {
        if (start < 0)
        {
            start = 0;
        }
        if (length <= 0)
        {
            return "";
        }

        var over = Math.Max((start + length) - Length, 0);
        length = length - over;

        return ToString().Substring(start, length);
    }

    //shorthand for SubString(0,length), for when you wish to cut a string down to size
    public SafeString ReduceTo(int length)
    {
        if (length > Length)
        {
            return this;
        }
        return SubString(0, length);
    }


    public bool IsEmpty { get { return string.IsNullOrEmpty(_theString); } }
    public bool IsWhitespace { get { return string.IsNullOrWhiteSpace(_theString); } }


    public static implicit operator string(SafeString ss)
    {
        return ss.ToString();
    }

    public static implicit operator SafeString(string s)
    {
        return new SafeString(s);
    }

    public override int GetHashCode()
    {
        return ToString().GetHashCode();
    }

    public override bool Equals(object obj)
    {            
        if (obj is SafeString)
        {
            return Equals((SafeString) obj);
        }
        if (obj is string)
        {
            return Equals((string) obj);
        }

        return false;
    }

    public bool Equals(SafeString other)
    {
        return ToString() == other.ToString();
    }

    public bool Equals(string other)
    {
        if (other == null)
        {
            return ToString() == ""; //we are never 'null'
        }
        return other == ToString();
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ I have rolled back the last edit - no biggie, please see what you may and may not do after receiving answers on Meta. \$\endgroup\$ – Mathieu Guindon Jan 13 '15 at 21:38
  • \$\begingroup\$ @Mat'sMug sorry, didn't realize. i didn't consider the mutable thing an important part of the implementation, but now i suspect most answers will address that.. \$\endgroup\$ – dan Jan 13 '15 at 23:12
5
\$\begingroup\$

System.String is a class, a reference type. Wrapping it in a value type makes for a pretty confusing start: it's like the opposite of boxing, where a value type is wrapped with a reference type. It's ...awkward.

Value types should be immutable; _theString doesn't immediately strike me as a readonly field:

private string _theString;

And _theString is a very, very bad name - I would have simply gone with _value, like this:

private readonly string _value;

Calling ToString shouldn't have side-effects - here calling ToString is mutating the instance, changing the reference stored in _theString:

public override string ToString()
{
    //perform initialization in case this was created without using the ctor
    return _theString ?? (_theString = "");
}

There is little to no gain in doing this, and it makes your value type violate a fundamental rule: value types should be immutable, no ifs, no buts. I'd change it to this:

public override string ToString()
{
    return _value ?? string.Empty;
}

The only way client code can retrieve the encapsulated value, is by calling ToString - this is practical and useful, but not exactly instinctive. I would expose a getter:

public string Value { get { return ToString(); } }

The Equals(string) implementation relies on implementation details of another method:

return ToString() == "" //we are never 'null'

I don't see a need for an explanatory comment when using "normal" string methods:

return ToString().IsNullOrEmpty();

You have hard-coded "" in a bunch of places - I find string.Empty conveys "" in a more unambiguous way, and I would replace all occurrences of "" with string.Empty.


I think your struct is violating the Single Responsibility Principle, with methods like ReduceTo - I think these members belong as extension methods, extending the String type itself.

Also when writing code at that level, you should make sure every public member has proper XML comments, so that client code consuming it has a bit of documentation in IntelliSense.

\$\endgroup\$
5
  • \$\begingroup\$ Thanks. I suppose it's fine to not coalesce but instead do that in ToString or Value. \$\endgroup\$ – dan Jan 13 '15 at 20:51
  • \$\begingroup\$ I don't understand your comment about the Equals method relying on the implementation details of another method. Could you clarify? \$\endgroup\$ – dan Jan 13 '15 at 20:58
  • \$\begingroup\$ Regarding single responsibility principle, would you also agree that the string class itself contains too many methods? ;) \$\endgroup\$ – dan Jan 13 '15 at 20:58
  • \$\begingroup\$ I agree that it can be confusing to wrap a reference type with a value type, however if it were a class then we'd lose the value of a "non-nullable string type" because it'd be possible for the instance to be null. \$\endgroup\$ – dan Jan 13 '15 at 21:00
  • \$\begingroup\$ I meant that Equals is assuming that the result of ToString isn't going to be null - while a fair assumption to make, using .IsNullOrEmpty instead of comparing to an empty string effectively removes that assumption. As for the many members of System.String, I think it's fine as it is - I meant to say that ReduceTo (and possibly other non-listed members) double up functionality that one would/might want to see in the String type, and you'd find yourself implementing it as both a member of this SafeString and as an extension method to System.String. \$\endgroup\$ – Mathieu Guindon Jan 13 '15 at 21:04
5
\$\begingroup\$

If A equals B, then B should equal A, right?

Well, not in your case.

public bool Equals(string other)
{
    if (other == null)
    {
        return ToString() == ""; //we are never 'null'
    }
    return other == ToString();
}

I find it weird that your class can be considered equal to null, but null is not considered to be equal to anything else besides null. I don't consider this a correct Equals implementation.


You have this comment in your ToString method:

//perform initialization in case this was created without using the ctor

I think this is a bad idea. If someone manages to instantiate an object without using the constructor then honestly, suit themselves. Do you really want to support this edge-case? I would not. And as @Mat'sMug says, don't mutate in the ToString method. Simply returning _theString is enough there.

public bool IsEmpty { get { return string.IsNullOrEmpty(_theString); } }

Considering the constructor, it feels like you don't really need to test for null in this method, although it seems like C# doesn't provide a default IsEmpty method.


Overall, I think your class is not worth being an own class. It definitely sounds like the additional functionality you are adding here should be extension methods.

Looking at your example, there is an easy way to improve it without adding this class:

//with normal string
public void WriteName(string name)
{
    if(name != null && name.Length > 30)
    { name = name.Substring(0,30); }

    Console.WriteLine(name);
}

//with safestring
public void WriteName(string name)
{
    name = name ?? "";
    Console.WriteLine(name.Substring(0, Math.Min(name.Length(), 30)));
}

This functionality is easy to make methods for, no matter if you want it as extension methods or not, but I don't consider it worth it's own class.


Feature-request:

As you re-implemented the Substring method, perhaps consider PHP's implementation which has support for negative parameters. It is one of the few things I actually miss from PHP.

\$\endgroup\$
3
  • \$\begingroup\$ Currently I have things like the SafeSubstring in a library as extension methods on string. However that library has grown to the point where there is a SafeXXX version of most string methods. I wanted to experiment with the idea of encoding the safeness in the type system rather than by convention of calling methods whose name start with Safe. \$\endgroup\$ – dan Jan 13 '15 at 23:17
  • \$\begingroup\$ I believe you can end up with a default(SafeString) when a class has a field of type SafeString and it isn't explicitly assigned (this is a struct, not a class). I wanted to cover that case, else I think this construct wouldn't be as useful (like how an int field is 0 by default). However, I agree that it's dumb to mutate a struct. I knew it was dumb when I wrote this code and did it anyways--imagine that! \$\endgroup\$ – dan Jan 13 '15 at 23:23
  • \$\begingroup\$ The equals code is up for debate. Since this is a construct that treats a null string as an empty string, I figured it made sense for new SafeString(null) == new SafeString("") == ((string)null). \$\endgroup\$ – dan Jan 13 '15 at 23:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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