3
\$\begingroup\$

Update: see also version 2


The Service

This code uses two API's.

List

 https://LokiAstari.com/sample/list
 https://LokiAstari.com/sample/list?token=<continuation Token>

This API returns a JSON object. The first version starts a list and will return an array of user ID (not all of them). The second version takes a token that was provided in a previous result and returns the next set of user ID's continuing from the previous location. If all users have been returned then the token is null.

 {
     result:  [ <List of User ID> ],
     token:   "<token>" or null
 }

Detail

 https://LokiAstari.com/sample/detail/<User-ID>

This returns a JSON object with details of the user specified by the ID.

{
        "id":       <User ID: Number>,
        "name":     "<User Name: String>",
        "age":      <User Age: Number>,
        "number":   "<User Tel Number: String>",
        "photo":    "<User Image: URL(String)>",
        "bio":      "<User Bio: String>"
}

Notes:

It uses two of my other Libraries to make stuff simpler:

ThorsSerializer

Used to serialize JSON to/from C++ objects.

ThorsStream

Used to wrap CURL handle so that it looks like a std::istream.

Internally it uses MCURL handle to handle multiple CURL handles simultaneously with a single thread. A thread that reads from a stream that has an empty buffer will be released to do other work until there is data available in the buffer at which point that stream will become re-used.

What the app does:

The code finds the 5 youngest users who have valid US telephone numbers. It sorts the 5 Users by name and prints out the result.

A valid US telephone number is defined as:

<3 Digits><Sep><3 Digits><Sep><4 Digits>

Digit:  0-9
Sep:    <space> or -

Code

#include <iostream>
#include <future>
#include <vector>
#include <string>
#include <memory>
#include <algorithm>
#include <regex>
#include <mutex>

#include "ThorSerialize/Traits.h"
#include "ThorSerialize/SerUtil.h"
#include "ThorSerialize/JsonThor.h"
#include "ThorsStream/ThorsStream.h"

using namespace std::string_literals;

// Some global constants.
const std::string api       = "https://LokiAstari.com/sample"s;
const std::string apiList   = api + "/list"s;
const std::string apiDetail = api + "/detail/"s;
const std::regex  phoneNumber("^[0-9][0-9][0-9][- ][0-9][0-9][0-9][- ][0-9][0-9][0-9][0-9]$");

// In this app List and User
// are simply property bags no nead to have access functions.
// If this was a more complex app then we would consider having other methods.
struct List
{
    std::vector<int>                result;
    std::unique_ptr<std::string>    token;
};

struct User
{
        int                     id;
        std::string             name;
        int                     age;
        std::string             number;
        std::string             photo;
        std::string             bio;

};

// Set up comparison functions used on user.
const auto youngestUser = [](User const& lhs, User const& rhs){return lhs.age < rhs.age;};
const auto nameTest     = [](User const& lhs, User const& rhs){return lhs.name < rhs.name;};

// Set up List and User to be read from JSON stream.
// See: jsonImport() and jsonExport() below
ThorsAnvil_MakeTrait(List, result, token);
ThorsAnvil_MakeTrait(User, id, name, age, number, photo, bio);


// A generic Job.
// Simply reads an object from an istream.
// If the read worked then processes it.
// Note: An istream treats a CURL socket like a standard C++ stream.
template<typename T>
class Job
{
    ThorsAnvil::Stream::IThorStream     istream;
    public:
        Job(std::string const& url)
            : istream(url)
        {}
        virtual ~Job()
        {}

        void run(std::vector<User>& result)
        {
            using ThorsAnvil::Serialize::jsonImport;
            T data;
            if (istream >> jsonImport(data)) {
                processesData(result, data);
            }
            else {
                // Do some error handling
            }
        }

        virtual void processesData(std::vector<User>& result, T const& data) = 0;
};

