3
\$\begingroup\$

I've just started to learn C++.

I have this class and I don't know where to put the following two lines because of the catch:

delete[] chost;
delete[] cport;

I think they are in the right place, but I have doubts because in C++ doesn't exist finally.

Header:

#pragma once
#include <string>
#include "TelnetClient.h"

class Ephemeris
{
public:
    Ephemeris();

    ~Ephemeris();

    bool Connect(std::string host, std::string port);

private:
    TelnetClient* telnet = nullptr;
};

CPP:

#include "Ephemeris.h"

#include <iostream>
#include <boost/array.hpp>
#include <boost/asio.hpp>

using boost::asio::ip::tcp;

Ephemeris::Ephemeris()
{
}

Ephemeris::~Ephemeris()
{
    if (telnet != nullptr)
        delete telnet;
}

bool Ephemeris::Connect(std::string host, std::string port)
{
    bool result = false;

    // Convert std::string host to char* host.
    size_t host_length = host.length() + 1;
    char* chost = new char[host_length];
    strcpy_s(chost, host_length, host.c_str());

    // Convert std::string port to char* port.
    size_t port_length = port.length() + 1;
    char* cport = new char[port_length];
    strcpy_s(cport, port_length, port.c_str());

    try
    {
        boost::asio::io_service io_service;
        // Resolve the host name and port number to an iterator that can be used to 
        // connect to the server.
        tcp::resolver resolver(io_service);
        tcp::resolver::query query(chost, cport);
        tcp::resolver::iterator iterator = resolver.resolve(query);

        // Create a Telnet client
        telnet = new TelnetClient(io_service, iterator);

        // Run the IO service as a separate thread, so the main thread can block on standard input
        boost::thread t(boost::bind(&boost::asio::io_service::run, &io_service));

        result = true;
    }
    catch (std::exception& e)
    {
        if (telnet != nullptr)
            delete telnet;

        result = false;
    }
    
    // Free resources.
    delete[] chost;
    delete[] cport;

    return result;
}

If you have any suggestions, related or not with my doubt, please, tell me.

The original code is this one, "Boost Telnet Client". I'm adapting it to my own needs.

\$\endgroup\$
2
  • \$\begingroup\$ In addition to my answer, I'd recommend my Code Project article. \$\endgroup\$
    – JDługosz
    Commented Jul 27, 2021 at 14:21
  • 2
    \$\begingroup\$ C++ doesn't exist finally. True we have a better technique called destructors that are guaranteed to be called even when exceptions are thrown. \$\endgroup\$ Commented Jul 27, 2021 at 17:33

3 Answers 3

9
\$\begingroup\$
Ephemeris::Ephemeris()
{
}

No need for a constructor if it does nothing.

Ephemeris::~Ephemeris()
{
    if (telnet != nullptr)
        delete telnet;
}

The test here is pointless, since delete works fine with a null argument. We can improve on this by making telnet a smart pointer (probably std::unique_ptr<TelnetClient>), so that a default destructor is sufficient.

Changing to smart pointer also means we don't need to override or delete the copy constructor and assignment operator, which we have failed to do (meaning that after copying, one object's destructor would delete the other's data).

bool Ephemeris::Connect(std::string host, std::string port)

Do we really need modifiable copies of host and port? It looks like we should be fine with const references:

bool Ephemeris::Connect(const std::string& host, const std::string& port)
size_t host_length = host.length() + 1;
char* chost = new char[host_length];
strcpy_s(chost, host_length, host.c_str());

Is there a need to copy the string from host? We're going to pass this to the resolver::query constructor, which accepts a const std::string&, so it's completely pointless to copy the characters to create a new string. We can simply pass our existing strings:

    tcp::resolver::query query(host, port);
   telnet = new TelnetClient(io_service, iterator);

We have a memory leak here, as we have not freed the previous value of telnet before assigning this new one. A smart pointer takes care of that for us.

    boost::thread t(boost::bind(&boost::asio::io_service::run, &io_service));

boost:thread is not declared - I recommend including <boost/thread.hpp> to get a definition in scope.

       }

It's a bad idea to destruct a running joinable thread. If we really want it to continue in the background we should detach() it first.

     catch (std::exception& e)

It's generally a bad idea to catch all kinds of exception like this. It's probably better to allow our function to throw exceptions, rather than returning a status code that the caller could forget to examine.

    if (telnet != nullptr)
        delete telnet;

That's risky. We now have a dangling pointer to the deleted telnet object. The smart pointer helps here, because we can clear it, which both deletes the object and changes the pointer to a null pointer.

    return result;

With proper destructors, there's no need to store the return value - we can return immediately and let C++ clean up our objects.


Modified code

#include <memory>
#include <string>

class Ephemeris
{
public:
    void Connect(const std::string& host, const std::string& port);

private:
    std::unique_ptr<TelnetClient> telnet = {};
};
#include <boost/array.hpp>
#include <boost/asio.hpp>
#include <boost/thread.hpp>

using boost::asio::ip::tcp;

void Ephemeris::Connect(const std::string& host, const std::string& port)
// may throw boost::thread_resource_error or boost::system::system_error
{
    boost::asio::io_service io_service;
    // Resolve the host name and port number to an iterator that can
    // be used to connect to the server.
    tcp::resolver resolver{io_service};
    tcp::resolver::query query{host, port};
    tcp::resolver::iterator iterator = resolver.resolve(query);

    try {
        // Create a Telnet client
        telnet = std::make_unique<TelnetClient>(io_service, iterator);

        // Run the IO service as a separate thread, so the main thread
        // can block on standard input
        boost::thread t{boost::bind(&boost::asio::io_service::run, &io_service)};
        t.detach();
    } catch (...) {
        telnet.reset();
        throw;
    }
}
\$\endgroup\$
9
  • \$\begingroup\$ Wow, I'm thinking about how many time I will need to know everything you know. \$\endgroup\$
    – VansFannel
    Commented Jul 27, 2021 at 6:49
  • \$\begingroup\$ I think you have to add #include "TelnetClient.h" in the header file of the modified code. Or maybe, I'm wrong. \$\endgroup\$
    – VansFannel
    Commented Jul 27, 2021 at 7:15
  • \$\begingroup\$ You said: "With proper destructors, there's no need to store the return value - we can return immediately and let C++ clean up our objects.". But, there is no destructor in MyClass. What destructors are you talking about? \$\endgroup\$
    – VansFannel
    Commented Jul 27, 2021 at 7:22
  • 1
    \$\begingroup\$ I have no idea what MyClass is - that's not in the code here. I was talking about the (implicit) destructor in Ephemeris as one example - because telnet is a smart pointer, it will get cleaned up for us. Oh, I just noticed that you reset telnet when the thread can't be created - I'll adjust my replacement code. \$\endgroup\$ Commented Jul 27, 2021 at 7:42
  • \$\begingroup\$ Sorry, MyClass is Ephemeris. My apologies. \$\endgroup\$
    – VansFannel
    Commented Jul 27, 2021 at 8:01
4
\$\begingroup\$

I agree with everything Toby Speight wrote. I'd like to add:

Name things according to what they represent

You have this line of code:

tcp::resolver::iterator iterator = resolver.resolve(query);

But while iterator is indeed an iterator, that's not what is important about this variable. What really is important is that it is the result of the DNS query. So I would write this instead:

auto query_result = resolver.resolve(query);

Alternatively, you could also name it addresses, as the query result is the resolved list of addresses for the given hostname and port combination.

Prefer the standard library over Boost

Boost is very nice, but depending on it has a cost: it adds dependencies and there is a compile-time overhead. Furthermore, the standard library is evolving as well, and many features that were once only available in external libraries like Boost are now available in the STL. So instead of:

boost::thread t(boost::bind(&boost::asio::io_service::run, &io_service));

I would prefer to write this:

std::thread t(&boost::asio::io_service::run, &io_service);

Remove unused #include directives

