-
Notifications
You must be signed in to change notification settings - Fork 4
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
Greeting service - ABC madness. #2
base: master
Are you sure you want to change the base?
Conversation
self._reader = list(csv.DictReader(csv_input)) | ||
|
||
def parse(self) -> Generator: | ||
assert self._reader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is good, however throwing an AssertionError is not really good when it comes to the user experience. It would be better to create your own verbose exception like:
class ConnectionException(Exception):
pass
def parse(self):
if self._reader is None:
raise ConnectionError("Source is not connected. Call connect method first.")
This is better:
Traceback (most recent call last):
File "main.py", line 4, in <module>
raise ConnectionError("Source is not connected. Call connect method first.")
ConnectionError: Source is not connected. Call connect method first.
than this:
File "main.py", line 6, in func
assert self._reader
AssertionError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
absolutely agree, I am completely not fun of assertion, but this specific assertion is caused by mypy - it throws error because by default self._parser = None
. :)
It can take a while to refactor mypy here, just decided to make a simplest fix.
self._resource = resource | ||
|
||
def _select_birthday_person(self, person_data): | ||
person_birthday = dateutil.parser.parse(person_data[" date_of_birth"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've already mentioned it, you should strip whitespaces from the headers first, so you can use date_of_birth
without extra space. It's much healthier approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, you are right. The problem is we used "csv.DictReader" - so we have no control over how headers look. But today morning I have got an interesting idea - we could just use ", " ("a comma with a space") as .csv delimiter.
|
||
|
||
class Sender: | ||
def __init__(self, handler, service, message='Happy Birthday from Coding Dojo 2020!'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can be multiple scenarios involved regarding the message.
Lets say that the Client wants to specify the message but as a template.
Example:
Hello {name}!
It's Your birthday today so here is an extra 100€ to Your upcoming payment
How many things would need to change in order to do that?
But very good job, really nice and clean code!
def _select_birthday_person(self, person_data): | ||
person_birthday = dateutil.parser.parse(person_data[" date_of_birth"]) | ||
|
||
if int(person_birthday.month) == datetime.datetime.today().month == 2 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few things regarding the IF statements you have here.
One: inconsistency
Sometimes you check int(person_birthday.month) == 2
the other time you do person_birthday.day == '29'
. Try to be consistent as there should be one and only way to do it.
Two: complex IF's
Your IF's are not really readable. You can try to split and give them a meaningful names.
today = datetime.date.today()
born_month = int(person_birthday.month)
born_day = int(person_birthday.day)
born_on_february_29 = born_month == 2 and born_day == 29
today_is_february_28 = today.day == 28
born_today = today.day == born_day and today.month == born_month
if born_today:
return True
elif (born_on_february_29 and today_is_february_28):
return True
return False
It should make the logic more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, you are right! this piece of code left untouched from Dojo :) and during meetup this code was couple of times refactored and left with some mess of strings/ints comparing.
people = self.handler.build_birthdays_people() | ||
final_data = {} | ||
for person in people: | ||
final_data[person.address] = self.generate_message_for_person(person) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edge case here would be if two people would use the same email, but in general we assume that emails are indeed unique.
Apart from google that treats [email protected]
the same way as [email protected]
or [email protected]
. It means that I would get most likely few emails.
|
||
def send(self): | ||
for address, msg in self.prepare_final_messages().items(): | ||
self.service("Where: {:40} | Message: {:>25}".format(address, msg)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the usage of string interpolation along with formatting, good job.
|
||
@abc.abstractmethod | ||
def connect(self): | ||
... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably raise NotImplentedError('connect')
or pass
is better, ...
is ok but is just a not returned variable
class CSVResource(BaseResource): | ||
def __init__(self, path): | ||
self._path = path | ||
self._reader: Optional[List[Dict[str, Union[str, datetime]]]] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dict[str, Union[str, datetime]]
you can move as constant, this type is big
def test_select_birthday_person(birthday_normal): | ||
handler = Handler(resource=MockResource(birthday_normal)) | ||
|
||
assert len(list(handler.build_birthdays_people())) == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can create a helper def count(gen): return len(list(gen))
it should be increase a readability
return [person] | ||
|
||
|
||
class MockResource(BaseResource): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nice, Did you think about Mock from unittest?
To run the script run:
python -m app
To run tests run:
pytest app/tests.py
We have made only small cosmetic changes comparing to Coding Dojo meetup. Mainly focused on fixing tests (by that time we had 2 working and 2 not working because of spaces in .csv file, which we did not take into account. :) ). Also we refactored a bit our
.parse()
method, so the solution is now really generic and can be easily replaced by different data resources (html, etc).Authors:
Andrii Isiuk
Marcin Josiński