8
\$\begingroup\$

I've came across this question https://stackoverflow.com/questions/11760420/what-is-an-optimal-way-to-find-duplicate-files-in-c and there are two answers for it both the answers convey that it is bit difficult and other suggested C++ is not suited for this purpose so as a student I am interested to test my C++ skill on this tough work!

The Idea I have implemented the code is below:

  1. A scanner* which will get the list of files recursively in a directory/drive including sub folders

  2. Once a file is got the location is got into a temporary variable

  3. STL map is made use of here where key is the size of the file and the value is the location

  4. Since STL map does not allow duplicates once a duplicate key (files with same sizes) if found dupe keys then both the file location is added into the STL list.

  5. So at this point we got the list of files with same sizes.

  6. Using iterators each file location is accessed and hashed using MD5 algorithm and that is added into another map where hash is a key and value is the location

  7. Likewise when duplicate keys (same hashes) => exactly duplicate files is found it is added into another list

  8. So we got complete list of the locations of duplicate files!

Talk is cheap here is the code I have implemented the same!

Tangerine.cpp

/ SWAMI KARUPPASWAMI THUNNAI
// Tangerine.cpp : Defines the entry point for the console application.
//


#include<iostream>
#include<fstream>
#include<Windows.h>
#include<string>
#include"md5.h"
#include"scan.h"
using namespace std;



string Process::generate_digest(char* location)
{
    string md5;
    md5 = CALL_MD5_Function(location);
    cout << md5 << endl;
    return md5;
}

int main()
{
    cout << "Tangerine Solutions\n";
    wchar_t array[MAX_PATH];
    cout << "enter the location" << endl;
    wcin >> array;
    Scan scanner;
    scanner.ListDirectoryContents(array);
    scanner.hasher();
    scanner.display();
    int stay;
    cin >> stay;
    return 0;
}

scan.h

#pragma once

#include<iostream>
#include<string>
#include<map>
#include<list>
using namespace std;

// Processing the files


class Process
{
private:
    map<double, wchar_t*> fileduplicates;
    list<wchar_t*> list_of_duplicates;
    map<string, wchar_t*>hashes;
    map<string, wchar_t*>duplicates;
    list<wchar_t*> entries;
public:
    string generate_digest(char* location);
    int process(wchar_t* file);
    int file_size_calculator(wchar_t* file);
    bool file_duplication_detector(double size, wchar_t* location);
    void hasher();
    void display();
};

class Scan:public Process
{
public:
    bool ListDirectoryContents(const wchar_t *sDir);
};

scan.cpp

#include<iostream>
#include<conio.h>
#include<Windows.h>
#include<fstream>
#include"scan.h"
using namespace std;

bool Scan::ListDirectoryContents(const wchar_t *sDir)
{
    WIN32_FIND_DATA fdFile;
    HANDLE hFind = NULL;

    wchar_t sPath[2048];

    //Specify a file mask. *.* = We want everything! 
    wsprintf(sPath, L"%s\\*.*", sDir);

    if ((hFind = FindFirstFile(sPath, &fdFile)) == INVALID_HANDLE_VALUE)
    {
        wprintf(L"Path not found: [%s]\n", sDir);
        return false;
    }

    do
    {
        //Find first file will always return "."
        //    and ".." as the first two directories. 
        if (wcscmp(fdFile.cFileName, L".") != 0
            && wcscmp(fdFile.cFileName, L"..") != 0)
        {
            //Build up our file path using the passed in 
            //  [sDir] and the file/foldername we just found: 
            wsprintf(sPath, L"%s\\%s", sDir, fdFile.cFileName);

            //Is the entity a File or Folder? 
            if (fdFile.dwFileAttributes &FILE_ATTRIBUTE_DIRECTORY)
            {
                wprintf(L"Directory: %s\n", sPath);
                ListDirectoryContents(sPath); //Recursion, I love it! 

            }
            else {
                process(sPath);
            }
        }
    } while (FindNextFile(hFind, &fdFile)); //Find the next file. 

    FindClose(hFind); //Always, Always, clean things up! 

    return true;
}


int Process::process(wchar_t* file)
{
    wcout << "\nProcessing: " << file << endl;
    double size = file_size_calculator(file);
    cout << "\n\n Size of the file :" << size << "\n";
    return 0;
}

int Process::file_size_calculator(wchar_t* file)
{
    wcout << "\nComputing the size of " << file<<endl;
    ifstream in;
    in.open(file, ifstream::ate | ifstream::binary);
    double size = in.tellg();
    bool duplicate_size = file_duplication_detector(size, file);
    if (duplicate_size == true)cout << "Files with duplicate sizes have been found" << endl;
    return size;
}


// This memeber function is used to detect files with same sizes
// Note files with same sizes even in bytes is not said to be duplicates!
// It needs to be processed further
bool Process::file_duplication_detector(double size, wchar_t* file)
{
    map<double, wchar_t*>::iterator itr;
    itr = fileduplicates.find(size);
    if (itr != fileduplicates.end())
    {
        // This will create a list of files with same sizes
        list_of_duplicates.push_back(itr->second);
        list_of_duplicates.push_back(file);
        return true;
    }
    else
    {
        fileduplicates[size] = file;
    }
    return false;
}

void Process::hasher()
{
    //Half open and closed iterator implementation!
    // I know there are other ways to do this
    // But half open and closed method is standard for almost all STL stuffs like vector,deque etc.,
    list<wchar_t*>::iterator itr1 = list_of_duplicates.begin();
    list<wchar_t*>::iterator itr2 = list_of_duplicates.end();
    //Common iterator
    // I've heard from someone ++something is faster than something++ for STL iterators
    //I've forgetten why :) you may have the answer for it if yes drop some comments please!
    map<string, wchar_t*>::iterator dupe;
    for (list<wchar_t*>::iterator itr = itr1; itr != itr2; ++itr)
    {
        char hash[MAX_PATH];
        wcstombs(hash, *itr, MAX_PATH);
        string md5 = generate_digest(hash);
        //Create an iterator for map
        dupe = duplicates.find(md5);
        if (dupe != duplicates.end())
        {
            entries.push_back(dupe->second);
            entries.push_back(*itr);
        }
        else
        {
            duplicates[md5] = *itr;
        }
    }
}

void Process::display()
{
    list<wchar_t*>::iterator itr1 = entries.begin();
    list<wchar_t*>::iterator itr2 = entries.end();
    for (list<wchar_t*>::iterator itr = itr1; itr != itr2; ++itr)
    {
        cout << "\n";
        wcout << "=> Duplicates" << *itr << "\n";
    }
}

However this program contains a small bug(does not display the duplicate files)

I know using namespace std is bad and I will not do the same in future I am student so every small comments will help me to increase my knowledge and understanding the depth of C++ so please comment the improvements made so that I can learn from you and use it on my further works

\$\endgroup\$
  • \$\begingroup\$ Did you debug what the issue is? Are the MD5 different? \$\endgroup\$ – thepace Nov 8 '16 at 16:39
  • \$\begingroup\$ @thepace, yes sire I used my debugger I've created the break points on if all maps,lists and iterators, I used an intermediate window(of VS-2015) and found the what values is being stored in containers each container map, and preliminary list contains the proper values stored in the container! but the when adding to the last container (list) it becomes unicode(some chinese fonts) of-course I've changed my programming environment from multibyte to unicode still final container consists of some unsual entries. The code itself doesn't contains bug MD5 values have been got properly \$\endgroup\$ – VISWESWARAN NAGASIVAM Nov 9 '16 at 15:10
  • \$\begingroup\$ File sizes can be found in their WIN32_FIND_DATA information (as fdFile.nFileSizeLow and fdFile.nFileSizeHigh, so you could use the information you already have and speed things up by skipping the process of opening each file just to get its size. \$\endgroup\$ – Christopher Oicles Nov 12 '16 at 1:09
2
\$\begingroup\$

I'm not looking at weather your code could run faster, or use less memory, I'm looking at making you code readable and maintainable. Even my code, which is always perfect in every respect :), will have to be modified at some point and the poor individual that has to update it needs every help they can get. If the code is 'clean' then spotting improvements to it is much easier.

A plus point that I noticed is the order you have placed the include files in. Placing them with system files first all the way down to the local ones mean that if you get an error because of duplicate type or function it is reported in the local code, not the 'system' files. You can change the prototype for printf() in stdio.h so it doesn't clash with printf() in MyFile.h, but its not a recommended course of action.

Another plus point is the code is fairly easy to read, you aren't afraid of the space bar.

OK so the negative points, as you said "using std" is my first point, but what about comments personally I write them before I code, as a way of designing the code. Its also nice to explain what functions and classes are supposed to do and why.

Tangerine.cpp - Process::generate_digest

  1. location isn't being changed so why not make it a const reference?
  2. You don't check to see if location is NULL or invalid, is this done in the CALL_MD5_Function?
  3. Assign the value to md5 on the same line as you declare it, even better initialise md5 with the correct value.
  4. You are streaming without checking if it is empty or massively large is that a safe thing to do?
  5. Why is this function in this file and not in Process.cpp

Tangerine.cpp - main

  1. This is purely a style issue, either declare the variables at the top (C Style) or as near to use as you can (C++ Style), so move the declaration of array down one line.

  2. You MUST validate the value in array. It could be anything, even NULL.

  3. It might be worth telling the user to press a key to exit. Otherwise the app appears hung.

Scan.h

  1. A header file should only contain one class definition. It makes it easier to find the code.
  2. You are mixing wchar and char.

Scan.cpp

  1. Why not use MAXPATH (?) for the size of sPath?
  2. Recursion : OK but what happens when you encounter symbolic links (search for Junction.exe)
  3. In process file and size could be constants.
  4. In process() why return 0 why not make the function return void?
  5. In file_size_calculator you are calculating the size as a double and then returning it as an int.
  6. In file_duplication_detector the parameters could be constant.
  7. In file_duplication_detector the iterator could be constant.
  8. In hasher I think you iterators could all be const_iterators, but I would get rid of itr1 and itr2 they are confusing.
  9. In display I think you iterators could all be const_iterators, but I would get rid of itr1 and itr2 they are confusing.
\$\endgroup\$
  • \$\begingroup\$ Thank you sire for your valuable comments and for finding a bug (improper return value) in my code and could you please say what are the areas I should concentrate more? I am a student this may help me to code better \$\endgroup\$ – VISWESWARAN NAGASIVAM Nov 9 '16 at 15:17
  • 1
    \$\begingroup\$ There is no particular area you need to concentrate on, you just need more experience. Look through GIT and look at other peoples coding styles, don't worry about understanding the code. It would be a good idea to update this code and repost it, people will always find things 'wrong' with it. You could turn the warning level up on your compiler, it should have picked most of those things up, and treat all warnings as errors. There is no reason why "simple" (I mean not doing really complex stuff) code like this should have warnings in it. 35 years after my first code, I'm still learning. \$\endgroup\$ – Code Gorilla Nov 10 '16 at 12:24

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.