C ++ Best Coding Practice

Question

I am wondering if there is a better way to write code when there are functions with status.

The following is an example. (Please ignore simple code errors, if any. I specifically talk about structure. In addition, I am at work and do not have a compiler on this computer)

#include "Session.h" Session::Session(const char * IPaddress, unsigned int openPort) { ssh_session mySession; hostIP = IPaddress; port = openPort; } int Session::cBeginSession() { try { int status = ssh_options_set(mySession, SSH_OPTIONS_HOST, &hostIP); if (status == 0) { status = ssh_options_set(mySession, SSH_OPTIONS_LOG_VERBOSITY, SSH_LOG_PROTOCOL); if(status == 0) { status = ssh_options_set(mySession, SSH_OPTIONS_PORT, &port); if (status == 0) { std::cout << "Session started\n"; return 0; } else { std::cout << "Unable to set port\n"; return -3; } } else { std::cout << "Protocol option log verbosity unable to set\n"; return -2; } } else { std::cout << "Unable to set Host address\n"; return -1; } } catch (...) { std::cout << "Unknown exception occurred\n"; return -8; } } 

I usually use if-else statements with state parameters, but as a rule, I have large sockets for if-else statements if more than one or two functions are involved. Is there a more readable way to write something like this? It quickly turns into a nest of rats.

EDIT: Thanks for all the answers. I think I have some ideas on how to better structure my code. I appreciate all diligent suggestions.

Answer 1

In modern C ++ programming, as a rule, if you encounter an error when the program cannot continue, then I think it is better to throw exception.

This way your function will not return anything (i.e. void). Whenever he encounters a situation that cannot continue, you should throw exception that reports what the error is. Then the calling code will handle the error.

The advantage of this is that you can choose where to handle the error. For example, a stack can disable everything up to main .

