Added a hex-bin.py file in conversion.py #4433
Conversation
|
@ktsrivastava29 I see you have not addressed all the requested changes. Sit back and fix all the required changes before asking for a review. Also, feel free to ask for help. |
There was a problem hiding this comment.
Click here to look at the relevant links ⬇️
🔗 Relevant Links
Repository:
Python:
Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.
algorithms-keeper commands and options
algorithms-keeper actions can be triggered by commenting on this PR:
@algorithms-keeper reviewto trigger the checks for only added pull request files@algorithms-keeper review-allto trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.
There was a problem hiding this comment.
Sir, worked on your suggestion
- I've modified the code that catches the exception
- I removed the lines which you asked to omit
- I've also improved the doctests
Sir, the only thing I didn't do is I didn't remove the input statement,
sir my rest of the code gets greatly affected if I try to remove the input statement.
Therefore, I've not removed it.
rest of the thing I modified as you suggested.
ghost
removed
the
require type hints
|
@ktsrivastava29 still you have not addressed all requested changes. Let's start one by one. 1st and foremost I want you to push a change that handles the conversion of a string to hexadecimal no. inside your function. You need to understand that all your code should be encapsulated within a wrapper function, which in turn can call any other function. Roll out a commit doing just this and nothing else then we will move to other areas of improvement. |
|
Sir, now I've modified it as you advised. |
conversions/hex-bin.py
Outdated
| bin_str = num3 + bin_str | ||
| num2 = num2 >> 1 | ||
|
|
||
| if flag: |
There was a problem hiding this comment.
One last change, maybe we can make this if, else clause more pythonic.
Something like
return x if condition else y
conversions/hex-bin.py
Outdated
| import math | ||
|
|
||
| def convert(num: str) -> str: | ||
|
|
There was a problem hiding this comment.
Also remove this whitespace
|
@cclauss I am unable to push to his branch for correcting some changes related to |
|
Sir, I really want to contribute something... I'm putting all my efforts to do something concrete but I see it's taking too much of your time... I genuenly apologize for this. Sir I've made some changes could you please review it and make me aware of what else should be done. |
conversions/hex-bin.py
Outdated
| from typing import Union | ||
|
|
||
|
|
||
| def convert(num: str) -> Union[bool, int]: |
There was a problem hiding this comment.
I know that this was recommended in a previous review but I would like to request a change.
When someone calls this function, they expect to get back a binary number which they may then use in a mathematical calculation. Returning a bool or an int in this case is not considered Pythonic because it breaks their expectations so let's agree that this function should return an int or should raise an Exception if garbage data is provided as input. This is the way that Python's builtin functions behave:
>>> hex(123)
'0x7b'
>>> hex("dog")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: 'str' object cannot be interpreted as an integer
Also, the name convert() is not very self-documenting and neither is num so let's go for:
| def convert(num: str) -> Union[bool, int]: | |
| def hex_to_bin(hex_num: str) -> int: |
There was a problem hiding this comment.
Apologies to the contributor for derailing but this makes more sense.
|
Sir, I changed the names, and no doubt it has become very appealing now. Sir are there any further changes required, if so please let me know. I'll be happy to do that. |
conversions/hex-bin.py
Outdated
| try: | ||
| int_num: int = int(hex_str, 16) | ||
| except ValueError: | ||
| return False |
There was a problem hiding this comment.
Instead of implicitly silencing the error as mentioned by @cclauss please use the previous format of int_num: int = int(hex_str, 16) as this explicitly raises the error.
Sorry for the turnaround, but it makes more sense. I would have pushed this change myself but I can't due to some permisions.
conversions/hex-bin.py
Outdated
| if not hex_str: | ||
| return False |
There was a problem hiding this comment.
Same as below. Simply raise a ValueError instead of returning a boolean
conversions/hex-bin.py
Outdated
| 1111111111111111 | ||
| >>> convert("F-f") | ||
| >>> hex_to_bin("F-f") | ||
| False |
There was a problem hiding this comment.
The function returns an int or it raises an Exception.
conversions/hex-bin.py
Outdated
| False | ||
| >>> convert("") | ||
| >>> hex_to_bin("") | ||
| False |
There was a problem hiding this comment.
The function returns an int or it raises an Exception.
conversions/hex-bin.py
Outdated
| False | ||
| """ | ||
|
|
||
| bin_str: str = "" | ||
| hex_str: str = num.strip() | ||
| hex_str: str = hex_num.strip() | ||
|
|
||
| if not hex_str: | ||
| return False |
There was a problem hiding this comment.
The function returns an int or it raises an Exception.
|
Sirs, I've modified the code again, could you please look into it and see if any further modification is demanded. |
|
Sir, I'm glad you merged it. It is my first contribution, although it took some 3 days for me to present an algorithm that can get merged but yeah finally I did it. Thank you so much @cclauss and @onlinejudge95 .... you people are really amazing... You kept suggesting improvement without being rude.... and your suggestions helped me learn some exciting things. |
|
Nice work @ktsrivastava29 👍🏻 |
|
You can see that I removed type hints inside the function like:
because both Python and mypy are smart enough to recognize that these are bool, int, and str respectively. The place where type hints are super important are on the function parameters and return value: |
|
Thank you sir, you added a new thing to my reasoning. |
* Added a file that converts hexa to binary * Added file to convert hexadecimal to binary * Update hex-bin.py * added type hint in the code * Added doctest * Added code to handle exception * Resolved doctest issue * Update hex-bin.py * Modified convert function * Added WhiteSpace around operators. * Made more pythonic * removed whitespace * Updated doctest command * Removed whitespace * imported union * Replaced flag with is_negative * updated return type * removed pip command * Resolved doctest issue * Resolved doctest error * Reformated the code * Changes function name * Changed exception handling statements * Update and rename hex-bin.py to hex_to_bin.py * Update newton_method.py * Update matrix_operation.py * Update can_string_be_rearranged_as_palindrome.py * Update hex_to_bin.py Co-authored-by: Christian Clauss <cclauss@me.com>
Describe your change:
I made a slight modification in the conversion file, now it is also able to convert hexadecimal to binary
Checklist:
Fixes: #{$ISSUE_NO}.