I see #include <boost/array.hpp>, but no boost::arrays are used anywhere. Remove the #include to avoid an unnecessary dependency and compile-time overhead.

\$\endgroup\$
2
\$\begingroup\$

If you just started learning C++, I suggest reading through (and bookmarking) the C++ Core Guidelines. One example that pops out, from your opening remarks, is C.149, R.11, and P.9.

That is, rather than being puzzled as to where to put the delete[] statements due to exception handling, you should not be writing explicit calls to delete in your code.


#pragma once
#include <string>
#include "TelnetClient.h"

class Ephemeris
{
public:
    Ephemeris();

    ~Ephemeris();

    bool Connect(std::string host, std::string port);

private:
    TelnetClient* telnet = nullptr;
};

Just looking at this header:

  1. You are passing std::string arguments by value.
  2. Your destructor is declared but not defined in the header... so it must do something complex I suppose. But the only data in the class is one (raw) pointer. I'm guessing that what this class does is allow you to have an std::optional<TelnetClient>? That is, what's it do beyond simply using the TelnetClient directly?
  3. I'm supposing that telnet ought to be a std::unique_ptr. Then you don't need to write the destructor explicitly.
  4. You didn't write a copy constructor or copy assignment operator, and the defaults won't work right. Use the rule of 3/5/0. If you used a std::unique_ptr the copying would be automatically disabled and you would not have to deal with it.

Looking at the implementation now...

Ephemeris::Ephemeris()
{
}

Just don't. Literally, don't write an empty constructor because it doesn't add anything over =default and it prevents the compiler from understanding just how trivial the class is and optimizing better. If you did need a (very simple) constructor, you should put it in the header directly in the class. In this case, you don't need to define it at all since the compiler automatically supplies it.

Ephemeris::~Ephemeris()
{
    if (telnet != nullptr)
        delete telnet;
}

Again, put very simple destructors in the header so they can be inlined.
delete already does a null-check, so don't repeat it.
You should be using a std::unique_ptr which already has the destructor tightly wrapped around a pointer, so you don't need to write a destructor for this class. Don't use new/delete directly in your own code, unless you are writing a container or resource manager of some kind.

Basically, this class doesn't do anything. You want a free function Connect that returns a std::unique_ptr<TelnetClient>.


// Convert std::string host to char* host.
    size_t host_length = host.length() + 1;
    char* chost = new char[host_length];
    strcpy_s(chost, host_length, host.c_str());

    // Convert std::string port to char* port.
    size_t port_length = port.length() + 1;
    char* cport = new char[port_length];
    strcpy_s(cport, port_length, port.c_str());

Why? Why can't query take the port.c_str() directly? I see you're never deleting cport and chost, so does the Boost class take ownership? I find that bizarre, since Boost is a C++ library and would be using std::string to keep ownership of a string.

If you did need to do this for some reason, don't repeat yourself. You have identical code except for the parameters, so this is very simple to make into a function.

In fact, isn't strdup already a standard C function?

Again, if you used std::unique_ptr you would not need to write a try/catch block in Connect. Your resource (the pointer) is not wrapped tightly enough and you're forced to handle it in this function which has more stuff going on. By wrapping resources tightly, you avoid dealing with them in the enclosing code. This is a very important principle to learn.

// Free resources.
    delete[] chost;
    delete[] cport;

OK, you do this at the end. So why did you make a copy of the string data in this manner? Clearly you know how to get the const char* nul-terminated string from the std::string class, since you actually call .c_str() in order to make the copy. As you already know, you're not freeing the data if an exception happens, and you should not need to be doing this (even if you did need to make a copy) directly.


Postscript

I would further improve Toby's rewrite by not having the Ephemeris class at all; just a free function:

unique_ptr<TelnetClient> Connect (const std::string& host, const std::string& port);
\$\endgroup\$
1
  • 2
    \$\begingroup\$ strdup() is a POSIX function, but not part of the standard C or C++ library. \$\endgroup\$ Commented Jul 27, 2021 at 20:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.