Added a hex-bin.py file in conversion.py #4433
Conversation
|
Sir, I've made changes as per you advised. |
|
@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. |
Click here to look at the relevant links
|
|
Sir, worked on your suggestion
Sir, the only thing I didn't do is I didn't remove the input statement, |
|
@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. |
|
Good work on completing a major change. Now I expect you to go over these review comments and update the code in the same manner. Please ask out for help in this PR if you are stuck somewhere. |
| bin_str = num3 + bin_str | ||
| num2 = num2 >> 1 | ||
|
|
||
| if flag: |
onlinejudge95
May 18, 2021
Collaborator
One last change, maybe we can make this if, else clause more pythonic.
Something like
return x if condition else y
|
@cclauss I am unable to push to his branch for correcting some changes related to |
|
As documented in CONTRIBUTING.md, on your local machine, please run: |
|
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. |
| from typing import Union | ||
|
|
||
|
|
||
| def convert(num: str) -> Union[bool, int]: |
cclauss
May 19, 2021
Member
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: |
onlinejudge95
May 20, 2021
Collaborator
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. |
| try: | ||
| int_num: int = int(hex_str, 16) | ||
| except ValueError: | ||
| return False |
onlinejudge95
May 20, 2021
Collaborator
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.
| if not hex_str: | ||
| return False |
onlinejudge95
May 20, 2021
Collaborator
Same as below. Simply raise a ValueError instead of returning a boolean
| False | ||
| """ | ||
|
|
||
| bin_str: str = "" | ||
| hex_str: str = num.strip() | ||
| hex_str: str = hex_num.strip() | ||
|
|
||
| if not hex_str: | ||
| return False |
|
Sirs, I've modified the code again, could you please look into it and see if any further modification is demanded. |
|
LGTM... Cool algorithm. Thanks for doing this. |
|
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. |
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}.