// A job to handle the details from getting a user object.
class UserJob: public Job<User>
{
    public:
        using Job<User>::Job;
        virtual void processesData(std::vector<User>& users, User const& user) override
        {
            // Check if the phone number is OK.
            if (std::regex_search(user.number, phoneNumber)) {

                // Mutex shared across all objects (notice the static).
                static std::mutex  mutex;

                // Lock the mutex when modifying "users"
                std::lock_guard<std::mutex>   lock(mutex);

                // Add the user to a heap.
                // The heap is ordered by youngest person.
                users.emplace_back(std::move(user));
                std::push_heap(users.begin(), users.end(), youngestUser);
                if (users.size() == 6) {
                    // If we have more than 5 people the pop the oldest one off.
                    // Thus we maintain a heap of the 5 youngest people.
                    std::pop_heap(users.begin(), users.end(), youngestUser);
                    users.pop_back();
                }
            }
        }
};

// A job to handle the list object.
class ListJob: public Job<List>
{
    public:
        using Job<List>::Job;
        virtual void processesData(std::vector<User>& users, List const& data) override
        {
            if (data.token.get()) {
                // If we have a continuation token
                // Then add another job ("ListJob") to the async queue.
                std::async([&users, job = std::make_unique<ListJob>(apiList + "?token=" + *data.token)](){job->run(users);});
            }
            for(auto const& userId: data.result) {
                // For each user add a job ("UserJob") to the async queue.
                std::async([&users, job = std::make_unique<UserJob>(apiDetail + std::to_string(userId))](){job->run(users);});
            }
        }
};

int main()
{
    std::vector<User>   users;

    std::async([&users, job = std::make_unique<ListJob>(apiList)](){job->run(users);});
    // This will not return until all async jobs have completed.

    std::sort(users.begin(), users.end(), nameTest);
    using ThorsAnvil::Serialize::jsonExport;
    std::cout << jsonExport(users) << "\n";
}
\$\endgroup\$
2
  • \$\begingroup\$ Do valid area codes really start with zero? \$\endgroup\$
    – pacmaninbw
    Nov 3, 2019 at 16:14
  • \$\begingroup\$ @pacmaninbw I did not set the definition of a valid phone number. I was just making sure it was explained for this context. \$\endgroup\$ Nov 3, 2019 at 16:59

2 Answers 2

5
\$\begingroup\$

Not really a C++ coder, and certainly not a reviewer, yet I'd be commenting on that phone number expression.

I guess we can just do a minor simplification on that expression by adding a single-boundary quantifier,

^[0-9]{3}[ -][0-9]{3}[ -][0-9]{4}$

Demo 1

