HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Simple file renamer in C

Submitted by: @import:stackexchange-codereview··
0
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;

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 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 found

char *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.