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

Added an inventory tab to the shop activity #8

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

arnaudcoj
Copy link

Added an "Inventory" tab (which is similar to Heroinfo_Inventory) in the shop screen to make the item comparison easier.

arnaudcoj and others added 2 commits February 21, 2015 17:40
Deleted some spaces I previously added unvoluntarly.
@Zukero
Copy link
Owner

Zukero commented Feb 23, 2015

Why do you copy Heroinfo_Inventory's code into Shopinfo_Inventory, instead of having Shopinfo_Activity extends Heroinfo_Activity ?
Code duplication just calls for bug duplication...

I could understand "duplicating" some methods, in order to alter behavior (prevent equipment changes for instance), but copying the whole class just seems wrong.

You could alter visibility of methods and members in the superclass if needed though (private -> protected).

@arnaudcoj
Copy link
Author

I agree with you, but with Heroinfo_Inventory being final, I thought it was better to duplicate. Now HeroinfoActivity_Inventory is no longer final and ShopActivity_Inventory extends it.

@Zukero
Copy link
Owner

Zukero commented Mar 6, 2015

Looks good to me. I'll just need to test it for a while before I merge it in master.
In the mean time you can add yourself (yourselves ?) to the res.strings.authors.xml credits.

Zukero pushed a commit that referenced this pull request Oct 30, 2018
merge fromzukero master
Chriz76 pushed a commit to Chriz76/andors-trail that referenced this pull request Mar 15, 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.

2 participants