vcert-python icon indicating copy to clipboard operation
vcert-python copied to clipboard

Refactor and optimize code

Open rvelaVenafi opened this issue 5 years ago • 0 comments

@warrior-abhijit has suggested several changes to vcert-python code. We can address them together in one issue.

switch case will be better here ? Originally posted by @warrior-abhijit in https://github.com/Venafi/vcert-python/pull/41#discussion_r488175192

address todo now ?? as these are lot of if, else in here Originally posted by @warrior-abhijit in https://github.com/Venafi/vcert-python/pull/41#discussion_r488176457

regex match API would be lot better here and will remove lot of duplicate code below w.r.t regex match Originally posted by @warrior-abhijit in https://github.com/Venafi/vcert-python/pull/41#discussion_r488177162

switch case may be here as well ? Originally posted by @warrior-abhijit in https://github.com/Venafi/vcert-python/pull/41#discussion_r488177862

There is a handy Python wrapper called @property. This can be handy here. It would look like this: @property def base_url(self): # This is a getter return self._base_url

@base_url.setter def base_url(self, value): # This is the setter method self._base_url = self._normalize_and_verify_base_url(value)

It's nicer for refactoring and is pretty explicit. Originally posted by @HELGAHR in https://github.com/Venafi/vcert-python/pull/41#discussion_r492452816

How safe is it in this method to assume that these dictionary keys resolve? I'm new to this code, but I usually think thrice before trying to access a node in the dictionary without .get(). Originally posted by @HELGAHR in https://github.com/Venafi/vcert-python/pull/41#discussion_r493173154

Just a tidbit of input: Python string objects have a .startswith() method that's easier to read than a regex, although a regex works fine. Originally posted by @HELGAHR in https://github.com/Venafi/vcert-python/pull/41#discussion_r493173571

No use in having a doc string if the parameters aren't described, IMO. Originally posted by @HELGAHR in https://github.com/Venafi/vcert-python/pull/41#discussion_r493174068

rvelaVenafi avatar Sep 15 '20 21:09 rvelaVenafi