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

Default to bonus city for refining #84

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

Conversation

Nfinished
Copy link

@Nfinished Nfinished commented Dec 30, 2023

Adds a couple utils for determining production bonuses, and defaults to production bonus city when refining. I was a little confused by the logic in profit-tree so I didn't give it the same default treatment, I've left a comment on the bit I found confusing.

It looks like the linter also created some whitespace changes so I'd recommend hiding those when reviewing

@ghost
Copy link

ghost commented Dec 30, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

return itemName.includes(keyword);
});
}
const hasProductionBonus = getDoesItemHaveProductionBonusForCity(city === 'Caerleon' ? itemName : parentItem, city);
Copy link
Author

Choose a reason for hiding this comment

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

I replicated the logic around Caerleon here but I'm not sure I understand it. What needs to be done in mutations.ts to replicate it for setting the default city correctly on currentItem change?

Copy link
Owner

Choose a reason for hiding this comment

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

Idk, it's probably some kind of legacy code. Everything should works anyway with parentItem and itemName. I would prefer to leave itemName here so that we don't actually depend on the parent element.

Copy link
Owner

Choose a reason for hiding this comment

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

If you disable the "multiple cities" setting, each city should be changed to the value of the "sell items" city/"main city" (the very first in the list). The exception is the Black market. If the "sell items" city is the black market, then all the others must be Caerleon when you disable "multiple cities"

@Nfinished Nfinished changed the title Default to bonus city Default to bonus city for refining Dec 30, 2023
@Nfinished Nfinished force-pushed the default-to-bonus-city branch from e59a29c to 2b8e74f Compare December 30, 2023 20:14
export const MARKET_SELL_ORDER_FEE = MARKET_ORDER_FEE + MARKET_SETUP_FEE;
export const PRODUCTION_BONUS_BY_CITY = {
Copy link
Owner

Choose a reason for hiding this comment

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

It doesn't look like its place is in constants. It would probably be better in a separate .json file

if (hasProductionBonus) {
return useFocus ? 53.9 : 36.7;
} else {
return useFocus ? 43.5 : 15.2;;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
return useFocus ? 43.5 : 15.2;;
return useFocus ? 43.5 : 15.2;

@GeekaN2
Copy link
Owner

GeekaN2 commented Dec 31, 2023

Meanwhile, PR looks good, I'll take a better look after the New Year

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