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.
-
\$\begingroup\$ If there's a new error, ask about it on SO if needed. This isn't the place for fixing it. \$\endgroup\$Jamal– Jamal2015年01月29日 14:10:40 +00:00Commented Jan 29, 2015 at 14:10
-
\$\begingroup\$ Note that FileHandlers makes this very easy. \$\endgroup\$BCdotWEB– BCdotWEB2015年01月29日 15:16:21 +00:00Commented Jan 29, 2015 at 15:16
2 Answers 2
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
.
-
\$\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– ldam2015年01月27日 14:47:59 +00:00Commented 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\$JᴀʏMᴇᴇ– JᴀʏMᴇᴇ2015年01月27日 15:18:43 +00:00Commented Jan 27, 2015 at 15:18
-
\$\begingroup\$ Another option might be to create a (private, static)
Spaces(int numSpaces)
function that uses the demonstratedstring(char, int)
constructor to return a string with the desired number of spaces and implement the property aspublic 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 aprivate const string eightSpaces = new string(' ', 9);
kinda situation. \$\endgroup\$RobIII– RobIII2015年01月27日 18:13:05 +00:00Commented 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\$ANeves– ANeves2015年01月27日 18:36:55 +00:00Commented 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\$ldam– ldam2015年01月29日 11:00:06 +00:00Commented Jan 29, 2015 at 11:00
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.
-
\$\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– ldam2015年01月28日 12:48:33 +00:00Commented Jan 28, 2015 at 12:48 -
1\$\begingroup\$ You are completely, almost ridiculously, over-optimizing. \$\endgroup\$Bryan B– Bryan B2015年01月28日 15:18:28 +00:00Commented 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\$RobIII– RobIII2015年01月28日 21:24:18 +00:00Commented 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\$ldam– ldam2015年01月29日 12:40:04 +00:00Commented 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\$Bryan B– Bryan B2015年01月29日 15:51:57 +00:00Commented Jan 29, 2015 at 15:51