Skip to content

Ruby_CarolynQi#82

Open
qicarolyn1 wants to merge 18 commits into
Ada-C19:mainfrom
qicarolyn1:main
Open

Ruby_CarolynQi#82
qicarolyn1 wants to merge 18 commits into
Ada-C19:mainfrom
qicarolyn1:main

Conversation

@qicarolyn1

Copy link
Copy Markdown

No description provided.

@mmcknett-ada mmcknett-ada left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Green 🟢!

# *********************************************************************
assert len(vendor.inventory) == 3
assert item not in vendor.inventory
assert not result No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You could do assert result is False to make sure that an actual False comes out of the function instead of just a falsey value

Comment thread swap_meet/item.py
Comment on lines +4 to +6
if not id:
id = uuid.uuid4().int
self.id = id

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A ternary is also fine here:

        self.id = id if id else uuid.uuid4().int

Comment thread swap_meet/item.py
return f"An object of type {self.get_category()} with id {self.id}."

def get_category(self):
return f"{self.__class__.__name__}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[nit] __name__ already returns a string, so you don't need the f string.

Suggested change
return f"{self.__class__.__name__}"
return self.__class__.__name__

Comment thread swap_meet/item.py
return f"{self.__class__.__name__}"

def condition_description(self):
return str(self.condition) No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The requirements ask you to create a descriptive condition, rather than just returning the stringified number:

These can be basic descriptions (eg. 'mint', 'heavily used') but feel free to have fun with these (e.g. 'You probably want a glove for this one...").

Comment thread swap_meet/vendor.py

def get_first_item(self):
if not self.inventory:
return False

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[opinion] I'd pick return None instead of return False when there is no first item to return.

Comment thread swap_meet/vendor.py
def swap_first_item(self, other_vendor):
my_first_item = self.get_first_item()
other_first_item = other_vendor.get_first_item()
return my_first_item and other_first_item and self.swap_items(other_vendor, my_first_item, other_first_item)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

swap_items will do the right thing with None values, so you can get away with

        return  self.swap_items(other_vendor, my_first_item, other_first_item)

Comment thread swap_meet/vendor.py
Comment on lines +45 to +49
items_match_category = []
for item in self.inventory:
if item.get_category() == category:
items_match_category.append(item)
return items_match_category

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Notice this pattern of:

result_list = []
for element in source_list:
    if some_condition(element):
        result_list.append(element)

can be rewritten using a list comprehension as:

result_list = [element for element in source_list if some_condition(element)]

Which here would look like:

items_match_category = [item for item in self.inventory if item.get_category() == category]
return items_match_category

At first, this may seem more difficult to read, but comprehensions are a very common python style, so I encourage y’all to try working with them!

Comment thread swap_meet/vendor.py
Comment on lines +53 to +59
best_condition = 0.0
best_item = None
for item in items_match_category:
if float(item.condition_description()) > best_condition:
best_condition = float(item.condition_description())
best_item = item
return best_item

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can simplify this by using item.condition directly instead of turning condition_description() into a float. You can also use max to your advantage:

        if not items_match_category:
            return None
        return max(items_match_category, key=lambda item: item.condition)

Comment thread swap_meet/vendor.py
Comment on lines +71 to +77
newest_item_age = self.inventory[0].age
newest_item = self.inventory[0]
for item in self.inventory:
if item.age < newest_item_age:
newest_item_age = item.age
newest_item = item
return newest_item

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think you could use min here

return min(self.inventory, lambda item: item.age)

But I wasn't able to check since you didn't write tests for the age optional enhancement. 😉

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.

2 participants