As part of learning C, I wrote the following code to combine directory name with file name. Eg: combine("/home/user", "filename")
will result in /home/user/filename
. This function is expected work across platforms (atleast on all popular linux distributions and windows 32 and 64bit).
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.
char*
string.malloc
. I am not sure this is the right way to do this. Is caller expected to free the result? How can I indicate the caller that he has to free the result? Is there a less error prone method available?Any help would be great.
Edit
Fixed all the issues discussed and implemented the changes suggested. 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, caller has to allocate enough buffer and send to combine
method. This avoids the use of malloc
and free
issue. Here is the usage
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 more improvements?
Combine(sourcePath, fileName); string destFile = System. IO. Path.
string filename = "image1. jpg"; int fileCount = 1; string renameTo = string. Format("{0}_{1}{2}", Path. GetFileNameWithoutExtension(filename), fileCount, Path.
Path. Combine uses the Path. PathSeparator and it checks whether the first path has already a separator at the end so it will not duplicate the separators. Additionally, it checks whether the path elements to combine have invalid chars.
This command uses Join-Path to combine a path with a childpath. Since the command is executed from the FileSystem provider, it provides the \ delimiter to join the paths.
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:
;;
in line 4.strlen(path2) == 0
with path2[0] == '\0''
or just !path2[0]
.last_char
, and use const char last_char = path1[strlen(path1) - 1];
if(append_directory_separator)
to if(last_char != directory_separator[0])
. And so you don't need the variable append_directory_separator
any more.destination
, similar to strcpy(dst, src)
, which returns dst
.Edit: And your loop for last_char
has a bug: it always returns the end of path1
, and so you could end up with a double slash // in your answer. (But Unix will treat this as a single slash, unless it is at the start). Anyway, my suggestion fixes this--which I see is quite similar to jdmichal's answer. And I see that you had this correct in your original code (which I admit I only glanced at--it was too complicated for my taste; your new code is much better).
And two more, slightly-more subjective, opinions:
stpcpy()
, to avoid the inefficiency of strcat()
. (Easy to write your own, if need be.)strcat()
and the like as being unsafe. However, I think your usage here is perfectly fine.last_char
is in the comparision to check if the last character is a separator.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 account for the possibility of multiple character separators, you can use the following. But be sure when allocating the combined string to add strlen(directory_separator) instead of just 1.
/* 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;
}
The less error-prone method would be to have the user give you the destination buffer and its length, much the way strcpy
works. This makes it clear that they must manage allocating and freeing the memory.
The process seems decent enough. I think there's just some specifics that can be worked on, mostly with doing things in a clunky way. But you are doing well, in that you can already recognize that happening and ask for help.
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With