iamantony / CppNotes

My notes about C++
MIT License
1 stars 3 forks source link

all possible permutations #20

Closed nikhilchowdary closed 8 years ago

iamantony commented 8 years ago

Hi @nikhilchowdary!

First of all, thank you for your interest in my small project. And thank you that you decided to help me with issue 10.

Unfortunately, I can't accept your pull request. I will try to explain why:

  1. File name
    1.1 In CppNotes project file names are written in lowercase. Fix: allpossiblepermuations
    1.2 You forget to add file extension. Fix: allpossiblepermuations.hpp
    1.3 File name should be more general: permutations.hpp. In issue 10 I wanted to implement permutation algorithm for a string, but I forget that permutations algorithm is a general algorithm. It could be applied to other containers (for example, vector). So it would be also bad idea to name file like stringpermutations.hpp ). Also with general algorithm it is possible to produce not all permutations, but some of them. For example, you have string "mister abc" and you want to permute only last three symbols to get strings like "mister abc", "mister acb" and so on. That is why I think that name of the file should not contain allpossible.
  2. File should be located in folder src/algorithms/, not src/algorithms/euler/
  3. Information about new file should be added to the list of header filed in src/algorithms/CMakeLists.txt
  4. Code
    4.1 In C++ project should exist only one function with name main(). This function is already present in file src/main.cpp
    4.2 Algorithm should be placed in separate function (Permute()), that can accept several arguments (at least container object).
    4.3 As I see, you get this permutations algorithm from here. Well, it has several advantages: it works, it based on STL functions and algorithms, it is simple. But it hides all real work and useless if you can't use STL. In my opinion project should also contain general permutations algorithm (like this).

If you fix your code I'll be happy to accept your pull request.