2

I am writing a program, that requires writing of the current date and time to a log file, although the code is working, there are a lot of repeating code. The code is

#include<iostream>
#include<fstream>
#include<unistd.h>

using namespace std;

string logFile="/home/shared/c++/time.log";
char timeBuffer[80];

int main()
{
     struct tm * timeInfo;
     time_t rawtime;

     ofstream vLog(logFile.c_str(), ios_base::app | ios_base::out);
          {
               {
                    time (&rawtime);
                    timeInfo = localtime(&rawtime);
                    strftime(timeBuffer,sizeof(timeBuffer),"%A %d %b %Y %r %Z",timeInfo);
                    string timeNow(timeBuffer);
                    cout << timeNow << " - Start of log." << endl;
                    vLog << timeNow << " - Start of log." << endl;
               }
               // Do a part of the code
               {
                    time (&rawtime);
                    timeInfo = localtime(&rawtime);
                    strftime(timeBuffer,sizeof(timeBuffer),"%r",timeInfo);
                    string timeNow(timeBuffer);
                    cout << " " << timeNow << " - 1st Line of log." << endl;
                    vLog << " " << timeNow << " - 1st Line of log." << endl;
               }
               // Do more code
               {
                    time (&rawtime);
                    timeInfo = localtime(&rawtime);
                    strftime(timeBuffer,sizeof(timeBuffer),"%r",timeInfo);
                    string timeNow(timeBuffer);
                    cout << " " << timeNow << " - 2nd Line of log." << endl;
                    vLog << " " << timeNow << " - 2nd Line of log." << endl;
                }
                // Do the last part of the code
                {
                    time (&rawtime);
                    timeInfo = localtime(&rawtime);
                    strftime(timeBuffer,sizeof(timeBuffer),"%A %d %b %Y %r %Z",timeInfo);
                    string timeNow(timeBuffer);
                    cout << timeNow << " - End of log." << endl;
                    vLog << timeNow << " - End of log." << endl;
                }
        }
}

I have had to include the { } on each section so that the variables are only used for the one particular code block. If I do not use them, then I get errors on compilation.

time.cpp: In function "int main()":
time.cpp:54:29: error: redeclaration of "std::string timeNow"
    string timeNow(timeBuffer);
                             ^
time.cpp:45:11: error: "std::string timeNow" previously declared here
    string timeNow(timeBuffer);
           ^

With the braces included, then it compiles and runs with no issue.

The information that will be written to the log file will be varied, so it needs to stand separate from the time.

As I am new to C++, I feel like I am over complicating the issue, so any guidance would be appreciated. I am running CentOS 7, and g++ (GCC) 4.8.5 20150623 (Red Hat 4.8.5-11)

Regards Amber-Marie

Update

Thank you for the help. The full code now reads:

#include<iostream>
#include<fstream>
#include<unistd.h>

std::string logFile="/home/shared/c++/time.log";
char timeBuffer[80];
void getTime(std::ofstream &vLog, const std::string &format_args, const std::string &message)
{
    struct tm * timeInfo;
    time_t rawtime;
    time (&rawtime);
    timeInfo = localtime(&rawtime);
    strftime(timeBuffer,sizeof(timeBuffer),format_args.c_str(),timeInfo);
    std::string timeNow(timeBuffer);
    std::cout << timeNow << message << std::endl;
    vLog<< timeNow << message << std::endl;
}

int main()
{
    std::ofstream vLog(logFile.c_str(), std::ios_base::app | std::ios_base::out);
    getTime(vLog, "%A %d %b %Y %r %Z", " - Start of logging");
    // Do part of the code
    getTime(vLog, " %r", " - 1st line of log");
    // Do more code
    getTime(vLog, " %r", " - 2nd line of log");
    // Do the last part of the code
    getTime(vLog, "%A %d %b %Y %r %Z", " - End of logging");
    vLog << std::endl;
    return (0);
}

Hopefully others will find this helpful.

Amber-Marie

3 Answers3

1

Write a function to wrap your code. The parameters should be ofstream, strftime format argument and the log message ( "- 1st line of code", ..)

void foo(ofstream &vLog, const string &format_args, const string &message)
{
    struct tm * timeInfo;
    time_t rawtime;

    time (&rawtime);
    timeInfo = localtime(&rawtime);
    strftime(timeBuffer,sizeof(timeBuffer),format_args.c_str(),timeInfo);
    string timeNow(timeBuffer);
    cout << timeNow << message << endl;
    vLog<< timeNow << message << endl;
}

