Code Review: Time based notification program

[boost lexical cast]

I wrote a program that sends notifications on my computer (Linux (Debian Jessie 8.7 LXDE)) when the time comes. A file contains all the information about the events (what notifications to send, the time, and the day) in a specific way so it can be parsed. What it does it very simple.

I would like a code review with constructive criticism, how to improve, what I did wrong. and maybe some feedback on where I wrote the code well.

The code is here via Pastebin (https://pastebin.com/) and downloadable source files (split up into multiple files).

main.cpp:
Pastebin: https://pastebin.com/cCKDrSQf
Download: http://s000.tinyupload.com/index.php?file_id=20141914832342222291

notify_time.cpp:
Pastebin: https://pastebin.com/HQ6UGrnF
Download: http://s000.tinyupload.com/index.php?file_id=21305210035626727726

notify_time.h:
Pastebin: https://pastebin.com/8yaTqe2S
Download: http://s000.tinyupload.com/index.php?file_id=00834217561917379061

event.cpp:
Pastebin: https://pastebin.com/3KPubS00
Download: http://s000.tinyupload.com/index.php?file_id=14666130551528139443

event.h:
Pastebin: https://pastebin.com/TGyrJYtJ
Download: http://s000.tinyupload.com/index.php?file_id=05998472350871314577

notify_time_conf (example):
Pastebin: https://pastebin.com/JuU8pVc0
Download: http://s000.tinyupload.com/index.php?file_id=12734417644175985900 (line break off would be best)

I plan to add these features:

1. If no times match to send a notification then it will show the time left to the nearest event.

2. Time left until start in warning notification.

3. Time left until end in start notification.

4. Example: 23:50 to 00:50 is two different days. Plan to implement.

Well, that's it. Appreciate you guys looking at my code!

[GorfTheFrog2]

I would like to understand your program.
+ Is it a console program or a WinAPI program?
+ How will the program display notifications? Message box or something?

[helios]

Couldn't the same have been accomplished with a cron job?

[boost lexical cast]

GorfTheFrog2 wrote:

I would like to understand your program.
+ Is it a console program or a WinAPI program?
+ How will the program display notifications? Message box or something?

It's neither (I said I was on Linux; also it doesn't output anything). It uses a command line tool and calls it with int std::system(const char*). Would appreciate a review.

helios wrote:

Couldn't the same have been accomplished with a cron job?

I know there are tools/programs out there which does the same thing or better. I wrote this for practice. Anyways, I don't think Cron can show the time left before the start of an event, the time left before ending of event, time left until nearest event, show if event is in progress, show if it is about to start. Would appreciate a review.

[JLBorges]

The code is not yet ready for a review.

At the very least, the header must have comments that tells a programmer how to use the component.

In particular, for each (public) function:
a. What are the input parameters? Are there any pre-conditions (invariants) associated with them?
b. What does the function do if a pre-condition is violated?
c. What does the function actually do (what are the post-conditions on exit?)
d. For non-trivial functions, an example of usage would be good

The component to be reviewed should be presented along with its test frame. For example:
event.h, event.cpp - the component to be reviewed
event_tc.txt - test cases with expected results (isolation test aka unit test)
event_td.cpp - the test driver (with main) that can be used to test the component.

[boost lexical cast]

@JLBorges

So... it seems I don't know much about a code review.

I will do these things:

1. I will comment my code so others can easily understand (especially the headers; so programmers can use it)

2. Comment to explain parameters of functions.

3. Comment what fuction does if a pre-condition is violated.

4. Comment what the function does and how it works.

5. Example of usage for non-trivial functions.

6. Test cases and output of my program.

7. Test cases and output of my functions/classes ?

Is this right?

[JLBorges]

> 2. Comment to explain parameters of functions.

If they are not self-explanatory. In general do not comment what can be expressed in code
(for example, regurgitating the type of a parameter in a comment is worse than useless.)


> 4. Comment what the function does and how it works.

In the header, only what the function does.
In the implementation, how it works, only if it is not obvious.
In general, do not over-comment (other than in material used for teaching programming).


> 7. Test cases and output of my functions/classes ?

Test cases are typically written on a per component (per cpp file) basis.
For a code-review, black-box test cases tend to be more important.


Also compile the code with warnings set to a high level before submitting the code for review. For example,
GNU and compatibles: -Wall -Wextra -std-c++14 -pedantic-errors
Microsoft: -Za -W4 -analyze (Visual Studio 2017, also include CppCoreGuidelines checks as part of code analysis)

[boost lexical cast]

Thanks, that was helpful. I will do this soon when I have time.

P.S. Would compiling with g++ main.cpp notify_time.cpp event.cpp notify_time.h event.h -Wall -Wextra -std=c++14 -pedantic-errors -O2 -s -o notify_time be okay, then?

[JLBorges]

Yes. Asking more than one compiler to look at the code is always a good idea.

For instance, Visual Studio 2017 (with the options indicated earlier) would complain about:

const char* event_t::unparse_cmd( const std::string& notif ) const
{
    return ( time_constant::cmd + " " + notif ).c_str();
}

... (lifetimes rule 1). 'return of unparse_cmd' was invalidated by 'end of scope for ...'

[boost lexical cast]

JLBorges wrote:

... (lifetimes rule 1). 'return of unparse_cmd' was invalidated by 'end of scope for ...'

Is this a compile error that only shows up in Visual Studio 2017? Because, I compiled with the same options in g++ (same as what I posted above) and I didn't get that error. Does it have something to do with the C string getting released from the memory?

[TheIdeasMan]

@boost lexical cast

Just wondering why you would compile the header files, both of them are included in the cpp files?

[JLBorges]

> Is this a compile error that only shows up in Visual Studio 2017?

It is indeed a programming error; but the diagnostic that the compiler would generate is a warning.


> I compiled with the same options in g++ (same as what I posted above) and I didn't get that error.

There is no requirement that undefined behaviour must be diagnosed. Compilers try (if we let them) to assist programmers by pointing out (generating warnings about) potential problems in the code. Compiling the code, with multiple compilers (forcing standard conformance and using a high warning level) improves the chances that more errors are detected.

Since your development environment is Linux, consider compiling the code with both g++ and clang++ (the two mainstream compilers available on the platform).


> Does it have something to do with the C string getting released from the memory?

The string object that is the result of evaluating ( time_constant::cmd + " " + notif ) is an anonymous temporary object; it would be destroyed at the end of the full expression. The pointer that is being returned is therefore a dangling pointer; the data that it points to does not exist when control returns from the function.

As an aside, if the function was public, a code review would have asked you to justify why a raw pointer pointing to the underlying characters stored in a std::string is being returned (even if the string involved is not a temporary object).

[boost lexical cast]

@JLBorges

Then I guess I will install clang++ and change the code to this:

const char* event_t::unparse_cmd( const std::string& notif ) const
{
    std::string full_notif = time_constant::cmd + " " + notif;
    return (full_notif).c_str();
}

JLBorges wrote:

As an aside, if the function was public, a code review would have asked you to justify why a raw pointer pointing to the underlying characters stored in a std::string is being returned (even if the string involved is not a temporary object).

Because int std::system(const char*) only accepts a C string?

[JLBorges]

> change the code to this:

It does not fix the problem. When does the life-time of std::string full_notif end?


> Because int std::system(const char*) only accepts a C string?

Consider std::system( str.c_str() ) where str is an object of type std::string

[boost lexical cast]

@JLBorges Alright, I think I went too far into splitting my code into functions. I'll combine send_notif() and unparse_cmd().

I think I will have time tomorrow to comment my code and edit it for code review.