as said, other devs that are as experienced as I am disagree with this last
point. So it is more bike shedding and should not be overrated. Other points
are more clear cut.
I suggest reading effective c++ and effective c++11 if you want to learn more
about good code design - both are really exc
I like learning good coe design though, so I'll put it on the todo-list :)
--
https://code.launchpad.net/~widelands-dev/widelands/one_tribe/+merge/274832
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/one_tribe.
___
> This function is used in the GameSettingsProvider classes before an egbase is
> created. Is there a better design than using static for this?
IMHO, yes. Pull the method out into a stand alone function. It is more modular,
and less coupled (static methods can access private static members and p
The proposal to merge lp:~widelands-dev/widelands/one_tribe into lp:widelands
has been updated.
Status: Needs review => Merged
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/one_tribe/+merge/274832
--
Your team Widelands Developers is subscribed to branch
lp:~wi
I have a question regarding this comment:
// NOCOM(#codereview): no need to be static?
std::vector Tribes::get_all_tribeinfos()
This function is used in the GameSettingsProvider classes before an egbase is
created. Is there a better design than using static for this?
--
https://code.launchpad.
Thanks for the review!
Regarding the lookup tables
http://bazaar.launchpad.net/~widelands-dev/widelands/one_tribe/revision/7464
We do need the compatibility, because map loading has a building packet etc. I
only included those entities in it that can't be avoided for map loading. I
decided to r
Review: Approve
> Renaming "outputs" to "occupants" won't work, because this is a feature of
> production sites in general and not just militarysites.
Add a TODO that militarysite should not be a productionsite? Outputs is really
weird for militarysites.
Okay, done with the review. I added a
[Num Glob vs list_directory]
I doubt that the regex matching is the slow part, but I can understand that
listing all files in the directories and then filtering the ones we are not
interested in is slow. The numglob is only checking if files exist, which will
be faster.
But I just grepped the
Not yet done. I reviewed everything but src/logic/, I cannot concentrate
anymore now though. I'll finish the rest tomorrow.
--
https://code.launchpad.net/~widelands-dev/widelands/one_tribe/+merge/274832
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev
Regarding
http://bazaar.launchpad.net/~widelands-dev/widelands/one_tribe/revision/7452#tribes/buildings/militarysites/atlanteans/guardhouse/init.lua
Renaming "outputs" to "occupants" won't work, because this is a feature of
production sites in general and not just militarysites.
--
https://code
Still working on the review. Have another long train ride today and hope to get
it done then. I'll get back to this thread then.
In general I find very little to complain about in the code - really great job
:)
> Am 01.11.2015 um 11:01 schrieb GunChleoc :
>
> Some comments to the code revie
Some comments to the code review - posting them here rather than in the code in
order not change the branch, in case you're still working on it:
Regarding
http://bazaar.launchpad.net/~widelands-dev/widelands/one_tribe/revision/7452#tribes/init.lua
We do need the number glob for parsing the anim
12 matches
Mail list logo
