Basic volume issue representation

Question

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;
    }
}

Answer 1

I don't see anything that stands out in this. It's fairly small and quite compact. The only thing I might suggest is changing the public fields to properties.

public string Volume { get; private set; }

public string Issue { get; private set; }

This will ensure your implementation details of Volume and Issue are hidden as mentioned by @Hosch250.

Also, as this is an immutable struct both in it's definition of struct being a value type and the fact that you are only using these fields in the ToString(). Hence you could consider building the ToString() field up in the constructor thus saving you the task of checking on every ToString() method.

You might end up with something along the lines of:

public struct VolumeIssue
{
    public string Volume { get; private set; }

    public string Issue { get; private set; }

    private readonly string description;

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

        description = BuildDescription();
    }

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

    private string BuildDescription() 
    {
        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();
    }
}

Answer 2

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.

Answer 3

This is pretty good. However, in this case, I would probably use a class instead of a struct:

public class VolumeIssue { ... }

Then, I would change these to private:

private string Volume;
private string Issue;

This ensures they cannot be accessed except by functions within the class. Also, while readonly is perfectly fine, you don't really need that now because they cannot be accessed except by functions within the class. Also, if you wish, you could provide a function to change the values, and only that function will be allowed to do so. To be able to access these values, you could do something like this:

public string GetVolume()
{
    return Volume;
}

public string GetIssue()
{
    return Issue;
}