patterncMinor
Simple file renamer in C
Viewed 0 times
filesimplerenamer
Problem
It's been some time since I wrote something in C. I am looking for some advices related to my code in terms of coding style, bad/wrong logic, optimization (performance) and good practices.
The code speaks for itself, but if there are any questions, I'll add some comments to make it clearer.
```
#include
#include
#include
#define START_COUNTING 0
#define NEW_FILENAME_MAXCHARS 25
#define CURRENT_INDEX_MAXCHAR 25
typedef unsigned int uint;
// Basic logger.
struct Logger
{
uint failed;
uint succeeded;
};
void print_warning(void);
// Add a token at the end of a file
char add_token(char path, char *token);
void rename_files(char *path);
// If the cwd is not the path where the files are located
// than change the cwd to that path.
void check_dir(const char *path);
int main(int argc, char *argv[])
{
print_warning();
if(argc != 2)
{
printf("Usage: \n");
exit(EXIT_FAILURE);
}
rename_files(argv[1]);
return 0;
}
void check_dir(const char *path)
{
char *buffer = _getcwd(NULL, 0);
if(buffer == NULL)
{
printf("FAILED TO GET THE CURRENT WORKING DIRECTORY\n");
exit(EXIT_FAILURE);
}
else
{
if(strcmp(buffer, path) != 0)
{
#ifdef DEBUG
printf("Changing path to: %s\n", path);
#endif
_chdir(path);
}
}
free(buffer);
}
char add_token(char path, char *token)
{
char *new_array = malloc(strlen(path) + strlen(token) + 1);
if(new_array == NULL)
{
printf("MALLOC FAILED");
exit(EXIT_FAILURE);
}
strcpy(new_array, path);
strcat(new_array, token);
return new_array;
}
void rename_files(char *path)
{
// INIT FUNCTIONS
check_dir(path);
path = add_token(path, "\\*");
// INIT DATA
WIN32_FIND_DATA find_files;
HANDLE handle_FindFiles = FindFirstFile(path, &find_files);
uint current_index = START_COUNTING;
// INIT LOGGER
struct Logger logger;
The code speaks for itself, but if there are any questions, I'll add some comments to make it clearer.
```
#include
#include
#include
#define START_COUNTING 0
#define NEW_FILENAME_MAXCHARS 25
#define CURRENT_INDEX_MAXCHAR 25
typedef unsigned int uint;
// Basic logger.
struct Logger
{
uint failed;
uint succeeded;
};
void print_warning(void);
// Add a token at the end of a file
char add_token(char path, char *token);
void rename_files(char *path);
// If the cwd is not the path where the files are located
// than change the cwd to that path.
void check_dir(const char *path);
int main(int argc, char *argv[])
{
print_warning();
if(argc != 2)
{
printf("Usage: \n");
exit(EXIT_FAILURE);
}
rename_files(argv[1]);
return 0;
}
void check_dir(const char *path)
{
char *buffer = _getcwd(NULL, 0);
if(buffer == NULL)
{
printf("FAILED TO GET THE CURRENT WORKING DIRECTORY\n");
exit(EXIT_FAILURE);
}
else
{
if(strcmp(buffer, path) != 0)
{
#ifdef DEBUG
printf("Changing path to: %s\n", path);
#endif
_chdir(path);
}
}
free(buffer);
}
char add_token(char path, char *token)
{
char *new_array = malloc(strlen(path) + strlen(token) + 1);
if(new_array == NULL)
{
printf("MALLOC FAILED");
exit(EXIT_FAILURE);
}
strcpy(new_array, path);
strcat(new_array, token);
return new_array;
}
void rename_files(char *path)
{
// INIT FUNCTIONS
check_dir(path);
path = add_token(path, "\\*");
// INIT DATA
WIN32_FIND_DATA find_files;
HANDLE handle_FindFiles = FindFirstFile(path, &find_files);
uint current_index = START_COUNTING;
// INIT LOGGER
struct Logger logger;
Solution
Improvements
-
What does program do? "Simple file renamer" clearly states a file is to be renamed and it is is evident the program takes only 1 argument. Yet I would expect a comment to state the purpose of code and what the file name is changed to.
-
When using using typedef'd
-
Buffer size bordering on being too small. Consider either right-sizing the buffer and/or use
-
When printing an error message about an invalid string, bracketing the offending string helps highlight strings that may have leading/trailing whitespace or strange characters. Note: better to use
-
Missing check if
Minor
-
Unclear what
-
-
-
-
-
What does program do? "Simple file renamer" clearly states a file is to be renamed and it is is evident the program takes only 1 argument. Yet I would expect a comment to state the purpose of code and what the file name is changed to.
-
When using using typedef'd
uint, take time to minimize work should the type change.// printf("%s => %u.%s\n", find_files.cFileName, current_index, ext);
printf("%s => %u.%s\n", find_files.cFileName, (unsigned) current_index, ext);
// or better
printf("%s => %llu.%s\n", find_files.cFileName,
(unsigned long long) current_index, ext);
// or best (but I suspect your windows compiler does not like ju)
printf("%s => %ju.%s\n", find_files.cFileName, (uintmax_t) current_index, ext);-
Buffer size bordering on being too small. Consider either right-sizing the buffer and/or use
snprint()-
When printing an error message about an invalid string, bracketing the offending string helps highlight strings that may have leading/trailing whitespace or strange characters. Note: better to use
fprintf() for error messages.// printf("FAIL TO FIND FIRST FILE IN DIRECTORY: %s\n",
printf("FAIL TO FIND FIRST FILE IN DIRECTORY: \"%s\"\n",-
Missing check if
'.' not foundchar *ext = strrchr(find_files.cFileName, '.') + 1;
assert(ext);
printf("%s => %u.%s\n", find_files.cFileName, current_index,
strcat(new_filename, ext);Minor
-
Unclear what
NEW_FILENAME_MAXCHARS precisely means. Is it the maximum length of a string containing a filename like foo.txt (7) or the size of the buffer to hold a maximal long file name (8)? Suggest NEW_FILENAME_SIZE instead for clarity.// char current_index_str[CURRENT_INDEX_MAXCHAR];
char current_index_str[NEW_FILENAME_SIZE];-
DEBUG vs NDEBUG. NDEBUG is well defined in the C spec. Consider negating the macros and using NDEBUG.-
rename_files(char path) does not change elements of path. Consider rename_files(const char path) Like-wise char add_token(const char path, const char *token)-
void rename_files(char *path) returns nothing, yet such a function certainly has many chances for I/O, allocation errors. Better to return an error code.-
CURRENT_INDEX_MAXCHAR 25 is so removed from use and not detailed as to why it is 25 leads to a potential future problem when it gets changed from 25 to TBD. Suggest a more meaningful name and size to meet the needs// #define CURRENT_INDEX_MAXCHAR 25
...
// char current_index_str[CURRENT_INDEX_MAXCHAR];
// sprintf(current_index_str, "%d", current_index);
// Size needed for a decimal string of an unsigned integer.
#define INT_STRING_SIZE(type) ((sizeof (type)*CHAR_BIT - 1)/3 + 3)
#define UINT_STRING_SIZE(type) (sizeof (type)*CHAR_BIT/3 + 2)
...
char current_index_str[UINT_STRING_SIZE(unsigned long long)];
sprintf(current_index_str, "%llu", (unsigned long long) current_index);Code Snippets
// printf("%s => %u.%s\n", find_files.cFileName, current_index, ext);
printf("%s => %u.%s\n", find_files.cFileName, (unsigned) current_index, ext);
// or better
printf("%s => %llu.%s\n", find_files.cFileName,
(unsigned long long) current_index, ext);
// or best (but I suspect your windows compiler does not like ju)
printf("%s => %ju.%s\n", find_files.cFileName, (uintmax_t) current_index, ext);// printf("FAIL TO FIND FIRST FILE IN DIRECTORY: %s\n",
printf("FAIL TO FIND FIRST FILE IN DIRECTORY: \"%s\"\n",char *ext = strrchr(find_files.cFileName, '.') + 1;
assert(ext);
printf("%s => %u.%s\n", find_files.cFileName, current_index,
strcat(new_filename, ext);// char current_index_str[CURRENT_INDEX_MAXCHAR];
char current_index_str[NEW_FILENAME_SIZE];// #define CURRENT_INDEX_MAXCHAR 25
...
// char current_index_str[CURRENT_INDEX_MAXCHAR];
// sprintf(current_index_str, "%d", current_index);
// Size needed for a decimal string of an unsigned integer.
#define INT_STRING_SIZE(type) ((sizeof (type)*CHAR_BIT - 1)/3 + 3)
#define UINT_STRING_SIZE(type) (sizeof (type)*CHAR_BIT/3 + 2)
...
char current_index_str[UINT_STRING_SIZE(unsigned long long)];
sprintf(current_index_str, "%llu", (unsigned long long) current_index);Context
StackExchange Code Review Q#127877, answer score: 2
Revisions (0)
No revisions yet.