Skip to content
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

Kunzite - Diana Salazar & Allie Soliz #78

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Conversation

d3897
Copy link

@d3897 d3897 commented Apr 7, 2023

No description provided.

Copy link

@spitsfire spitsfire left a comment

Choose a reason for hiding this comment

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

Good job, Diana and Allie! I've left some suggestions and feedback below in your code!

Things to consider:

  • Refactoring the super().__init__ method so that the condition attribute is passed through to the parent constructor to handle.
  • Combine some of the conditional expressions into compound conditionals to save some lines, so we aren't duplicating the same return statements

import uuid

Choose a reason for hiding this comment

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

Are we creating an instance of uuid in this file? Doesn't look like it, so we an remove this import!

Suggested change
import uuid

Comment on lines +8 to +10
self.fabric = fabric
self.condition = condition

Choose a reason for hiding this comment

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

the parent class Item already had a condition attribute, so let's use that! We don't want to reinvent the wheel if we don't have to!

    super().__init__(id,condition)
    self.fabric = fabric


def get_category(self):
return "Clothing"

Choose a reason for hiding this comment

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

This works! But what might happen if the name of this class was suddenly changed? We'd have to remember to change it down here as well. What's one way we could always make sure get_category will dynamically return the class's name?


def __str__(self):
return f"An object of type Clothing with id {self.id}. It is made from {self.fabric} fabric."

Choose a reason for hiding this comment

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

Instead of hard coding in the class's name as we did in get_category above, how could we refactor this in order to use get_category instead?

import uuid

Choose a reason for hiding this comment

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

Suggested change
import uuid

Comment on lines +50 to +54
my_item = self.inventory[0]
a = self.swap_items(other_vendor, my_item, their_item)

return a

Choose a reason for hiding this comment

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

This works fine! If we wanna get wild, we can combine these all into one line:

return self.swap_items(other_vendor, self.inventory[0], other_vendor.inventory[0])

return a

def get_by_category(self, category):

Choose a reason for hiding this comment

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

👍


def get_best_by_category(self, category):

Choose a reason for hiding this comment

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

Suggested change

return list_of_objects

def get_best_by_category(self, category):

Choose a reason for hiding this comment

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

👍

Comment on lines +79 to +82
return False
if not their_best_item:
return False

Choose a reason for hiding this comment

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

Let's combine these!

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