Basic volume issue representation

I have the following struct:

public struct VolumeIssue
{
    public readonly string Volume;

    public readonly string Issue;

    public VolumeIssue(string volume, string issue)
    {
        this.Volume = volume;
        this.Issue = issue;
    }

    public bool IsNullOrWhiteSpace()
    {
        return string.IsNullOrWhiteSpace(this.Volume) && string.IsNullOrWhiteSpace(this.Issue);
    }

    public override string ToString()
    {
        StringBuilder stringRep = new StringBuilder("");

        if (!string.IsNullOrWhiteSpace(this.Volume))
        {
            stringRep.AppendFormat("v. {0}", this.Volume);
            if (!string.IsNullOrWhiteSpace(this.Issue))
            {
                stringRep.AppendFormat(" iss. {0}", this.Issue);
            }
        }

        return stringRep.ToString();
    }
}

Overall I am happy with it. But being still a relatively inexperienced developer, I would like some input on the design of this struct. Ideas on how to flesh it out are also greatly appreciated.

In order to add more context, here is one of the use cases:

public class IssueRangeCoverage
{
    public VolumeIssue Start { get; set; }

    public VolumeIssue End { get; set; }

    public override string ToString()
    {
        if (this.Start.IsNullOrWhiteSpace() && this.End.IsNullOrWhiteSpace())
        {
            return "";
        }

        string endString = this.End.ToString();

        if (string.IsNullOrWhiteSpace(endString))
        {
            endString = "Current";
        }
        return this.Start.ToString() + " - " + endString;
    }
}

Figured I would raise a few points:

Having read your responses/replies, we can gather you have the following specs (roughly):

• Small memory footprint

• Optimization

Before I go over those two points, Let's first step through the code:

I feel the name VolumeIssue is not very descriptive. In fact, it seems odd to have the struct name be the combination of both of its fields.

Next, I would keep the readonly members but make them private, also drop the uppercase. Instead, use publicly accessible properties in order to access these fields as follows:

private readonly string volume;
public string Volume { get { return this.volume; } }

At this stage, you are most likey asking "why?" and probably for good reason. I mean, you have the following case covered:

• Immutability - You've exposed the fields but they are readonly

Why would you bother adding more code when the current solution gives you the same solution for less...or does it?

The code may be functionally equivalent, but we there are more things to be considered. The struct is public. This means that you are allowing other developers to interact/use/inspect and interface to your value type.

Jon Skeet states the following:


For every type you write, you should consider its interface to the
rest of the world

You see, by exposing your fields, you are exposing the implementation of your struct, and I would recommend against this.

Jon goes on to say:


A property communicates the idea of "I will make a value available to
you, or accept a value from you." It's not an implementation concept,
it's an interface concept.

Currently, you are violating the design principle of hiding the implementation and for this reason I would also argue in favouring the property/field combo. I've linked the article quotes at the bottom, it's a good read and should give you some interesting points to consider.

Having looked at you use of the struct, it is important to note that you only use the fields within the struct itself. Now, as these fields will only be given once (during the constructor), I would build the contents of the ToString method there and then as @dreza pointed out.

Back to your use case

I would say you are right in choosing to use a struct. Your struct only contains two fields and is embedded inside another object. See more detail here.

Briefly back to the first two points:

Memory footprint

Yup, you've got this covered.

Optimisation

One key thing that should be pointed out is that your value type should implement IEquatable. I would always do this.
This way, you avoid the cost of boxing/unboxing your value type upon any equivalence comparison.

StackExchange Code Review Q#74415, answer score: 7