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