3
\$\begingroup\$

I have a big collection of MP4 files and an unstable internet connection so I use this batch file to check for the files integrity.

Having posted this primitive code and received valuable reviews, I decided to take into consideration as much as I can, especially not mixing C code unless when using Windows API and implement the said batch file using that code as the basis.

This program works in CMD it takes a path to MP4 location as argument.

#include <iostream>
#include <string>
#include <windows.h>

#ifndef INVALID_FILE_ATTRIBUTES
#define INVALID_FILE_ATTRIBUTES ((DWORD)-1)
#endif

bool DirectoryExists(const std::string& path)
{
   DWORD present = GetFileAttributesA(path.c_str());

   if (present == INVALID_FILE_ATTRIBUTES)
          return false;

   if (present & FILE_ATTRIBUTE_DIRECTORY)
          return true;

   return false;
}

std::string replace (const std::string& path)
{
   std::string in = path;
   std::replace(in.begin(), in.end(), '\\', '/');
   return in;
}

void findVideos (std::string& fspath)
{
   while(*(fspath.rbegin()) =='/')  //to get rid of trailing '/' like a user inputs path/to/file//////\\\
       fspath.pop_back();

   size_t i=1;
   WIN32_FIND_DATA FindFileData;
   std::string destpath = fspath + std::string("/")+ std::string("*.mp4");
   std::string ffmpegPath = "ffmpeg.exe -v error -f null - -i ";

   HANDLE hFind = FindFirstFile(destpath.c_str(), &FindFileData);

   if (hFind != INVALID_HANDLE_VALUE)
   {
        do 
          {
            std::string fullpath = std::string(fspath)+std::string("/")+std::string(FindFileData.cFileName);
            std::cout<<i<<"-"<<"Checking "<<fullpath<<" for errors"<<std::endl;
            std::string command = ffmpegPath +std::string("\"")+fullpath+std::string("\"");
            system(command.c_str());
            i++;
          } 
        while(FindNextFile(hFind, &FindFileData));
   }
   FindClose(hFind);      
}

int main(int argc, char**argv)
{
    const char* path = argv[1];

    if (path == nullptr) 
    {
        std::cout<<"No path provided"<<std::endl;
        return 0;
    } 
    else  
        if ( DirectoryExists(path) )
           std::cout<<"Provided path is: "<<path<<std::endl;
        else 
             {
             std::cout<<"Path doesn't exist"<<std::endl;
             return 0;
             }

   std::string fspath;
   fspath = replace(path);
   findVideos (fspath);
   return 0;
}

The code is working, I am open to criticism and any way to improve it especially if I work in a professional company and asked to implement such program.

EDIT: Please forgive my ignorance about indentation and other programming paradigms, I graduated a year ago and failed to get an internship or a job in the domain. I tried to go with the comments as much as I can after reading some articles about indentations.

