Combine directory and file path - C
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 semi-colon
;;
in line 4. - In line 6, replace
strlen(path2) == 0
withpath2[0] == '\0''
or just!path2[0]
. - Similarly in line 9.
- Remove loop determining
last_char
, and useconst char last_char = path1[strlen(path1) - 1];
- Change
if(append_directory_separator)
toif(last_char != directory_separator[0])
. And so you don't need the variableappend_directory_separator
any more. - Have your function also return
destination
, similar tostrcpy(dst, src)
, which returnsdst
.
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:
- I would use
stpcpy()
, to avoid the inefficiency ofstrcat()
. (Easy to write your own, if need be.) - Some people have very strong opinions about
strcat()
and the like as being unsafe. However, I think your usage here is perfectly fine.
- The only time you use
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.