Successful implementation of Duplicate Files Finder in C++

Question

on my initial attempt of creating a duplicate files finder(see Duplicate files finder in C++ @ this code review site) I have finally came across the successful and optimized implementation of this code. The idea is same like the but I have made changes according to the advice of some professional here and also have fixed some bugs. Tangerine.cpp

// SWAMI KARUPPASWAMI THUNNAI
// Tangerine.cpp : Defines the entry point for the console application.
// Programmed by VISWESWARAN N (C), 2016


#include<iostream>
#include<fstream>
#include<Windows.h>
#include<string>
#include"scan.h"



int main()
{
    std::cout << "Tangerine Solutions\n";
    std::string location;
    std::cout << "Enter the location :" << std::endl;
    getline(std::cin, location);
    Process process;
    process.scan(location);
    int stay;
    std::cin >> stay;
    return 0;
}

scan.h

    #pragma once

#include<iostream>
#include<string>
#include<map>
#include<list>
#include<map>
#include<list>
#include"preliminary.h"

// Processing the files



class Process:public PreliminaryTest
{
public:
    bool scan(std::string location);
    double getSize(std::string location);
};

scan.cpp

#include<iostream>
#include<conio.h>
#include<fstream>
#include<Windows.h>
#include<boost\filesystem.hpp>
#include"scan.h"
namespace v = boost::filesystem;
bool Process::scan(std::string location)
{
    for (v::recursive_directory_iterator end, file(location); file != end; ++file)
    {
        if (file->status().type() != v::regular_file) std::cout << "|| System file has been found skipping\n";
        else
        {
            std::string loc = file->path().string(); // We will get the location
            std::cout << "|| Processing " << loc << "\n";
            std::cout << "|| SIZE: " << getSize(loc) << "\n";
            std::cout << "|| Performing the preliminary test...\n";
            double size_of_current_file = getSize(loc);
            start_preliminary_test(size_of_current_file, loc);
        }
    }
    send_the_report_for_confirmatory_test();
    return true;
}

double Process::getSize(std::string location)
{
    std::ifstream in;
    in.open(location, std::ifstream::ate | std::ifstream::binary);
    double size = in.tellg();
    return size;
}

preliminary.h

// SWAMI KARUPPASWAMI THUNNAI
#pragma once

#include"headers.h"


class PreliminaryTest:public ConfirmatoryTest
{
private:
    // This map is used to identify files with same sizes
    std::map<double, std::string> preliminary_tester;
    // This list will contain the files with same sizes
    std::list<std::string> preliminary_result;
public:
    bool start_preliminary_test(double size,std::string location);
    void send_the_report_for_confirmatory_test();
};

preliminary.cpp

//   SWAMI KARUPPASWAAMI THUNNAI
#include"headers.h"

bool PreliminaryTest::start_preliminary_test(double size,std::string location)
{
    std::map<double, std::string>::iterator test;
    test = preliminary_tester.find(size);
    if (test != preliminary_tester.end())
    {
        // List of files with same sizes
        std::cout << "\n Files with similar sizes have been found\n";
        preliminary_result.push_back(location);
        preliminary_result.push_back(test->second);
        std::ofstream file;
        file.open("SAME SIZED FILES.txt",std::ios::app);
        file << location << "\n";
        file << test->second << "\n";
        file.close();
    }
    else preliminary_tester[size] = location;
}

void PreliminaryTest::send_the_report_for_confirmatory_test()
{
    // Use iterators to access to the algorithms
    // I know this is lengthy but this is standard for almost all STL containers like deque, vectors etc.,
    // This method is called half open and closed iterators
    // itr = iterators which links algo to the container
    std::list<std::string>::iterator itr1 = preliminary_result.begin();
    std::list<std::string>::iterator itr2 = preliminary_result.end();
    for (std::list<std::string>::iterator itr = itr1; itr != itr2; ++itr)
    {
        std::cout << "\nSending the result for confirmatory test...\n";
        preliminary_test_result.push_back(*itr);
        std::cout << *itr;
    }
    std::list<std::string>::iterator itr3 = preliminary_test_result.begin();
    std::list<std::string>::iterator itr4 = preliminary_test_result.end();
    for (std::list<std::string>::iterator itr = itr3; itr != itr4; ++itr)
    {
        std::cout << "\nProcessing confirmatory test...\n";
        std::cout << *itr;
        confirm(*itr);
    }
}

confirmatory.h

// SWAMI KARUPPASWAMI THUNNAI
#pragma once
#include"headers.h"


class ConfirmatoryTest
{
private:
    // Map to identify the hash matching
    std::map<std::string, std::string> confirmatory_tester;
    // Final Neat and Clean list of the locations of duplicate files are been stored here
    std::list<std::string> confirmatory_result;
// The Member of This access specifier is used between member function of different class - so no provate
protected:
    std::list<std::string> preliminary_test_result;
public:
    std::string get_hash_for(const std::string location);
    void confirm(std::string location);
};

confirmatory.cpp

// SWAMI KARUPPASWAMI THUNNAI

#include"md5.h"
#include "headers.h"