\$\endgroup\$
  • 2
    \$\begingroup\$ Could you please fix the indentation before receiving your first review? Consistent indentation inside curly braces (whether it's the usual 4 spaces per level, or 2 spaces, or even 1 or 3 or whatever) is important for readability. (But once someone's reviewed the code, it's too late to edit it, because you might invalidate their comments.) \$\endgroup\$ – Quuxplusone May 2 at 16:45
  • 2
    \$\begingroup\$ <s>Why do you have code outside a function?</s> Oh, wait, there's just no indentation. \$\endgroup\$ – S.S. Anne May 2 at 16:50
  • \$\begingroup\$ Do yourself and many others a favor by getting a C/C++ editor that automatically indents for you. There are some that are free, such as Visual Studio Community(visualstudio.microsoft.com/vs/community) and Eclipse (eclipse.org/downloads/packages). There are also some that don't cost that much such as CLion by JetBrains jetbrains.com. In addition to auto indent, they will help you with auto complete for variables and function names. \$\endgroup\$ – pacmaninbw May 2 at 19:09
  • 2
    \$\begingroup\$ I have Visual Studio, but I am using Notepad to learn from scratch, I will read about proper indentation, but my lack of experience is failing me seeing anything catastrophic in my code, please point the unacceptable parts. \$\endgroup\$ – user10191234 May 2 at 19:43
1
\$\begingroup\$

Missing Header File

You need to include algorithm so that std::replace() can be accessed.

Error Checking

Rather than checking if argv[1] is null, it might be better to use argc in the error check. If argc is not greater than 1 than there is an error. This is what argc is meant to be used for, as well as for a loop control when there are more arguments. It might be good to give an example of the proper program call as part of the error message.

The else that follows the error check is not necessary because of the return statement.

The first return statement in main() should not return zero, it should return 1 to indicate a failure. What might be even better is to include cstdlib and use the system defined symbols EXIT_SUCCESS and EXIT_FAILURE.

It would be better to report errors to std::cerr.

int main(int argc, char** argv)
{
    if (argc <= 1)
    {
        std::cerr << "No path provided" << std::endl;
        return EXIT_FAILURE;
    }

    const char* path = argv[1];
    if (!DirectoryExists(path))
    {
        std::cerr << "Path doesn't exist" << std::endl;
        return EXIT_FAILURE;
    }

    std::cout << "Provided path is: " << path << std::endl;

    std::string fspath;
    fspath = replace(path);
    findVideos(fspath);

    return EXIT_SUCCESS;
}

Horizontal Spacing

A best practice in many programming languages is to put spaces between operators and operands, this makes the code much more readable and maintainable:

void findVideos (std::string& fspath) {
    while (*(fspath.rbegin()) == '/')
    {
        fspath.pop_back();
    }

    size_t i = 1;
    WIN32_FIND_DATA FindFileData;
    std::string destpath = fspath + std::string("/") + std::string("*.mp4");
    std::string ffmpegPath = "ffmpeg.exe -v error -f null - -i ";

    HANDLE hFind = FindFirstFile(destpath.c_str(), &FindFileData);

    if (hFind != INVALID_HANDLE_VALUE)
    {
        do
        {
            std::string fullpath = std::string(fspath)+std::string("/") + std::string(FindFileData.cFileName);
            std::cout << i << "-" << "Checking " << fullpath << " for errors" << std::endl;
            std::string command = ffmpegPath +std::string("\"") + fullpath+std::string("\"");
            system(command.c_str());
            i++;
        }
        while(FindNextFile(hFind, &FindFileData));
    }
    FindClose(hFind);
}

Prefer Modern C++ constexpr Over #define

In C++ creating a constant using constexpr is preferred over a macro because it it type safe and a macro is not type safe.

#ifndef INVALID_FILE_ATTRIBUTES
constexpr DWORD INVALID_FILE_ATTRIBUTES = ((DWORD)-1);
#endif

Indentation

It would be better not to mix spaces with tabs when indenting. Many programmers use 4 spaces for indenting, by default a tab is 8 spaces. This can lead to inconsistent indentation as demonstrated in the function bool DirectoryExists(const std::string& path) in the question. An IDE such as Visual Studio allows you to set the number of spaces used in a tab to prevent this problem, where notepad does not do this.

Replace

It's not clear that the function std::string replace (const std::string& path) since windows understand both the forward and backward slash in file specifications.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for your time and notes taken for all points, just 2 clarifications. std::replace is working with the current file without other includes, I am using Digital Mars as compiler (the same reason I use notepad) to learn about compilers, a task not easy with a 2 GB one like LLVM or MSVC. \$\endgroup\$ – user10191234 May 3 at 15:47
  • \$\begingroup\$ As for the replace function, at least in my case, giving a path like c:\Windows\system32 doesn't work, either replace '\' by '/' or the path should be like C:\\Windows\\System32, but is it professional to big software companies like MS, Oracle, Google...? \$\endgroup\$ – user10191234 May 3 at 15:57
  • 1
    \$\begingroup\$ If you're learning about compilers a good source might be the Dragon book en.wikipedia.org/wiki/…. Also look into parser generators such as YACC and Bison. Parser generators generally use a push down automata (state machine coupled with a stack). \$\endgroup\$ – pacmaninbw May 3 at 18:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.