• Announcements

    • khawk

      Download the Game Design and Indie Game Marketing Freebook   07/19/17

      GameDev.net and CRC Press have teamed up to bring a free ebook of content curated from top titles published by CRC Press. The freebook, Practices of Game Design & Indie Game Marketing, includes chapters from The Art of Game Design: A Book of Lenses, A Practical Guide to Indie Game Marketing, and An Architectural Approach to Level Design. The GameDev.net FreeBook is relevant to game designers, developers, and those interested in learning more about the challenges in game development. We know game development can be a tough discipline and business, so we picked several chapters from CRC Press titles that we thought would be of interest to you, the GameDev.net audience, in your journey to design, develop, and market your next game. The free ebook is available through CRC Press by clicking here. The Curated Books The Art of Game Design: A Book of Lenses, Second Edition, by Jesse Schell Presents 100+ sets of questions, or different lenses, for viewing a game’s design, encompassing diverse fields such as psychology, architecture, music, film, software engineering, theme park design, mathematics, anthropology, and more. Written by one of the world's top game designers, this book describes the deepest and most fundamental principles of game design, demonstrating how tactics used in board, card, and athletic games also work in video games. It provides practical instruction on creating world-class games that will be played again and again. View it here. A Practical Guide to Indie Game Marketing, by Joel Dreskin Marketing is an essential but too frequently overlooked or minimized component of the release plan for indie games. A Practical Guide to Indie Game Marketing provides you with the tools needed to build visibility and sell your indie games. With special focus on those developers with small budgets and limited staff and resources, this book is packed with tangible recommendations and techniques that you can put to use immediately. As a seasoned professional of the indie game arena, author Joel Dreskin gives you insight into practical, real-world experiences of marketing numerous successful games and also provides stories of the failures. View it here. An Architectural Approach to Level Design This is one of the first books to integrate architectural and spatial design theory with the field of level design. The book presents architectural techniques and theories for level designers to use in their own work. It connects architecture and level design in different ways that address the practical elements of how designers construct space and the experiential elements of how and why humans interact with this space. Throughout the text, readers learn skills for spatial layout, evoking emotion through gamespaces, and creating better levels through architectural theory. View it here. Learn more and download the ebook by clicking here. Did you know? GameDev.net and CRC Press also recently teamed up to bring GDNet+ Members up to a 20% discount on all CRC Press books. Learn more about this and other benefits here.
Sign in to follow this  
Followers 0
Stavros Kokkineas

Replace part of string in C++

47 posts in this topic

Ok, so I'm making this text adventure game so i can get into the logic of basic programming and then move on to 2D probably.

I have a misfortune though. At some point in the game the player can enter his name. So, from that point on I want the texts to have the player's name.

The thing is, to have a much simpler code and more freedom in creating the game, I read the texts from external texts files. That means I read each line of the file and then print it on the screen.

So what I thought is create a sequence of characters that I can read and replace them in the program.

So I came up with the "/NAME/" sequence. So, the player enters his name, "Nick" for example. The program should replace the "/NAME/" sequence with "Nick" and present the rest of the text normally.


So I wrote this code:

[code]#include <iostream>
#include <cstdio>
#include <cstdlib>
#include <string>
#include <fstream>
#include <sstream>
using namespace std;

int main (){

string line;
char name[20];
fstream file;
file.open("Rebels_after_start.txt");

cin >> name;

while(file.good()){

getline(file,line);

line.replace(line.find("/N"),line.find("E/"), name ,6);
//line.replace(19,1,1,'s'); Even with this line it crashes
cout << line << '\n';
}

file.close();

}
[/code]

But the program keeps getting an out_of_range instance from the replace function. I've tried all iterations of string.replace and nothing works. Even if I replace a character with a character the program crashes.

Any help please?
0

Share this post


Link to post
Share on other sites
Are you sure that the string contains "/NAME/" ?
If not then you are trying it replace something that does not exists... meaning you change something outside the memory bounds of string and cause a random crash in your program.
0

Share this post


Link to post
Share on other sites
The exact contents of the file are the following:


[quote]So, you're name is /NAME/.

The woman with the long red hair was checking him continuously. She probably was a Rebel.[/quote]

The output I get every time is this:

[quote]Nick
So, you're name is Nick
terminate called after throwing an instance of 'std::out_of_range'
what(): basic_string::replace
Aborted


------------------
(program exited with code: 134)
Press return to continue

[/quote]

What it should give me is this:

[quote]So, you're name is Nick.

The woman with the long red hair was checking him continuously. She probably was a Rebel.[/quote]

Also, as I have commented on the code even if I replace any character, with any other character the program crashes.

NOTE: I am developing under Linux Mint 12.Is there any chance this is a memory reallocation problem?
0

Share this post


Link to post
Share on other sites
When processing the file, you read it line by line.
You get the first line. It contains /NAME/ and your code correctly replaces it and gives the desired output.
Now the next line is read. From what you posted it is an empty line. So your call to
line.find("/N")
will return string::npos. Same for
line.find("E/")
which is then passed to replace and causes the access out of bounds. Check the return values of your find calls and do not call replace if one of them is equal to string::npos.
1

Share this post


Link to post
Share on other sites
Why dont you store the string you want to display with a %s in it. And then when it comes time to show it you sprintf the name into the buffer you use for display?
0

Share this post


Link to post
Share on other sites
That was it! Just added

[code]if (line.find("/N") != string::npos) line.replace( line.find("/N"),name.length()-1, name);[/code]

and it works flawlessly.


Just a question because i looked up iterators a little. An iterator is actually a reference to an element of a sequence?
0

Share this post


Link to post
Share on other sites
[quote name='NightCreature83' timestamp='1330523146' post='4917737'] Why dont you store the string you want to display with a %s in it. And then when it comes time to show it you sprintf the name into the buffer you use for display? [/quote]

That would be cool too. One of those cases where in C it's actually more simple. Just to completely understand what you are talking about instead of putting "/NAME/" as a "flag" I just put %s in the text file and with sprintf i get acces to the actual buffer as if used printf(%s, name) right?
0

Share this post


Link to post
Share on other sites
[quote]
Just to completely understand what you are talking about instead of putting "/NAME/" as a "flag" I just put %s in the text file and with sprintf i get acces to the actual buffer as if used printf(%s, name) right?
[/quote]

I would recommend against using format strings loaded from external locations with sprintf(). This is a really easy way to corrupt memory, if you change either the format strings or the arguments and forget to change the other, or if your data file is corrupt. You should either write a type safe wrapper around it, which validates that the format string and arguments match one another.

Not to mention that sprintf() has a fixed buffer length.

If you were going to do this, I'd recommend keeping your current format, but switch to format strings in-memory, once you've validated that the text only contains the format strings you want. This also is good for the user, who doesn't need to worry about the types involved.

[quote]
[color=#282828][font=helvetica, arial, verdana, tahoma, sans-serif][size=3][left]That was it! Just added[/left][/size][/font][/color]


[color=#000088]if[/color][color=#000000] [/color][color=#666600]([/color][color=#000000]line[/color][color=#666600].[/color][color=#000000]find[/color][color=#666600]([/color][color=#008800]"/N"[/color][color=#666600])[/color][color=#000000] [/color][color=#666600]!=[/color][color=#000000] [/color][color=#000088]string[/color][color=#666600]::[/color][color=#000000]npos[/color][color=#666600])[/color][color=#000000] line[/color][color=#666600].[/color][color=#000000]replace[/color][color=#666600]([/color][color=#000000] line[/color][color=#666600].[/color][color=#000000]find[/color][color=#666600]([/color][color=#008800]"/N"[/color][color=#666600]),[/color][color=#000000]name[/color][color=#666600].[/color][color=#000000]length[/color][color=#666600]()-[/color][color=#006666]1[/color][color=#666600],[/color][color=#000000] name[/color][color=#666600]);[/color]

[color=#282828][font=helvetica, arial, verdana, tahoma, sans-serif][size=3][left]and it works flawlessly.[/left][/size][/font][/color]
[/quote]
Why not:
[code]

#include <string>
#include <iostream>

namespace {
static const std::string NAME_PLACEHOLDER = "/NAME/";
}

void format(std::string &text, const std::string &name)
{
std::string::size_type index = text.find(NAME_PLACEHOLDER);
if (index != std::string::npos) {
text.replace(index, NAME_PLACEHOLDER.length(), name);
}
}

void test(std::string text, const std::string &name) {
format(text, name);
std::cout << text << '\n';
}

int main()
{
std::string text = "So, you're name is /NAME/.\nThe woman with the long red hair was checking him continuously. She probably was a Rebel.\n...\n";

test(text, "Alexander");
std::cout << "-----------------------\n";
test(text, "Al");
}

[/code]
Note that I tried your original code, and the output this the following:
[quote]

So, you're name is AlexanderThe woman with the long red hair was checking him continuously. She probably was a Rebel.
...

-----------------------
So, you're name is AlNAME/.
The woman with the long red hair was checking him continuously. She probably was a Rebel.
...

[/quote]
You should test edge cases, in this situation such cases include smaller than the placeholder, and larger than the placeholder.

[quote]
Just a question because i looked up iterators a little. An iterator is actually a reference to an element of a sequence?
[/quote]
Yes. It is an abstraction of the pointer/index based way of accessing elements of a sequence.
0

Share this post


Link to post
Share on other sites
[quote name='rip-off' timestamp='1330526213' post='4917761']
I would recommend against using format strings loaded from external locations with sprintf(). This is a really easy way to corrupt memory, if you change either the format strings or the arguments and forget to change the other, or if your data file is corrupt. You should either write a type safe wrapper around it, which validates that the format string and arguments match one another.

Not to mention that sprintf() has a fixed buffer length.
[/quote]

I think the latter is the more pressing concern, and it's easily remedied by using snprintf() instead (part of the C99 and C++11 standards, and most likely provided by your compiler even if you are still using C89 or C++98).

You may also want to take a look at the [url="http://www.boost.org/doc/libs/1_49_0/libs/format/"]Boost Format library[/url].
0

Share this post


Link to post
Share on other sites
[quote name='Darklink_' timestamp='1330523686' post='4917741']
That was it! Just added

[code]if (line.find("/N") != string::npos) line.replace( line.find("/N"),name.length()-1, name);[/code]

and it works flawlessly.
[/quote]

That's something you should wrap in a function, for future ease-of-use. [img]http://public.gamedev.net//public/style_emoticons/default/smile.png[/img]

Is it intentional that you aren't searching for "/NAME/" but searching for [i]anything[/i] that begins with "/N" and ends with "E/"? For example, "/NEW GAME/", you want to have replaced with the player's name?

Also, rhetorical question: What happens if a single line uses "/NAME/" twice? [img]http://public.gamedev.net//public/style_emoticons/default/wink.png[/img]

You might want to make your own function like this:

[CODE]std::string ReplaceAll(const std::string &text, const std::string &toReplace, const std::string &replaceWith)
{
std::string newText = text;

size_t pos = newText.find(toReplace);
while(pos != std::string::npos)
{
newText.replace(pos, toReplace.size(), replaceWith);
//Just to make sure we don't go into an infinite loop, if 'replaceWith' contains a match of 'toReplace'.
pos += replaceWith.size();
pos = newText.find(toReplace, pos);
}

return newText;
}
[/CODE]

Then you can just go like this, to your heart's content:
[code]std::string text = "Hello /NAME/, you're looking mighty spry for a /CLASS/ of /AGE/ years.\n"
"I see you are still dying your hair /HAIR-COLOR/, which is mightily becoming of a /RACE/ like yourself.\n"
"You're putting on a few pounds, however. You look like you weigh about /WEIGHT/ lbs,\n"
"and your belly is bulging out quite alot. Is that a size /PANT-SIZE/ pantaloons you are wearing?\n"
"You need to get into shape before your journey, /NAME/, if you don't mind me pointing out the obvious.\n";

text = ReplaceAll(text, "/NAME/", playerName);
text = ReplaceAll(text, "/AGE/", playerAge);
text = ReplaceAll(text, "/WEIGHT/", playerAge);
text = ReplaceAll(text, "/CLASS/", playerClass);
text = ReplaceAll(text, "/RACE/", playerRace);
text = ReplaceAll(text, "/HAIR-COLOR/", playerHairColor);
text = ReplaceAll(text, "/PANT-SIZE/", playerPantSize);

std::cout << text << std::endl;[/code]
0

Share this post


Link to post
Share on other sites
What Servant of the lord proposed is a huge improvement in readability, but substituting strings is expensive. I think it would be even cleaner and much more efficient to fill out a map (or unordered_map) from tag to value and then use a function that does all the substitutions in one pass.

[code]std::map<std::string, std::string> tags;
tags["NAME"] = playerName;
tags["AGE"] = playerAge;
//...

std::string template = "Hello /NAME/, you're looking mighty spry for a /CLASS/ of /AGE/ years. [...]";

std::string text = apply_template(template, tags);
[/code]
0

Share this post


Link to post
Share on other sites
[quote name='alvaro' timestamp='1330545199' post='4917922']
What Servant of the lord proposed is a huge improvement in readability, but substituting strings is expensive.
[/quote]
Premature optimization (on a text-based game) leads the OP down roads that are wastes of time... not program execution time, but the OP's development time. You're trading off program simplicity so the OP can understand it (Important!) for unnecessary and unneeded program execution speed (Not important). In the current situation, it's not a trade-off that's worth doing.

Besides, it's just as easy to alter the string inplace with a minor modification, if desired.
[code]void ReplaceAll(std::string &textToModify, const std::string &toReplace, const std::string &replaceWith)
{
size_t pos = textToModify.find(toReplace);
while(pos != std::string::npos)
{
textToModify.replace(pos, toReplace.size(), replaceWith);
//Just to make sure we don't go into an infinite loop, if 'replaceWith' contains a match of 'toReplace'.
pos += replaceWith.size();
pos = textToModify.find(toReplace, pos);
}
}[/code]

It actually makes the code cleaner as well, but with the removal of the ability of using the function directly in a parameter of another function.
1

Share this post


Link to post
Share on other sites
[quote name='Servant of the Lord' timestamp='1330545628' post='4917925']
[quote name='alvaro' timestamp='1330545199' post='4917922']
What Servant of the lord proposed is a huge improvement in readability, but substituting strings is expensive.
[/quote]
Premature optimization (on a text-based game) leads the OP down roads that are wastes of time... not program execution time, but the OP's development time. You're trading off program simplicity so the OP can understand it (Important!) for unnecessary and unneeded program execution speed (Not important). In the current situation, it's not a trade-off that's worth doing.
[/quote]

The OP should develop a sense for what work is involved in what he is doing, and this is a good opportunity to realize that your implementation takes time O(n*k) to run, where n is the number of characters in the input and k is the number of tags (assuming tags and values are reasonably short), while the implementation I propose takes time O(n*log(k)), or even O(n) if you use unordered_map.

However, I proposed the alternative primarily because having alternatives is a good thing, and because I think the resulting calling code is actually more readable (it better expresses the intent of the code, leaving the details of whether you substitute the words one by one or all at the same time to the implementation).

Also, try not to get so defensive: I started by praising your proposal. [img]http://public.gamedev.net//public/style_emoticons/default/smile.png[/img]
0

Share this post


Link to post
Share on other sites
[quote]
... it's easily remedied by using snprintf() instead.
[/quote]
This prevents buffer overruns, but doesn't correct the behaviour.

[quote]
The OP should develop a sense for what work is involved in what he is doing, and this is a good opportunity to realize that your implementation takes time O(n*k) to run, where n is the number of characters in the input and k is the number of tags (assuming tags and values are reasonably short), while the implementation I propose takes time O(n*log(k)), or even O(n) if you use unordered_map.
[/quote]
Pity the poor user who is faced with a description long enough to warrant Big O analysis!
0

Share this post


Link to post
Share on other sites
[quote name='alvaro' timestamp='1330547071' post='4917934']
Also, try not to get so defensive: I started by praising your proposal. [img]http://public.gamedev.net//public/style_emoticons/default/smile.png[/img]
[/quote]
Good advice. I do seem to be rather defensive lately. [img]http://public.gamedev.net//public/style_emoticons/default/mellow.png[/img]
0

Share this post


Link to post
Share on other sites
Ok, I am really tired to do much coding now but thanks A LOT for the replies and tommorow morning I will surely improve my code. For the moment, and just for the particular use I want to use the code for this seems to work flawlessly indeed.

[code]void replace (string &amp; line, string &amp; name){

size_t pos;

pos = line.find("/NAME/");

while(pos != string::npos){

if (pos != string::npos) line.replace( (line.begin() + pos), (line.begin() + pos + 6), name);

pos += name.size()+1;
pos = line.find("/NAME/", pos);
} //while

}// replace FUNCTION

//
//
//


int main (){
...
replace(line, name);
...
}[/code]

This is a very rough solution I came up in like 10 minutes. Tommorow I will probably try all solutions suggested, though the map-tags solution seems really good.

Also some questions if you can answer please:

[code]namespace {
static const std::string NAME_PLACEHOLDER = "/NAME/";
}
[/code]

Why namespace and what is the name of the namespace?



I will look it up tommorow but just a small briefing, what are maps?


Also, I used iterators instead of the name's length as the function replaces the rest of the text if the name is larger than 6 characters.


Again, thanks a lot for you replies.
0

Share this post


Link to post
Share on other sites
Ok, so I tried almost everything mentioned although finally I didn't realize what's the use of templates as the input of the name must always be a string. Anyway, of course you can use templates if you want but I don't see the use for this particular program. It just gets more complicated for the programmer. Do we gain more speed as the template executes each time it's called? Also can you elaborate a little what OP is?

Anyway, what I finally came up with is the following code which as long as I tested it works for every case. That is if the user inputs a string with a space in between characters, if there are multiple arguments of the same type in a line and if there isn't any argument in a line.

[code]#include <iostream>
#include <cstdio>
#include <cstdlib>
#include <string>
#include <fstream>
#include <sstream>
#include <map>

using namespace std;


class ReadGameFile {

public:

string line;
fstream file;
int count;
string args[7];


void enterDetails (){

for (count = 0; count <= 6; count++){

if (count == 0){ cout << "Please enter name: "; getline(cin, args[count]); }

else if (count == 1){ cout << "Please enter age: "; getline(cin, args[count]); }

else if (count == 2){ cout << "Please enter weight: "; getline(cin, args[count]); }

else if (count == 3){ cout << "Please enter class: "; getline(cin, args[count]); }

else if (count == 4){ cout << "Please enter race: "; getline(cin, args[count]); }

else if (count == 5){ cout << "Please enter hair color: "; getline(cin, args[count]); }

else if (count == 6){ cout << "Please enter pant size: "; getline(cin, args[count]); }

}

readFile();

}
void readFile (){

file.open("Rebels_after_start.txt");

while(file.good()){

getline(file,line);

repTags(line, args);

cout << line << '\n';
}

file.close();

}
void repTags(string &line, string args[]){

static map<int,string> tags;
size_t pos = 0;

tags[0] = "/NAME/";
tags[1] = "/AGE/";
tags[2] = "/WEIGHT/";
tags[3] = "/CLASS/";
tags[4] = "/RACE/";
tags[5] = "/HAIR-COLOR/";
tags[6] = "/PANT-SIZE/";

for (count = 0; count <= 6; count++){
while ( (pos = line.find(tags[count])) != string::npos){

line.replace(pos, tags[count].size(), args[count], 0, args[count].size());
pos += args[count].size() + 1;

}// while
}//for


} // mapTEXT FUNCTION
}; // ReadGameFile CLASS

int main (){

ReadGameFile game;

game.enterDetails();

return 0;

}
[/code]


So the output I get is something like this:


[quote]Hello Commander Shepard, you're looking mighty spry for a infiltrator of 26 years.
I see you are still dying your hair (I don't have any hair... :/), which is mightily becoming of a human like yourself.
You're putting on a few pounds, however. You look like you weigh about 70 lbs, and your belly is bulging out quite alot. Is that a size 32 pantaloons you are wearing?

You need to get into shape before your journey, Commander Shepard, if you don't mind me pointing out the obvious.[/quote]


Now, I want to ask is there any other way to optimize the code and make it faster/better/more readable/more efficient?

If, so can you propose some ideas?

Again thanks a lot.
0

Share this post


Link to post
Share on other sites
[quote]
Ok, so I tried almost everything mentioned although finally I didn't realize what's the use of templates as the input of the name must always be a string. Anyway, of course you can use templates if you want but I don't see the use for this particular program. It just gets more complicated for the programmer. Do we gain more speed as the template executes each time it's called?
[/quote]
I believe alvaro was not talking about the C++ language feature when they said "template", but rather that the string featuring replacement sequences acts as a kind of "template string".

[quote]
Also can you elaborate a little what OP is?
[/quote]
OP generally stands for "Original Poster".

[quote]
Now, I want to ask is there any other way to optimize the code and make it faster/better/more readable/more efficient?
[/quote]
My comments on your current source are:
[list]
[*] The string constants ("name", "/NAME/" (etc) and the number of constants 6 is hard coded in a number of places.
[*] You have code duplication in the loop body of enterDetails
[*] Your replacement loop doesn't leverage the algorithmic gains that alvaro hinted were possible.
[*] Don't abuse [i]static[/i] here.
[/list]
In particular, for number 3 your solution is probably less efficient - you spend time building and iterating the map, but the algorithm is unchanged. The result is more work being done.

Here is my attempt at fixing some of these flaws:
[code]

#include <iostream>
#include <cstdio>
#include <cstdlib>
#include <string>
#include <fstream>
#include <sstream>
#include <map>
#include <stdexcept>
#include <iterator>

using namespace std;

const char Placeholder = '/';

const string PlayerDetails[] = {
"name", "age", "weight", "class", "race", "hair color", "pant size"
};

const unsigned NumDetails = sizeof(PlayerDetails) / sizeof(PlayerDetails[0]);

typedef map<string, string> PlayerInfo;

string makePlaceholder(const string &attribute) {
string result;
result.push_back(Placeholder);
for(string::size_type i = 0 ; i < attribute.size() ; ++i) {
char c = attribute[i];
if(c == ' ') {
c = '-';
} else {
c = toupper(c);
}
result.push_back(c);
}
result.push_back(Placeholder);
return result;
}

PlayerInfo enterDetails(istream &input, ostream &output) {
PlayerInfo result;
for(unsigned i = 0 ; i < NumDetails ; ++i) {
const string &attribute = PlayerDetails[i];
output << "Please enter " << attribute << ": ";
string line;
getline(input, line);
string placeholder = makePlaceholder(attribute);
result.insert(make_pair(placeholder, line));
}
return result;
}

string readFile(const string &filename) {
fstream stream(filename.c_str());
if(!stream) {
throw runtime_error("Failed to open: " + filename);
}
return string((istreambuf_iterator<char>(stream)), istreambuf_iterator<char>());
}

void replaceTags(string &tagged, const PlayerInfo &player) {
string::size_type begin = 0;
while( (begin = tagged.find(Placeholder, begin)) != string::npos) {
string::size_type end = tagged.find(Placeholder, begin + 1);
if(end == string::npos) {
stringstream error;
error << "No matching " << Placeholder << " at position " << begin;
throw runtime_error(error.str());
}
int length = (end - begin) + 1;
string tag = tagged.substr(begin, length);

PlayerInfo::const_iterator it = player.find(tag);
if(it == player.end()) {
stringstream error;
error << "Unknown tag: '" << tag << "' between " << begin << " and " << end;
throw runtime_error(error.str());
}
tagged.replace(begin, length, it->second);
}
}

int main() {
PlayerInfo player = enterDetails(cin, cout);

// Show player information (debug)
for(PlayerInfo::iterator i = player.begin() ; i != player.end() ; ++i) {
cout << i->first << " -> " << i->second << '\n';
}
cout << "\n---------\n";

string tagged = readFile("Rebels_after_start.txt");

cout << tagged;
std::cout << "\n---------\n";

replaceTags(tagged, player);

cout << tagged;
std::cout << "\n---------\n";
}
[/code]
Note that I have one definitive list of string constants. All other parts of the program infer information from this. So the user prompts use it, the /PLACEHOLDER/ values are built from these strings (uppercased with spaces replaced with hyphens). Even the number of constants is inferred from the size of the array.

Essentially you can now add or remove player attributes by making a single change to that array.

You can ignore the implementation of readFile() if you like. Essentially it gets "iterators" for the file, and constructs a string from these iterators. The result is the contents of the file in a string.

Perhaps the most interesting part is the implementation of replaceTags. It is [i]relatively[/i] efficient - it only makes a single pass over the input string. It looks for patterns of two forward slashes, and attempts to determine the replacement using the player map. Unknown tags or cases where only a single forward slash is found are handled via exceptions.

One important gain is that the player attribute map is built once. If we were displaying reams of text to the user, we can keep re-using the same mapping over and over.

However, the result is that the code is becoming less clear and harder to read. Some of Servant Of The Lords earlier examples were better from this point of view, it is immediately clear what this code wants to do:
[code]

text = ReplaceAll(text, "/NAME/", playerName);
text = ReplaceAll(text, "/AGE/", playerAge);
text = ReplaceAll(text, "/WEIGHT/", playerAge);
text = ReplaceAll(text, "/CLASS/", playerClass);
text = ReplaceAll(text, "/RACE/", playerRace);
text = ReplaceAll(text, "/HAIR-COLOR/", playerHairColor);
text = ReplaceAll(text, "/PANT-SIZE/", playerPantSize);
[/code]
In the above code, it is less clear that this is going to happen.
1

Share this post


Link to post
Share on other sites
Ok thank you very much. In general is it preferable to do a more efficient code or a more readable one?

In enterDetails wouldn't be better if we initialized the line variable before the for loop?

I will optimize my code and come back.

Again thanks.
0

Share this post


Link to post
Share on other sites
[quote name='alvaro' timestamp='1330547071' post='4917934'] [quote name='Servant of the Lord' timestamp='1330545628' post='4917925'] [quote name='alvaro' timestamp='1330545199' post='4917922'] What Servant of the lord proposed is a huge improvement in readability, but substituting strings is expensive. [/quote] Premature optimization (on a text-based game) leads the OP down roads that are wastes of time... not program execution time, but the OP's development time. You're trading off program simplicity so the OP can understand it (Important!) for unnecessary and unneeded program execution speed (Not important). In the current situation, it's not a trade-off that's worth doing. [/quote] Also, try not to get so defensive: I started by praising your proposal. [img]http://public.gamedev.net//public/style_emoticons/default/smile.png[/img] [/quote]

I didn't find his response defensive, just good advise. I don't recommend giving an obvious beginner (who's writing a Text game) advanced, unnecessary solutions to simple problems.
0

Share this post


Link to post
Share on other sites
[quote name='Darklink_' timestamp='1330630887' post='4918319'] Ok thank you very much. In general is it preferable to do a more efficient code or a more readable one? In enterDetails wouldn't be better if we initialized the line variable before the for loop? I will optimize my code and come back. Again thanks. [/quote]

IMO, For almost all cases here in the Beginner's forum, you should be going for readability.
0

Share this post


Link to post
Share on other sites
[quote name='Darklink_' timestamp='1330630887' post='4918319']
Ok thank you very much. In general is it preferable to do a more efficient code or a more readable one?
[/quote]

More readable code is preferable as a good default, within reason. However, there is no need to write glaringly slow algorithms where simple better ones are available. Over time you will develop an intuition for what kind of simple things that run slowly are acceptable and which ones are not. Sometimes it comes down to a matter of taste.

But if you are going to err on one end, err on the side of slow and easy to understand. If it's easy to understand, you can always make it faster if you need to.

And the most important thing (in my opinion) is clean interfaces, so that the implementation can be made complicated and fast if need be, without making the calling code any harder to read.
0

Share this post


Link to post
Share on other sites
Ok, so just to get an idea the code that rip-off posted above is efficient but not readable or is it readable enough that it can be accepted?

Also, can you explain a little further what you mean by clean interfaces? Do you mean less hard-coding and more variable usage? Or create templates for example?
0

Share this post


Link to post
Share on other sites
[quote name='Darklink_' timestamp='1330637060' post='4918367']
Ok, so just to get an idea the code that rip-off posted above is efficient but not readable or is it readable enough that it can be accepted?

Also, can you explain a little further what you mean by clean interfaces? Do you mean less hard-coding and more variable usage? Or create templates for example?
[/quote]

What I mean by clean interfaces is what rip-off did. If you read his `main', it is very clean, and you can tell what each function call is doing. That's what a clean interface provides. It makes me care much less about how complicated the code inside those functions is. Of course, this applies at every level, so it would be better if those functions were easy to read (they are not horrible), but the containment of the complexity is essential.
1

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!


Register a new account

Sign in

Already have an account? Sign in here.


Sign In Now
Sign in to follow this  
Followers 0