unless there would have been maybe a reason of some kind (that I wouldn't know), and we are not using the {} quantifier:

^[0-9][0-9][0-9][- ][0-9][0-9][0-9][- ][0-9][0-9][0-9][0-9]$

I also like [0-9] better than \d construct.


Here, we are assuming that,

"123 456-7890"
"123-456 7890"
"000-000 0000"

are valid. In case, those would be considered invalid values, we can likely modify our expression with a back-reference, similar to:

^[0-9]{3}([ -])[0-9]{3}\1[0-9]{4}$

Demo 2

Or I guess, a simple alternation might suffice here:

^[0-9]{3}(?:-[0-9]{3}-| [0-9]{3} )[0-9]{4}$

Demo 3


Another way, which might be much simpler, would be to collect the digits and remove the non-digits, and check upon those digits to see if they'd fulfill our 10-digits validation criteria.


Of-course, for the "real validation" of those numbers, there should be some APIs, which I guess, that's not what we're trying to do here.


Overall, your codes look pretty great.


If you wish to simplify/modify/explore the expression, it's been explained on the top right panel of regex101.com. If you'd like, you can also watch in this link, how it would match against some sample inputs.


RegEx Circuit

jex.im visualizes regular expressions:

enter image description here

\$\endgroup\$
4
\$\begingroup\$

This is nice code, but I have some suggestions for how it might be improved.

Use a better data structure

The use of the heap is not bad and is intuitively a reasonable structure for keeping the five youngest users, but because it's only five entries, I'd suggest that a std::array<User,5> might be better. Even a linear search would require a very few comparisons and the advantage is that it's a fixed size structure.

Do the cheaper tests first

Right now, the processesData function compares phone number first and then age. Since the age comparison does not use a regex, I would strongly suspect that it is a less computationally expensive comparison, so it would probably make sense to do that first. Obviously this is somewhat data-dependent, but it's worth thinking about.

Use regex_match to match a whole string

The current code is using regex_search which looks for a match anywhere within the string, but the regex itself starts with '^' and ends with '$', so clearly the intent is to only match the entire string. For that, regex_match is more appropriate than regex_search and you can omit the start and end tokens from the regex.

Minimize the time a mutex is held

Right now the code holds a mutex lock even before we know that this will actually alter the underlying structure. That is, we may add a user who is older than the oldest person currently in the heap, only to remove that user again. That's inefficient and holds the lock for longer than the mimimum time. Instead, I'd do something like this:

#include <iostream>
#include <regex>
#include <string>
#include <array>
#include <mutex>

const std::regex  phoneNumber("[0-9][0-9][0-9][- ][0-9][0-9][0-9][- ][0-9][0-9][0-9][0-9]");

struct User {
    std::string phone;
    int age{999};   // start with invalid age
};

std::ostream& operator<<(std::ostream& out, const User& user) {
    return out << "age: " << user.age << ", phone: " << user.phone;
}

const auto youngestUser = [](User const& lhs, User const& rhs){return lhs.age < rhs.age;};

int main() {
    using namespace std;

    vector<User> samples{
        {"212-123-4567", 10},
        {"212-123-4568", 81},
        {"212-123-4569", 18},
        {"2 2-123-4570", 99},
        {"212-123-4571", 57},
        {"2 2-123-4572", 45},
        {"212-123-4573", 33},
        {"212-123-4574", 21},
        {"212-123-4575", 18},
        {"2 2-123-4576", 16},
        {"212-123-4577", 30},
        {"2 2-123-4578", 50},
        {"212-123-4579", 77},
        {"2 2-123-4580", 23},
    };

    array<User, 5> result;
    cout << "before:\n";
    copy(result.begin(), result.end(), ostream_iterator<User>{cout, "\n"});
    for (const auto& person: samples) {
        if (person.age < result.back().age && regex_match(person.phone, phoneNumber)) {
            User youngerPerson(person);
            lock_guard<mutex> lock(mutex);
            if (person.age < result.back()) {
                swap(youngerPerson, result.back());
                sort(result.begin(), result.end(), youngestUser); 
            }
        }
    }
    cout << "after:\n";
    copy(result.begin(), result.end(), ostream_iterator<User>{cout, "\n"});
}

Obviously this sample code is single-threaded, but it shows the suggested lock placement accurately. It also shows doing one last comparison after the lock is obtained to avoid data race problems in which another thread has modified result between the time of the check and the time this thread obtains the lock.

Don't write misleading comments

The code contains this:

std::async([&users, job = std::make_unique<ListJob>(apiList)](){job->run(users);});
    // This will not return until all async jobs have completed.

However, that's not really true. An asynchronous call is, well, asynchronous, so depending on the launch policy (which isn't shown in this code), it might very well return immediately. Since the intent seems to be to run the code synchronously here, just remove the std::async wrapper and execute the lambda.

\$\endgroup\$
2
  • \$\begingroup\$ Yea. I though that comment was true at the time of writing. But I was wrong. Fixed in version 2. :-) \$\endgroup\$ Nov 4, 2019 at 16:57
  • \$\begingroup\$ I only saw version 2 after I had reviewed this one, but I'm hoping to get some time soon to review that one as well. \$\endgroup\$
    – Edward
    Nov 4, 2019 at 16:59

Your Answer

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

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