std::string ConfirmatoryTest::get_hash_for(const std::string location)
{
    // I know this the below line may be omitted but it adds clarity
    std::string current_location = location;
    char* processed_location = new char[current_location.length() + 1];
    strcpy(processed_location, location.c_str());
    std::cout << "\n\n\n===>" << location;
    std::string md5;
    md5 = CALL_MD5_Function(processed_location);
    delete[] processed_location; // it is important to free up the memory
    return md5;
}

void ConfirmatoryTest::confirm(std::string location)
{
    std::string current_hash;
    current_hash = get_hash_for(location);
    std::map<std::string, std::string>::iterator test;
    test = confirmatory_tester.find(current_hash);
    if (test != confirmatory_tester.end())
    {
        confirmatory_result.push_back(location);
        confirmatory_result.push_back(test->second);
        // Some logging will help to identify the problem
        std::ofstream file;
        file.open("DUPLICATES.txt", std::ios::app);
        file << location << "\n";
        file << test->second << "\n";
        file.close();
    }
    else
    {
        // This will save the current hash and locations
        confirmatory_tester[current_hash] = location;
    }
}

headers.h - came accross with a LNK2005 error so I have used this

#pragma once
#include<iostream>
#include<string>
#include<map>
#include<list>
#include<map>
#include<list>
#include<fstream>
#include"confirmatory.h"
#include"preliminary.h"

ADVANTAGES 1. The previous one is windows dependent and this is capable of running in on any platform (cross-platformed)

2. The previous one gained insane access to all system files when a manifest file is added but this could only scan the regular file

3. Two way processing check file size and then compute hashes so time is greatly reduced!

4. Every single bug has been fixed proper duplicate file location are stored in a DUPLICATES.txt file

5. The initial code made used of an answer posted by the professional here which causes several license issue finally I've found my own solution so now I can happily host this on GitHub :) https://github.com/VISWESWARAN1998/Tangerine-Duplicate-Files-Finder

I am a student and I will keep all the advices you drop here you can find that I have improved my code with previous answers and comments posted by the professionals like not used namespace std, endl, using const etc and etc so kindly inform the areas where I have to improve this code so that I can learn from you and gain knowledge and use on my future work :) Thank you

Answer 1

Well apart from code review fix these bugs, Bug 1:

The previous one is windows dependent and this is capable of running in on any platform (cross-platformed) - VISWESWARAN1998

Have you actually tested this? I am quite sure you are not. In Tangerine.cpp remove the windows header file #include<Windows.h>

Bug 2: (May be called as a bug) remove this additional header file in scan.h and headers.h

#include<list>
#include<map>
#include<list>

Bug 3: No exception is handled if invalid location of the file is entered by an user fix it please...

Bug 4: IMPORTANT your implementation is something like this, if a file irrespective of duplicates, it is done using map if another file with same size is found the current file location which is found recently and the one which is in the map is added, so by this do you mean there must be maximum two duplicates? Your implementation of course works but it is not the proper way let me you this example, say a folder contains 3 duplicate files file1,file2,file3

Your program will do the following, file1 will be added to the map when file2 is found to be the duplicate both file1 and file2 will be added to the list. when another file(file3) is found as the duplicate file1 and file3 will be pushed back to the list. So finally your list contains this,

file1

file2

file3

file1

you can see that file1 is repeatedly added to the list, your program should handle this. Conditional statement will do this task before adding to the list.

Ok now, the above process is repeated for MD5 the final list of duplicates may contain like this

file1

file1

file2

file1

file3

file1

file1

