8
\$\begingroup\$

I have been trying to write a sample code block to remove multiple backward or forward slashes from a path (e.g. //a//b should be /a/b; on Windows, c:\\a\\b\\\c should be c:\a\b\c).

I am assuming that:

  1. Code must be platform independent
  2. Should work in all cases (or report a proper error)
  3. It should be readable and maintainable
  4. It shouldn't be over-complex and shouldn't have errors

Please provide point-wise comments for the following code sample (you may ignore main and assume that path is not NULL - it is just for demonstration purposes). Or else you may provide a better mechanism for the same.

    void
    die( int error_code, char* message )
    {
        fprintf( stderr, message );
        exit( error_code );
    }

    /*Following function removes duplicate slashes from a given path*/
    char*
    validate_path ( char* path )
    {
        char* path_copy = path;
        int dup_flag = 0;
        char* path_without_dup_slash = (char*)malloc(strlen(path)+1);
        if ( path_without_dup_slash == NULL ) {
             die( EXIT_FAILURE, "Failed to allocate memory for processing path.");
        }
        char* copy_pwds = path_without_dup_slash;
        /* Travel through given path */
        while( *path_copy != '\0' ) {
              /* If there is '\' or '/' then copy a single '/'
               * , ignore others until a real char comes */
               if ( (( *path_copy == '\\') || ( *path_copy == '/' )) && 
                    (dup_flag == 0) ) {
                    *copy_pwds = *path_copy;
                    dup_flag = 1;
                    copy_pwds++;
                }else if (( *path_copy != '\\') && ( *path_copy != '/' ))  {
                    *copy_pwds = *path_copy;
                    dup_flag = 0;
                    copy_pwds++;
                }
                path_copy++;
         }
         *copy_pwds = '\0';
         path_without_dup_slash = (char *)realloc( path_without_dup_slash, 
                                  strlen( path_without_dup_slash ) + 1 );
         if ( path_without_dup_slash == NULL ) {
               die( EXIT_FAILURE, "Failed to allocate memory for processing path.");
         }
         return path_without_dup_slash;
    }
    /*You please ignore the parts written in main - it is just for demonstration purpose. */
    int
    main( void ) {
         char* bad_path_1 = malloc(100);
         char* bad_path_2 = malloc(100);
         char* p1 = bad_path_1;
         char* p2 = bad_path_2;
         char* path_1;
         char* path_2;
         bad_path_1 = "//a//b//c";
         bad_path_2 = "c:\\\\a\\\\b\\\\c";
         path_1 = validate_path(bad_path_1);
         printf( "Good path:%s Bad Path:%s\n",path_1,bad_path_1);
         path_2 = validate_path(bad_path_2);
         printf( "Good path:%s Bad Path:%s\n",path_2,bad_path_2);
         free(path_1);
         free(path_2);
         free(p1);
         free(p2);
         return EXIT_SUCCESS;

     }
\$\endgroup\$
  • \$\begingroup\$ What should be returned for /a///b? \$\endgroup\$ – Simon Kirsten Oct 22 '16 at 16:55
  • \$\begingroup\$ @SimonKirsten, /a/b. Note that I'm not OP. \$\endgroup\$ – Incomputable Oct 22 '16 at 17:06
  • 3
    \$\begingroup\$ What about UNC paths like \\ComputerName\Share\blah\zzx.txt? \$\endgroup\$ – 200_success Oct 22 '16 at 20:01
  • 3
    \$\begingroup\$ Note: if you are talking about POSIX paths, then you should be careful: paths starting with exactly two slashes have implementation-defined meaning. IOW: ///foo/bar is the same as (and can be canonicalized to) /foo/bar, and /foo//bar is the same as (and can be canonicalized to) /foo/bar, but //foo/bar, //foo//bar, etc. can not be canonicalized. Their meaning is implementation-defined, and the "extra" slashes are semantically meaningful. \$\endgroup\$ – Jörg W Mittag Oct 23 '16 at 2:11
  • 2
    \$\begingroup\$ @JörgWMittag Agreed. Also, I am worried about valid path like /a/b/\\\\c \$\endgroup\$ – Shivendra Mishra Oct 23 '16 at 8:50
10
\$\begingroup\$

Here are some suggestions for how you might improve your code.

Use the appropriate #includes

This program fragment requires headers, which should be included:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

Use const where practical

Because the validate_path function does not alter the passed string, the parameter should be declared const:

char *validate_path ( const char* path )

Don't make pointless copies of variables

When you pass a parameter to a function, it's a copy, so there's no reason for this line:

char* path_copy = path;

Just use path directly.

Check for NULL before dereferencing

The first thing the validate_path does is to pass path to strlen, but this results in undefined behavior if path isn't a '\0'-terminated string. A simple safety check would be to check to see if it's a NULL pointer and exit if it is.

Consider a change to the interface

Instead of allocating memory within the function, consider instead altering the function so that it is passed an input and output buffer. This way, the code would not need to allocate memory and the caller would do both allocation and freeing. That makes for a more robust interface and it makes your code simpler.

Prefer for to while

The while loop within the code would likely be more succinctly expressed as a for loop. That way it's easier to see what is being iterated through and how.

Use a more accurate function name

I would expect that validate_path would do just that and validate a path, but that's not really what's happening here. Instead, it seems that a more accurate function name would be something like remove_dup_slashes.

Simplify your code

Here's an alternative implementation that uses these suggestions:

char *validate_path(const char *path, char *curr) {
    if (path == NULL || curr == NULL) {
        return NULL;
    }
    char *begin = curr;
    // always copy first character
    *curr = *path;
    // if it's just a single `NUL`, we're done
    if (*curr == '\0') {
        return curr;
    }
    for (path++; *path; ++path) {
        // path points to next char to read
        // curr points to last written
        if (*curr != *path || (*curr != '\\' && *curr != '/')) {
            // only copy if not duplicate slash
            *(++curr) = *path;
        }
    }
    *curr = '\0';  // terminate string
    return begin;
}

Note that the interface is still not a very good one. It could be improved by also passing in the length of the output buffer and maybe the length of the input buffer as well.

\$\endgroup\$
  • \$\begingroup\$ The proposed change of interface is a good direction, but arguments to validate_path are in the wrong order. The destination array should come first and its size should be specified as a second argument: char *validate_path(char *dest, size_t size, const char *path);. It should be OK to pass the same address for dest and path. \$\endgroup\$ – chqrlie Oct 29 '16 at 18:14
  • \$\begingroup\$ @chqrlie: that's a reasonable alternative, but if you're going to argue that your suggestion is "right" and alternatives are "wrong," we'll have to differ on that. For example, see the standard void* memcpy(void *dest, const void *src, size_t count) and note that the length is the third argument and not the second. \$\endgroup\$ – Edward Oct 29 '16 at 18:21
  • 1
    \$\begingroup\$ OK, I should have written the order of arguments to validate_path is inconsistent with common C library usage. Regarding the destination size, it is passed after the pointer in snprintf and fgets and the scanf_s family. The count argument in memcpy() is semantically different, as is the third argument in strncat(). It represents the number of elements to copy, not the size of the destination array, which may be larger (and should be for strncat()) \$\endgroup\$ – chqrlie Oct 29 '16 at 18:46
  • \$\begingroup\$ @chqrlie: it's a fair comment and I agree. I was trying to emphasize the evolutionary nature of the code (from OP's original to this one), but in real code, I would probably follow the convention you cite, and for the same reason. \$\endgroup\$ – Edward Oct 29 '16 at 18:49
