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.

Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Jan 27, 2015 at 14:37
\$\endgroup\$
2
  • \$\begingroup\$ If there's a new error, ask about it on SO if needed. This isn't the place for fixing it. \$\endgroup\$ Commented Jan 29, 2015 at 14:10
  • \$\begingroup\$ Note that FileHandlers makes this very easy. \$\endgroup\$ Commented Jan 29, 2015 at 15:16

2 Answers 2

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.

answered Jan 27, 2015 at 14:45
\$\endgroup\$
7
  • \$\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\$ Commented Jan 27, 2015 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\$ Commented Jan 27, 2015 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\$ Commented Jan 27, 2015 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\$ Commented Jan 27, 2015 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\$ Commented Jan 29, 2015 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.

answered Jan 27, 2015 at 18:20
\$\endgroup\$
6
  • \$\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\$ Commented Jan 28, 2015 at 12:48
  • 1
    \$\begingroup\$ You are completely, almost ridiculously, over-optimizing. \$\endgroup\$ Commented Jan 28, 2015 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\$ Commented Jan 28, 2015 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\$ Commented Jan 29, 2015 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\$ Commented Jan 29, 2015 at 15:51

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.