Removing multiple slashes from path

Question

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;

     }

Answer 1

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.

Answer 2

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.

Answer 3

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);