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

Fixes #49: Character cannot have two equal items #90

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ttldtor
Copy link
Member

@ttldtor ttldtor commented Feb 5, 2020

Fixes #49

  • The Knife case class converted to class so knifes became unique
  • Now we take into account the requirements of items and the possibilities of body parts

@ForNeVeR ForNeVeR self-requested a review February 5, 2020 16:39
}

object Knife {
def apply(name: String, damage: Int = 50) = new Knife(name, damage)
Copy link
Member

@ForNeVeR ForNeVeR Feb 5, 2020

Choose a reason for hiding this comment

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

Honestly, I dislike the approach taken. Now we'll be required to be very careful when declaring our item classes to not hit the same issue with any of them.

There's an alternate route with marking our items with identifiers (the same as we already use for actors and factions).

@ttldtor, what do you think about this alternative? Would it be worse?

Copy link
Member Author

@ttldtor ttldtor Feb 7, 2020

Choose a reason for hiding this comment

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

@ForNeVeR I don't see any faction id: case class Faction(name: String). Yep, I agree with you. I'll add id to items.

Copy link
Member

@ForNeVeR ForNeVeR left a comment

Choose a reason for hiding this comment

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

Very good work, thank you. I believe that minor changes may improve this PR.

backpack -= item
equipment += item
} else {
return
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to return the missing capabilities from the canEquip method, log them here, and add a // TODO: Show missing capabilities on screen.

Copy link
Member Author

Choose a reason for hiding this comment

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

def equipped(item: EquipmentItem): Boolean = {
inventory.equipment.contains(item)
}

def toggleSelected() = {
def toggleSelected(): Unit = {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a test for the new behavior of this method?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ForNeVeR ForNeVeR removed their assignment Feb 5, 2020
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.

Character cannot have two equal items
2 participants