6
\$\begingroup\$
String Name = "yyyy MM DD abjwg kelk.exe"

public static bool IsNameRight(string Name)
{
    string[] temp = Name.Split(' ');
    string[] de= Name.Split('.');

    if (de[1] == "pdf" && temp .Length == 5)
    {
        if (temp [0].Length == 4
            && temp [1].Length == 2 
            && temp [3].Length == 2
            && temp [4] == "abjwg" 
            && temp [5] == "kelk.exe")
            return true;
        else
            return false;
    }
    return false;
}

I am using .NET 3.5 and C#

EDIT

You could use Regular expressions but I have never used it or know how to use it, and don't know if it is a good practice.

\$\endgroup\$
  • 1
    \$\begingroup\$ What will this code do if given foo.pdf.exe? \$\endgroup\$ – Jay Bazuzi Oct 26 '12 at 19:23
7
\$\begingroup\$

The method name would be better called IsNameValid and the parameter name should be camel cased.

public static bool IsNameValid(string name)
{
    // firstly always validate parameters, either throw an exception or return false.
    if (name == null) { throw new ArgumentNullException("name"); }

    // use more descriptive variable names, what does 'de' mean?
    string[] temp = name.Split(' ');
    string[] de= name.Split('.');

    // check that the arrays contain the correct number of values or you will get an exception accessing the indexer.
    if (de.Lengh < 2 || temp.Length < 5) { return false; }

    return de[1] == "pdf"
            && temp[0].Length == 4
            && temp[1].Length == 2 
            && temp[2].Length == 2
            && temp[3] == "abjwg" 
            && temp[4] == "kelk.exe";
}
\$\endgroup\$
  • \$\begingroup\$ It also would be better if you wrap the statements after return into another method: return hasValidArgumentAndExtensions(de, temp); \$\endgroup\$ – Alvin Oct 26 '12 at 21:34
15
\$\begingroup\$

As you wanted to use RegEx I have a recommendation for you:

public static bool IsValidName(string Name)
{
    if(String.IsNullOrEmpty(Name))
        return new ArgumentNullException();

    string pattern = @"^[0-9]{4} [0-9]{2} [0-9]{2} abjwg kelk\\.exe$";
    return Regex.IsMatch(Name, pattern);
}
\$\endgroup\$
  • \$\begingroup\$ getting error here "\.exe$" for the decimal \$\endgroup\$ – tereško Oct 26 '12 at 13:09
  • 2
    \$\begingroup\$ you may want to try to escape the backslash by using another backslash: kelk\\.exe$ \$\endgroup\$ – fjdumont Oct 26 '12 at 17:27
10
\$\begingroup\$

If you are trying to get the extension of a file, I would use

System.Io.Path.GetExtension

Update

Since de seem to only be used for getting the extension.

string[] de= Name.Split('.');

could be changed to

string de = System.Io.Path.GetExtension(Name);
\$\endgroup\$
  • 1
    \$\begingroup\$ You might want to add a code example of what you are suggesting. \$\endgroup\$ – Jeff Vanzella Oct 26 '12 at 15:52
8
\$\begingroup\$
private static readonly Regex pattern = new Regex(@"[0-9]{4} [0-9]{2} [0-9]{2} [A-Za-z0-9]{5} [A-Za-z0-9]{4}\.exe");

public static bool IsNameRight(string name)
{
    return pattern.IsMatch(name);
}
  • [0-9] means a character which is a digit between 0 and 9.
  • [A-Za-z0-9] means a character which is between A and Z (uppercase) or a and z (lowercase) or a digit between 0-9.
  • {4} exactly four times
\$\endgroup\$
  • \$\begingroup\$ if the whole executable name is variable yours is of course better than mine \$\endgroup\$ – fixagon Oct 26 '12 at 12:51
  • \$\begingroup\$ @fix_likes_coding: I believe it is based upon 'As I want to confirm that my string is in this format "yyyy MM dd ABCDE FGHI.EXE"'. \$\endgroup\$ – Paul Ruane Oct 26 '12 at 13:00
2
\$\begingroup\$

I can think of at least few things with the code. Skip some of the redundant else and if clasues.

String name = "yyyy MM DD abjwg kelk.exe"

public static bool IsNameRight(string Name)
{
    string[] temp = Name.Split(' ');
    string[] de = Name.Split('.');

    if (de[1] != "pdf" && temp.Length != 5)
      return false;

  if (temp [0].Length == 4
    && temp [1].Length == 2 
    && temp [3].Length == 2
    && temp [4] == "abjwg" 
    && temp [5] == "kelk.exe")
    return true;
  return false;
}
\$\endgroup\$
2
\$\begingroup\$

Here's another way of improving your code:

String name = "yyyy MM DD abjwg kelk.exe"

public static bool isNameRight( string name )
{
    string[] de   = name.Split( "." );
    string[] temp = de[0].Split( " " );

    if ( ! ( de[1] == "pdf" && temp.Length == 5 ) )
        return false;

    return ( temp[0].Length == 4
                && temp[1].Length == 2 
                && temp[2].Length == 2
                && temp[3] == "abjwg" 
                && temp[4] == "kelk" );
}

I know this isn't about finding and fixing bugs, but it seemed important to note that given your name string and your function logic, it will always return false, because you're testing for temp [5] == "kelk.exe" after de[1] == "pdf" so in this case, if de[1] equals "pdf" then temp[5] will never have the .exe part, it would have .pdf. That's also why I removed the extension from temp[4] value.

Additionally, I would use @the_lotus way of dealing with the file extension.

I hope you find this helpful.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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