int main()
{
    ofstream vLog(logFile.c_str(), ios_base::app | ios_base::out);

    foo(vLog, "%A %d %b %Y %r %Z", " - 1st Line of log.");
    // Do a part of the code
    foo(/* second call */);
    // Other stuff
    foo(/* 3rd call */);
    // ...

    return 0;
}
Cristian Ionescu
  • 181
  • 3
  • 10
  • Thank you for that, it has made the code a lot simpler to read and very flexible due to the sending of the strftime variables, and the log message – Amber-Marie May 18 '17 at 15:19
0

The issue is that you are redeclaring the variable "timeNow" and calling its constructor to give it the value of "timeBuffer". This works with brackets because each bracket is a new scope. Each redeclaration of "timeNow" is invisible to all other declarations of it, so there is no confusion on the compiler's part. To get rid of this, use assignment instead of trying to redeclare the variable. Use timeNow = timeBuffer

instead of

string timeNow(timeBuffer);

I tried this code and it works using g++ on my end:

 int main()
    {
         struct tm * timeInfo;
         time_t rawtime;

         ofstream vLog(logFile.c_str(), ios_base::app | ios_base::out);


                        time (&rawtime);
                        timeInfo = localtime(&rawtime);
                        strftime(timeBuffer,sizeof(timeBuffer),"%A %d %b %Y %r %Z",timeInfo);
                        string timeNow(timeBuffer);
                        cout << timeNow << " - Start of log." << endl;
                        vLog << timeNow << " - Start of log." << endl;

                   // Do a part of the code

                        time (&rawtime);
                        timeInfo = localtime(&rawtime);
                        strftime(timeBuffer,sizeof(timeBuffer),"%r",timeInfo);
                        timeNow = timeBuffer;
                        cout << " " << timeNow << " - 1st Line of log." << endl;
                        vLog << " " << timeNow << " - 1st Line of log." << endl;

                   // Do more code

                        time (&rawtime);
                        timeInfo = localtime(&rawtime);
                        strftime(timeBuffer,sizeof(timeBuffer),"%r",timeInfo);
                        timeNow = timeBuffer;
                        cout << " " << timeNow << " - 2nd Line of log." << endl;
                        vLog << " " << timeNow << " - 2nd Line of log." << endl;

                    // Do the last part of the code

                        time (&rawtime);
                        timeInfo = localtime(&rawtime);
                        strftime(timeBuffer,sizeof(timeBuffer),"%A %d %b %Y %r %Z",timeInfo);
                        timeNow = timeBuffer;
                        cout << timeNow << " - End of log." << endl;
                        vLog << timeNow << " - End of log." << endl;
}
Jayson Boubin
  • 1,504
  • 2
  • 11
  • 19
  • I have changed the code as per your suggestion and it works for me as well, so a good improvement. Is there a way to use the time code once, or twice depending upon the format, and then use that to write to the log? – Amber-Marie May 18 '17 at 13:54
0

Yes, that's possible :)

Let's first factor out the whole getting-and-formatting-the-time business. The only variable thing in the duplicated code is the format string, so let's make it a parameter:

std::string formattedCurrentTime(char const *const format) {
    std::time_t const rawTime = std::time(nullptr);
    std::tm const *const timeInfo = std::localtime(&rawTime);

    char buffer[80];
    std::size_t const length = std::strftime(buffer, sizeof buffer, format, timeInfo);

    return {buffer, buffer + length};
}

Then we can look at the duplication of the logging lines. A general solution is a bit tricky to implement because of the whole operator << overloading and polymorphism business, but your case looks consistent enough to benefit from a tailored solution:

auto const logBookend = [&](std::string const &message) {
    auto const timeNow = formattedCurrentTime("%A %d %b %Y %r %Z");

    std::cout << timeNow << " - " << message << std::endl;
    vLog      << timeNow << " - " << message << std::endl;
};

auto const logLine = [&](std::string const &message) {
    auto const timeNow = formattedCurrentTime("%r");

    std::cout << timeNow << " - " << message << std::endl;
    vLog      << timeNow << " - " << message << std::endl;
};

Note: please see this question on why not to use using namespace std;.

See it live on Coliru!

Community
  • 1
  • 1
Quentin
  • 62,093
  • 7
  • 131
  • 191