Non-pythonic code

[miedzinski]

Code shown in this repository is mostly non-pythonic. It doesn't follow any conventions from PEP8 and all algorithms have a lot of anti-patterns. For example, binary search as shown in Python docs -- https://docs.python.org/3/library/bisect.html#searching-sorted-lists.

I don't know the exact purpose of this repository, but it certainly doesn't show well-implemented basic algorithms in Python.

[topaz1874]

Cant agree more, even from a newbie like me ,still can tell these code can write much better.

[dynamitechetan]

We all are learners, if you think something is wrong please send a pull request and make this more efficient.

[harshildarji]

Agree with @dynamitechetan If you've found any problem in our code, then I request you to make a pull request.

[AnupKumarPanwar]

Yeah exactly you can think of it as an Open Source book

[SergeyTsaplin]

I made selection sort more pythonic, please merge it https://github.com/TheAlgorithms/Python/pull/5

Later I can make more but now I haven't got enough time for it

[miedzinski]

@dynamitechetan, no offence, but I think there is something wrong in every line in this repository. Somehow this repository ended up in my favorite weekly newsletter, so I'm assuming this was posted somewhere as a resource for Python novices. Unfortunately, this "open source book" isn't ready to be published and teaches many bad habits.

Looking at binsearch, flake8 output:

30:1: W391 blank line at end of file
2:1: W293 blank line contains whitespace
6:1: W293 blank line contains whitespace
8:1: W293 blank line contains whitespace
14:1: W293 blank line contains whitespace
16:1: W293 blank line contains whitespace
20:1: W293 blank line contains whitespace
28:7: E225 missing whitespace around operator
28:11: E201 whitespace after '('
11:14: E111 indentation is not a multiple of four
12:14: E111 indentation is not a multiple of four
7:16: E225 missing whitespace around operator
21:18: E712 comparison to False should be 'if cond is False:' or 'if not cond:'

It looks like there is a warning every second line, including those empty. It's not good. Regarding efficiency, I don't think it is that important when showing Python to newcomers. Code should be friendly and clean. Anyway, this could be rewritten using bultin modules and we would achieve two things for free ("C speed" and reducing line count). Copy-pasting from Python docs:

import bisect

def binary_search(sequence, value):
    index = bisect.bisect_left(sequence, value)
    if index != len(sequence) and sequence[index] == value:
        return index
    raise ValueError

Using this, we can easily write another function, which returns True/False depending on presence of item in sequence (just like current implementation as of 4c2c3c5c05ce47598e465f31755e25e2ff854fc3)

def bin_exists(sequence, value):
    try:
        binary_search(sequence, value)
    except ValueError:
        return False
    else:
        return True

Voilà, done. Side-effects free, clean, concise and pythonic.

Add some docstrings, examples of usage, if __name__ == '__main__' idiom for testing from terminal and it's ready for PR.

Big difference, IMO.

[dynamitechetan]

@miedzinski I will try to fix it asap. And I would also like to know which weekly newsletter this repo got posted. Thank you for your help!

[miedzinski]

@dynamitechetan, Pycoder's Weekly, issue #227.

[SergeyTsaplin]

Made binary_search algo usual way. Also binary search method used stdlib added (suggested to @miedzinski comment) - binary_search - pure implementation binary_search_std_lib - implementation using stdlib's bisect Just merge https://github.com/TheAlgorithms/Python/pull/6

[harshildarji]

@SergeyTsaplin Merged!

[miedzinski]

@SergeyTsaplin, much better. But you introduced one, very bad, thing to with new implementation. Your code is O(nlogn), because of assertion which sorts the sequence. You could do it O(n), but in the same way you could implement linear search, which beats the purpose of doing binary search in first place. Just drop it.

[SergeyTsaplin]

Guys, don't name files such way please:

• Simple Substitution Cipher.py
• BubbleSort.py etc

First example is absolutely amateurish. Second - is not pythonic If you see such pull request - just reject it. Also it's very useful to use some CI-service to prevent bad code

[harshildarji]

@SergeyTsaplin Thank you for suggestion. Actually it's my mistake, and I just solved it.

[miedzinski]

I recommend reading this and then this.

[harshildarji]

@SergeyTsaplin I've included test cases in all cipher scripts and this tests are running well locally as shown in below image:

But, when I commit and push the same, Travis says that build failed. Can you please take a look at this?

[miedzinski]

Read the build log. It's explained there.

---

I am closing this. Clearly some changes to code were made.