Проходя собеседование в одну из компаний, я столкнулся с задачей, которая выделялась на фоне стандартных вопросов про алгоритмы и структуры данных. Вместо привычной реализации алгоритма мне предложили отревьюить легаси-код. Это было гораздо легче, чем алгоритмы, но не менее эффективно для проверки широты знаний. С тех пор я не расстаюсь с этой задачей уже как собеседующий. Работает гораздо лучше, чем вопросы. Это дает гораздо более реалистичное представление о навыках кандидата, чем типичные вопросы о сортировках или деревьях.
Задача заключалась в том, чтобы найти как можно больше плохих практик (далее “вонючек”) в коде и предложить варианты улучшений, не ломая существующие интерфейсы. Код не использовался в реальном проекте, но специально наполнен анти-паттернами и типичными проблемами, с которыми можно столкнуться при поддержке старых проектов. Это дало возможность продемонстрировать свои знания не только в области разработки, но и в вопросах рефакторинга и улучшения качества кода.
Почему мне нравится такое задание больше, чем задачи на алгоритмы
Метод задачи на ревью кода, по моему опыту, оказался гораздо более интересным и эффективным способом проверки знаний кандидатов, чем традиционные задачи на алгоритмы. Вот почему:
Приближенность к реальной работе. В реальных проектах редко приходится ежедневно реализовывать сложные алгоритмы. Гораздо чаще разработчики сталкиваются с задачами по поддержке и улучшению существующего кода. Именно это задание дало возможность продемонстрировать умение ориентироваться в реальных проектах, где не всё идеально, и находить решения в рамках существующей системы.
Комплексный подход. При ревью кода кандидат сталкивается с задачами из разных областей: архитектура приложения, работа с базой данных, асинхронные операции, поддержка уже существующей логики. Важно уметь решать такие задачи на стыке нескольких дисциплин.
Критическое мышление. В этом задании требовалось не просто написать код, а внимательно анализировать его, находить потенциальные проблемы и предлагать улучшения. Это включает как простые недочёты, вроде дублирования кода, так и более серьёзные архитектурные проблемы.
Практическая проверка культуры кода. Важно было не просто улучшить код, а сделать это так, чтобы не сломать существующие интерфейсы и поддерживать читаемость для других разработчиков. Это проверяет, насколько кандидат готов поддерживать и улучшать код в реальной команде.
Скрытый текст
А еще в задачах про алгоритмы обычно нет души. Сейчас поясню, что я имею в виду. Входные данные не имеют никаких ассоциативных связей с реальным миром. Набор несвязанных букв в строке, названия функций и сама постановка задачи лишена смысла. Обойти дерево - а зачем?
Перейдем к коду
Код для онлайн-магазина GigaStore, из которого выдернули несколько методов в разных слоях бизнес-логики. Задача - ревью и рефакторинг. Пример кода на C#, хотя некоторые концепции легко обнаружить, даже если вы не писали на этом языке. Для удобства ревью код содержит сразу несколько классов (что само по себе не самая распространенная практика). Строки подключения к БД, устройство моделей и DbContext тоже можно пропустить при ревью. Все остальное можно нещадно ревьюить.
Обычно в зависимости от скорости кандидата и его уровня предлагаю несколько режимов:
Устное ревью, я записываю замечания в виде TODO комментариев (тудушек)
Кандидат рассказывает и записывает тудушки сам
Кандидат ревьюит и по возможности тут же рефакторит
Кандидат рефакторит и учитывает факт, что кодом уже могут пользоваться
На задание дается 20 минут. А вот и код:
using System.Data.SqlClient;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Authorization;
using Microsoft.EntityFrameworkCore;
namespace GigaStore
{
public class UserSession
{
public static List<string> UserNames { get; set; } = new List<string>();
public static void SaveUser(string userName)
{
UserNames.Add(userName);
}
public static bool IsUserLoggedIn(HttpRequest request)
{
request.Cookies.TryGetValue("User", out var userName);
return UserNames.Contains(userName);
}
}
public class RetailController : ControllerBase
{
private DataAccessLayer _dataAccessLayer;
private BackgroundService _backgroundService;
public EmailService EmailService;
public RetailController()
{
_dataAccessLayer = new DataAccessLayer();
_backgroundService = new BackgroundService();
EmailService = new EmailService();
}
[AllowAnonymous]
[HttpGet]
[Route("PlaceOrder/{customerId}")]
public IActionResult PlaceOrder(int customerId, [FromBody] Order order)
{
if (UserSession.IsUserLoggedIn(Request))
{
_dataAccessLayer.AddOrder(customerId, order);
_backgroundService.UpdateInventory(order);
EmailService.SendOrderConfirmationEmailAsync(customerId, order);
return Ok();
}
return BadRequest();
}
}
public class DataAccessLayer
{
public void AddOrder(int customerId, Order order)
{
using (var context = new RetailDbContext())
{
var customer = context.Customers.FirstOrDefault(c => c.Id == customerId);
var customerOrders = context.Orders.Where(o => o.CustomerId == customerId).ToList();
customerOrders.Add(order);
context.Orders.Add(order);
context.SaveChanges();
}
}
}
public class BackgroundService
{
public void UpdateInventory(Order order)
{
using (var connection = new SqlConnection("..."))
{
connection.Open();
var currentDate = DateTime.Now;
var query = $"UPDATE Inventory SET Quantity = Quantity - {order.Quantity}, LastUpdated = '{currentDate}' WHERE ProductId = {order.ProductId}";
using (var command = new SqlCommand(query, connection))
{
command.ExecuteNonQuery();
}
}
}
}
public class EmailService
{
public async void SendOrderConfirmationEmailAsync(int customerId, Order order)
{
try
{
var emailContent = $"Order confirmation for order ID: {order.Id}.";
var email = new Dictionary<string, string>
{
{ "to", $"customer{customerId}@example.com" },
{ "subject", "Order Confirmation" },
{ "body", emailContent }
};
var content = new FormUrlEncodedContent(email);
var httpClient = new HttpClient();
var response = httpClient.PostAsync("https://email-api.example.com/send", content).Result;
if (!response.IsSuccessStatusCode)
{
throw new InvalidOperationException("Failed to send order confirmation email.");
}
}
catch (Exception ex)
{
Console.WriteLine($"Error sending email: {ex.Message}");
}
}
}
public class RetailDbContext : DbContext
{
public RetailDbContext() :
base(new DbContextOptionsBuilder().UseSqlite("...").Options)
{
}
public DbSet<Product> Products { get; set; }
public DbSet<Order> Orders { get; set; }
public DbSet<Customer> Customers { get; set; }
public DbSet<Inventory> Inventories { get; set; }
}
public class Product
{
}
public class Inventory
{
}
public class Customer
{
public int Id { get; set; }
}
public class Order
{
public object Quantity { get; set; }
public object ProductId { get; set; }
public object Id { get; set; }
public int CustomerId { get; set; }
}
}
В качестве подсказки, примерный список классов проблем, которые присутствуют в коде:
Статические члены класса
Проблемы с асинхронностью
SQL-инъекции
Проблемы с масштабируемостью
Нарушение принципов REST
Неоптимальное использование базы данных (Entity Framework)
Проблемы с обработкой исключений
Нарушение принципов SOLID
Хардкодинг строк и данных
Неоптимальные наименования переменных
Отсутствие использования зависимостей через DI (Dependency Injection)
Логика бизнес-процессов в контроллере
Отсутствие валидации данных
Проблемы с тестируемостью
Отсутствие логирования
Проблемы с управлением состоянием (State Management)
Отсутствие транзакций
Попробуйте найти как можно больше “вонючек” в коде за ограниченное время. Это отличная практика для того, чтобы развить навыки рефакторинга и улучшения кода. Можно потренироваться в выполнении таких заданий на мок-собеседованиях (статья).
Вступайте в нашу Telegram группу - там готовимся к собесам в IT и помогаем друг другу. Больше полезных ресурсов для поиска работы в IT у меня в профиле.
Комментарии (12)
ForestDront
05.11.2024 09:35Интересный вариант. Но на практике, когда приходишь на проект, никогда не разрешают переделывать код. Всегда требуют писать в том же стиле, что уже есть на проекте.
AleksSl
05.11.2024 09:35Да, это действительно неплохой подход, лично мне такое нравится. Однако, тут может быть несколько вопросов.
Во первых разные подходы в понимании "что такое хорошо и что такое плохо". Например людям может быть "наплевать" на солид, но они любят KISS и т.д. Это я привел просто какой-то крайний случай различий. И тут получается, что да, мы берём специалиста "под себя", но мы не можем гарантировать, что выбор будет лучшим.
Код слишком длинный. Лично я просто "запутался" пока читал ) Да, 20 минут исправят ситуацию само собой, но не слишком ли это будет долго? Собеседование в любом случае стрессовая ситуация, а ещё и сидеть так долго и "под присмотром". Это конечно чисто мое имхо, но я бы сократил код и просто дал бы меньше времени.
Но, в любом случае, это действительно лучше чем решать тупые стандартные задачки.
orefkov
05.11.2024 09:35На нескольких собесах сталкивался с таким. Вроде и в Яндексе просили чужой код прокомментировать. Потом в реальной работе обидно бывает, когда "работает же, не трогай, нам сейчас не до этого, пили новую фичу, потом как нибудь отрефакторим".
anonym0use
05.11.2024 09:35Да, такой подход получше чем олимпиадные задачки, особенно если вы не гугл.
sergeykonshin
05.11.2024 09:35Нахожу 5 ошибок, дальше отправляю лесом(передаю коллегам). Ибо в реальности отправлю автора такого кода на... performance improvement plan, т.к. в работать с такими очень сложно.
Сам проходил через это со стороны ревьюера, поэтому более профессиональным считаю по максимуму отдать такую работу линтерам, стайлчекерам и т.п. анализаторам кода. (читать статью "Вы не умеете делать code-review")
serjeant
05.11.2024 09:35Отличный подход! Спасибо за статью! Надеюсь рано или поздно компании откажутся от лавкодинга в пользу рефакторинга.
Когда приходишь на новую работу в первую очередь на тебя вываливают кучу легаси кода и с ним приходится разбираться. А не деревья вертеть как на собесе.
AnSt
05.11.2024 09:35Два раза встречался с таким подходом на собеседовании - и оба раза мне понравилось. Опыта просмотра кода много, т.к. преподаю и применяю подход: студенты проводят ревью моего кода, а я их. Поэтому более-менее просто могу увидеть что-то что мне не понравится в коде. Но, опять же, у меня один подход к написанию, у кого-то другого - другой. И не всегда он совпадает. Понятно, что есть что-то "фундаментально" как именование, но есть и что-то более размытое. Например, DI (Dependency Injection).
Но бывают ситуации, про которые выше уже писали, что на собеседовании ты делаешь ревью кода - а на работе "потом, сейчас некогда...". И это очень печалит.
mmusin
05.11.2024 09:35Очень простой и скучный код в примере. Все проблемы расскажет любой толковый студент.
Но если у вас задачи уровня «сложить формы в базу», для которых студентов достаточно, то норм.
dontuseit Автор
05.11.2024 09:35Этот код содержит как простые ошибки, которые заметит любой толковый студент, так и менее заметные и концептуальные. Например, HttpClient socket exhaustion или отсутствие TimeProvider. К тому же про каждую из таких ошибок - можно рассказать более развернуто с примерами из опыта. Я насчитал как минимум 50 недочетов. Уверен, их можно найти больше.
Получается занимательная беседа двух разработчиков которые общаются наравне - гораздо приятнее, когда оценивают не тебя, а ты оцениваешь код в комфортной беседе с интервьюером. Еще классно, когда кандидат находит новую ошибку или предлагает идею, которую я раньше не замечал.
panzerfaust
Хороший подход. Пока активно собесил, всегда строил практическую часть разговора вокруг код-ревью. Кусок кода был гораздо грязнее и, что называется, "с особым цинизмом". Я там насчитывал до 26 проблем. Сеньоры без проблем находили почти все, джуны и самозванцы могли и пяти не осилить. Выборка около 100 кандидатов. Алгосы и теория всегда оставляют сомнение, а ревью почти никогда.