9
\$\begingroup\$

Your specification has holes in it.

What will be if user passes NULL? Undefined behavior? Return?

What if the path is already valid? Copy the string? Return itself? The latter case is probably unexpected, and will make following code to cause UB:

char* path = malloc(...);
char* valid_path = validate_path(path);
free(path); //tsunami is probably created by this line

I would use what C standard library does: it doesn't mess with memory allocations when doing something else (do note that strdup() is not standard C).

Code Review:

void
die( int error_code, char* message );

Strange. Why would you need this? Error handling in C is very clumsy, but in my opinion, if your function failed, it should return NULL. It is the best signal that function failed, so that users will do what they want, not what you want on failure.

char* validate_path ( char* path );

Since you're not marking path as const, I'd expect that you make changes in place. But you're saying "SURPRISE! I'll allocate memory!". If you're not modifying the source, mark it as const, it will tell the callers that you won't modify the input.

(char*)malloc(strlen(path)+1);

Redundant cast. void* is implicitly converted into any other pointer type.

/* Travel through given path */

Don't state the obvious. It just creates noise and disturbances while reading the code.

           if ( (( *path_copy == '\\') || ( *path_copy == '/' )) && 
                (dup_flag == 0) ) {
                *copy_pwds = *path_copy;
                dup_flag = 1;
                copy_pwds++;
            }else if (( *path_copy != '\\') && ( *path_copy != '/' ))  {
                *copy_pwds = *path_copy;
                dup_flag = 0;
                copy_pwds++;
            }

Very complicated logic in my opinion. You could just track the last character.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for the input. What is plain english equivalent of this sentence : "I'd advocate that the comment is written by @CaptainObvious bot which is run in the 2nd monitor chat room." ? \$\endgroup\$ – Shivendra Mishra Oct 22 '16 at 17:45
  • \$\begingroup\$ @ShivendraMishra, in the 2nd monitor chat room on this site, there is a bot posting recent questions, and it just turned out that it has the funny name. Don't mind it, I'm just kidding. \$\endgroup\$ – Incomputable Oct 22 '16 at 17:46
1
\$\begingroup\$

Removing multiple slashes from path

Removing repeated slashes, in place, can be done simply:

char *RemoveRepeatedSlashes(char *s) {
  if (s) {
    const char *src = s;
    char *dst = s;
    // while (*dst = *src) {  // Too code-golfish
    while ((*dst = *src) != '\0') {
      do {
        src++;
      } while ((*dst == '/' || *dst == '\\')  && (*src == '/' || *src == '\\'));
      dst++;
    }
  }  
  return s;
}

Modify 0 to '\0' and while (*dst = *src) to your coding style


If code needs to form a copy, let the calling routine handle allocation. If needed, create a string duplication function.

char *strdup(const char *s) {
  size_t sz = strlen(s) + 1;
  char *dest = malloc(sz);
  if (dest) {
    memcpy(dest, s, sz);
  }
  return dest;
}        

// Sample use
char *StrippedPath = RemoveRepeatedSlashes(strdup(OringialPath));

Right-sizing the allocated memory to account for the stripped slashes in only needed in tight memory usage. Recommend to simply use with original buffer size and not realloc().


Path validation should be done with a separate function than the above two, take a const char * and return a simple result. Path validation cannot meet "Code must be platform independent" as the correctness of a path is very platform dependent.

bool validate_path(const char* path);
\$\endgroup\$
  • 1
    \$\begingroup\$ What about //implementation-defined and \\UNC-path? \$\endgroup\$ – Oskar Skog Oct 23 '16 at 19:35
  • \$\begingroup\$ @Oskar Skog Comment unclear, please expand. \$\endgroup\$ – chux Oct 23 '16 at 19:38
  • 1
    \$\begingroup\$ I was referring to the comments to the question by 200_success and Jörg W Mittag. //foo should not be turned into /foo because double leading slash may have a special meaning. \$\endgroup\$ – Oskar Skog Oct 23 '16 at 19:40
  • \$\begingroup\$ @Oskar Skog "//foo should not be turned into /foo" is a platform dependent issue - in general. In this tagged Linux and maybe windows, leading slash slash is OK. IMO, OP's validate_path () coding goal is too convoluted and path de-slashing and path validation need to be separate functions. \$\endgroup\$ – chux Oct 23 '16 at 20:42
  • \$\begingroup\$ while (*dst = *src) is a terrible coding style. In order to use it, you must disable useful compiler warnings. Sooner or later, you will get bitten. \$\endgroup\$ – chqrlie Oct 29 '16 at 18:16

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.