7
\$\begingroup\$

I'm working off a specification that says a bunch of fields within a record (one record being one line) must be blank. For example, it says that chars 6-14 must be blank. I am building a class hierarchy structure that represents the layout of the file, and within this class hierarchy I'm including every field for each type of record, including the blank ones.

Basically I want to know if I should make each field return a blank literal, or if I should pad string.Empty with spaces.

Example:

public string ImmediateDestinationRoutingNumber { get { return "        "; } }

Versus

public string ImmediateDestinationRoutingNumber { get { return string.Empty.PadRight(8, ' '); } }

The second option of padding string.Empty looks the most readable to me, but I don't know if the runtime cost is significant enough to worry about.

\$\endgroup\$
  • \$\begingroup\$ If there's a new error, ask about it on SO if needed. This isn't the place for fixing it. \$\endgroup\$ – Jamal Jan 29 '15 at 14:10
  • \$\begingroup\$ Note that FileHandlers makes this very easy. \$\endgroup\$ – BCdotWEB Jan 29 '15 at 15:16
11
\$\begingroup\$

Instead of returning a " " or string.Empty.PadRight(8, ' ') you should extract the values to some meaningful constants.

The easiest way would be for your example to just use the overloaded constructor of the String class like

private const string eightSpaces = new string(' ', 8);  

this makes it clear for Mr./Mrs.Maintainer what it stands for. But because this seems to produce CS0133: The expression being assigned to <constant> must be constant. you should just use a readonly string like

private readonly string eightSpaces = new string(' ', 8);  

Using your first version will lead for each bug which needs to be solved to count the spaces which are returned.

The name of this property is also poorly named. It doesn't represent an ImmediateDestinationRoutingNumber but instead ImmediateDestinationRoutingNumberSpaces.

\$\endgroup\$
  • \$\begingroup\$ Ah, I wasn't aware of that constructor, thank you. I'm mapping the names 1:1 with the specification, hence the name. \$\endgroup\$ – ldam Jan 27 '15 at 14:47
  • 4
    \$\begingroup\$ I don't think the constructor was the point. It's more the fact that it's pretty much a hard-coded string in your OP, at least this way it's a constant - kept consistent and independent from the property. \$\endgroup\$ – JᴀʏMᴇᴇ Jan 27 '15 at 15:18
  • \$\begingroup\$ Another option might be to create a (private, static) Spaces(int numSpaces) function that uses the demonstrated string(char, int) constructor to return a string with the desired number of spaces and implement the property as public string ImmediateDestinationRoutingNumber { get { return Spaces(8); } }. This would, IMHO, improve readability even more. Also it would be more resistant to future (spec) changes; when the spec changes to require, say, 9 spaces for the field chances are you'd end up with a private const string eightSpaces = new string(' ', 9); kinda situation. \$\endgroup\$ – RobIII Jan 27 '15 at 18:13
  • \$\begingroup\$ I disagree with the last sentence of this answer. Adding "Spaces" to the end does not improve the name. I would either leave the name as is, or consider a name such as EmptyImmediateDestinationRoutingNumber. \$\endgroup\$ – ANeves Jan 27 '15 at 18:36
  • \$\begingroup\$ I'm getting CS0133 trying to compile with constants and the new string() method: The expression being assigned to <constant> must be constant. \$\endgroup\$ – ldam Jan 29 '15 at 11:00
5
\$\begingroup\$

As I commented earlier; I would opt for something like this:

class MyClass
{
    public string ImmediateDestinationRoutingNumber { get { return Spaces(8); } }

    private static string Spaces(int numSpaces)
    {
        return new string(' ', numSpaces);
    }
}

Aside the, agreeable, not terrificly named Spaces() method and aside the ImmediateDestinationRoutingNumber(Spaces) naming as mentioned by Heslacher I think this would offer the following advantages:

  • Best(?) Better readability
  • Refactor-proof; Should you opt for the eightSpaces constant, as mentioned by Heslacher, there's a chance Mr./Mrs.Maintainer will 'fix' any changes in the spec (assume the field has now 9 spaces per the new spec as opposed to the old 8 spaces in the previous spec) as follows:

private const string eightSpaces = new string(' ', 9);

Ofcourse this should never happen, but it won't be the first time a maintainer "quick fixes" an issue in this way (TDWTF is full of these examples).

Also, assuming for a second that there are X more properties that need to return spaces: you would need to introduce either a constant per property or a constant per number-of-desired-spaces, which, to me, are both not desireable solutions.

\$\endgroup\$
  • \$\begingroup\$ I agree with you completely on that last point, I don't want to have a constant for every single field in the spec since that effectively adds another layer of abstraction that any maintainer has to follow through. I like your idea of the Spaces method, but I'm afraid that this will affect runtime performance (or am I overoptimizing?) \$\endgroup\$ – ldam Jan 28 '15 at 12:48
  • 1
    \$\begingroup\$ You are completely, almost ridiculously, over-optimizing. \$\endgroup\$ – Bryan Boettcher Jan 28 '15 at 15:18
  • \$\begingroup\$ Altough I mostly agree with @insta; there might be cases where it could be worth using constants. In tight loops with millions of executions of Spaces() a const will outperform my suggestion; however: that is only to be determined by a profiler and examining your use-case and then you maybe start optimizing. Until then; I'd prefer my solution because maintainability and readability is worth a lot. \$\endgroup\$ – RobIII Jan 28 '15 at 21:24
  • \$\begingroup\$ Right, thanks, I tend to do that and need a sanity check now and then. Thankfully I'm not dealing with loops so I should be good. \$\endgroup\$ – ldam Jan 29 '15 at 12:40
  • \$\begingroup\$ @RobIII: even then, whatever mechanism he's sending this data to (disk, network, or printer) is going to be much slower than string instantiation -- but I see where you're coming from. "Profile it" is the biggest thing, as you said :) \$\endgroup\$ – Bryan Boettcher Jan 29 '15 at 15:51

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.