HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Using a TSV text file and map for a Q&A program

Submitted by: @import:stackexchange-codereview··
0
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

#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 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 more
commonly 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 remove
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 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 writing
and 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 just
a pair of "pure" functions: readFile() and writeFile(). You don't
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 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.