Skip to content

Comments

feat: Added Minimum Edit Distance Algorithm#1472

Merged
ayaankhan98 merged 21 commits intoTheAlgorithms:masterfrom
Nirzak:master
Apr 12, 2021
Merged

feat: Added Minimum Edit Distance Algorithm#1472
ayaankhan98 merged 21 commits intoTheAlgorithms:masterfrom
Nirzak:master

Conversation

@Nirzak
Copy link
Contributor

@Nirzak Nirzak commented Apr 5, 2021

Description of Change

Checklist

  • Added description of change
  • Added file name matches File name guidelines
  • Added tests and example, test must pass
  • Added documentation so that the program is self-explanatory and educational - Doxygen guidelines
  • Relevant documentation/comments is changed or added
  • PR title follows semantic commit guidelines
  • Search previous suggestions before making a new one, as yours may be a duplicate.
  • I acknowledge that all my contributions will be made under the project's license.

Notes:

@Panquesito7 Panquesito7 added automated tests are failing Do not merge until tests pass enhancement New feature or request Proper Documentation Required requested to write the documentation properly labels Apr 5, 2021
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is not up to the repository standards.
Please read them carefully and follow them.

@Panquesito7 Panquesito7 added the requested changes changes have been requested label Apr 5, 2021
@Nirzak
Copy link
Contributor Author

Nirzak commented Apr 6, 2021

Code is not up to the repository standards.
Please read them carefully and follow them.

Hi, Please check now. I have added the required changes. Hope this is now according to the guidelines.

Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can see the typical structure of a program to see how to make your program as per the repository standards. 🙂

@Nirzak
Copy link
Contributor Author

Nirzak commented Apr 6, 2021

You can see the typical structure of a program to see how to make your program as per the repository standards. 🙂

Yeah I have updated the code and included all the structures that are defined on the guidelines. Separate namepsace section, a test section. I have have also studied other codes of the repo and they are also like this.

Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename the file to minimum_edit_distance.cpp.

@Nirzak
Copy link
Contributor Author

Nirzak commented Apr 6, 2021

Please rename the file to minimum_edit_distance.cpp.

Ok. But actually I had changed that. Don't know why does it still the same after the second commit and I will add extra documentation and let you know thanks.

@Panquesito7 Panquesito7 changed the title Added Minimum Edit Distance Algorithm feat: Added Minimum Edit Distance Algorithm Apr 6, 2021
@Nirzak Nirzak requested a review from Panquesito7 April 7, 2021 11:51
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nirzak
Copy link
Contributor Author

Nirzak commented Apr 7, 2021

👍 Good work.
Please enable GitHub Actions in your repository of this fork in this link: https://github.com/Nirzak/C-Plus-Plus/actions

Yeah I enabled it

Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix clang-tidy warnings.

@Nirzak
Copy link
Contributor Author

Nirzak commented Apr 7, 2021

Please fix clang-tidy warnings.

Don't know why does it showing return non-zero exit status but I have clearly defined return value 0 and the code is returning exit status 0 on my end.

@Nirzak
Copy link
Contributor Author

Nirzak commented Apr 7, 2021

Please fix clang-tidy warnings.

Code is just fine I have just checked the code in my PC with clang-tidy. I think -std=c++11 is causing the error. But I don't know why. The code works fine on my PC even when I used c++11 compiler. But from clang-tidy when I choose -std=c++14 instead of c++11 the errors were gone.

Nirzak added 2 commits April 8, 2021 00:59
clang-tidy suggested moving all if-else statements under braces. So, I did it.
Using of arrays changed to vectors as clang-tidy suggested to use vector instead of arrays.
@Nirzak
Copy link
Contributor Author

Nirzak commented Apr 7, 2021

@Panquesito7 Oh! It's because I used arrays for the Dynamic approach. Now when I have used vector instead of arrays, the Code Formatter check has been passed! Haha.

@Panquesito7 Panquesito7 removed the automated tests are failing Do not merge until tests pass label Apr 7, 2021
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Good work!

@Nirzak
Copy link
Contributor Author

Nirzak commented Apr 8, 2021

