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.

Here is my code, I use it to verify whether an input path is correct or not. I assume that the input path is encoded in UTF8. Is there any way to make it better?

#define MAX_PATH_LEN MAX_PATH

#define IN
#define OUT

typedef int Bool_T;

static
int GetCharsNumInPath( IN const char * path )
{
    int cnt;

    int i;
    int n = strlen( path );

    for( i = 0; i < n; i++ )
    {
        if( ( path[i] < 0x80 ) || ( path[i] > 0xbf ) )
            cnt++;
    }

    return cnt;
}


Bool_T PathValid( IN const char * path )
{
    if( path != NULL )
    {
        if( GetCharsNumInPath( IN path ) <= MAX_PATH_LEN )
        {
            int i;
            int n = strlen( path );

            for( i = 0; i < n; i++ )
            {
                switch( path[i] )
                {
                    // ? " / < > * |
                    // these characters can not be used in file or folder names
                    //
                    case '?':
                    case '\"':
                    case '/':
                    case '<':
                    case '>':
                    case '*':
                    case '|':
                        return false;

                    // Can meet only between a local disk letter and full path
                    // for example D:\folder\file.txt
                    //
                    case ':':
                    {
                        if( i != 1 )
                        {
                            return false;

                        }else{

                            break;
                        }
                    }

                    // Space and point can not be the last character of a file or folder names
                    //
                    case ' ':
                    case '.':
                    {
                        if( ( i + 1 == n ) || ( path[i+1] == PATH_SEPERATOR_CHAR ) )
                        {
                            return false;

                        }else{

                            break;
                        }
                    }

                    // two backslashes can not go straight
                    //
                    case PATH_SEPERATOR_CHAR:
                    {
                        if( i > 0 && path[i - 1] == PATH_SEPERATOR_CHAR )
                        {
                            return false;

                        }else{

                            break;
                        }
                    }
                }
            }

            return true;

        }else{ // if( GetCharsNumInPath( IN path ) <= MAX_PATH_LEN )

            LOG_ERROR( "PathValid FAILURE --> path is too long" );
            return false;
        }

    }else{ // if( path != NULL )

        // wrong argument
        //
        return false;
    }
}
share|improve this question

2 Answers 2

up vote 3 down vote accepted

Personally if the preconditions to the function are not met then I like to return quickly and this not add to the nesting:

Bool_T PathValid( IN const char * path )
{
    if( path != NULL )
    {
        // wrong argument
        return false;
    }

    if( GetCharsNumInPath( IN path ) <= MAX_PATH_LEN )
    {
        LOG_ERROR( "PathValid FAILURE --> path is too long" );
        return false;
    }

    // Main Function body here.
}

Of course in C you don't want to have too many return points from a function (as at each return point you need to close all open resources and tidy up. So in C (unlike C++) I find my code looking like this:

<type>   function(<Parameters>)
{
      if (!<pre-condition1>)
      {    return <errorStat1>;
      }

      if (!<pre-condition2>)
      {    return <errorStat2>;
      }

      ....

      if (!<pre-condition-N>)
      {    return <errorStat-N>;
      }


      <type>   result = <initial Result>;
      <Set all Resource to NULL>

      // Code Which can generate errors>
      // If it does then set error state. and stop processing.
      // BUT DO NOT RETURN from this code.

      < Close all resource >
      return result;
}

Your check for invalid characters is Windows specific. The OS will usually have a platform or OS specific set of functions for checking valid characters.

  • Your set of invalid characters is not true for all platforms.
  • The use of ':' as the second character is Windows only.
    • If it is there the first character should be 'A' - 'Z'
  • Limiting the use of space is Windows only
  • Are you sure about "." not being allowed as last character?
  • On windows two '\\' can be used as the first two characters to indicate network resource.
  • Windows has more than one path separator.
    • The old style '\' (traditional backslash)
    • The new style '/' (normal slash).

In your function counting characters in the path. You basically traverse the string twice. Why not combine the two traversals into a single pass:

// int n = strlen( path );
// Not requires (all this does is look for '\0')

for( i = 0; path[i]; i++ )
{       //  ^^^^^^^  Test for '\0'  character

    if( ( path[i] < 0x80 ) || ( path[i] > 0xbf ) )
    {    cnt++;
    }
}
share|improve this answer
1  
Regarding the multiple-returns and resource disposal, that would be a great opportunity to use the "dreaded" goto statement to handle all that. –  Jeff Mercado Aug 19 '11 at 20:05
    
@Jeff Mercado: On my last project that was actually the standard. But I just found it made the code more convoluted to read. Sorry but I can not even recommend it (goto) for this situation (It also one of the reason I left that team). –  Loki Astari Aug 19 '11 at 20:10
    
At least IMO, this is one place that C++ (exception handling, to be specific) provides a major improvement. –  Jerry Coffin Aug 19 '11 at 20:54
    
@Martin, about traversing 2 times when counting num of characters good point, thanks). I'll edit, add some new checking and paste the second version of this function. –  akmal Aug 20 '11 at 4:35
    
@Tux-D - Your user name is "Tux" and you left a team for the use of goto error handling? Better not look at those kernel sources... :-) –  asveikau Sep 5 '11 at 19:47

Recognizing a path correctly is a bit more complex than you seem to realize (not surprising -- it's more complex than almost anybody seems to realize -- even quite a bit of Microsoft's software gets at least a few things wrong). I think I'd start by defining a grammar for what you're going to recognize. Since this seems to be based on Windows paths, I'd start with something like this:

path = local_path | share_path

local_path = <drive> FSpath

drive = [A-Za-z] ":"

share_path = "\\" share_name absolute_path

FSpath = relative_path | absolute_path

absolute_path = path_sep relative_path

relative_path = { name path_sep } file_name

path_sep = [\/]

share_name = name

name = [^?<>|\*:]+

file_name = name <fork_name>

fork_name = ":" name

Here I've used <whatever> to mean that item is optional and {whatever} to mean it can be repeated arbitrarily. [Edit: also be aware that I just typed this in based on what I remember -- I could easily have missed something so it's even a bit more complex than I've shown.]

Once you have that, it's pretty easy to write a small function for each of these to recognize that individual piece. To deal with optional parts of the path, I think I'd follow the convention of the function returning either the number of characters it matched from the input string, or else a pointer to the spot in the input string after the part it matched. Starting from the top, they'd look like:

bool path(char const *input) { 
    return local_path(input) || share_path(input);
}

size_t local_path(char const *input) { 
    size_t drive_len = drive(input);
    return drive_len + FSpath(input+drive_len);
}

size_t drive(char const *input) { 
    if (isalpha(input[0]) && input[1] == ':")
        return 2;
    return 0;
}

This style will probably result in more total code (especially overhead for function headers and such) but it breaks the problem down into pieces that are each small and simple so it should be fairly easy to figure out what's happening at each step of the way, so if (for example) my understanding of the overall syntax for a path is wrong, it should be fairly easy to find exactly which code relates to the part I have wrong. Likewise, when/if Microsoft changes the rules to add or remove restrictions, it should be quite easy to do that and be sure you're getting the right result. Just for example, right now I have share_name defined as being exactly the same as any other directory/file name. In all honesty, I'm not sure that's really correct. A share name may be more restricted than a normal directory name, but I'm honestly not sure -- but if it is different, this style makes it really easy to find and correct. This also makes it easy to deal with things that are only allowed at specific points along the path, or have different rules at different points (e.g., ':' being used for two different purposes: preceded by a drive letter, or followed by a named file fork on NTFS).

share|improve this answer
    
According to this A:B:C:D:\Plop is a valid path. Need a slight tweak there. –  Loki Astari Aug 19 '11 at 20:04
    
In your comments: [Whatever] means optional, but here [^?<>|\*:] you are using it to mean the RE any one of these –  Loki Astari Aug 19 '11 at 20:06
    
+1: For pointing out that paths are not obviously trivial. –  Loki Astari Aug 19 '11 at 20:07
    
@Martin: Oops -- I defined path twice as two different things, and used [] twice to mean to different things as well. I'll edit. –  Jerry Coffin Aug 19 '11 at 20:20
    
@Jerry Coffin, the path will come to my functions from the application explorer, so I think it is not so important to check all this things. The main purpose of this function is to check is the length of path is ok, and is file/folder name is correct, that's all. So what you suggest me to do? –  akmal Aug 20 '11 at 7:29

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.