Page 1 of 1
API change: Get quarterly company information.
Posted: 10 Jun 2011 15:38
by Morloth
Hey guys,
I read through the API change topic
http://www.tt-forums.net/viewtopic.php?f=65&t=44249 and noticed someone asked for a way to get to the quarterly company information. Since I also need this information for the next version of my AI I decided to implement these features. The given patch adds the following functions:
Money GetQuarterlyIncome(CompanyID company, uint32 quarter);
Money GetQuarterlyExpenses(CompanyID company, uint32 quarter);
int32 GetQuarterlyCargoDelivered(CompanyID company, uint32 quarter);
int32 GetQuarterlyPerformanceRating(CompanyID company, uint32 quarter);
Money GetQuarterlyCompanyValue(CompanyID company, uint32 quarter);
if quarter is equal to 0 (AICompany::CURRENT_QUARTER) it will display the current value (except for the Performance Rating, because it is calculated at the end of every quarter).
I have not updated the regression test, but I'll do so once people are happy with the code provided. Note that int32 GetQuarterlyPerformanceRating(CompanyID company, uint32 quarter); makes AICompany::GetPerformanceRating(CompanyID company) redundant - I haven't removed it but if this patch gets accepted I suspect the latter will eventually be phased out.
Hope it gets included
.
Bram
Re: API change: Get quarterly company information.
Posted: 10 Jun 2011 17:47
by planetmaker
I think that
Code: Select all
+ * @pre ResolveCompanyID(company) != COMPANY_INVALID.
is actually not true. You handle every CompanyID, including COMPANY_INVALID within your code. Even though current code doesn't always follow it: the doxygen comments / documentation should be near the actual code, not in the header files (i.e. doxygen code goes in ai_company.cpp).
And... I think adding regression test update also initially doesn't hurt
It's easily forgotten.
Re: API change: Get quarterly company information.
Posted: 10 Jun 2011 19:05
by Morloth
Hi planetmaker, thanks for your comments.
planetmaker wrote:I think that
Code: Select all
+ * @pre ResolveCompanyID(company) != COMPANY_INVALID.
is actually not true. You handle every CompanyID, including COMPANY_INVALID within your code. Even though current code doesn't always follow it: the doxygen comments / documentation should be near the actual code, not in the header files (i.e. doxygen code goes in ai_company.cpp).
I don't see any doxygen comments in the actual implementation - only in the declarations. I'm quite sure they should be in the header files. Also the @pre should be included, if you look at the other functions they also state the same requirement but handle an invalid company in the same way.
planetmaker wrote:
And... I think adding regression test update also initially doesn't hurt
It's easily forgotten.
Very true.
Re: API change: Get quarterly company information.
Posted: 10 Jun 2011 20:09
by planetmaker
Morloth wrote:planetmaker wrote:Even though current code doesn't always follow it: the doxygen comments / documentation should be near the actual code, not in the header files (i.e. doxygen code goes in ai_company.cpp).
I don't see any doxygen comments in the actual implementation
No need to continue the "wrong" way.
http://wiki.openttd.org/Coding_style#Documentation wrote: The documentation should be as close to the actual code as possible to maximise the chance of staying in sync.
Comments for functions go in the .cpp file.
Comments for inlines go in the .h/.hpp file.
Re: API change: Get quarterly company information.
Posted: 10 Jun 2011 20:20
by Yexo
Planetmaker: in this particular case Morloth is correct by putting the documentation in the header file. The AI API has all function-level documentation in the header files and the code in the cpp files. The cpp files only have some lines of documentation where it's needed to clarity the code.
Edit: haven't looked at your patch yet, but I'll take a look tomorrow.
Re: API change: Get quarterly company information.
Posted: 11 Jun 2011 13:20
by Morloth
Updated patch, included the regression test.
Re: API change: Get quarterly company information.
Posted: 11 Jun 2011 13:35
by Yexo
I'd write the functions like this, checking all preconditions first, returning results later.
Code: Select all
/* static */ Money AICompany::GetQuarterlyIncome(AICompany::CompanyID company, uint32 quarter)
{
company = ResolveCompanyID(company);
if (company == COMPANY_INVALID) return -1;
if (quarter > EARLIEST_QUARTER) return -1;
if (quarter == CURRENT_QUARTER) {
return ::Company::Get((::CompanyID)company)->cur_economy.income;
}
return ::Company::Get((::CompanyID)company)->old_economy[quarter - 1].income;
}
Note also the cast to (::CompanyID) instead of (CompanyID). This is to make sure we cast to the globally defined CompanyID, not to AICompany::CompanyID.
Morloth wrote:(except for the Performance Rating, because it is calculated at the end of every quarter).
Please make this an explicit precondition and check for it.
Note that int32 GetQuarterlyPerformanceRating(CompanyID company, uint32 quarter); makes AICompany::GetPerformanceRating(CompanyID company) redundant - I haven't removed it but if this patch gets accepted I suspect the latter will eventually be phased out.
I can't find AICompany::GetPerformanceRating at all. Any hints to where I can find it?
Hope it gets included
.
The patch looks good, so if you could fix the small points above it certainly will.
Re: API change: Get quarterly company information.
Posted: 11 Jun 2011 15:41
by Morloth
Yexo wrote:I'd write the functions like this, checking all preconditions first, returning results later.
Code: Select all
/* static */ Money AICompany::GetQuarterlyIncome(AICompany::CompanyID company, uint32 quarter)
{
company = ResolveCompanyID(company);
if (company == COMPANY_INVALID) return -1;
if (quarter > EARLIEST_QUARTER) return -1;
if (quarter == CURRENT_QUARTER) {
return ::Company::Get((::CompanyID)company)->cur_economy.income;
}
return ::Company::Get((::CompanyID)company)->old_economy[quarter - 1].income;
}
Note also the cast to (::CompanyID) instead of (CompanyID). This is to make sure we cast to the globally defined CompanyID, not to AICompany::CompanyID.
Are you sute about this? Because other functions like GetCompanyValue, GetBankBallance, and AutoRenew* have the same cast I used in my patch. Would you like me to change those too?
Yexo wrote:Morloth wrote:(except for the Performance Rating, because it is calculated at the end of every quarter).
Please make this an explicit precondition and check for it.
Done.
Yexo wrote:Note that int32 GetQuarterlyPerformanceRating(CompanyID company, uint32 quarter); makes AICompany::GetPerformanceRating(CompanyID company) redundant - I haven't removed it but if this patch gets accepted I suspect the latter will eventually be phased out.
I can't find AICompany::GetPerformanceRating at all. Any hints to where I can find it?
Sorry I meant AICompany::GetCompanyValue.
Yexo wrote:Hope it gets included
.
The patch looks good, so if you could fix the small points above it certainly will.
Sweet!
Please find the new patch in the first post (v3).
Re: API change: Get quarterly company information.
Posted: 11 Jun 2011 21:33
by Yexo
Morloth wrote:Yexo wrote:
Note also the cast to (::CompanyID) instead of (CompanyID). This is to make sure we cast to the globally defined CompanyID, not to AICompany::CompanyID.
Are you sute about this? Because other functions like GetCompanyValue, GetBankBallance, and AutoRenew* have the same cast I used in my patch. Would you like me to change those too?
Hmm, the current code is already inconsistent there, I didn't see that at first. Leave it however it is now in your patch and it can be made consistent later.
Yexo wrote:Note that int32 GetQuarterlyPerformanceRating(CompanyID company, uint32 quarter); makes AICompany::GetPerformanceRating(CompanyID company) redundant - I haven't removed it but if this patch gets accepted I suspect the latter will eventually be phased out.
I can't find AICompany::GetPerformanceRating at all. Any hints to where I can find it?
Sorry I meant AICompany::GetCompanyValue.[/quote]Since this patch makes GetCompanyValue obsolete, please remove it completely. In bin/ai/compat_0.7.nut (and 1.0 / 1.1, but not 1.2) add the following code:
Code: Select all
AICompany.GetCompanyValue <- function(company)
{
return AICompany.GetQuarterlyCompanyValue(company, AICompany.QUARTER_CURRENT);
}
Also note this change in ai_changelog.hpp and adjust the regression test. After that I'd say your patch is done
Re: API change: Get quarterly company information.
Posted: 12 Jun 2011 18:48
by Morloth
Updated the patch to v4, thanks for your feedback
.
Re: API change: Get quarterly company information.
Posted: 13 Jun 2011 15:24
by Yexo
I added the changes to ai_changelog.hpp and committed your patch in r22584. Thanks for a very nice patch
Re: API change: Get quarterly company information.
Posted: 13 Jun 2011 16:21
by Morloth
Thank you very much for including it!
Re: API change: Get quarterly company information.
Posted: 13 Jun 2011 19:51
by Zuu
Thanks for the work with this patch. I think it is useful for the current (but stalled) work with PAXLink where i manually tried to compute the income and profit.