Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
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.

share|improve this question
    
What will this code do if given foo.pdf.exe? –  Jay Bazuzi Oct 26 '12 at 19:23
add comment

6 Answers

up vote 5 down vote accepted

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";
}
share|improve this answer
    
It also would be better if you wrap the statements after return into another method: return hasValidArgumentAndExtensions(de, temp); –  Alvin Oct 26 '12 at 21:34
add comment

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);
}
share|improve this answer
    
getting error here "\.exe$" for the decimal –  tereško Oct 26 '12 at 13:09
2  
you may want to try to escape the backslash by using another backslash: kelk\\.exe$ –  fjdumont Oct 26 '12 at 17:27
add comment

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);
share|improve this answer
1  
You might want to add a code example of what you are suggesting. –  Jeff Vanzella Oct 26 '12 at 15:52
add comment
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
share|improve this answer
    
if the whole executable name is variable yours is of course better than mine –  fix_likes_coding Oct 26 '12 at 12:51
    
@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"'. –  Paul Ruane Oct 26 '12 at 13:00
add comment

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;
}
share|improve this answer
add comment

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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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