Code review comment for lp:~brandesign/armagetronad/0.4-conquest-output

Revision history for this message
dlh (dlh) wrote :

== Make more translatable

I think this can be cleaned up a little. It's also difficult to translate (hardcoded " and ").

I think a better solution is: build up a list of team names, and then join all of the strings except the penultimate together (either using boost::algorithm::join or a custom implementation). The last team name would be the \4 parameter to the localized string.

So, we currently have:
* player_win_conquest_specific \1 was awarded \2 points for conquering \3's base.\n

And the new localized string for the condensed message shares the same parameters with the additional \4 parameter:
* players_win_condense_conquest \1, and \4 were awarded \2 points for conquering \3's base.\n

I think localized strings should be the full string—it's easier for translators to understand. Simply adding a new localized string for " and " can be confusing and may require looking at the source code to see how it's used.

== Should this be a configurable setting?

Does this need to be a configurable setting? And if it a setting, maybe it should be enabled by default.

== Other minor critiques

* "where awarded" should be "were awarded"
* The RequestSync() you added at the end isn't necessary.
* "score > 0 ? score : -score" is abs(score), but it looks like this was copied from eTeam::AddScore().

review: Needs Fixing

« Back to merge proposal