Code Style Changes: Finding Best Bang for The Buck

Game Talk The Bunker
1

I feel there have been a few style changes done during this project that have resulted in bug generation and ugly code. In my mind, the center piece to most of the bugs is the complete avoidance of pointers in the original code. When I was much more naive, I tried to simply change this all on my own. The problem though is that there is very little modularity. I feel that if we start improving modularity, bug resilience, and overall prettiness, it will be easier to improve more of the previous three things. This means that small yet effective changes to the code will have immense impact in the long run.

I randomly skimmed some code and the choose one biggish problem. I see a lot of extensive UI logic mixed with small amounts of game logic. Here is an example method.

[code]bool player::install_bionics(game g, it_bionic type)
{
if (type == NULL) {
debugmsg(“Tried to install NULL bionic”);
return false;
}
std::string bio_name = type->name.substr(5); // Strip off "CBM: "

WINDOW* w = newwin(25, 80, (TERMY > 25) ? (TERMY-25)/2 : 0, (TERMX > 80) ? (TERMX-80)/2 : 0);
WINDOW* w_description = newwin(3, 78, 21 + getbegy(w), 1 + getbegx(w));

werase(w);
wborder(w, LINE_XOXO, LINE_XOXO, LINE_OXOX, LINE_OXOX,
LINE_OXXO, LINE_OOXX, LINE_XXOO, LINE_XOOX );

int pl_skill = int_cur +
skillLevel(“electronics”) * 4 +
skillLevel(“firstaid”) * 3 +
skillLevel(“mechanics”) * 2;

int skint = int(pl_skill / 4);
int skdec = int((pl_skill * 10) / 4) % 10;

// Header text
mvwprintz(w, 1, 1, c_white, “Installing bionics:”);
mvwprintz(w, 1, 21, type->color, bio_name.c_str());

// Dividing bars
for (int i = 1; i < 79; i++) {
mvwputch(w, 2, i, c_ltgray, LINE_OXOX);
mvwputch(w, 20, i, c_ltgray, LINE_OXOX);
}

mvwputch(w, 2, 0, c_ltgray, LINE_XXXO); // |-
mvwputch(w, 2, 79, c_ltgray, LINE_XOXX); // -|

mvwputch(w, 20, 0, c_ltgray, LINE_XXXO); // |-
mvwputch(w, 20, 79, c_ltgray, LINE_XOXX); // -|

// Init the list of bionics
for (int i = 1; i < type->options.size(); i++) {
bionic_id id = type->options[i];
mvwprintz(w, i + 3, 1, (has_bionic(id) ? c_ltred : c_ltblue),
bionics[id].name.c_str());
}
// Helper text
mvwprintz(w, 3, 39, c_white, “Difficulty of this module: %d”,
type->difficulty);
mvwprintz(w, 4, 39, c_white, “Your installation skill: %d.%d”,
skint, skdec);
mvwprintz(w, 5, 39, c_white, “Installation requires high intelligence,”);
mvwprintz(w, 6, 39, c_white, “and skill in electronics, first aid, and”);
mvwprintz(w, 7, 39, c_white, “mechanics (in that order of importance).”);

int chance_of_success = int((100 * pl_skill) /
(pl_skill + 4 * type->difficulty));

mvwprintz(w, 9, 39, c_white, “Chance of success:”);

nc_color col_suc;
if (chance_of_success >= 95)
col_suc = c_green;
else if (chance_of_success >= 80)
col_suc = c_ltgreen;
else if (chance_of_success >= 60)
col_suc = c_yellow;
else if (chance_of_success >= 35)
col_suc = c_ltred;
else
col_suc = c_red;

mvwprintz(w, 9, 59, col_suc, “%d%%%%”, chance_of_success);

mvwprintz(w, 11, 39, c_white, “Failure may result in crippling damage,”);
mvwprintz(w, 12, 39, c_white, “loss of existing bionics, genetic damage”);
mvwprintz(w, 13, 39, c_white, “or faulty installation.”);
wrefresh(w);

if (type->id == itm_bionics_battery) { // No selection list; just confirm
mvwprintz(w, 3, 1, h_ltblue, “Battery Level +%d”, BATTERY_AMOUNT);
mvwprintz(w_description, 0, 0, c_ltblue, “
Installing this bionic will increase your total battery capacity by %d.\n
Batteries are necessary for most bionics to function. They also require a\n
charge mechanism, which must be installed from another CBM.”, BATTERY_AMOUNT);

InputEvent input;
wrefresh(w_description);
wrefresh(w);
do
input = get_input();
while (input != Confirm && input != Close);
if (input == Confirm) {
practice(“electronics”, (100 - chance_of_success) * 1.5);
practice(“firstaid”, (100 - chance_of_success) * 1.0);
practice(“mechanics”, (100 - chance_of_success) * 0.5);
int success = chance_of_success - rng(1, 100);
if (success > 0) {
g->add_msg(“Successfully installed batteries.”);
max_power_level += BATTERY_AMOUNT;
} else
bionics_install_failure(g, this, success);
werase(w);
delwin(w);
g->refresh_all();
return true;
}
werase(w);
delwin(w);
g->refresh_all();
return false;
}

int selection = 0;
InputEvent input;

do {

bionic_id id = type->options[selection];
mvwprintz(w, 3 + selection, 1, (has_bionic(id) ? h_ltred : h_ltblue),
bionics[id].name.c_str());

// Clear the bottom three lines…
werase(w_description);
// …and then fill them with the description of the selected bionic
mvwprintz(w_description, 0, 0, c_ltblue, bionics[id].description.c_str());

wrefresh(w_description);
wrefresh(w);
input = get_input();
switch (input) {

case DirectionS:
mvwprintz(w, 2 + selection, 0, (has_bionic(id) ? c_ltred : c_ltblue),
bionics[id].name.c_str());
if (selection == type->options.size() - 1)
selection = 0;
else
selection++;
break;

case DirectionN:
mvwprintz(w, 2 + selection, 0, (has_bionic(id) ? c_ltred : c_ltblue),
bionics[id].name.c_str());
if (selection == 0)
selection = type->options.size() - 1;
else
selection–;
break;

}
if (input == Confirm && has_bionic(id)) {
popup(“You already have a %s!”, bionics[id].name.c_str());
input = Nothing;
}
} while (input != Cancel && input != Confirm);

if (input == Confirm) {
practice(“electronics”, (100 - chance_of_success) * 1.5);
practice(“firstaid”, (100 - chance_of_success) * 1.0);
practice(“mechanics”, (100 - chance_of_success) * 0.5);
bionic_id id = type->options[selection];
int success = chance_of_success - rng(1, 100);
if (success > 0) {
g->add_msg(“Successfully installed %s.”, bionics[id].name.c_str());
add_bionic(id);
} else
bionics_install_failure(g, this, success);
werase(w);
delwin(w);
g->refresh_all();
return true;
}
werase(w);
delwin(w);
g->refresh_all();
return false;
}[/code]

2

I suspect this should go in a different forum?

That being said, I’ve actually noticed the same sorts of issues when I started playing with the code. In my case, I had to duplicate a fair amount of code in order to provide visual feedback about whether a certain item supports a certain action, because the code that checked feasibility was completely intermixed with message logging code (among other things). I think I’ve got an idea about how to fix that, though.

The player::eat function also appears to be some ungodly monolith, but luckily I didn’t have to tinker with it too much for a bugfix I just pushed.

3

If you wanted to tell the devs that the code is ugly, mission accomplished, but we already knew that.

Still, code beautification is done on an “as-needed” basis, because large-scale code style changes are problematic.

Feel free to join us at irc:newnet.net/#Cataclysm-DDA

4

I’ll add that the largest single refactor of the code to-date was committed today. And while it is merely a single step, I think we’re all aware that modularity is sorely needed moving forward.

But change is hard, and slow. The only thing you can really do about that is… well… fix it, and submit the fix.

5

You might as well move this to the modding forum…
Okay, I have a method that should make code in mapgen.cpp a little more readable. I want to check if people would like the method done differently or anything.
Method signature and usage

[code]void formatted_set_terrain(map* m, const int startx, const int starty, const char* cstr, const char* pre, …);
This code
ter_set(SEEX - 1, SEEY - 2, t_tree_young);
ter_set(SEEX , SEEY - 2, t_tree_young);
ter_set(SEEX - 1, SEEY + 2, t_tree_young);
ter_set(SEEX , SEEY + 2, t_tree_young);
ter_set(SEEX - 2, SEEY - 1, t_tree_young);
ter_set(SEEX - 2, SEEY , t_tree_young);
ter_set(SEEX + 2, SEEY - 1, t_tree_young);
ter_set(SEEX + 2, SEEY , t_tree_young);
turns into this
formatted_set_terrain(m, SEEX-2, SEEY-2,
“
t t\n
t t\n
\n
t t\n
t t\n
”, “(bind){t}”, t_tree_young);

And this
ter_set(SEEX - 1, SEEY - 2, t_tree_young);
ter_set(SEEX , SEEY - 2, t_tree_young);
ter_set(SEEX - 1, SEEY + 2, t_tree_young);
ter_set(SEEX , SEEY + 2, t_tree_young);
ter_set(SEEX , SEEY , t_water_sh);
ter_set(SEEX - 2, SEEY - 1, t_tree_young);
ter_set(SEEX - 2, SEEY , t_tree_young);
ter_set(SEEX + 2, SEEY - 1, t_tree_young);
ter_set(SEEX + 2, SEEY , t_tree_young);
turns into this
formatted_set_terrain(m, SEEX-2, SEEY-2,
“
t t\n
t t\n
w\n
t t\n
t t\n
”, “(bind){t, w}”, t_tree_young, t_water_sh);
[/code]

I haven’t tested it yet but here is the code for the method.

[code]void formatted_set_terrain(map* m, const int startx, const int starty, const char* cstr, const char* pre, …)
{
const char* p = pre;
std::vector toBind; // a list of characters that will be binded
while(*p != 0) {
while(*p != ‘(’) {
assert(*p == ’ ');
p++;
}
p++;
std::string command;
while(*p != ‘)’) {
assert(*p != 0);
assert(*p != ’ ');
command += *p++;
}
p++;
while(*p != ‘{’) {
assert(*p == ’ ');
p++;
}
p++;
std::string param_string;
while(*p != ‘}’) {
assert(*p != 0);
if(*p != ’ ')
param_string += *p;
p++;
}
if(command == “bind”) {
bool getNewBind = true; // are we currently looking for a new character to bind?
for(int i = 0; i < param_string.size(); i++) {
if(param_string[i] == ‘,’) {
assert(!getNewBind);
getNewBind = true;
}
else
toBind.push_back(param_string[i]);
}
}
else
assert(false); // the command does not exist
}
std::map<char, ter_id> bindings;

bindings[’ '] = t_null;
va_list vl;
va_start(vl,pre);
for(int i = 0; i < toBind.size(); i++) {
bindings[toBind[i]] = (ter_id)va_arg(vl,int);
}
va_end(vl);

p = cstr;
int x = startx;
int y = starty;
while(*p != 0) {
if(*p == ‘\n’) {
y++;
x = startx;
}
else {
ter_id id = bindings[*p];
if(id != t_null)
m->ter_set(x, y, id);
x++;
}
p++;
}
}[/code]
I really should go and test this. I’m not entirely sure va_list will work with enums.

In the mean time, what do you think. I know using a string to specify the bindings might look a little verbose. In the future, I want to add a few more options and I’m not quite sure what would be better for that.

EDIT: After slightly modifing the method, it works.

[code]void formatted_set_terrain(map* m, const int startx, const int starty, const char* cstr, const char* pre, …)
{
const char* p = pre;
std::vector toBind; // a list of characters that will be binded
while(*p != 0) {
while(*p != ‘(’) {
assert(*p == ’ ');
p++;
}
p++;
std::string command;
while(*p != ‘)’) {
assert(*p != 0);
assert(*p != ’ ');
command += *p++;
}
p++;
while(*p != ‘{’) {
assert(*p == ’ ');
p++;
}
p++;
std::string param_string;
while(*p != ‘}’) {
assert(*p != 0);
if(*p != ’ ')
param_string += *p;
p++;
}
if(command == “bind”) {
bool getNewBind = true; // are we currently looking for a new character to bind?
for(int i = 0; i < param_string.size(); i++) {
if(param_string[i] == ‘,’) {
assert(!getNewBind);
getNewBind = true;
}
else
toBind.push_back(param_string[i]);
}
p++;
}
else
assert(false); // the command does not exist
}
std::map<char, ter_id> bindings;

bindings[’ '] = t_null;
va_list vl;
va_start(vl,pre);
for(int i = 0; i < toBind.size(); i++)
bindings[toBind[i]] = (ter_id)va_arg(vl,int);
va_end(vl);

p = cstr;
int x = startx;
int y = starty;
while(*p != 0) {
if(*p == ‘\n’) {
y++;
x = startx;
}
else {
ter_id id = bindings[*p];
if(id != t_null)
m->ter_set(x, y, id);
x++;
}
p++;
}
}[/code]

6

I think this sort of in-depth, behind-the-scenes code proposal should probably happen over on the Cataclysm:DDA Github repository, since it’s easier to compare different versions of code that way, and the project maintainers get pinged about stuff that happens on Git.

Feel free to hop on IRC if you need help figuring out how Github works, btw.

7

I like the formatted terrain thing, that will likely make some things clearer in mapgen.cpp.

As far as tacking the intertwined display and logic code, that’s a rather difficult problem, and we need to proceed there incrementally.
I think as an intermediate step we can extract the command code from game.cpp into it’s own layer, hopefully keeping the user interaction code somewhat centralized.

After that some more drawing primitives would be nice, though sometimes you really do want a custom widget of some kind (the view nearby items dialog is a good example of this.)

8

I got rid of the second string parameter and replaced it with something else.

Here’s the demo code

mapf::formatted_set_terrain(m, 0, 0, // 3 5 7 9 // 4 6 8 A 2 4 6 8 A 2 4 "\ xxxxxxxxxxxxxxxxxxxxxxxx\n\ |-+------------------+-|\n\ | . . 7 . . |\n\ | . . . . |\n\ |# . ..... . #|\n\ |# . . #|\n\ |# . . #|\n\ |# . . #|\n\ |# . . #|\n\ |# . . #|\n\ |# . . #|\n\ |......................|\n\ |# . . #|\n\ |# . . #|\n\ |# . . #|\n\ |# . . #|\n\ |# . . #|\n\ |# . . #|\n\ |# . ..... . #|\n\ | . . . . |\n\ | . . 7 . . |\n\ |-+------------------+-|\n\ \n\n", mapf::basic_bind(". 7 # | - +", t_pavement_y, t_backboard, t_bench, t_chainfence_v, t_chainfence_h, t_chaingate_l), mapf::simple_method_bind("x", &grass_or_dirt), mapf::end() );

The rest of the test project is in an here, I was feeling too lazy to add a Makefile so it’s just the cbp file. The two files that are intended to the main project are mapgenformat.h and mapgenformat.cpp.