I'll make no comment on the use of C-style <stdio.h>
, since that's likely an artefact of the surrounding code, rather than something you'd have written yourself.
A missing header and a typo/oversight
The header required for std::make_pair
is <utility>
. Your implementation may have defined it as a side-effect of including <map>
, but it's not portable to rely on that.
size_t pos
ought to be std::size_t pos
.
Test program
I managed to re-create some input from your sample output:
int main()
{
static auto const messages = {
"<3>CTRL-EVENT-SCAN-STARTED ",
"<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00",
"<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01",
"<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02",
"<3>CTRL-EVENT-SCAN-RESULTS ",
"<3>WPS-AP-AVAILABLE ",
"<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)",
"<3>Associated with 00:00:00:00:00:00",
"<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0",
"<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]",
"<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]",
};
wpa_monitor monitor;
for (auto m: messages)
monitor.handle_wpa_message(m);
}
Naming
I avoid names that include their type, as it can be confusing if you later change that type. So I'd call the map mac_to_ssid
instead. That also has the benefit of being shorter, so easy to type and to read.
Extensibility
You'll find that a single large method gets harder to navigate as it gets bigger. As hinted in another answer, it makes sense to delegate to a bunch of simple methods that each handle a single case. You could iterate through a vector, but I'd stick to something simple like this:
class wpa_monitor
{
public:
bool handle_wpa_message(const std::string& message);
private:
std::map<std::string, std::string> mac_ssid_association_map = {};
bool handle_associate(const std::string& message);
bool handle_connected(const std::string& message);
bool handle_scanresult(const std::string& message);
};
bool wpa_monitor::handle_wpa_message(const std::string& message)
{
DEBUG_PRINT("WPA event: '%s'\n", message.c_str());
return handle_associate(message)
|| handle_connected(message)
|| handle_scanresult(message);
}
Now we can look at each matcher in turn.
Refactor some common code
We repeatedly search for a string and take an interest in its end, like this:
auto pos = message.find("Trying to associate with ");
if (pos == std::string::npos)
return false;
std::string mac_substring = message.substr(pos + sizeof "Trying to associate with " - 1);
Obviously there's scope for error if we don't keep the two copies of the string in agreement. Instead, we can write a small function to do this:
static std::size_t find_end(const std::string& s, const std::string& substr)
{
auto pos = s.find(substr);
if (pos != std::string::npos)
pos += substr.size();
return pos;
}
That makes the caller simpler and more robust:
auto pos = find_end(message, "Trying to associate with ");
if (pos == std::string::npos)
return false;
auto const mac_substring = message.substr(pos);
Actually, that's not really the MAC substring, as it continues to the end of the string. We really want another helper, accepting a termination character:
static std::string find_in_delimiters(const std::string& s,
const std::string& open, const char close)
{
auto start = s.find(open);
if (start == std::string::npos)
return {};
start += open.size();
auto end = s.find(close, start);
if (end == std::string::npos)
return {};
return s.substr(start, end-start);
}
And we simplify the callers even more:
auto mac_addr = find_in_delimiters(message, "Trying to associate with ", ' ');
if (mac_addr.empty()) {
return false;
}
More on make_pair()
I get a failure to compile, as GCC seems to want rvalue references for make_pair<string,string>()
. I could resolve that with a std::move()
like this:
mac_ssid_association_map.insert( std::make_pair< std::string, std::string >( std::move(mac_addr), std::move(ssid) ) );
However, we can let template type deduction do its thing:
mac_ssid_association_map.insert(std::make_pair(std::move(mac_addr), std::move(ssid)));
Or even just:
mac_ssid_association_map.insert({std::move(mac_addr), std::move(ssid)});
Use auto
instead of exact iterator types
Here's a place where changing the map type could be a maintenance headache:
std::map< std::string, std::string >::iterator it = mac_ssid_association_map.find( mac_addr );
We can use auto
, which also makes it more readable:
auto it = mac_ssid_association_map.find(mac_addr);
My version
#include <cstdio>
#include <string>
#include <map>
#include <utility>
#define DEBUG_PRINT std::printf
class wpa_monitor
{
public:
bool handle_wpa_message(const std::string& message);
private:
std::map<std::string, std::string> mac_to_ssid = {};
bool handle_associate(const std::string& message);
bool handle_connected(const std::string& message);
bool handle_scanresult(const std::string& message);
};
// Helper method - returns substring of s contained between
// open and close, or empty if not matched
static std::string find_in_delimiters(const std::string& s,
const std::string& open, const char close)
{
auto start = s.find(open);
if (start == s.npos)
return {};
start += open.size();
auto end = s.find(close, start);
if (end == s.npos)
return {};
return s.substr(start, end-start);
}
// Handlers for specific message types
bool wpa_monitor::handle_associate(const std::string& message)
{
auto mac_addr = find_in_delimiters(message, "Trying to associate with ", ' ');
if (mac_addr.empty())
return false;
auto ssid = find_in_delimiters(message, "SSID='", '\'');
if (ssid.empty())
return false;
DEBUG_PRINT("Associating mac_address '%s' with ssid '%s'\n", mac_addr.c_str(), ssid.c_str());
mac_to_ssid.insert({std::move(mac_addr), std::move(ssid)});
return true;
}
bool wpa_monitor::handle_connected(const std::string& message)
{
auto mac_addr = find_in_delimiters(message, "CTRL-EVENT-CONNECTED - Connection to ", ' ');
if (mac_addr.empty())
return false;
auto it = mac_to_ssid.find(mac_addr);
if (it != mac_to_ssid.end()) {
DEBUG_PRINT("We are now connected to ssid '%s'\n", it->second.c_str());
}
return true;
}
bool wpa_monitor::handle_scanresult(const std::string& message)
{
auto pos = message.find("CTRL-EVENT-SCAN-RESULTS");
if (pos == message.npos)
return false;
//scan results ready
DEBUG_PRINT("Scan results are ready\n");
return true;
}
// Test program
int main()
{
static auto const messages = {
"<3>CTRL-EVENT-SCAN-STARTED ",
"<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00",
"<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01",
"<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02",
"<3>CTRL-EVENT-SCAN-RESULTS ",
"<3>WPS-AP-AVAILABLE ",
"<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)",
"<3>Associated with 00:00:00:00:00:00",
"<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0",
"<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]",
"<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]",
};
wpa_monitor monitor;
for (auto m: messages)
monitor.handle_wpa_message(m);
}
wpa_monitor
and/or the necessary#include
lines, and amain()
that shows how to call your function. It can really help reviewers if they are able to compile and run your program. A sample (anonymised) input would also help, if you can provide that. \$\endgroup\$