Countdown timer for new year

Question

Please help me improve my code. Note that it runs just fine. Especially with the way I use SFML, I need lots of guidance since I've just started learning how to use this library. I'm still learning C++ as well.

main.cpp

#include <SFML/Graphics.hpp>
#include "counter.hpp"

std::string day(int num) { return (num!=1)? "s":""; } //the 's' in the word day(s) in the text displayed

int main()
{
    sf::RenderWindow window(sf::VideoMode(800,300), "New Year Counter");
    window.setVerticalSyncEnabled(true);
    window.setPosition(sf::Vector2i(sf::VideoMode::getDesktopMode().width/2 -400, sf::VideoMode::getDesktopMode().height/2 -150)); //put window in the middle of the screen

    sf::Font font;
    font.loadFromFile("Beyond Wonderland.ttf");
    sf::Text text;
    text.setFont(font);
    text.setCharacterSize(80);
    text.setFillColor(sf::Color::White);
    text.move(25, 10);

    sf::Text counter = text; //initialize with the same attributes as text so that I won't have to write everything again.

    while(window.isOpen())
    {
        sf::Event event;
        while(window.pollEvent(event))
        {
            if(event.type == sf::Event::Closed)
                window.close();
        }      

        TimeToNewYear year;
        window.clear(sf::Color::Black);
        text.setString("Until New Year "+year.get_newYear()+"\n"+year.get_daysLeft()+" day"+day(std::stoi(year.get_daysLeft()))+" left\n\t\tand");
        window.draw(text);
        counter.setString(year.get_counter());
        counter.setCharacterSize(120);
        counter.setPosition(360, 140);
        window.draw(counter);
        window.display();
    }
}

counter.hpp

#ifndef COUNTER_HPP
#define COUNTER_HPP
#include <string>
#include <ctime>

class TimeToNewYear
{
private:
    time_t m_nowTime = time(0);
    time_t m_tmny; //for mktime (time new year)
    tm *m_locTime = localtime(&m_nowTime);
    tm m_newYear {};
    int m_timeLeft;
    int m_daysLeft;
    std::string m_counter;

    void addNum(int num) //to display single digits with a 0 ex: 03
    {
        if(num<=9)
            m_counter += "0";
        m_counter += std::to_string(num);
    }
public:
    TimeToNewYear()
    { 
        m_newYear.tm_mday =1; 
        m_newYear.tm_year = 1+ m_locTime->tm_year; //set m_newYear
        m_tmny = mktime(&m_newYear);
        m_timeLeft = difftime(m_tmny, m_nowTime);
        //calculate days left
        m_daysLeft = m_timeLeft/(24*3600);
        m_timeLeft %= 24*3600;
    }

    std::string get_daysLeft() const { return std::to_string(m_daysLeft); }
    std::string get_newYear()  const { return std::to_string(m_newYear.tm_year +1900); } //tm_year is the years from 1900
    std::string get_counter()
    {
        addNum(m_timeLeft/3600);
        m_counter += ":";
        m_timeLeft %= 3600;
        addNum(m_timeLeft/60);
        m_counter += ":";
        addNum(m_timeLeft%60);
        return m_counter;
    }
};

#endif

Answer 1

I see a number of things that may help you improve your code.

Separate interface from implementation

The interface goes into a header file and the implementation (that is, everything that actually emits bytes including all functions and data) should be in a separate .cpp file. The reason is that you might have multiple source files including the .h file but only one instance of the corresponding .cpp file. In other words, split your existing counter.hpp file into a .h file and a .cpp file.

Add error checking

The program will only run successfully if the specific font you've specified is in the current working directory when the program is run. If the font file does not exist there, the program will run but create only a black box on the screen. We can do better! I'd suggest doing something like this:

const std::string fontfile{"Beyond Wonderland.ttf"};
if (!font.loadFromFile(fontfile)) {
    std::cerr << "Error: could not load font \"" << fontfile << "\"\n";
    return 1;
}

Think of the user

The user might want to use a different font or have different colors. It would be nice to allow the user to specify these kinds of things from the command line.

Eliminate "magic numbers"

In a number of cases, the code uses "magic numbers" such as 400 and 150 that have no obvious meaning. These would be better as named constants to make the code easier to understand and maintain.

Eliminate work

The way the code is currently written, the code creates and destroys a copy of the TimeToNewYear class every iteration through the loop. That's really not necessary. What would make more sense is instead to create everything once outside the loop and only update things that need updating within it.

Simplify your class

The current TimeToNewYear class has more private variables than it really needs. I'd suggest rethinking this class. Since it's already quite specific in its purpose, you could further specialize it and have it return the two formatted strings that are used by the program. Alternatively, it could be made more generic by providing a countdown function to any arbitrary future date and time. Here's an alternative that uses the std::chrono standard library.

class CountdownTimer
{
private:
    using tp = std::chrono::time_point<std::chrono::system_clock>;
    tp end;
    std::tm end_tm;
public:
    CountdownTimer(tp end);
    int daysLeft() const;
    int secondsLeft() const;
    std::string HMS_left() const;
    int endYear() const;
};

Two of the member functions are shown here as a start:

CountdownTimer::CountdownTimer(tp end) : end{end}
{
    time_t tt = std::chrono::system_clock::to_time_t(end);
    end_tm = *localtime(&tt);
}

int CountdownTimer::daysLeft() const { 
    using days = std::chrono::duration<int, std::ratio<24*60*60, 1>>;
    auto now = std::chrono::system_clock::now();
    return std::chrono::duration_cast<days>(end - now).count();
}

Use better naming

The variables named text and year are not particularly descriptive and possibly even misleading. Better names more clearly describe the purpose for the variables.

Consider other system resources

Although synchronizing with the vertical sync (which this code does) reduces the load somewhat, there is a lot of string creation and destruction happening every time through the loop, whether anything visually changes or not. I'd suggest keeping the last seconds count and only updating the strings if that changes. It won't make any major change here, but it's good to get into the habit of minimizing the work the computer needs to do, especially if it's called in a loop.

Answer 2

First of all congratulations on a good start to C++ and SFML. You should consult good C++ design and programming practices such as https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md and/or https://www.gitbook.com/book/lefticus/cpp-best-practices/details. But I will point out a few things quickly here:

• Typically the hpp file would consist of declarations and the implementation would be done in cpp file. This is recommended for significant implementations such as TimeToNewYear() ctor and the get_counter() method.
• Just as you are scoping std::string, doing the same for time, tm, etc. as std::time, std::tm is a good idea - improves readability of the code.
• You can also pretty much take all of the contents of your ctor and initialize all the variables as TimeToNewYear::TimeToNewYear() : m_nowTime {std::time(0)}, m_locTime {std::localtime(&m_nowTime)}, ... {} etc. But watch out for the order of initialization which depends on the order of member specification!
• You can always return a reference to m_counter by having const std::string& TimeToNewYear::get_counter() so that clients cannot modify the contents. Haven't looked but am almost positive sf::Text::setString method takes a const std::string&. Saves time on making temporaries.

I am not an expert on the SFML side, but having used other windowing systems, these are general guidelines:

• I would derive from sf::RenderWindow or compose it in your application specific class such as class MyAppWindow () : m_sfWindow {sf::VideoMode(), ...}, ... {} where m_sfWindow is a private member of your class.
• This would permit you to have the event loop inside MyAppWindow::display() or some such method. It is likely that you can sf::RenderWindow::draw() inside the event loop in an already displayed sf::RenderWindow, thus avoiding the sf::RenderWindow::display() call.
• It is also possible to have a New Year Countdown button, which when clicked, displays the countdown text box. You can get rid of it with a Close button.

Welcome to C++ (and SFML) development! After doing C++ for 20 years, I am still learning, and am in it for the long haul. Good luck!