Author Topic: Criticize a newbie  (Read 7790 times)

0 Members and 1 Guest are viewing this topic.

I've been practising a little C++ lately - I already have some basic Java experience from university, but C++ seemed like a better language to learn (not in the least because of FSO! maybe one day...). Here's the code of my first useful program - it searches for all .fs2 files in its directory, reads every bit of text between XSTR-quotes, then writes all this to a textfile. For spellchecking purposes.

Now, since I have about one week of C++ experience so far, I'm probably doing some things terribly wrong here, and I'd like to know. Poor=performance code, cross-platform compatibility, etc. etc. Any (constructive) criticism you have would be most appreciated :)

(Note: this version of the code is outdated, an updated version is a few posts down)

Code: [Select]
#include <iostream>
#include <fstream>
#include <string>
#include <vector>
#include <dirent.h>
#include <direct.h> // _getcwd
#include <cstdlib> // system("PAUSE")

using namespace std;

class Mtxt {

private:

string alltxt;

// Constructor, does all the hard work
public: Mtxt(string mname)
{
int max_strlen = 999999; //Max length of string temp
string temp; //contains text to be added

char temp2 [7]; //for mission parsing
int pointloc; //pointer location

// Open the mission file
ifstream curmis;
curmis.open (mname);

// Check if opening is successful
if (curmis.is_open())
{
// cout << "Mission " << mname << " successfully opened." << endl;

// Now try to read the mission
while (curmis.good())
{
pointloc = (int) curmis.tellg();

if (pointloc == -1)
{
cout << "Error reading file." << endl;
break;
}

//Read next series of characters
curmis.get(temp2,7);

//If at XSTR, get the string up to the closing quote; else go to next character
if( strcmp( temp2, "XSTR(\"") == 0)
{
getline(curmis,temp,'"');
alltxt.append(temp);
alltxt.append("\n");
}
else
{
curmis.seekg(pointloc + 1,ios_base::beg);
}

//If all that's left is a newline, go to next character
while( curmis.peek() == '\n' )
{
curmis.seekg(2,ios_base::cur);
}

//Append an additional newline between mission sections
/* This triggers for every hash, also e.g. table entries.
May want to fix that at some point, but it's just cosmetic. */
if (curmis.peek() == '#')
{
alltxt.append("\n");
}
}

// Close mission file
// cout << "Closing mission " << mname << endl << endl;
curmis.close();
}
else
cout << "Error opening mission " << mname << endl;
}

// Text getter
public: string getText()
{
return alltxt;
}

}; // Class definition has a trailing semicolon!


int main()
{
vector<string> mnames; //will contain all mission names

char workdir [999];
DIR *dir;
struct dirent *entry;
string strentry;

ofstream txt;


/*******************************************************************/
// Look for missions

_getcwd(workdir,999);
if( dir = opendir(workdir) )
{
while(entry =  readdir(dir))
{
if( strcmp(entry->d_name,  ".") != 0
&& strcmp(entry->d_name, "..") != 0)
{
/* entry -> d_name is always 261 characters
  so cast to string*/
strentry = entry -> d_name;

// Only .fs2 files are interesting
if( strentry.find(".fs2") != -1)
{
// Found an .fs2 file! Now add it to the mnames vector.
mnames.push_back(strentry);
}
}
}
closedir(dir);
}


/*******************************************************************/
// Parse missions

txt.open("all.txt");

if (txt.is_open())
{
for (int i = 0; i < (int) mnames.size(); i++)
{
cout << "Working on mission " << mnames[i] << endl;

// Make mission text object for current mission
Mtxt cur_mtext (mnames[i]);

// Write the text to file
txt << cur_mtext.getText();
}

cout << "\nDone! " << flush;

}

else
{
cout << "Error opening output file" << endl;
}

system("PAUSE");
return 0;
}
« Last Edit: March 24, 2011, 06:19:01 am by FreeSpaceFreak »

 

Offline Mongoose

  • Rikki-Tikki-Tavi
  • Global Moderator
  • 212
  • This brain for rent.
    • Minecraft
    • Steam
    • Something
Re: Criticize a newbie (MTexPorter source)
I took a few semesters of C++ at school, and I remember absolutely nothing about classes, so you're already one up on me. :p

 

Offline blackhole

  • Still not over the rainbow
  • 29
  • Destiny can suck it
    • Black Sphere Studios
Re: Criticize a newbie (MTexPorter source)
In C++, brackets are almost always done on a separate line. In addition, unlike Java, C++ does not specify public/private/protected for each individual item (thank god), and classes are defined as having everything as private by default. Hence, public: should be on its own line, and you should specify access "chunks" when declaring class members, as demonstrated below.

Code: [Select]
class bar
{
  string private;
  string blah;

public:
  void thisispublic();
  void foo();
  char blar();

protected:
  int derp;
  int morederp;
  bool derpy;
};

Note that some OOP fundamentalists claim that protected is the most evil keyword ever and you should never use it. I personally think they have their head up their ass and should go **** themselves, but that's your decision.

 

Offline LHN91

  • 27
Re: Criticize a newbie (MTexPorter source)
The biggest issue with protected (at least as i remember from java 3 months ago) is that it specifies a seldom required scheme of access. 99% of the time either a public or a private class will work well as long as the code is written around it. The idea with an Object Oriented language is to keep things as compartmentalized (read *in objects*) as much as possible, and protected is a scheme of access that lets you bypass that while appearing like you aren't.

On the other hand, as far as I'm concerned, if it makes sense to use protected in your specific program, go for it.

 

Offline Spicious

  • Master Chief John-158
  • 210
Re: Criticize a newbie (MTexPorter source)
It doesn't seem sensible to use C++ for this task. It would be simpler and easier to use a scripting language.
(Style) Comments inline, some repetitions omitted:
Code: [Select]
#include <iostream>
#include <fstream>
#include <string>
#include <vector>
#include <dirent.h>
#include <direct.h> // _getcwd
#include <cstdlib> // system("PAUSE")

using namespace std;

class Mtxt {

private:

string alltxt;

// Constructor, does all the hard work
                     ^ Why?
public: Mtxt(string mname)
{
        ^ Why is this here?
int max_strlen = 999999; //Max length of string temp
                ^ static const?    ^ why?
string temp; //contains text to be added
                        ^ meaningless name

char temp2 [7]; //for mission parsing
                            ^ magic number
                 ^ Why mix string and char[]?
int pointloc; //pointer location
                        ^don't abbreviate unless it's well known, meaningless comment

// Open the mission file
ifstream curmis;
                           ^ abbreviations...
curmis.open (mname);
                           ^ no space

// Check if opening is successful
                      ^ Pointless comment
if (curmis.is_open())
{
// cout << "Mission " << mname << " successfully opened." << endl;

// Now try to read the mission
while (curmis.good())
{
pointloc = (int) curmis.tellg();
                                             ^ use C++ style cast (i.e. static_cast<int>(curmis.tellg())

if (pointloc == -1)
{
cout << "Error reading file." << endl;
break;
}

//Read next series of characters
curmis.get(temp2,7);

//If at XSTR, get the string up to the closing quote; else go to next character
if( strcmp( temp2, "XSTR(\"") == 0)
                                  ^ missing space, extra space
{
getline(curmis,temp,'"');
alltxt.append(temp);
alltxt.append("\n");
}
else
{
curmis.seekg(pointloc + 1,ios_base::beg);
}

//If all that's left is a newline, go to next character
while( curmis.peek() == '\n' )
{
curmis.seekg(2,ios_base::cur);
}

//Append an additional newline between mission sections
/* This triggers for every hash, also e.g. table entries.
May want to fix that at some point, but it's just cosmetic. */
if (curmis.peek() == '#')
{
alltxt.append("\n");
}
}

// Close mission file
                            ^ Pointless
// cout << "Closing mission " << mname << endl << endl;
curmis.close();
}
else
cout << "Error opening mission " << mname << endl;
                 ^ Don't skip braces; it won't end well.
}

// Text getter
public: string getText()
{
return alltxt;
}

}; // Class definition has a trailing semicolon!


int main()
{
vector<string> mnames; //will contain all mission names

char workdir [999];
DIR *dir;
struct dirent *entry;
string strentry;

ofstream txt;


/*******************************************************************/
// Look for missions

_getcwd(workdir,999);
if( dir = opendir(workdir) )
{
while(entry =  readdir(dir))
{
if( strcmp(entry->d_name,  ".") != 0
&& strcmp(entry->d_name, "..") != 0)
{
/* entry -> d_name is always 261 characters
  so cast to string*/
strentry = entry -> d_name;

// Only .fs2 files are interesting
if( strentry.find(".fs2") != -1)
{
// Found an .fs2 file! Now add it to the mnames vector.
mnames.push_back(strentry);
}
}
}
closedir(dir);
}


/*******************************************************************/
// Parse missions

txt.open("all.txt");

if (txt.is_open())
{
for (int i = 0; i < (int) mnames.size(); i++)
{
cout << "Working on mission " << mnames[i] << endl;

// Make mission text object for current mission
Mtxt cur_mtext (mnames[i]);

// Write the text to file
txt << cur_mtext.getText();
}

cout << "\nDone! " << flush;

}

else
{
cout << "Error opening output file" << endl;
}

system("PAUSE");
                  ^ Just don't
return 0;
}

In C++, brackets are almost always done on a separate line.
Citation needed.

 
Re: Criticize a newbie (MTexPorter source)
Okay, that's useful feedback so far, thanks :)

Question: how do I fix the issue of MSVCP100.dll missing? Adding it to the download seems like a bad idea.

 

Offline Tomo

  • 28
Re: Criticize a newbie (MTexPorter source)
Arguments about bracing style are pretty pointless.

I use:
Code: [Select]
bool MyObject::doThing()
{
   do_stuff;
};
Except in header files for trivial accessors, in which case I put the whole lot on one line:
Code: [Select]
bool getEnabled() const {return m_enabled;}- If it doesn't fit on one line, then it's not staying in the header because it's not trivial enough.

One reason for this is that in constructors I can lay out multiply-inherited base class stuff nicely without breaking style:
Code: [Select]
MyClass::MyClass()
  : MyBaseA(),
    MyBaseB()
{
   do_my_stuff;
};

I also ensure that all loops and if - else structures etc use braces, even if they're not actually necessary. Saves issues later when the one-liner turns into two or a macro. It also helps in a source control diff as it means that you can see at a glance whether the change was to the predicate or the consequent/alternative.

Aside from that:
I would strongly suggest that you use separate header and body files. You may not have seen these yet, but they are pretty much essential to making non-trivial programs.

In general, the Constructor shouldn't really do anything much - just prepare the ground. It's there to build the object, not to process lots of data.

Either use std:string or don't. Mixing char[] and std::string really is asking for trouble - it will blow up quite spectacularly when the string stops being pure ASCII.

Variables called "temp" and "temp2" are usually foolish. Give them a descriptive name - even if they really are temporary scratch variables, they've still got a purpose, so name them appropriately.

Question: how do I fix the issue of MSVCP100.dll missing? Adding it to the download seems like a bad idea.
This DLL is a requirement of your compiler and contains a load of standard stuff that pretty much all programs need.
You can force the compiler to include it inside the program executable - this is called "Static Linking". Have a look in your compiler documentation and online and you should be able to find out how to do that.
« Last Edit: March 10, 2011, 02:35:20 pm by Tomo »

 

Offline Iss Mneur

  • 210
  • TODO:
Re: Criticize a newbie (MTexPorter source)
Okay, that's useful feedback so far, thanks :)

Question: how do I fix the issue of MSVCP100.dll missing? Adding it to the download seems like a bad idea.
Link the redist like chief did in the other thread, staticly link the runtime (search your compilers help for the details on this, MSVC it should be under the linker options for the project file), include the one dll (which Microsoft has licensed you to do), or include the redist that chief linked to.

Arguments about bracing style are pretty pointless.
Yes, a better suggestion would be to pick a style and stick with it.  The primary goal is to be consistent.  The lack of consistency (in naming, code style, etc) is one of the primary reasons that the FSO code base is so hard to follow.
"I love deadlines. I like the whooshing sound they make as they fly by." -Douglas Adams
wxLauncher 0.9.4 public beta (now with no config file editing for FRED) | wxLauncher 2.0 Request for Comments

 
Re: Criticize a newbie (MTexPorter source)
Okay, I think I got the DLL thing fixed, the EXE should now be statically linked.

I know I'm kinda stretching the purpose of a constructor here, but - of course I could make a separate readTexts() method out of it, and then call that from the constructor, but that would only make things more complicated, wouldn't it?

I'd much prefer to use strings instead of char[] , but according to the documentation, only ifstream::get can get the next n characters out of the stream, and it only works with char[]. Or am I missing out on something?

Also, what about cross-platforminess? Or is that too much to handle for beginning coders?

Latest version:

Code: [Select]
#include <iostream>
#include <fstream>
#include <string>
#include <vector>
#include <dirent.h>
#include <direct.h> // _getcwd
#include <algorithm> // sort
//#include <cstdlib> // system("PAUSE")

using namespace std;

class Mtxt {

private:

string alltxt;

// Constructor, does the hard work
public:
Mtxt(string mname)
{
string addthis; //contains text to be added

//string XSTR(" has seven characters, ends with a closing \0
//this is a char[] because ifstream::get() works like that
char parser [7];

int pointerloc;

// Open the mission file
ifstream curmis;
curmis.open (mname);

if (curmis.is_open())
{
// Now try to read the mission
while (curmis.good())
{
pointerloc = static_cast<int>(curmis.tellg());

if (pointerloc == -1)
{
cout << "Error reading file." << endl;
break;
}

//Read next series of characters
curmis.get(parser,7);

//Append tags where needed;
//skip Command briefing because currently
//indistinguishable from #Command (message source)
if (strcmp(parser,"#Brief") == 0)
{
alltxt.append("\nBRIEFING\n\n");
}

else if (strcmp(parser,"#Debri") == 0)
{
alltxt.append("\nDEBRIEFING\n\n");
}

else if (strcmp(parser,"#Objec") == 0)
{
alltxt.append("\nCARGO\n\n");
}

else if (strcmp(parser,"#Event") == 0)
{
alltxt.append("\nDIRECTIVES\n\n");
}

else if (strcmp(parser,"#Goals") == 0)
{
alltxt.append("\nGOALS\n\n");
}

else if (strcmp(parser,"#Messa") == 0)
{
alltxt.append("\nMESSAGES\n\n");
}

//If at XSTR, get the string up to the closing quote; else go to next character
else if (strcmp(parser, "XSTR(\"") == 0)
{
getline(curmis,addthis,'"');

if (addthis != "Nothing")
{
alltxt.append(addthis);
alltxt.append("\n");
}
}
else
{
curmis.seekg(pointerloc + 1,ios_base::beg);
}

//If all that's left is a newline, go to next character
while( curmis.peek() == '\n' )
{
curmis.seekg(2,ios_base::cur);
}
}
curmis.close();
}
else
{
cout << "Error opening mission " << mname << endl;
}
}

string getText()
{
return alltxt;
}
};


int main()
{
vector<string> mnames; //will contain all mission names

char workdir [999];
DIR *dir;
struct dirent *entry;
string strentry;

ofstream txt;

cout << "MTExPorter, v0.2 -- by FSF\n" << endl;

/*******************************************************************/
// Look for missions

_getcwd(workdir,999);
if( dir = opendir(workdir) )
{
while(entry =  readdir(dir))
{
if( strcmp(entry->d_name,  ".") != 0
&& strcmp(entry->d_name, "..") != 0)
{
//make string for better workability
strentry = entry -> d_name;

// Only .fs2 files are interesting
if( strentry.find(".fs2") != -1)
{
// Found an .fs2 file! Now add it to the mnames vector.
mnames.push_back(strentry);
}
}
}
closedir(dir);
}


// Sort missions
sort (mnames.begin(),mnames.end());


/*******************************************************************/
// Parse missions

txt.open("all.txt");

if (txt.is_open())
{
for (int i = 0; i < (int) mnames.size(); i++)
{
cout << "Parsing " << mnames[i] << endl;

// Make mission text object for current mission
Mtxt cur_mtext (mnames[i]);

// Write the text to file
txt << cur_mtext.getText() << "\n\nNEXT MISSION\n\n" << endl;
}
}

else
{
cout << "Error making output file" << endl;
}

//system("PAUSE");
return 0;
}

« Last Edit: March 10, 2011, 03:43:52 pm by FreeSpaceFreak »

 

Offline blackhole

  • Still not over the rainbow
  • 29
  • Destiny can suck it
    • Black Sphere Studios
Re: Criticize a newbie (MTexPorter source)
Quote
If it doesn't fit on one line, then it's not staying in the header because it's not trivial enough.

Unless you are building a template-ized class in which case the whole she-bang has to be in some sort of #include'd file.

Quote
Also, what about cross-platforminess? Or is that too much to handle for beginning coders?

That is a giant mess of spagettified monstrosities you do not want to touch for a long long time.

Quote
I'd much prefer to use strings instead of char[] , but according to the documentation, only ifstream::get can get the next n characters out of the stream, and it only works with char[]. Or am I missing out on something?

Strinsg have .c_str() that you can use to access their internal char[] array. Although, you should note that char[] and char* are the same thing, and in my experience [] is used to refer to static arrays, where in this case its a dynamic char* array. But that's semantics.

Quote
Yes, a better suggestion would be to pick a style and stick with it.  The primary goal is to be consistent.  The lack of consistency (in naming, code style, etc) is one of the primary reasons that the FSO code base is so hard to follow.

I wasn't supporting one bracing style over another - I tend to use the one that seems most appropriate for the situation. I was just pointing out that C++ tends towards having braces on separate lines, especially for classes and struct declarations.

Quote
I know I'm kinda stretching the purpose of a constructor here, but - of course I could make a separate readTexts() method out of it, and then call that from the constructor, but that would only make things more complicated, wouldn't it?

Do this. It'll save you headaches when you inevitably have like 5 or 6 different constructors who all have to do almost exactly the same thing except for a few minor changes.

Quote
99% of the time either a public or a private class will work well as long as the code is written around it.

This is my issue with people saying protected is evil. Is it technically bad practice to use protected? Yes. It breaks encapsulation. Is it PRACTICAL to put in Get/Set methods for EVERY SINGLE VARIABLE anyone would EVER HAVE TO ACCESS even when its an internal class structure used by the engine that no outside API would ever see? NO. It's a stupid waste of time that would be better spent making your program actually work then adhere to a bunch of ridiculous coding practices that need to be taken with a grain of salt before being applied. And if this actually makes your code more maintainable then you need to hire programmers that aren't idiots.

 

Offline LHN91

  • 27
Re: Criticize a newbie (MTexPorter source)
^ thus why I qualified it by dividing the reasoning behind not using protected from the practical, real world answer. That, yes, the reasoning behind not using protected is that it breaks encapsulation. At the same time, I agree, it is not always practical or necessary to follow that religiously.

If it isn't going to add significant amounts of code and difficulty to your code layout, try to not use protected. Otherwise, use it, just make sure you understand the scoping issues involved in the use of it.

 

Offline Tomo

  • 28
Re: Criticize a newbie (MTexPorter source)
Sounds like there's a lot of confusion about protected.

Protected is there for subclassing.

In pretty much every non-trivial inheritance tree there will be some stuff that *all* subclasses will want (or need) to be able to do - but that really shouldn't be available to things that aren't a subclass.

Private - Mine! Mine! my preciouussssss.
Protected - Only my subclasses can have this.
Public - Anybody can use this.

There are a lot of programs that you simply couldn't do without protected - you either end up writing the exact same method over and over (defeating the whole point), or you have to make things public that really shouldn't be available (and hence can break the application in the future).

I think you're both thinking of "Friends", which are simply not OOP. Avoid these when practical - they are never needed for an application to work as you can always do it another way - but friends may offer size and/or speed improvements in some circumstances.

 

Offline blackhole

  • Still not over the rainbow
  • 29
  • Destiny can suck it
    • Black Sphere Studios
Re: Criticize a newbie (MTexPorter source)
Sounds like there's a lot of confusion about protected.

Protected is there for subclassing.

In pretty much every non-trivial inheritance tree there will be some stuff that *all* subclasses will want (or need) to be able to do - but that really shouldn't be available to things that aren't a subclass.

Private - Mine! Mine! my preciouussssss.
Protected - Only my subclasses can have this.
Public - Anybody can use this.

There are a lot of programs that you simply couldn't do without protected - you either end up writing the exact same method over and over (defeating the whole point), or you have to make things public that really shouldn't be available (and hence can break the application in the future).

I think you're both thinking of "Friends", which are simply not OOP. Avoid these when practical - they are never needed for an application to work as you can always do it another way - but friends may offer size and/or speed improvements in some circumstances.

See, this is what I use protected for. However, I have seen a lot of stupid people on the internet specifically declaring that the protected keyword is evil and that no one should ever use it, because it still technically breaks encapsulation, its just that anyone religiously adhering to that rule is completely missing the point of programming in the first place. Friends, obviously, are a much more egregious offender of OOP, but in some cases you want one class to have access to a variable and no other class for internal engine stuff, and building an entire seperated class structure is both impractical and makes the program structure more complicated and difficult to follow.

In general, I will always take the path that results in simpler, easier to understand code structure, since the vast majority of the time this also gives you performance benefits. Of course in other cases what I'm trying to do is just weird, so I'll go with the path of least resistance.

 

Offline Goober5000

  • HLP Loremaster
  • Administrator
  • 214
    • Goober5000 Productions
Re: Criticize a newbie (MTexPorter source)
Here's the code of my first useful program - it searches for all .fs2 files in its directory, reads every bit of text between XSTR-quotes, then writes all this to a textfile. For spellchecking purposes.

This is a very useful program. :yes:

An even more useful program would be one that keeps track of the XSTR numbers, generates a new XSTR index for every XSTR that has an index of -1, and inserts this new index back into the original file.  This would be a godsend for campaign and mod translators. :)

 
Re: Criticize a newbie (MTexPorter source)
Or, basically, this? m!m already did that :P

 

Offline Goober5000

  • HLP Loremaster
  • Administrator
  • 214
    • Goober5000 Productions
Re: Criticize a newbie (MTexPorter source)
Wow, I forgot about that. :nervous:

But it can't hurt to have two of them, can it? :)  And it'll help you improve your coding skills. :yes:

 

Offline WMCoolmon

  • Purveyor of space crack
  • 213
Re: Criticize a newbie (MTexPorter source)
Understand what your options are and the pros and cons of each. Try to standardize and adopt (or create) conventions in your code as much as possible, but recognize when a specific situation warrants from breaking from convention.

For example: I always try to put {}s on their own line because it helps emphasize separate code blocks. But when there's a big list of logic statements with one line commands, I will prefer same-line brackets for readability.
Code: [Select]
if (strcmp(parser,"#Brief") == 0) {
alltxt.append("\nBRIEFING\n\n");
} else if (strcmp(parser,"#Debri") == 0) {
alltxt.append("\nDEBRIEFING\n\n");
} else if (strcmp(parser,"#Objec") == 0) {
alltxt.append("\nCARGO\n\n");
} else if (strcmp(parser,"#Event") == 0) {
alltxt.append("\nDIRECTIVES\n\n");
} else if (strcmp(parser,"#Goals") == 0) {
alltxt.append("\nGOALS\n\n");
} else if (strcmp(parser,"#Messa") == 0) {
alltxt.append("\nMESSAGES\n\n");
} else if (strcmp(parser, "XSTR(\"") == 0) {
//If at XSTR, get the string up to the closing quote; else go to next character
getline(curmis,addthis,'"');

if (addthis != "Nothing")
{
alltxt.append(addthis);
alltxt.append("\n");
}
} else {
curmis.seekg(pointerloc + 1,ios_base::beg);
}

What I like about your code:
- Good comments
- Checks for errors
- Not hard to read

This short list is not to be underestimated. These are fundamental important coding concepts that are very commonly forgotten, and which are very considerate to the people you're coding with.

Other thoughts:
- Make class names and public function names descriptive, since those are most likely what other people are using
- Avoid magic numbers. Use const variables or #defines. That gives a single place that they can change it that will also automatically update "curmis.get(parser,7);"
- Separate class member definitions from member declarations. Don't worry about inlining a function as big as Mtext.
- Put class declarations in a .hpp file with include guards
- Use a consistent capitalization convention for functions, and a consistent (preferably different) capitalization convention for variables. Classes can go with either one.
- Differentiate class member variables from local variables with some kind of convention ("m_*" is common)
- Use references and const where you can (so the compiler does not need to copy - see GetText() in below example).
- Don't do stuff that can fail in the constructor (ie open a file)
- Use if(error){return} rather than if(good){work}else{return} to reduce indenting
- Don't use C casts like (int). Use C++ casts like static cast. Also, prefer the right type of the variable.
- Use cout for output, cerr for errors

I didn't compile the below stuff and only fixed some obvious errors. But this is to give you an idea of how I personally would restructure it.

MissionText.hpp
Code: [Select]
#ifndef _MISSIONTEXT_HPP_
#define _MISSIONTEXT_HPP_
#include <string>

class MissionText
{
private:
std::string m_allText;

public:
MissionText();

//Load a mission file; false on failure
bool Load(std::string const &mname);

std::string const &GetText() const
{ return m_allText; }
};

#endif /* _MISSIONTEXT_HPP_ */

MissionText.cpp
Code: [Select]
#include "MissionText.hpp"

#include <iostream>
#include <fstream>
#include <string>
//#include <cstdlib> // system("PAUSE")

using namespace std;

static int const gsc_parserLength = 7;

MissionText::MissionText()
{
}

bool MissionText::Load(string mname)
{
string addthis; //contains text to be added

//string XSTR(" has seven characters, ends with a closing \0
//this is a char[] because ifstream::get() works like that
char parser [gsc_parserLength];

int pointerloc;

// Open the mission file
ifstream curmis;
curmis.open (mname);

if(!curmis.is_open())
{
cout << "Error opening mission " << mname << endl;
return false;
}

// Now try to read the mission
while (curmis.good())
{
pointerloc = static_cast<int>(curmis.tellg());

if (pointerloc == -1)
{
cerr << "Error reading file." << endl;
curmis.close();
return false;
}

//Read next series of characters
curmis.get(parser,gsc_parserLength);

//Append tags where needed;
//skip Command briefing because currently
//indistinguishable from #Command (message source)

if (strcmp(parser,"#Brief") == 0) {
m_allText.append("\nBRIEFING\n\n");
} else if (strcmp(parser,"#Debri") == 0) {
m_allText.append("\nDEBRIEFING\n\n");
} else if (strcmp(parser,"#Objec") == 0) {
m_allText.append("\nCARGO\n\n");
} else if (strcmp(parser,"#Event") == 0) {
m_allText.append("\nDIRECTIVES\n\n");
} else if (strcmp(parser,"#Goals") == 0) {
m_allText.append("\nGOALS\n\n");
} else if (strcmp(parser,"#Messa") == 0) {
m_allText.append("\nMESSAGES\n\n");
} else if (strcmp(parser, "XSTR(\"") == 0) {
//If at XSTR, get the string up to the closing quote; else go to next character
getline(curmis,addthis,'"');

if (addthis != "Nothing")
{
m_allText.append(addthis);
m_allText.append("\n");
}
} else {
curmis.seekg(pointerloc + 1,ios_base::beg);
}

//If all that's left is a newline, go to next character
while( curmis.peek() == '\n' )
{
curmis.seekg(2,ios_base::cur);
}
}

curmis.close();
return true;
}

main.cpp
Code: [Select]
#include "MissionText.hpp"

#include <algorithm> // sort
#include <dirent.h>
#include <string>
#include <vector>

using namespace std;

int main()
{
vector<string> mnames; //will contain all mission names

char workdir [999];
DIR *dir;
struct dirent *entry;
string strentry;

ofstream txt;

cout << "MTExPorter, v0.2 -- by FSF\n" << endl;

/*******************************************************************/
// Look for missions

_getcwd(workdir,999);
if( dir = opendir(workdir) )
{
while(entry =  readdir(dir))
{
if( strcmp(entry->d_name,  ".") != 0
&& strcmp(entry->d_name, "..") != 0)
{
//make string for better workability
strentry = entry -> d_name;

// Only .fs2 files are interesting
if( strentry.find(".fs2") != -1)
{
// Found an .fs2 file! Now add it to the mnames vector.
mnames.push_back(strentry);
}
}
}
closedir(dir);
}


// Sort missions
sort (mnames.begin(),mnames.end());


/*******************************************************************/
// Parse missions

txt.open("all.txt");

if (!txt.is_open())
{
cerr << "Error making output file" << endl;
return 1;
}

Mtxt cur_mtext;
for (size_t i = 0; i < mnames.size(); i++)
{
cout << "Parsing " << mnames[i] << endl;

// Make mission text object for current mission
if(cur_mtext.load(mnames[i]))
{
// Write the text to file
txt << cur_mtext.GetText() << "\n\nNEXT MISSION\n\n" << endl;
}
}

//system("PAUSE");
return 0;
}

It might seem silly to split this up into separate files this but as you get to larger applications this will become useful.

Psychologically, it's also good to separate things so that you start thinking of a class's behavior by its external interface, rather than its internal behavior. If you rely on knowledge of a class's internal workings, you will very quickly get overwhelmed when dealing with large code.
« Last Edit: March 22, 2011, 03:56:54 am by WMCoolmon »
-C

 
Re: Criticize a newbie (MTexPorter source)
Okay, noted - I'll have to look into the #ifndef and #define stuff.

 

Offline Tomo

  • 28
Code: [Select]
#ifndef _MISSIONTEXT_HPP_
#define _MISSIONTEXT_HPP_

// HEADER FILE STUFF FOR MISSIONTEXT GOES HERE

#endif /* _MISSIONTEXT_HPP_ */

This is here to make sure that your application doesn't include the header file more than once.

Say you've got a big application, and this header file defines a class that gets used all over the place.
If you don't use this structure, the second time it gets #included then the compiler will complain that you're trying to redefine an existing class - when actually you meant that you want to be able to use the class, not knowing whether or not some other class added it to the program already.
« Last Edit: March 24, 2011, 03:19:59 pm by Tomo »

 
Oh, so that's what it's for! Umm, do only .h files need this, or .cpp files as well? Or does it depend on whether or not you define a class in the specific file?

I've been trying to imitate the Linux game Robots - for those still interested, here's the source of the first alpha. Am I doing it right with the splitting up into files?

Robots.h:
Code: [Select]
#ifndef _ROBOTS_H_
#define _ROBOTS_H_

bool newGame();
void drawHor(int Xmax);
char getInput();
int moveX(int inchar);
int moveY(int inchar);

#endif // _ROBOTS_H_

Robots.cpp:
Code: [Select]
#include "Robots.h"
#include "Things.cpp"

#include <iostream>
#include <vector>

using namespace std;

int newGame(int level)
{
/************** Initialize level **************/

//playfield size; coords counting from top left
const int XMAX = 60;
const int YMAX = 21;

const int NUMBOTS = 10*level;

vector<Robot> Bots; //All robots
vector<Pile>  Scrap; //All scrapheaps
char drawthis = ' '; //What to draw?
char inchar [] = {'\0', '\0'}; //Player input
bool in_good = false; //for giving player another chance at faulty input

/************** Place player & bots **************/

Player You (XMAX/2, YMAX/2);

while (static_cast<int> (Bots.size()) < NUMBOTS)
{
bool badbot = true;
Robot MakeBot (1, 1);

while (badbot)
{
MakeBot.setXY (1 + rand() % XMAX, 1 + rand() % YMAX);

//Check if on top of the player
badbot = MakeBot.collide(You);

//Check if on top of another robot
for (int j = 1; j < static_cast<int> (Bots.size()); j++)
{
badbot = MakeBot.collide(Bots[j]);
}
}
Bots.push_back(MakeBot);
}

/************** Play game **************/

do
{
cout << "Level: " << level << " - Bots remaining: " << Bots.size() << endl;

//Show situation

drawHor (XMAX);

for (int j = 1; j <= YMAX; j++) // vertical (y-) coordinate
{
cout << '|';

for (int i = 1; i <= XMAX; i++) // horizontal (x-) coordinate
{
//Check what's here:

//A robot?
for (int k = 0; k < static_cast<int>(Bots.size()); k++)
{
if ( Bots[k].isHere(i,j) )
{
drawthis = '+';
}
}

//A scrapheap?
for (int k = 0; k < static_cast<int>(Scrap.size()); k++)
{
if ( Scrap[k].isHere(i,j) )
{
drawthis = 'X';
}
}

//The player?
if (You.isHere(i,j) )
{
drawthis = '@';
}

cout << drawthis;
drawthis = ' ';
}

cout << "|" << endl;
}

drawHor (XMAX);

//Player won?

if (Bots.size() == 0)
{
cout << "Congratulations! You beat this level! Continue? (y/n)";
inchar[0] = getInput();

if (strcmp(inchar,"n") == 0) {
return 0;
}
else {
return ++level;
}
}


//player input
in_good = false;
while (!in_good)
{
cout << "Enter a character and press Enter: ";
inchar[0] = getInput();

if (strcmp(inchar,"*") == 0)
{
bool bad_position = true;
int newx, newy;

while (bad_position)
{
newx = 1 + rand() % XMAX;
newy = 1 + rand() % YMAX;

// Check collision with scrapheaps
bad_position = false;
for (int l = 0; l < static_cast<int>(Scrap.size()); l++)
{
if (Scrap[l].isHere(newx,newy))
{
bad_position = true;
}
}
}

You.setX(newx);
You.setY(newy);

in_good = true;
}
else if (strcmp(inchar,"+") == 0)
{
cout << "Safe teleporting not implemented yet!" << endl;
}
else if (strcmp(inchar,"w") == 0)
{
cout << "Waiting doesn't really look good in the command-line, I'm afraid." << endl;
cout << "You'll have to do it the old-fashioned way by pressing 5." << endl;
}
else if (strcmp(inchar,"q") == 0)
{
cout << "Really quit? (y/n) ";
inchar[0] = getInput();

if (strcmp(inchar,"y") == 0)
{
return false;
}
}
else
{
int newx = You.getX() + moveX(atoi(inchar));
int newy = You.getY() + moveY(atoi(inchar));

// Check collision with scrapheaps
in_good = true;
for (int l = 0; l < static_cast<int>(Scrap.size()); l++)
{
if (Scrap[l].isHere(newx,newy))
{
// not using in_good = !Scrap[l].isHere
// because that would override earlier bots
in_good = false;
}
}

if (in_good)
{
You.setXY (newx, newy);
}
}
}

//check if player out of bounds
if (You.getX() > XMAX){
You.setX(XMAX);
}
if (You.getX() < 1){
You.setX(1);
}

if (You.getY() > YMAX){
You.setY(YMAX);
}
if (You.getY() < 1){
You.setY(1);
}

//First update all bots

for (int k = 0; k < static_cast<int>(Bots.size()); k++)
{
Bots[k].update(You.getX(), You.getY());
}

//Then check if something happens
for (int k = 0; k < static_cast<int>(Bots.size()); k++)
{
//Bot caught player!
if (Bots[k].collide(You))
{
cout << "\nGame over! Try again? (y/n)";
inchar[0] = getInput();

if (strcmp(inchar,"n") == 0) {
return 0;
}
else {
return 1;
}
}

else
{
// Check collision with other bots
for (int l = 0; l < static_cast<int>(Bots.size()); l++)
{
if (k != l && Bots[k].collide(Bots[l]))
{
// Add scrapheap, set robots for removal
Pile MakePile (Bots[k].getX(),Bots[l].getY());
Scrap.push_back(MakePile);

Bots[k].setRemove(true);
Bots[l].setRemove(true);
}
}

// Check collision with scrapheaps
for (int l = 0; l < static_cast<int>(Scrap.size()); l++)
{
if (Bots[k].collide(Scrap[l]))
{
Bots[k].setRemove(true);
}
}
}
}

//Remove all colliding bots
for (int k = Bots.size()-1; k >= 0; k--)
{
if (Bots[k].getRemove())
{
Bots.erase(Bots.begin() + k);
}
}
}
while (true);
return 1;
}

void drawHor (int Xmax)
{
for (int i = 0; i <= Xmax+1; i++)
{
cout << "=";
}
cout << endl;
}

char getInput()
{
/* old version didn't work
char input [2] = {'\0', '\0'}; //Input character + null for ending
char garb; //Everything else the player enters

while (input[0] == '\0') //Make sure to actually get input
{
cout << " Give input: ";
cin.get(input,2); //Get the first character
while ( cin.get ( garb ) && garb != '\n' ); //Purge input stream
cout << "Stream purged!" << endl;
}

return input[0];
*/
char input = '\0'; //Input character
char garb; //Everything else the player enters

do
{
cin.get (input); //Get the next character
}
while (input == '\0' || input == '\n'); //Make sure to actually get input

while (cin.get (garb) && garb != '\n' ); //Purge input stream
return input;
}

int moveX (int inchar)
{
switch (inchar)
{ // return (xmove)
case 1: {
return (-1);}
case 3: {
return (+1);}
case 4: {
return (-1);}
case 6: {
return (+1);}
case 7: {
return (-1);}
case 9: {
return (+1);}
default: {
return (0);}
}
}


int moveY (int inchar)
{
switch (inchar)
{ // return (ymove)
case 1: {
return (+1);}
case 2: {
return (+1);}
case 3: {
return (+1);}
case 7: {
return (-1);}
case 8: {
return (-1);}
case 9: {
return (-1);}
default: {
return(0); }
}
}


int main()
{
int level = 1;

while (level > 0)
{
level = newGame(level);
}
}

Things.cpp:
Code: [Select]
class Thing
{
protected:
int m_x, m_y;

public:
Thing ();

Thing (int xin, int yin)
{
m_x = xin;
m_y = yin;
}

int getX() {return m_x;}
int getY() {return m_y;}

void setX  (int newX) {m_x = newX;}
void setY  (int newY) {m_y = newY;}
void setXY (int newx, int newy){
m_x = newx; m_y = newy;}
void moveXY (int movex, int movey){
m_x += movex; m_y += movey;}

bool isHere (int isx, int isy)
{
if (m_x == isx && m_y == isy)
{
return true;
}
else
{
return false;
}
}

bool collide (Thing otherthing)
{
if (m_x == otherthing.getX() && m_y == otherthing.getY())
{
return true;
}
else
{
return false;
}
}
};

class Robot : public Thing
{

protected:
bool remove_this;

public:
Robot () : Thing () {};

Robot (int xin, int yin) : Thing(xin, yin)
{
remove_this = false;
}

void update (int playerx, int playery)
{
//update x-location
if (playerx > m_x) {
m_x++;
}
else if (playerx < m_x) {
m_x--;
}

//update y-location
if (playery > m_y) {
m_y++;
}
else if (playery < m_y) {
m_y--;
}
}

void setRemove (bool in) {remove_this = in;}
bool getRemove () {return remove_this;}
};

class Player : public Thing
{
public:

Player (int xin, int yin) : Thing(xin, yin) {}

//void update (int inchar)
//{

//}
};

class Pile : public Thing
{
public:
Pile (int xin, int yin) : Thing (xin,yin){};
};