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

[RDY] Annual report changes from AI Hospitals PR #1471

Merged
merged 1 commit into from Oct 16, 2018

Conversation

Projects
None yet
2 participants
@mugmuggy
Contributor

mugmuggy commented Sep 19, 2018

The ordering of fields is broken when multiple hospitals have the same value and they aren't in index order. So my change insteads passes through the index of the hospital being processed, finds the index of that hospital in the sorted list and returns that directly instead of using the 'dup_' values to try and do this.

The current code will draw names over the top of another and can leave with gaps in the list when multiple hospitals have the same value.

Sort functions changed to always put the local player hospital first assuming its at index 1.

I also aligned the date of the first year with TH as the Date class has this as a 1 indexed value, and its already incremented at this point for the next year.

@mugmuggy mugmuggy changed the title from Annual report changes from AI Hospitals PR to [RFC] Annual report changes from AI Hospitals PR Sep 19, 2018

@TheCycoONE

Some minor changes that I think will help the readability of this code. It took me a little too long to sort out what was going on.

end
local sort_order = function(a,b) return a>b end
local sort_order = function(a,b) return a[1] > b[1] or (a[1] == b[1] and a[2] == 1) end

This comment has been minimized.

@TheCycoONE

TheCycoONE Oct 12, 2018

Member

Naming nit: asc_order / desc_order. Also comment that it keeps the player on top for equivalence, that was not obvious.
Naming the keys and giving the player_hospital a variable would reduce the number of magic numbers and make the code more readable IMO:

for i, hospital in ipairs(world.hospitals) do
  self.money_sort[i] = { value: hospital.balance - hospital.loan, hosp_index: i }
  ...
end

-- When sorting keep the player hospital on top when values are the same
local player_index = 1
local desc_order = function(a, b) return a['value'] > b['value'] or (a['value' == b['value'] and a[hosp_index] == player_index) end
local asc_order = function(a, b) return a['value'] < b['value'] or (a['value'] == b['value'] and a[hosp_index] == player_index) end
local dup_value = 0
for _, player in ipairs(world.hospitals) do
for idx, player in ipairs(world.hospitals) do

This comment has been minimized.

@TheCycoONE

TheCycoONE Oct 12, 2018

Member

It'd be nice if this was switched from player to hospital to avoid confusion and be consistent with the loop above. For the same reason idx should match i above. I prefer i or hosp_index to idx.

Fix ordering of annual report fields
Corrected annual report year to start from 2000.
Added an in_order sort preferring local player (hospital[1] over competitors)
@TheCycoONE

This comment has been minimized.

Member

TheCycoONE commented Oct 16, 2018

looks good to me

@mugmuggy mugmuggy changed the title from [RFC] Annual report changes from AI Hospitals PR to [RDY] Annual report changes from AI Hospitals PR Oct 16, 2018

@mugmuggy

This comment has been minimized.

Contributor

mugmuggy commented Oct 16, 2018

yeah I also removed magic numbers from the rest of it, tested it and it looked ok still.

@TheCycoONE TheCycoONE merged commit e14d4ff into CorsixTH:master Oct 16, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@TheCycoONE

This comment has been minimized.

Member

TheCycoONE commented Oct 16, 2018

It occurs to me we could (and should) have used isPlayerHospital() rather than hard coding 1. It won't matter until multiplayer though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment