Minimizing string length checker in C# + format(ish) function [closed]

Question

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.

Answer 1

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;

Answer 2

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).

Answer 3

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.

Answer 4

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.

Answer 5

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.

Answer 6

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!