Merge directory and file path - C

Question

As part of C training, I wrote the following code to combine the directory name with the file name. For example: combine("/home/user", "filename") will result in /home/user/filename . It is expected that this feature will work on different platforms (at least in all popular Linux and Windows 32 and 64 bit distributions).

Here is the code.

 const char* combine(const char* path1, const char* path2) { if(path1 == NULL && path2 == NULL) { return NULL; } if(path2 == NULL || strlen(path2) == 0) return path1; if(path1 == NULL || strlen(path1) == 0) return path2; char* directory_separator = ""; #ifdef WIN32 directory_separator = "\\"; #else directory_separator = "/"; #endif char p1[strlen(path1)]; // (1) strcpy(p1, path1); // (2) char *last_char = &p1[strlen(path1) - 1]; // (3) char *combined = malloc(strlen(path1) + 1 + strlen(path2)); int append_directory_separator = 0; if(strcmp(last_char, directory_separator) != 0) { append_directory_separator = 1; } strcpy(combined, path1); if(append_directory_separator) strcat(combined, directory_separator); strcat(combined, path2); return combined; } 

I have the following questions regarding the above code.

• Consider the lines with numbers 1,2,3. All these 3 lines are designed to get the last item from the line. Looks like I'm writing more code for such a little thing. What is the correct method to get the last element from a char* string.
• To return the result, I highlight a new line using malloc . I am not sure if this is the right way to do this. Is the subscriber expected to release the result? How can I tell the caller to free the result? Is there a low probability method available?
• How do you rate the code (bad, average, good)? What are the areas that can be eliminated?

Any help would be great.

Edit

All problems discussed and implemented proposed changes are fixed. Here is the updated code.

 void combine(char* destination, const char* path1, const char* path2) { if(path1 == NULL && path2 == NULL) { strcpy(destination, "");; } else if(path2 == NULL || strlen(path2) == 0) { strcpy(destination, path1); } else if(path1 == NULL || strlen(path1) == 0) { strcpy(destination, path2); } else { char directory_separator[] = "/"; #ifdef WIN32 directory_separator[0] = '\\'; #endif const char *last_char = path1; while(*last_char != '\0') last_char++; int append_directory_separator = 0; if(strcmp(last_char, directory_separator) != 0) { append_directory_separator = 1; } strcpy(destination, path1); if(append_directory_separator) strcat(destination, directory_separator); strcat(destination, path2); } } 

In the new version, the caller must allocate enough buffer and send the combine method. This avoids problems with malloc and free . Here is the use

 int main(int argc, char **argv) { const char *d = "/usr/bin"; const char* f = "filename.txt"; char result[strlen(d) + strlen(f) + 2]; combine(result, d, f); printf("%s\n", result); return 0; } 

Any suggestions for improvement?

Answer 1

And there is a memory leak:

 const char *one = combine("foo", "file"); const char *two = combine("bar", ""); //... free(one); // needed free(two); // disaster! 

Edit: Your new code looks better. Some minor stylistic changes:

• Double half-column ;; in line 4.
• On line 6, replace strlen(path2) == 0 with path2[0] == '\0'' or just !path2[0] .
• Similarly on line 9.
• Remove the loop defining last_char and use const char last_char = path1[strlen(path1) - 1];
• Change if(append_directory_separator) to if(last_char != directory_separator[0]) . And you no longer need the append_directory_separator variable.
• You will also return a destination function similar to strcpy(dst, src) which returns dst .

Edit: And your loop for last_char has an error: it always returns the end of path1 , and so in your answer you can get a double slash //. (But Unix will see this as a single slash, unless it starts). Anyway, my suggestion corrects this, which I see is very similar to jdmichal's answer. And I see that you had it right in your source code (that I admit that I just looked - it was too difficult for my taste, your new code is much better).

And two more, somewhat more subjective, opinions:

• I would use stpcpy() to avoid the inefficiency of strcat() . (Easy to write your own if needed).
• Some people have very strong opinions about strcat() , etc. as unsafe. However, I think your use here is excellent.

Answer 2

• The only time you use last_char is in comparison to check if the last character is a delimiter.

Why not replace it with this:

 /* Retrieve the last character, and compare it to the directory separator character. */ char directory_separator = '\\'; if (path1[strlen(path1) - 1] == directory_separator) { append_directory_separator = 1; } 

If you want to consider the possibility of using multiple character delimiters, you can use the following. But be sure to add strlen (directory_separator) instead of 1 when assigning a combined string.

 /* First part is retrieving the address of the character which is strlen(directory_separator) characters back from the end of the path1 string. This can then be directly compared with the directory_separator string. */ char* directory_separator = "\\"; if (strcmp(&(path1[strlen(path1) - strlen(directory_separator)]), directory_separator)) { append_directory_separator = 1; } 

2. A less error prone method is for the user to give you the destination buffer and its length, how strcpy works. This makes it clear that they must manage the allocation and freeing of memory.

3. The process seems decent enough. I think that there are only a few features that you can work on, mostly doing it awkwardly. But you are doing well, because you can already realize this and ask for help.

Answer 3

This is what I use:

 #if defined(WIN32) # define DIR_SEPARATOR '\\' #else # define DIR_SEPARATOR '/' #endif void combine(char *destination, const char *path1, const char *path2) { if (path1 && *path1) { auto len = strlen(path1); strcpy(destination, path1); if (destination[len - 1] == DIR_SEPARATOR) { if (path2 && *path2) { strcpy(destination + len, (*path2 == DIR_SEPARATOR) ? (path2 + 1) : path2); } } else { if (path2 && *path2) { if (*path2 == DIR_SEPARATOR) strcpy(destination + len, path2); else { destination[len] = DIR_SEPARATOR; strcpy(destination + len + 1, path2); } } } } else if (path2 && *path2) strcpy(destination, path2); else destination[0] = '\0'; } 

Answer 4

Just a quick note to improve your function:

Windows supports both delimiters '/' and '\\' in paths. Therefore, I have to make the following call:

 const char* path1 = "C:\\foo/bar"; const char* path2 = "here/is\\my/file.txt"; char destination [ MAX_PATH ]; combine ( destination, path1, path2 ); 

The idea when writing a multi-platform project may be to convert '\\' to '/' in any input path (from user input, uploaded files ...), then you only have to deal with the characters '/' .

Sincerely.

Answer 5

A quick glance shows:

• you are using C ++ (//) comments that are not standard C
• you declare variables partially down the code - also not C. They must be defined at the beginning of the function.
• Your string p1 in # 1 has 1 too many bytes written to it on # 2, because strlen returns the length of the string, and you need 1 more byte for the null terminator.
• malloc does not allocate enough memory - you need path length1 + path length2 + delimiter length + zero delimiter.