Against the background of discussions thundering here on Habré about algorithmic puzzles and interviews in Yandex, I remembered how one of my friends, who works for one rather large (non-Russian) company, once offered me a problem from the interviews that they conduct. Now the problem has changed (they found a cooler example of the code), so with his permission, I publish the old version here.
A candidate who came to the position of a developer was not given algorithmic problems, was not driven through the wilds of the language, but was simply given a code for review. Code from a real project that once tried to freeze one green junior. Fortunately, they noticed it in time.
It was suggested to do the following:
Examine the code and guess in a nutshell what it does. (everything is pretty well documented in the project, but third-party libraries with documentation and comments sometimes have trouble, and developers have to understand what's going on there)
Conduct a code-review, point out suspicious and bad places and suggest how they can be improved or redone. You can ask any questions and google whatever you want.
The code is in C ++, although most of the bugs can be found even by developers in other languages. The code was something like this:
class SomeBusServiceClient {
public:
SomeBusServiceClient();
virtual ~SomeBusServiceClient();
bool CallAsync(const std::string &uri, const std::string ¶m,
const misc::BusServiceClient::ResponseCB &callback);
bool CallSync(const std::string &uri, const std::string ¶m,
const misc::BusServiceClient::ResponseCB &callback);
private:
misc::BusServiceClient ss_client_;
static const int kSleepMs = 100;
static const int kSleepCountMax = 50;
};
class SpecificUrlFetcher : public UrlFetcher {
public:
SpecificUrlFetcher();
virtual ~SpecificUrlFetcher();
SomeData FetchData(const URL &url, const UrlFetcher::ResponseCB &callback);
private:
bool SsResponse_returnValue{false};
char SsResponse_url[1024];
void SsResponseCallback(const std::string &response);
SomeServiceClient *ss_client_;
};
// ...
static const char ss_getlocalfile_uri[] = "bus://url_replace_service";
namespace net {
pthread_mutex_t g_url_change_callback_lock = PTHREAD_MUTEX_INITIALIZER;
SomeBusServiceClient::SomeBusServiceClient()
: ss_client_(misc::BusServiceClient::PrivateBus) {}
SomeBusServiceClient::~SomeBusServiceClient() {}
bool SomeBusServiceClient::CallAsync(
const std::string &uri, const std::string ¶m,
const misc::BusServiceClient::ResponseCB &callback) {
bool bRet;
bRet = ss_client_.callASync(uri, param, callback);
return bRet;
}
bool SomeBusServiceClient::CallSync(
const std::string &uri, const std::string ¶m,
const misc::BusServiceClient::ResponseCB &callback) {
boold bRet bRet = false;
int counter;
pthread_mutex_lock(&g_url_change_callback_lock);
ss_client_.callASync(uri, param, callback);
counter = 0;
for (;;) {
int r = pthread_mutex_trylock(&g_url_change_callback_lock);
if (r == 0) {
bRet = true;
pthread_mutex_unlock(&g_url_change_callback_lock);
} else if (r == EBUSY) {
usleep(kSleepMs);
counter++;
if (counter >= kSleepCountMax) {
pthread_mutex_unlock(&g_url_change_callback_lock);
break;
} else
continue;
}
break;
}
return bRet;
}
/**************************************************************************/
SpecificUrlFetcher::SpecificUrlFetcher() {}
SpecificUrlFetcher::~SpecificUrlFetcher() {}
void SpecificUrlFetcher::SsResponseCallback(const std::string &response) {
std::unique_ptr<lib::Value> value(lib::JSONReader::Read(response));
if (!value.get() || !value->is_dict()) {
pthread_mutex_unlock(&g_url_change_callback_lock);
return;
}
lib::DictionaryValue *response_data =
static_cast<lib::DictionaryValue *>(value.get());
bool returnValue;
if (!response_data->GetBoolean("returnValue", &returnValue) || !returnValue) {
pthread_mutex_unlock(&g_url_change_callback_lock);
return;
}
std::string url;
if (!response_data->GetString("url", &url)) {
pthread_mutex_unlock(&g_url_change_callback_lock);
return;
}
SsResponse_returnValue = true;
size_t array_sz = arraysize(SsResponse_url);
strncpy(SsResponse_url, url.c_str(), array_sz);
SsResponse_url[array_sz - 1] = 0;
pthread_mutex_unlock(&g_url_change_callback_lock);
}
SomeData SpecificUrlFetcher::FetchData(const URL &url,
const UrlFetcher::ResponseCB &callback) {
lib::DictionaryValue dictionary;
std::string ss_request_payload;
misc::BusServiceClient::ResponseCB response_cb =
lib::Bind(&SpecificUrlFetcher::SsResponseCallback, this);
SomeBusServiceClient *ss_client_ = new SomeBusServiceClient();
dictionary.SetString("url", url.to_string());
lib::JSONWriter::Write(dictionary, &ss_request_payload);
SsResponse_returnValue = false;
SsResponse_url[0] = 0x00;
ss_client_->CallSync(ss_getlocalfile_uri, ss_request_payload, response_cb);
URL new_url;
if (SsResponse_returnValue) {
new_url = URL::from_string(SsResponse_url);
}
delete ss_client_;
return UrlFetcher::FetchData(new_url, callback);
}
} // namespace net
, , .
, .
- UrlFetcher, , -- - - URL'. , - - , URL, URL, . Decorator.
:
1. ss_getlocalfile_uri - . ? .
2. , . .
3. , SsResponse_returnValue
-:
4. pthread-, std::thread, .
5. - strncpy(); std::string - .
6. ss_client_ . std::unique_ptr.
7. usleep() - std::this_thread::sleep()
:
8. SomeBusServiceClient::CallSync kSleepMs kSleepCountMax, . .
:
9. message bus . . , message bus, - , kSleepCountMax*kSleepMs, , - ( callASync - id ?). - , , URL, .
9. FetchData , new_url , .
10. FetchUrl, , . , , -- WTF? ? , ?
11. ( FetchUrl ), SsResponseCallback . , , . pthread undefined behavior.
12. Valuable note from @snizovtsev : "... you cannot use this synchronization primitive for such a task. In case of a timeout, unlock will be called 2 times. That is, the code is incorrect at the algorithm level and you need to rewrite everything to a condition variable"
Answers and comments from the candidate allowed us to get an idea of his level of knowledge of modern C ++ standards and good practices, understanding of asynchrony and multithreading, meticulousness in the review and the ability to "debug" the code in his head. Well, and set topics for further conversation heart to heart.