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 constructor 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.
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.
#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);
// 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();
}
