# Code Style Changes: Finding Best Bang for The Buck

**URL:** <https://discourse.cataclysmdda.org/t/code-style-changes-finding-best-bang-for-the-buck/649>\
**Category:** The Bunker\
**Created:** [March 30, 2013, 8:49pm UTC](https://discourse.cataclysmdda.org/t/code-style-changes-finding-best-bang-for-the-buck/649 "2013-03-30T20:49:57Z")\
**Posts on this page:** 8\
**Page:** 1

<div class="post-metadata">

**Author:** ![maseck](https://avatars.discourse-cdn.com/v4/letter/m/2bfe46/32.png) [@maseck](https://discourse.cataclysmdda.org/u/maseck)\
**Post date:** [March 30, 2013, 8:49pm UTC](https://discourse.cataclysmdda.org/t/code-style-changes-finding-best-bang-for-the-buck/649/1 "2013-03-30T20:49:57Z")

</div>

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]

---

<div class="post-metadata">

**Author:** ![Soron](https://sea2.discourse-cdn.com/flex016/user_avatar/discourse.cataclysmdda.org/soron/32/885_2.png) [@Soron](https://discourse.cataclysmdda.org/u/Soron)\
**Post date:** [March 30, 2013, 10:18pm UTC](https://discourse.cataclysmdda.org/t/code-style-changes-finding-best-bang-for-the-buck/649/2 "2013-03-30T22:18:26Z")

</div>

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.

---

<div class="post-metadata">

**Author:** ![Williham](https://sea2.discourse-cdn.com/flex016/user_avatar/discourse.cataclysmdda.org/williham/32/845_2.png) [@Williham](https://discourse.cataclysmdda.org/u/Williham)\
**Post date:** [March 30, 2013, 10:26pm UTC](https://discourse.cataclysmdda.org/t/code-style-changes-finding-best-bang-for-the-buck/649/3 "2013-03-30T22:26:41Z")

</div>

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

---

<div class="post-metadata">

**Author:** ![GlyphGryph](https://sea2.discourse-cdn.com/flex016/user_avatar/discourse.cataclysmdda.org/glyphgryph/32/847_2.png) [@GlyphGryph](https://discourse.cataclysmdda.org/u/GlyphGryph)\
**Post date:** [March 31, 2013, 3:29am UTC](https://discourse.cataclysmdda.org/t/code-style-changes-finding-best-bang-for-the-buck/649/4 "2013-03-31T03:29:42Z")

</div>

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.

---

<div class="post-metadata">

**Author:** ![maseck](https://avatars.discourse-cdn.com/v4/letter/m/2bfe46/32.png) [@maseck](https://discourse.cataclysmdda.org/u/maseck)\
**Post date:** [April 8, 2013, 9:44pm UTC](https://discourse.cataclysmdda.org/t/code-style-changes-finding-best-bang-for-the-buck/649/5 "2013-04-08T21:44:21Z")

</div>

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]

---

<div class="post-metadata">

**Author:** ![Soron](https://sea2.discourse-cdn.com/flex016/user_avatar/discourse.cataclysmdda.org/soron/32/885_2.png) [@Soron](https://discourse.cataclysmdda.org/u/Soron)\
**Post date:** [April 8, 2013, 11:14pm UTC](https://discourse.cataclysmdda.org/t/code-style-changes-finding-best-bang-for-the-buck/649/6 "2013-04-08T23:14:37Z")

</div>

I think this sort of in-depth, behind-the-scenes code proposal should probably happen over [on the Cataclysm:DDA Github repository](https://github.com/TheDarklingWolf/Cataclysm-DDA), 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.

---

<div class="post-metadata">

**Author:** ![kevin.granade](https://sea2.discourse-cdn.com/flex016/user_avatar/discourse.cataclysmdda.org/kevin.granade/32/1316_2.png) [@kevin.granade](https://discourse.cataclysmdda.org/u/kevin.granade)\
**Post date:** [April 10, 2013, 1:12am UTC](https://discourse.cataclysmdda.org/t/code-style-changes-finding-best-bang-for-the-buck/649/7 "2013-04-10T01:12:47Z")

</div>

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.)

---

<div class="post-metadata">

**Author:** ![maseck](https://avatars.discourse-cdn.com/v4/letter/m/2bfe46/32.png) [@maseck](https://discourse.cataclysmdda.org/u/maseck)\
**Post date:** [April 10, 2013, 1:26am UTC](https://discourse.cataclysmdda.org/t/code-style-changes-finding-best-bang-for-the-buck/649/8 "2013-04-10T01:26:08Z")

</div>

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](http://dl.dropbox.com/u/10554836/TestModule.zip), 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.