Альтернативное собеседование на позицию разработчика ПО +10


AliExpress RU&CIS

На фоне гремящих здесь на Хабре дискуссий про алгоритмические задачки и собеседования в Яндекс, вспомнил, как один мой приятель, работающий в одно довольно большой (не-российской) компании, как-то раз предлагал мне задачку с собеседований, которые проводят они. Сейчас задачка изменилась (они нашли более крутой пример кода), поэтому с его позволения, публикую старый вариант здесь.

Кандидату, приходящему на должность разработчика, не давали алгоритмических задачек, не гоняли по дебрям языка, а просто давали на ревью код. Код из реального проекта, который один раз пытался смёржить один зеленый джуниор. К счастью, вовремя заметили.

Предлагалось сделать следующее:

  1. Изучить код и в двух словах предположить, что он вообще делает. (в проекте все довольно хорошо документировано, но вот у сторонних библиотек с документацией и комментариями иногда беда, и разработчикам приходится понимать, что там вообще происходит)

  2. Провести code-review, указать на подозрительные и плохие места и предложить, как можно их улучшить или переделать. Можно задавать любые вопросы и гуглить все что угодно.

Код на C++, хотя большинство ошибок смогут найти даже разработчики на других языках. Код был примерно такой:

class SomeBusServiceClient {
public:
  SomeBusServiceClient();
  virtual ~SomeBusServiceClient();
  bool CallAsync(const std::string &uri, const std::string &param,
                 const misc::BusServiceClient::ResponseCB &callback);
  bool CallSync(const std::string &uri, const std::string &param,
                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 &param,
    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 &param,
    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

Ответы будут под спойлером, нажимайте на него осознанно, пути назад уже нет.

Итак, ответы.
  1. У нас есть какой-то класс UrlFetcher, задача которого, судя по всему -- получать какие-то данные по какому-то URL'у. Унаследованный от него класс делает то же самое, только перед запросом обращается по какой-то шине сообщений к какому-то внешнему сервису, отправляя ему запрошенный URL, и вместо него получает от этого сервиса некий другой URL, который и используется дальше. Этакий паттерн Decorator.

  2. Сначала по мелочам:

    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. Ценное замечание от @snizovtsev: "...использовать этот примитив синхронизации для такой задачи нельзя. В случае таймаута 2 раза вызовется unlock. Т.е. код неверен на уровне алгоритма и нужно всё переписывать на condition variable"

Ответы и замечания от кандидата позволяли составить представление о его уровне владения современными стандартами C++ и хорошими практиками, понимании асинхронности и многопоточности, дотошности на ревью и умении "отлаживать" код в голове. Ну и задать темы для дальнейшего разговора по душам.




Комментарии (16):

  1. svr_91
    /#22895672

    Проще все переписать, чем объяснить, что не так.

    1. ss_getlocalfile_uri — глобальная переменная. Зачем? Можно было объявить ее внутри одного из классов.

    А зачем? Мне кажется, глобальные переменные в таком виде лучше, не торчат из header-а класса

  2. SpiderEkb
    /#22895716

    Нравится такой подход.
    Понятно, что это не для джуна, а начиная хотя бы с мидла.

    • /#22895740

      Ну да, все верно. Джун при ревью скорее всего особо не поймет, что же в этом коде не так.

  3. m03r
    /#22895888

    А у вас специально в этом фрагменте форматирование чёрт знает какое? Или просто криво вставилось?

    • /#22896088

      Криво вставилось.
      Прогнал через clang-format еще раз, должно быть получше.

  4. leshakk
    /#22896256

    Подход принципиально правильный, но есть существенный момент: делается ли перерыв на несколько минут чтобы можно было подумать спокойно, не под сверлящим взглядом и нетерпеливым сопением собеседующего?

  5. snizovtsev
    /#22896522

    При этом получается, что мы разблокируем мьютекст не в том потоке, где мы его блокировали. Для pthread это явный undefined behavior.

    Да даже если бы разрешалось разблокировать мьютекс в другом треде — использовать этот примитив синхронизации для такой задачи нельзя. В случае таймаута 2 раза вызовется unlock. Т.е. код неверен на уровне алгоритма и нужно всё переписывать на condition variable, а потому в остальные мелочи даже вникать лень.

    • /#22896576

      Код неверен на уровне алгоритма, еще когда внутри предполагаемо асинхронной функции заведомо асинхронный запрос делается синхронным с помощью бесконечного цикла со sleep'ом и таймаутом…

      Но насчёт conditional variable тоже ценное замечание, да.

  6. dom1n1k
    /#22896540

    А я вот НЕ поддержу такой подход. Потому что «уметь найти чужие ошибки в готовом решении» и «не допустить свои ошибки, решая задачу с нуля» — это разные уровни. Во многих случаях второе достигается существенно позднее первого.

    • leshakk
      /#22896618

      Во всяком случае, эта задача существенно ближе к реальности,
      чем самописная сортировка пузырьком или обоснование геометрической формы канализационных люков.

    • realimba
      /#22897090

      99% задач обычного дева это багофикс и доработка уже написанного, довольно практичный примерчик, очень сомневаюсь что новоприбывший сразу будет строчить тысячи срок в прод..

  7. knagaev
    /#22897510

    Прошу прощения за глупый вопрос — а где определение класса SomeBusServiceClient?

    • knagaev
      /#22897530

      Я имею в виду, что есть определение только SomeServiceClient

      class SomeServiceClient {
      public:
        SomeServiceClient();
        virtual ~SomeServiceClient();
        bool CallAsync(const std::string &uri, const std::string &param,
                       const misc::BusServiceClient::ResponseCB &callback);


      а вот ниже идут реализации уже методов SomeBusServiceClient
      bool SomeBusServiceClient::CallAsync(
          const std::string &uri, const std::string &param,
          const misc::BusServiceClient::ResponseCB &callback) {
        bool bRet;
        bRet = ss_client_.callASync(uri, param, callback);
        return bRet;
      }


      Крышу немного рвёт…

      • /#22897688

        Да, это один и тот же класс. В процессе подготовки кода к публикации (чтобы не палить внутреннюю кухню) напутали немножко. Отредактировал.

  8. eptr
    /#22898856

    А сколько времени, предполагалось, кандидат должен был потратить на code-review этого фрагмента?

  9. Zuy
    /#22899036

    У меня в Google один из раундов был провести ревью кода на С. Очень понравилось по сравнению с классическими алгоритмическим раундами.