you see there are 7 duplicate files where there are actually 3-1(atleast one file is needed) I still suspect this code will work since you have handled the location check before removing the file actually well that is a good habit(followed up your git-hub update) but this bug needs to be fixed or time consumption will greatly increased if there or multiple duplicate files,(calculation MD5 and size of files for huge files will actually take several seconds and when it is repeated it will waste several minutes.

Bug 5:

The previous one gained insane access to all system files when a manifest file is added but this could only scan the regular file-VISWESWARAN1998

well I see no implementation for this! what will your program does when it access the file which requires administrative permission? I am quite sure it will throw an exception. fix these bugs Good Work By the way

Answer 2

I see a number of things that could help you improve your program.

Use the required headers

In a few files, we have this line

#include"headers.h"

That file doesn't exist, but even if it did, it's a terrible name for that file. Instead, use the actual headers that are required. In the case of confirmatory.h, it needs this:

#include <map>
#include <list>
#include <string>

Remove platform-dependent code

In the scan.cpp file, we have these lines

#include<conio.h>
#include<fstream>
#include<Windows.h>
#include<boost\filesystem.hpp>

First, the conio.h and Windows.h files are obviously not portable. Second, the path separator '\' is unique to Windows, so that line could be rewritten as

#include <boost/filesystem.hpp>

Separate interface from implementation

The interface is the part in the .h file and the implementation is in the corresponding .cpp file. Users of this code should be able to read and understand everything they need from the interface file. That means that only #includes essential to being able to understand the interface should be in the .h file. In the case of scan.h, only <string> and "preliminary.h" are needed, so all other #include lines in that file should be deleted. However, see the next suggestion.

Use aggregation rather than inheritance for composition

The inheritance mechanism is generally suited for "is-a" relationships. For example, if one had a Birthday class, it might reasonably be derived from a Date base class, since a Birthday is a Date. However, an PreliminaryTest is not really a ConfirmatoryTest and a Process is not a PreliminaryTest. Instead, the process consists of running multiple tests, so the inheritance seems faulty. Instead, it might make sense to either have Test objects contained within the Process class (which really needs a better name!) or more likely, have the tests represented instead as member functions.

Use const references where practical

The code currently declares many functions like this:

double getSize(std::string location);

This has two problems. First it passes by value, so a new std::string is created on every call. This is quite wasteful of both time and memory. Second, it should actually be a const reference.

double getSize(const std::string &location);

Use const where practical

The current Process::getSize() routine and others like it do not (and should not) modify the underlying object, and so it should be declared const:

double getSize(const std::string &location) const;

Avoid "reinventing the wheel"

Since your code already includes the Boost filesystem, it would probably make sense to use the file_size() function from that library instead of writing your own, not least because the library function handles error cases but your code does not.

Prefer using to renaming namespaces

The scan.cpp file currently has these lines:

namespace v = boost::filesystem;
bool Process::scan(std::string location)
{
    for (v::recursive_directory_iterator end, file(location); file != end; ++file)
    {
        if (file->status().type() != v::regular_file) std::cout << "|| System file has been found skipping\n";
        else
        // etc.

First, there is not really a need to declare the v namespace outside of the function. Second, there's not really a need to use it at all. Instead, we can be much more precise about what exactly we're using from boost by using like this:

bool Process::scan(std::string location)
{
    using boost::filesystem::recursive_directory_iterator;
    using boost::filesystem::regular_file;
    for (recursive_directory_iterator end, file(location); file != end; ++file)
    {
        if (file->status().type() != regular_file) std::cout << "|| System file has been found skipping\n";
        else
        // etc.

Use appropriate data types

The program is currently built around std::map<double, std::string> using the file size as a key and the file path as the associated data. However, this is probably not the right data structure because the file sizes are certainly not unique and are not floating point numbers. A better choice would likely be a std::unordered_multimap<uintmax_t, std::string> which is both more appropriate and gives better performance.

Always return an appropriate value

Your PreliminaryTest::start_preliminary_test() routine is declared to return a bool but does not return any bool value. This is an error and should be fixed.

Where practical, defer writing to files until you have all the data

The PreliminaryTest::start_preliminary_test() function opens, writes to, and closes a file each time it finds a file with size the same as one that's already been scanned. This is inefficient and unecessary because that data is gathered in the preliminary_result data structure also. Better would be to simply process all of the files and then do a single write of the data all at once.

Don't hard code file names

Another problem with the PreliminaryTest::start_preliminary_test() function is that it has a hardcoded file name. That's not particularly user friendly, and it's also buried deep inside the implementation file. Better would be to ask if the user even wants the file, and if so, what it should be named and where it should be located.

Don't duplicate duplicated things

The code for the get_hash_for routine currently looks like this:

std::string ConfirmatoryTest::get_hash_for(const std::string location) 
{
    // I know this the below line may be omitted but it adds clarity
    std::string current_location = location;
    char* processed_location = new char[current_location.length() + 1];
    strcpy(processed_location, location.c_str());
    std::cout << "\n\n\n===>" << location;
    std::string md5;
    md5 = CALL_MD5_Function(processed_location);
    delete[] processed_location; // it is important to free up the memory
    return md5;
}

First, unless your CALL_MD5_Function is doing something strange, I would expect that it would not alter the passed string. Second, the location string is already a copy because it's passed in via value rather than reference so yet another copy is absolutely not needed. The whole function could be rewritten like this:

std::string ConfirmatoryTest::get_hash_for(const std::string &location) {
    return CALL_MD5_Function(location.c_str());
}

Use better names

The names ConfirmatoryTest and PreliminaryTest and Process are not very good names. They don't give any clue as to what is being tested or processed. The above get_hash_for() function is a little better, but I'd say that could be better expressed as fileContentsHash().

Use "range for" to simplify your code

The entire send_the_report_for_confirmatory_test() (an overly long name, in my view) creates a list and then processes each item in the list. It could be replaced with much simpler code by using this:

void PreliminaryTest::send_the_report_for_confirmatory_test() {
    for (const auto &item : preliminary_result) {
        confirm(item);
    }
}

Note that the preliminary_test_result, (which was only used in this function and therefore should not have been a class data member) is no longer needed at all. Another way to do this might be with std::for_each

Rethink the algorithm

The current uses lots of data structures that aren't really needed. In fact, I'd suggest that two std::unordered_multimaps would be sufficient and that all other structures could be eliminated. Also, it would make sense to store MD5 values as they are calculated so that only a single pass is ever made through any particular file.

Add better error handling

There are many things that could go wrong that are not adequately addressed by this program, such as the user not entering an integer value at the end of the program, an invalid file path specified for the initial file, named pipes or other file-like structures, etc. Think carefully about all the things that could go wrong and seek to address them in your program.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.