Skip to content

Conversation

@LucyCo
Copy link
Owner

@LucyCo LucyCo commented Jan 21, 2017

No description provided.

@LucyCo LucyCo requested a review from yochaibl January 21, 2017 00:03
factory.py Outdated
# self.__creature_name = creature_name
# self.__creature_type = creature_type

def print_creature(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raise NotImplementedError

Copy link
Collaborator

@yochaibl yochaibl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

print "hello"
# not implemented

def get_creature_name(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raise NotImplementedError

factory.py Outdated
self._temp_of_fire = temp_of_fire

def print_creature(self):
print "This is a Dragon, is it %s and its' fire tempature is %d. Its' name is %s"%\
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it -> it is
Its' -> its :)

add a space before and after the percent (in the end of the line)

(self._num_of_eyes, self._num_of_teeth, self._num_of_legs, self.get_creature_name())


def creature_factory(creature_name, features):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

There's a more pythonic way to do it, I'll show you.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class Monster(Creature):

def __init__(self, creature_name, num_of_eyes, num_of_teeth, num_of_legs):
Creature.__init__(self, creature_name)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Say super(Monster, self).__init__(self, creature_name)

(self._num_of_eyes, self._num_of_teeth, self._num_of_legs, self.get_creature_name())


def creature_factory(creature_name, features):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

And here's how you can improve it: http://stackoverflow.com/a/60215/1206176

return Monster(creature_name, features["num_of_eyes"], features["num_of_teeth"], features["num_of_legs"])


def name_exists(name, list):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simply say return name in list

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing - don't use the word list as a variable. It's a reserved word.

elif name_exists(sys.argv[1], creatures):
new_creature = creature_factory(sys.argv[1], creatures[sys.argv[1]])
new_creature.print_creature()
else:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be nice if you moved this check to the beginning, so you get rid of the problem as soon as you can (and quit with error) and then, if everything is fine, continue as usual.

for creature_name, creature_data in creatures.iteritems():
new_creature = creature_factory(creature_name, creature_data)
new_creature.print_creature()
elif name_exists(sys.argv[1], creatures):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accessing an element in an array always gives me the feeling that something bad is going to happen.

Once in the beginning, determine how many arguments you got and assign each to a variable with a meaningful name, and don't mess with that array again. The code will become much more readable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants