Am I missing something, or is this code nonsensical?

Question

While exploring the dank catacombs and dusty dungeons of our legacy code, I came across this:

FormatString formatString = new FormatString();
if (formatString.containsAlpha(UPCE) != -1) 
{
    UPCLen = 11;
}

Am I missing something, or is my reaction to this, namely: How can formatString contain anything? Nothing has been assigned to it...It will always be -1, assuming that indicates 'not found'" correct?

UPDATE

In answer to the general confusion evident in the comments, I thought FormatString was some haywire stone ages .NET thing (this project uses .NET 1.1), but you're right - this is a homegrown class. Here is the constructor:

public FormatString()
{
}

...and the containsAlpha() method:

public int containsAlpha(string strToCheck)
{
    const string ALPHA_CHARS = "abcdefghijklmnopqrstuvwxyz";
    try
    {
        char[] tmpCharArry = ALPHA_CHARS.ToCharArray();
        return strToCheck.ToLower().IndexOfAny(tmpCharArry);
    }
    catch(Exception ex)
    {
        Duckbill.ExceptionHandler(ex, "FormatString.containsAlpha");
        return 0; // not -1?
    }
}

Now I ask you: Is "FormatString" a wrong-headed name for this class, or what? I found it very misleading (obviously).

Answer 1

For all we know, it could be something like this:

public class FormatString
{
    public int containsAlpha(object foo)
    {
        return 0;
    }
}

That would satisfy the example you showed. It doesn't even need a constructor definition.

You should right-click and "go to definition" to find out what it really does.

UPDATE

Based on your updated details, If you could use a newer .NET Framework, I'd say you could replace the entire function with this:

using System.Linq;

public static bool ContainsAlpha(string s)
{
    return s != null && s.Any(Char.IsLetter);
}

But since you are stuck, this should work just as well:

public static bool ContainsAlpha(string s)
{
    if (s == null) return false;

    for (int i = 0; i < s.Length; i++)
        if (Char.IsLetter(s, i))
            return true;

    return false;
}

Or alternatively:

public static bool ContainsAlpha(string s)
{
    if (s == null) return false;

    for (int i = 0; i < s.Length; i++)
        if (Char.IsLetter(s[i]))
            return true;

    return false;
}

Answer 2

It is possible that FormatString is initialized from some form of configuration. I would check the constructor. That is the only way this makes sense (to me).

Answer 3

Even though it's clearly weird (shouldn't "contains..." return a boolean?) and counter-intuitive (I have created an "empty" FormatString, i.e. nothing passed through its constructor, why should it do anything?), it doesn't strike me as entirely nonsensical.

"containsAlpha" might as well mean: Check if parameter 1 (UPCE in this case) contains alphabetic (probably alphanumeric) characters. So FormatString would be some generic class that checks for basic characteristics of how strings are formatted, i.e. do they contain numbers, letters, special chars etc.

It's also possible that FormatString is some specialized class (check the namespace?) that checks for the format of, e.g. UPCs, and would probably better be called UPCFormat, UPCFormatVerifier etc.

If I'm not completely mistaken, I don't recognize this as a .NET framework function. Which target framework version is this? Do you have the source of FormatString? What does it say?

Anyway, a beautiful piece of strange code. ;-)

Ziffusion's answer also makes sense, look at the construcor code. Perhaps it does more than it should, some initialization either in regard to some default state, or, even worse, something involving static global state stuff, which you never find out unless you run the whole system.

Edit: Now read your edit - as it turns out, I was right that FormatString does "generic string format checking." Probably StringFormat would be a slightly better name, but all in all I'd say: If you want to check the format of UPCs, have either an UPC class with a static factory/builder method which builds an UPC object from a string, including format checks, or create a UPCFormatCheck class which solely does the job of checking specific formats. This way, you avoid all too generic names that could mean anything. (In that class you can obviously use some cool LINQ-oneliner to do the actual check, as suggested.)

Answer 4

Is "FormatString" a wrong-headed name for this class, or what?

In my opinion, yes.
It should be named like StringUtils or StringHelper or something like that.
And also it should be static.

Update
What's more, I'd prefer it to be an extension method. Then it would be like:

string UPCE = // whatever
if(UPCE.ContainsAlpha())
{
 // ...
}