6
\$\begingroup\$

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;
    }
}
\$\endgroup\$
4
\$\begingroup\$

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();
    }
}
\$\endgroup\$
  • \$\begingroup\$ I am not sure what would be gained by that? Doesn't a field have a smaller footprint? \$\endgroup\$ – Mord Zuber Dec 21 '14 at 19:25
  • \$\begingroup\$ @Max I'm not sure on that, but as properties are sort of methods themselves and just contain backing fields under the hood, I don't see how the footprint size would matter. Unless ultimate optimization was your main goal? \$\endgroup\$ – dreza Dec 21 '14 at 19:32
  • \$\begingroup\$ Optimization was part of the goal, hence the choice of a struct over a class. Having only two methods plus a field supplied when using a property instead of simply one field sounds a little overdone to me \$\endgroup\$ – Mord Zuber Dec 21 '14 at 19:35
  • \$\begingroup\$ The idea of building the description in the ctor is great idea! Thanks! \$\endgroup\$ – Mord Zuber Dec 21 '14 at 19:37
  • \$\begingroup\$ @Max quick google search tells me properties take up no more room than fields. However structs do take up a smaller memory footprint (by the size of the bytes it takes to store the class reference). However classes are faster when being passed between methods. Hence it depends on what optimizations you are looking for I guess. \$\endgroup\$ – dreza Dec 21 '14 at 19:40
7
\$\begingroup\$

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.

\$\endgroup\$
5
\$\begingroup\$

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;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I don't think this produces quite the same result. Especially the change in ToString() which will not append anything even if Volume is not null and Issue is. \$\endgroup\$ – dreza Dec 21 '14 at 19:20
  • \$\begingroup\$ @dreza is actually correct. It is possible for there to be a volume without an issue. In which case the original code would produce v. 1 (which is the desired text), while your suggestion gives: v. 1 iss. \$\endgroup\$ – Mord Zuber Dec 21 '14 at 19:27
  • \$\begingroup\$ I've updated the question to add some more context \$\endgroup\$ – Mord Zuber Dec 21 '14 at 19:32
  • \$\begingroup\$ @Hosch250 I'm still not sure if that's correct. Now when Volume is null you might still append an issue, which isn't the same as the OP's. \$\endgroup\$ – dreza Dec 21 '14 at 19:34

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.