Sir , just read the algorithm section of the code. Here I have clearly mentioned about the Insert, replace and delete operations. then I have also commented them on the dp function section then I have also specified them on the min function section. (inside the function above the function). What else can I do tell me. If someone here going learn something from this code he/she should catch up with those information. You know here you have to do a minimum effort. One can not fully insert an algorithm to your mind. You have your own effort then you can grab them. We can just do as much as help we can.

  1. Else, (If last characters are not the same), we will consider all
  • three operations (Insert, Remove, Replace) on the last character of
  • the first string and compute the minimum cost for all three operations
  • and take the minimum of three values in the DP array.
  • For Insert: Recur for m and n-1
  • For Remove: Recur for for m-1 and n
  • For Replace: Recur for for m-1 and n-1

@Nirzak
Copy link
Contributor Author

Nirzak commented Apr 8, 2021

Here for an example an algorithm from your repository https://github.com/TheAlgorithms/C-Plus-Plus/blob/master/dynamic_programming/coin_change.cpp it doesn't have any algorithm explanation at all but it got accepted. But I have explained the algorithm. Then explained all the function with comments. Still you are suggesting some changes without reading the whole code without reading the concepts. If it goes like that then it will take forever to get accepted because in this world nothing is 100% perfect and you are searching that 100% perfectness in my code only.

@Panquesito7
Copy link
Member

Some codes out there are not documented. But, have you read the contributing guidelines? I don't wanna debate or discuss with you here publicly. The contributing guidelines clearly states that you should document your code.

@Nirzak Nirzak requested a review from Panquesito7 April 8, 2021 21:40
@Nirzak
Copy link
Contributor Author

Nirzak commented Apr 8, 2021

Some codes out there are not documented. But, have you read the contributing guidelines? I don't wanna debate or discuss with you here publicly. The contributing guidelines clearly states that you should document your code.

Yeah but the guideline also said that do not comment things that are obvious. So do I need to even document e return 0?Ok I will do it then and I apologize for above comments for being furious about some changes. I will comment every inch of line of the code tomorrow hope it will then accept. Good night.

@Panquesito7
Copy link
Member

Panquesito7 commented Apr 8, 2021

See https://thealgorithms.github.io/C-Plus-Plus and check out the documentation. Your code's documentation is required for Doxygen to make it like in the cloud for other users to see it.

Offtopic: Also, please be respectful, and try not to be rude.
Please read the Contributor Covenant Code of Conduct.

Thank you for reading.

@Nirzak
Copy link
Contributor Author

Nirzak commented Apr 8, 2021

See https://thealgorithms.github.io/C-Plus-Plus and check out the documentation. Your code's documentation is required for Doxygen to make it like in the cloud for other users to see it.

Offtopic: Also, please be respectful, and try not to be rude. Please read the Contributor Covenant Code of Conduct.

Thank you for reading.

Ok sir I understand. And yes That's why I have included cassert and test function and assert functions according to the guidelines.

@Panquesito7
Copy link
Member

Adding test functions will not add the documentation automatically to the cloud. You need to write it manually, as I have told you previously. 🙂

@Nirzak
Copy link
Contributor Author

Nirzak commented Apr 8, 2021

Adding test functions will not add the documentation automatically to the cloud. You need to write it manually, as I have told you previously. 🙂

Yeah that's what I am doing. Do I have to implement external function or use such to include them automatically or just writing them as previously does just fine.

@Nirzak
Copy link
Contributor Author

Nirzak commented Apr 9, 2021

Hi, Please check now. I hope now at least the documentation will be ok.

Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Good work!

@github-actions github-actions bot force-pushed the master branch 2 times, most recently from 05b80c7 to ca12b0c Compare April 10, 2021 06:59
@Nirzak Nirzak requested a review from Panquesito7 April 10, 2021 15:26
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last missing tweak. 😉

@Panquesito7 Panquesito7 removed the Proper Documentation Required requested to write the documentation properly label Apr 11, 2021
Co-authored-by: David Leal <[email protected]>
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work; I like how you've made & improved the code so well. 😄 🎉
Code and documentation are pretty good and refined; LGTM. 👍

@Panquesito7 Panquesito7 added approved Approved; waiting for merge and removed requested changes changes have been requested labels Apr 12, 2021
@Panquesito7 Panquesito7 requested a review from ayaankhan98 April 12, 2021 01:05
@Nirzak
Copy link
Contributor Author

Nirzak commented Apr 12, 2021

Amazing work; I like how you've made & improved the code so well. 😄 🎉
Code and documentation are pretty good and refined; LGTM. 👍

Thanks brother for the proper guidance.

@ayaankhan98 ayaankhan98 merged commit c854b78 into TheAlgorithms:master Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Approved; waiting for merge enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants