patterncppMinor
Using a TSV text file and map for a Q&A program
Viewed 0 times
mapfiletextandprogramusingfortsv
Problem
The program outputs a menu asking for a new question or to ask a question. If you ask a question, it will search a .tsv text file for the question and return the answer. It will then check a map that is used for time and date, if it exists, it will execute the function intended. If not, it will just print the answer. As for a new question, it simply edits the file and inputs the question then answer. I'd like to expand this program to be bigger in the future, multiple files, maps, and better deduction on questions.
So far, this is what I have: 5 files, a main, time and date header and cpp, and file header and .cpp.
getTime.h
getTime.cpp
```
#include "getTime.h"
#include
using namespace std;
void getTime::printCurrentTime() {
now = time(0);
current = localtime(&now);
hours = current->tm_hour;
minutes = current->tm_min;
seconds = current->tm_sec;
currentTime = to_string(hours) + ":" + to_string(minutes) + ":" + to_string(seconds);
cout tm_mday;
month = current->tm_mon;
year = current->tm_year;
switch (month) {
case 0:
strMonth = "Jan";
break;
case 1:
strMonth = "Feb";
break;
case 2:
strMonth = "Mar";
break;
case 3:
strMonth = "April";
break;
case 4:
strMonth = "May";
break;
case 5:
strMonth = "June";
break;
case 6:
strMonth = "July";
break;
case 7:
strMonth = "Aug";
break;
case 8:
strMonth = "Sep";
break;
case 9:
strMonth = "Oct";
break;
case 1
So far, this is what I have: 5 files, a main, time and date header and cpp, and file header and .cpp.
getTime.h
#ifndef H_getTime
#define H_getTime
#include
#include
using namespace std;
class getTime {
public:
void printCurrentTime();
void printCurrentDate();
private:
string currentTime;
string currentDate;
string strMonth;
int hours;
int minutes;
int seconds;
int day;
int month;
int year;
time_t now;
struct tm *current;
};getTime.cpp
```
#include "getTime.h"
#include
using namespace std;
void getTime::printCurrentTime() {
now = time(0);
current = localtime(&now);
hours = current->tm_hour;
minutes = current->tm_min;
seconds = current->tm_sec;
currentTime = to_string(hours) + ":" + to_string(minutes) + ":" + to_string(seconds);
cout tm_mday;
month = current->tm_mon;
year = current->tm_year;
switch (month) {
case 0:
strMonth = "Jan";
break;
case 1:
strMonth = "Feb";
break;
case 2:
strMonth = "Mar";
break;
case 3:
strMonth = "April";
break;
case 4:
strMonth = "May";
break;
case 5:
strMonth = "June";
break;
case 6:
strMonth = "July";
break;
case 7:
strMonth = "Aug";
break;
case 8:
strMonth = "Sep";
break;
case 9:
strMonth = "Oct";
break;
case 1
Solution
A few words on coding style:
Naming of user defined types in C++: The naming convention you've used
for the types
Usually, user defined names use
upper-case. Names using
commonly preferred for variable and method names.
But also,
The class seems more like a timer or clock, so it should have a name
that reflects that.
definitely change that. In the
it from the header you might also consider removing it from the source
files as well to be more consistent. It is not much more typing adding
the
Switch vs an array:
There is a very big
with the cases going from 0 to 11, for each month of the year. This would
look so much better as an array of strings:
Note: Or use
a C
ununiformity? What is the problem with using the
and reading a file?
Also, there seem to be no reason at all for the file handlers to be members
of
But taking a closer look,
a pair of "pure" functions:
need to make everything an object just because you are using a language
that fosters OOP. C++ is not a single paradigm language. A lot of problems
are better solved with simple functions.
Use more references:
I suspect some of your string parameters should be
The default in C++ is a copy, so it can get inefficient quite easily
if you overlook this aspect. As a rule of thumb, when passing a complex
object to a function, if the function is only going to read from this object
and not make copies of it, then the object should be passed by const reference (
Naming of user defined types in C++: The naming convention you've used
for the types
getTime and fileRW is not very usual for C++ code.Usually, user defined names use
CamelCase, with the first letter upper-case. Names using
camelCase with the first in lower-case, are morecommonly preferred for variable and method names.
But also,
getTime is a bad name for a type, as it reads like a command.The class seems more like a timer or clock, so it should have a name
that reflects that.
using namespace std in a header file is really a bad idea. You should definitely change that. In the
.cpp file it is fine. But once you removeit from the header you might also consider removing it from the source
files as well to be more consistent. It is not much more typing adding
the
std:: prefix to stuff. Your editor will certainly have auto-complete as well.Switch vs an array:
There is a very big
switch statement inside printCurrentDate() which is sequential,with the cases going from 0 to 11, for each month of the year. This would
look so much better as an array of strings:
static const char * months[] = {
"Jan",
"Feb",
"Mar",
"April",
"May",
"June",
"July",
"Aug",
"Sep",
"Oct",
"Nov",
"Dec"
};
// shield yourself from bad inputs with an assertion
assert(month >= 0 && month < 12);
const char * monthName = months[month];Note: Or use
std::string instead of char*. String is better if you need to manipulate the text, as it provides several methods for that.fileRW weirdness:fileRW keeps two different types of file interfaces as member data,a C
FILE * and a C++ ifstream. Why would you want all that ununiformity? What is the problem with using the
ifstream for writingand reading a file?
Also, there seem to be no reason at all for the file handlers to be members
of
fileRW, as the files are opened and closed inside the method.But taking a closer look,
fileRW shouldn't even exit. It is clearly justa pair of "pure" functions:
readFile() and writeFile(). You don'tneed to make everything an object just because you are using a language
that fosters OOP. C++ is not a single paradigm language. A lot of problems
are better solved with simple functions.
Use more references:
I suspect some of your string parameters should be
const references.The default in C++ is a copy, so it can get inefficient quite easily
if you overlook this aspect. As a rule of thumb, when passing a complex
object to a function, if the function is only going to read from this object
and not make copies of it, then the object should be passed by const reference (
const T&).Code Snippets
static const char * months[] = {
"Jan",
"Feb",
"Mar",
"April",
"May",
"June",
"July",
"Aug",
"Sep",
"Oct",
"Nov",
"Dec"
};
// shield yourself from bad inputs with an assertion
assert(month >= 0 && month < 12);
const char * monthName = months[month];Context
StackExchange Code Review Q#65004, answer score: 2
Revisions (0)
No revisions yet.