Skip to content

added doctests for compare_string and is_for_table #1138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 22, 2019

Conversation

pathak-deep15
Copy link
Contributor

compare_string('0010','0110')
'0_10'
is_for_table('__1','011',2)
True
The above doctests were added

>>>compare_string('0010','0110')
       '0_10'
>>> is_for_table('__1','011',2)
	True
The above doctests were added
@harshildarji harshildarji added the awaiting merge This PR is approved and ready to be merged label Aug 20, 2019
@cclauss
Copy link
Member

cclauss commented Aug 20, 2019

This is awesome and it works as expected but instead of putting all the tests into a single comment, I would encourage you to move the doctests under their appropriate functions. The reasons for this are code reuse and readability. Let's say that I read this file and I would like to copy the algorithm compare_string() and paste it into my own project. It would be good if the testcase moved along with the function. The readability dimension comes in when I am reading through the code, I see the definition of the function and immediately after I see a sample call which helps me to know if I properly understand the intention and effects of calling that function.

Great work and thanks for your contribution.

@poyea poyea merged commit 47cb394 into TheAlgorithms:master Aug 22, 2019
stokhos pushed a commit to stokhos/Python that referenced this pull request Jan 3, 2021
* added doctests for compare_string and is_for_table

>>>compare_string('0010','0110')
       '0_10'
>>> is_for_table('__1','011',2)
	True
The above doctests were added

* Update quine_mc_cluskey.py

* Update quine_mc_cluskey.py

* Update quine_mc_cluskey.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting merge This PR is approved and ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants