6
\$\begingroup\$

I'm taking on a project from a previous programmer. My goal is to make it a bit smaller and hopefully better. Originally the code has a bunch of length checks and Booleans as shown below.

C#

string[] splitStrArray = SNString.Split('-');
string part1 = splitStrArray[0];
string part2 = splitStrArray[1];
string part3 = splitStrArray[2];

bool part1ok = false;
bool part2ok = false;
bool part3ok = false;

if (part1.Length == 3)
{
    part1ok = true;
}

if (part2.Length == 8)
{
    part2ok = true;
}

if (part3.Length == 3)
{
    part3ok = true;
}

if(part1ok && part2ok && part3ok)
{
    return true;
}
else
{
    return false;
}

Is it a good practice to keep that or should I change it to my following idea?

string[] splitStrArray = SNString.Split('-');
string part1 = splitStrArray[0];
string part2 = splitStrArray[1];
string part3 = splitStrArray[2];

if(part1.Length == 3 && part2.Length == 8 && part3.Length == 3)
{
    return true;
}
else
{
    return false;
}

Basically a barcode is scanned, and a SN is passed to the application. The goal is to make sure the SN scanned is of a format XXX-XXXXXXXX-XXX. Sometimes there are occurrences of SNs that are formatted XXXXXXXXXXXXX or XXX-XXXXXXXXXX not complete or missing a specific dash.

\$\endgroup\$
  • 2
    \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – BCdotWEB Jan 29 '20 at 16:00
  • 10
    \$\begingroup\$ I note that the code is already possibly wrong. Who says that Split returns an array with exactly four elements? If you already know that there are exactly three hyphens in the string, then whatever code ensures that invariant should ensure the other invariants are met. If you do not already know that, then your program can crash. Either way, something seems wrong here. \$\endgroup\$ – Eric Lippert Jan 29 '20 at 23:14
  • 1
    \$\begingroup\$ Exactly three elements, I meant to say. Three regions separated by hyphens. \$\endgroup\$ – Eric Lippert Jan 29 '20 at 23:24
  • 5
    \$\begingroup\$ I'm mildly amused by this being considered "massive"... certainly large in comparison to what it could be, but it's not a thousand-line function like I've seen before. \$\endgroup\$ – DylanSp Jan 30 '20 at 0:44
  • 2
    \$\begingroup\$ This question lacks any indication of what the code is intended to achieve. To help reviewers give you better answers, please add sufficient context to your question, including a title that summarises the purpose of the code. We want to know why much more than how. The more you tell us about what your code is for, the easier it will be for reviewers to help you. The title needs an edit to simply state the task. \$\endgroup\$ – Toby Speight Jan 31 '20 at 8:48
23
\$\begingroup\$

Is it a good practice to keep that

No

or should I change it to my idea

Yours is better but it could be better.

The if..else could just be simplified like

return part1.Length == 3 && part2.Length == 8 && part3.Length == 3;
\$\endgroup\$
  • 5
    \$\begingroup\$ return new Regex(@"\d{3}-\d{8}-\d{3}").IsMatch(SNString);, or if you wanted to match just about any number/letters, the expression would be [a-zA-Z0-9]{3}-[a-zA-Z0-9]{8}-[a-zA-Z0-9]{3}, there's a lot of ways to skin this in a simple way, the OP should be aiming for readability/maintainability. \$\endgroup\$ – Ron Beyer Jan 30 '20 at 20:56
9
\$\begingroup\$

Your idea is definitely better than the original, but I think it could be better still.

Firstly;

if(part1.Length == 3 && part2.Length == 8 && part3.Length == 3)
{
    return true;
}
else
{
    return false;
}

This can be simplified into a single return statement, as follows;

return part1.Length == 3 && part2.Length == 8 && part3.Length == 3;

Secondly, it would probably be better if the length values you are comparing to were constants. It's definitely better if there's ever a chance that the expected length changes (then you know you only need to change one place in the code), but even if you know it will remain constant for the life of the codebase it's a lot easier to read and understand something.Length == XCodeLength than something.Length == 3 and having to know or find out that this is because XCodes always have length 3. (Obviously naming and set up of this are very context dependent...)

Thirdly, do the separate parts mean anything? I.e. is there a more meaningful name that could be used for each bit of the split? (e.g. if these were telephone numbers (which they aren't based on the numbers, but it's the best example I could think of) it'd be a lot more clear if part1 was instead called countryCode (or areaCode - pick your format, but the point is the same)). Naming these well would also link nicely into putting the lengths into constants.

As a final point, you may have a specific code style you need to follow, but it's more normal for C# to use var when possible rather than writing out the type of a variable when it can be inferred (you can still easily see the type in an IDE).

\$\endgroup\$
  • \$\begingroup\$ The input is always in a format of XXX-XXXXXXXX-XXX. The insides dont mean much to me yet \$\endgroup\$ – Travis Jan 29 '20 at 16:49
  • 13
    \$\begingroup\$ @Travis: I am confused. If the input is always in that format then why do you need the function at all? \$\endgroup\$ – Eric Lippert Jan 29 '20 at 23:25
  • 1
    \$\begingroup\$ @EricLippert Perhaps to validate that the input actually matches the format? \$\endgroup\$ – Zev Spitz Jan 30 '20 at 4:47
  • 2
    \$\begingroup\$ @ZevSpitz Eric is not confused; that's just a rhetorical question \$\endgroup\$ – Mr47 Jan 30 '20 at 10:00
  • 7
    \$\begingroup\$ No, I'm genuinely confused. If the input is always in the desired format then there is no need to write a validator that confirms that except, say, as an assertion. Also, as I noted, the original code assumes that the input is definitely in a form that has at least two dashes; it crashes otherwise, indicating that the code has never been tested in an environment where that invariant was violated. It is unclear to me what properties are actually being tested for here, and why the existing code assumes some invariants are met but not others. \$\endgroup\$ – Eric Lippert Jan 30 '20 at 19:19
7
\$\begingroup\$

I know it's answered, but I think the whole converting to char array is not needed.

You can simplify it further to (based on original code):

// SNString?.Length == 16                      ->  Should be exactly 16 characters
// SNString?[3] == '-'                         ->  the first dash should be in index number 3
// SNString?[12] == '-'                        ->  the first dash should be in index number 12
// SNString?.Replace("-", "")?.Length == 14    ->  if there is only 2 dashes, then removing them would result with length == 14, otherwise is false
return SNString?.Length == 16 && SNString?[3] == '-' && SNString?[12] == '-' && SNString?.Replace("-", "")?.Length == 14;

which is equivalent to :

if(SNString?.Length == 16)
{
    if (SNString?[3] == '-' && SNString?[12] == '-' && SNString?.Replace("-", "")?.Length == 14)
        return true;
}

return false;

You should check the length of the provided string, then apply the other validations. The string length will determine if you need to apply the other validations or not. if the string's length is not equal to 16, then surely it's not a valid format. if the string length == 16, then it's a valid format which needs to be validated. Then you just get the position of the first and last dashes. if they're in the correct position, then the first, second, and third parts should have the correct length. Then, you can use Replace to replace all dashes with empty string, and check the result length, if the result is 14, then there were only two dashes, which is valid, otherwise is not valid.

Now, you can validate three parts XXX-XXXXXXXX-XXX, if your characters group are all numbers (without the dashes), then you can add something like ulong.TryParse(SNString?.Replace("-", string.Empty), out ulong result), if true, then they're all numbers, otherwise it's false.

Doing that, would avoid converting it to char array.

There is one thing that you should consider, if there is certain format for each characters group, you might need to use Regex instead. For instance, if the first part should always contains letters only, or one digit with two letters ..etc. then the current validation process will provide a non-valid string. Also, you might consider adding a validation of English letters and Digits to avoid having some non-English inputs.

here is Regex example (for future reference) :

System.Text.RegularExpressions.Regex.Match(SNString, @"^[^-]{3}-[^-]{8}-[^-]{3}$");

With this, you'll have the flexablity to adjust the regex pattern to match the conditions you need.

\$\endgroup\$
  • 1
    \$\begingroup\$ @Travis ? is null conditional operator. when you use SNString?.Length you're saying ` if SNString != null then, return length, otherwise, return null. and null == 3 is false. also, it'll not throw a null exception. and since it's in numbers format, you'll need to use regex solution and adjust the pattern to accept only numbers, this would be the simplest approach. \$\endgroup\$ – iSR5 Jan 29 '20 at 20:23
  • 7
    \$\begingroup\$ The first solution is not correct, insofar as it produces a different result for some inputs than the original. \$\endgroup\$ – Eric Lippert Jan 29 '20 at 23:15
  • 3
    \$\begingroup\$ Moreover, I do not understand the last clause of the first solution. Surely if the string is already known to be of length 16 by the first clause, then the length of the substring starting at character 13 and going to the end is always of length 3. How can the last clause be false? \$\endgroup\$ – Eric Lippert Jan 29 '20 at 23:17
  • 2
    \$\begingroup\$ Your second solution, by contrast, is better than the original poster's solution, as it gives correct results for a larger class of strings than the original solution. \$\endgroup\$ – Eric Lippert Jan 29 '20 at 23:18
  • 7
    \$\begingroup\$ The first solution will also return true when you have e.g. "---------------" as an input. \$\endgroup\$ – SomeBody Jan 30 '20 at 7:51
5
\$\begingroup\$

The original code includes .Split('-') which makes an array and at least three new strings; the other answers include invoking the regex engine, or making new substrings. I wanted to write a version which does little more work than walking the string once, checking the indices. I don't suggest you should use it, but maybe if you like premature optimization

String SNString = "XXX-XXXXXXXX-XXX";

// Validate the string pattern without copying it.

return ((SNString.Length == 16)
     && (SNString.IndexOf('-'   0) ==  3)  // part1 must be length 3 for this to hold.
     && (SNString.IndexOf('-',  4) == 12)  // part2 must be length 12-4 == 8 for this.
     && (SNString.IndexOf('-', 13) == -1));// part3 ends length 16 string. 16-13 == 3.

I do like the readability of the original version more than the &&&&&&&& infested waters of the other options; you can keep a lot of that simplicity and shorten it down a lot, with something like this:

string[] parts = SNString.Split('-');

if (parts[0].Length != 3) {
    return false;
}

if (parts[1].Length != 8) {
    return false;
}

if (parts[2].Length != 3) {
    return false;
}

return true;

NB. this has the same risk of the original and some other answers that there might be more than three parts after splitting.

\$\endgroup\$
  • 1
    \$\begingroup\$ In the general case, the, "escape out quickly on any failure" approach is great. In the OP's specific code, using a single return statement is both simpler and more readable. \$\endgroup\$ – Brian Jan 30 '20 at 14:10
  • \$\begingroup\$ @Brian yeah there's not much in it - the single line answer is deceptive, as it still depends on doing a Split() and then assigning three parts of the array to three new string variables, so it ends up longer and less obviously correct from that. And if you inline the checks to parts[1].Length == 3 && then the added bracket indexing in one line starts to get a bit dense for my liking. It's A && B || C && !D style code which makes me unhappy - so hard to verify by eye that the tests do exactly what was intended in every case, but == && == && == could be simple enough. \$\endgroup\$ – TessellatingHeckler Jan 31 '20 at 1:04
1
\$\begingroup\$

Filter validation

For validation code, you could use a more compact but somewhat verbose pattern of failing early, failing fast.

{
    // Test for: XXX-XXXXXXXX-XXX

    var parts = SNString.Split( '-' );

    if ( parts.Length != 3 )
        return false;
    if ( parts[ 0 ].Length != 3 )
        return false;
    if ( parts[ 1 ].Length != 8 )
        return false;
    if ( parts[ 2 ].Length != 3 )
        return false;

    return true;
}

The function will have a lot a exit points, but only a "success" result, at the end. Think the validation process as a filter, what only let pass correct data.

I may nitpick this code is only testing what:

{
    // "Mask" test for: XXX-XXXXXXXX-XXX

    var hyphens = 0;
    for ( int pos = 0 ; pos < SNString.Length ; pos++ )
        if ( SNString[ pos ] == '-' )
            hyphens++;

    if ( hyphens == 2 && SNString.Length == 16 && SNString[ 3 ] == '-' && SNString[ 12 ] == '-' )
        return true;

    return false;
}

No string splits, no allocations, only validations in place.

\$\endgroup\$
  • 1
    \$\begingroup\$ With X-X-XXXXXXXX-XXX the .Split() code would fail it, and your second code would pass it. \$\endgroup\$ – TessellatingHeckler Jan 30 '20 at 23:15
  • \$\begingroup\$ Yes. I will fix that. \$\endgroup\$ – André LFS Bacci Jan 31 '20 at 15:07
1
\$\begingroup\$
string[] splitStrArray = SNString.Split('-');
int[] expectedLengths = { 3, 8, 3 };

if( splitStrArray.length != 3 ){
    return false;
}

for( int i = 0; i < expectedLengths; ++i ){
    int expectedLength = expectedLengths[i];
    int actualLength = splitStrArray[i].Length;

    if( expectedLength != actualLength ){
        return false;
    }
}

return true;

I prefer loops, early exits and an "innocent until proven guilty" approach.

With regards to making code simple and generic, the original code is terrible. Explicitly naming values in an array like that, then doing the same operation to them all is bad practise. Reasons being:

  • It's awkward to maintain
  • It's not scalable
  • It just causes code to become longer than needed

It might be overkill in this situation, but generalised code that don't have names like "part1, part2, part3" is just way easier for me to read and get to grips with.

Hope this helps!

\$\endgroup\$
  • \$\begingroup\$ Like many of the other answers to this question, this code also crashes when given bad input. What does your function do when handed, say, the empty string? \$\endgroup\$ – Eric Lippert Jan 31 '20 at 16:07
  • \$\begingroup\$ Ah, good catch there mate. I was just trying to refactor the existing functionality, rather than take any additional scenarios into account. \$\endgroup\$ – Jhal Feb 2 '20 at 16:16

Not the answer you're looking for? Browse other questions tagged or ask your own question.