The code might look like this:

 void Session::cBeginSession() { if (ssh_options_set(mySession, SSH_OPTIONS_HOST, &hostIP)) { // throw an exception } if (ssh_options_set(mySession, SSH_OPTIONS_LOG_VERBOSITY, SSH_LOG_PROTOCOL)) { // throw an exception } if (ssh_options_set(mySession, SSH_OPTIONS_PORT, &port)) { // throw an exception } } 

Once you get an encoding with exceptions, the code tends to be cleaner and more reliable, since you do not always worry about checking return codes.

EDIT

To respond to your comment. You can choose how and when to handle the error. You can just catch the exception above your call. But, in general, if you want to do something that can fail (but not end the program), you can make another function that returns a logical state.

bool Session::tryCBeginSession()

Now your original void Session::cBeginSession() function will be implemented in terms of this new function. I found that in most cases, recording these dual functions is performed only in a limited number of cases.

Answer 2

I like to reduce nesting, for example:

 status = fcn1(); if ( status == 0 ) { // something good status = fcn2(); } else { // something bad happened. report, and leave status reporting failure. } if ( status == 0 ) { // something good status = fcn3(); } else { // something bad happened. report, and leave status reporting failure. } if ( status == 0 ) { // something good status = fcn4(); } else { // something bad happened. report, and leave status reporting failure. } 

I like that printing errors is close to causing an error. Of course, when a failure occurs, the status value is checked additionally. But this is a small price to pay for simplicity.

It is also well suited for unallocating resources and closing files at the end, no matter where the error occurs.

Answer 3

In this particular case (error code handling), I favor early returns:

 int status = ssh_options_set(mySession, SSH_OPTIONS_HOST, &hostIP); if (status != 0) { std::cout << "Unable to set Host address\n"; return -1; } status = ssh_options_set(mySession, SSH_OPTIONS_LOG_VERBOSITY, SSH_LOG_PROTOCOL); if (status != 0) { std::cout << "Protocol option log verbosity unable to set\n"; return -2; } status = ssh_options_set(mySession, SSH_OPTIONS_PORT, &port); if (status != 0) { std::cout << "Unable to set port\n"; return -3; } std::cout << "Session started\n"; return 0; 

I find the code more readable because it is not nested, and error handling is maintained close to the point where the error occurred, rather than being buried in a more distant branch.

If you decide that it is better to use exceptions rather than error codes, you can keep the same structure and replace the return with throws.

Answer 4

This is a pretty perfect exception handling scenario (at least as it was intended). Exception handling is usually suitable for handling external input errors, since in this case the external input comes from the socket.

You already have a try / catch block, but I would suggest eliminating it, since there is no recovery code. Keep try / catch blocks mostly in areas where you are committing a change transaction. Capturing the exception, discards the changes, returns the system to an acceptable state, and possibly displays some kind of message.

Something like that:

  void Session::cBeginSession() { if (ssh_options_set(mySession, SSH_OPTIONS_HOST, &hostIP) != 0) throw runtime_error("Unable to set Host address"); if (ssh_options_set(mySession, SSH_OPTIONS_LOG_VERBOSITY, SSH_LOG_PROTOCOL) != 0) throw runtime_error("Protocol option log verbosity unable to set."); if (ssh_options_set(mySession, SSH_OPTIONS_PORT, &port) != 0) throw runtime_error("Unable to set port"); std::cout << "Session started\n"; } 

Let the client code that calls this function catch an exception on the node where it is suitable for handling and recovering from an error. Just talk about how to throw an exception in case of these external input errors.

Note that exception handling is generally extremely cheap in non-exceptional cases (where you don't drop) with optimizations such as zero cost EH. However, these optimizers for handling exception handlers make the rare case a lot slower when you actually throw an exception. Therefore, exceptions should be used for really exceptional cases caused by some kind of external input that your software usually cannot handle, as in this case.

Another caveat related to certain types of larger systems (e.g. plugin architecture) is that usually exceptions should not overlap module boundaries.

This is a somewhat opinionated opinion, but I do not recommend having many catch branches based on the type of exception (as is usually the case in Java, for example). Often there is no need to distinguish between the actual type of exception, as relaying the message to the user, for example. Choose exceptions as normal / rough as possible, and keep try/catch blocks to a minimum (high-level transaction-oriented mentality: transactions succeed in general or fail and roll back in general).

Otherwise, exceptions can really simplify such cases, and the entire C ++ library (and even parts of the language) usually raises exceptions (I really believe that C ++ and exception handling are inextricably linked), so this can be useful for using them. since a reliable program, as a rule, should generally catch them.

Answer 5

• You don't need if-else if you follow return or throw in the following example (this is a good example for throw ing BTW). A simple if will do.
• The type of messages you print is usually better for stderr rather than stdout ( cerr , not cout ).
• If you decide that you will continue to use error statuses, symbolic constants (or enumerations or definitions) are usually preferable to "magic numbers"

Answer 6

some suggestions noted in the code:

 // improvement - derive your own descriptive exception FROM A STANDARD EXCEPTION TYPE struct session_error : std::runtime_error { using std::runtime_error::runtime_error; }; // improvement in constructor: initialiser lists Session::Session(const char * IPaddress, unsigned int openPort) : mySession() , hostIP(IPaddress) , port(openPort) { } namespace { // use of anonymous namespace so this wrapper function does not pollute other compilation units void setopt(ssh_session& session, int opt, const void* val, const char* context) { if (ssh_options_set(session, opt, val)) throw session_error(context); } } void Session::cBeginSession() { // improvement - defer to wrapper function that handles nasty return code logic // and throws a sensible exception. Now your function is readable and succinct. setopt(mySession, SSH_OPTIONS_HOST, &hostIP, "setting host option"); setopt(mySession, SSH_OPTIONS_LOG_VERBOSITY, SSH_LOG_PROTOCOL, "setting verbosity"); setopt(mySession, SSH_OPTIONS_PORT, &port, "can't set port"); std::cout << "Session started\n"; }