Как-то в одной известной компании решили упразднить процесс под названием "код-ревью". Упразднили не на уровне компании, а на уровне отдела, но в итоге упразднили не процесс, а того самого менеджера (по офицальный версии причины были другие, но мы-то знаем). Процесс (код-ревью), как видно, полезный, если способен пережить одного менеджера, но видно, вопросы к нему имеются. Код-ревью представляет собой взаимодейсвие автора пулл реквеста и ревьюера, что-то типа игры с ненулевой суммой, где та самая сумма аккумулируется между участниками процесса.
Фикс критической ошибки (не архитектурной или по дизайну системы, а именно ошибки/баги) не добавляет профита ревьюеру. При удачной фиче похвала и бонус пойдут автору. То есть действия ревьюрера приносят ноль ему и что-то автору. Хотя ревьюер может не только помочь, но и навредить - заблочить пул реквест (и иногда действительно такое случается), бизнес невилирует отрицательные последствия - вмержат только в путь, когда бабло начнет течь мимо кассы. Не беря во внимание совесть ревьюера, получить максимальный профит можно на дейликах (ведь для этого утренние созвоны и нужны, так ведь?) - аккурат перед мержем пулл реквеста упомянуть о критической неисправности.
Ревью это про чтение кода - выбор золотой середины между мыслями автора и доходчивостью ревьюера. Последний может попросить скомпоновать код попроще, что скорее всего заставит автора выгрузить из оперативной памяти смысловые блоки под запись в файл. Данную аргументацию дополняет еще и условие: пишем раз, читаем много раз. Если ревьюеру хочется сделать фичу самому - лучше сделать ее самому, а если уже поздно - лучше не бурчать, а понять и простить. Но никто же не просит устранять неэффективность (говнокод) без удовольствия, так? Предлагаю рассмотреть примеры кода на языке "тайпскрипт", хотя идеи ниже могут быть применимы к любому языку программирования.
На всякий случай, примеры из пулл реквестов, увиденных автором буквально за прошедшую неделю.
Кого не спроси, каждый за иммутабельность, каждая книга вдалбливает идеальные неизменяемые модели. Так и есть? Да. Но вот порой код ну пишется так - прямо, четко, последовательно: создали обьект и добавили поле по условию. Ведь ну создали, и добавили по условию - все просто, да?
const authResponse: AuthResponse = { principalId };
if (effect && resource) {
authResponse.policyDocument = policyDocument;
}
authResponse.context = context;
return authResponse;
Все ок, только перед мержем пулл реквеста убираем мутабельность, и будет супер.
return {
principalId,
context,
...(effect && resource ? {policyDocument} : {}),
}
По-моему, кто-то когда-то очень жестко всех обманул (киданул), сказав что комменты в коде это легко и правильно, и сложно ошибиться. К сожалению, компилятор и линтер тут не помощники, и нужно хорошенько подумать, при каких условиях вы, следующий, будете их (комменты) читать. Ниже автор пулл реквеста предлагает удвоить себе работу в будущем, так как новый 160 придется править еще и в комменте.
// merge articles and galleries into a single array and organise by date
const contents = [...(articles ? articles : []), ...(galleries ? galleries : [])]
.sort((a, b) => (b.displayDate || b.publishDate || new Date()).getTime() - (a.displayDate || a.publishDate || new Date()).getTime())
.slice(0, 160); // limit to 160 items
Выкидываем комменты (включая первую строку, которая добавляет ровно ноль вэлью), упрощаем код и фиксим баг (который, если честно, не так легко заметить до рефакторинга):
const WINDOW_SIZE = 160;
const now = Date.now();
const selector = (d) => (d.displayDate ?? d.publishDate)?.getTime() ?? now;
const contents = [...(articles ?? []), ...(galleries ?? [])]
.sort((x, y) => selector(y) - selector(x))
.slice(0, WINDOW_SIZE);
Комменты, к сожалению, могут предпринять операцию по захвату мозга автора, в следствии чего он (автор) сразу же начинает играть в комментно-ориентированное программирование: код уже не главный, и удачно составленные записки на полях (хорошие комментарии) заменяются банальной константацией факта без особой пользы (иногда и во вред) для читающего.
// Call this after redirect from Authentik to complete login
export async function handleRedirectCallback() {
await userManager.signinRedirectCallback();
window.location.replace("/"); // Redirect to the home page
}
Не рассматривая относительную ценность комментария ниже, обычно если хочется что-то написать про блок кода (for-loop), лучше вынести его (блок) в отдельную функцию и придумать ей шикарное имя.
// For every button 'select article' select an article from those available
for (let i = 0; i < params.numberOfArticles; i++) {
await params.channelPage.channelArticlePlacementSelect.nth(i).click();
...
}
К сожалению, к следующей зарисовке оригинал я бесследно потерял (копия почти идентична оригиналу), но тут прям нужно показать прекрасный пример, в котором комментирование превращается в инструмент, который как бы обрисовывает приблизительный тип переменной и побеждает разработчика, который честно заленился импортировать тип.
// The type of the button will be 'ButtonView', just as in the articles or galleries.
const button = makeGreatButtonAgain(id, props);
Венцом комментно-ориентированного программирования есть прецедент увеличения кол-ва инструкций из-за неброского коммента (вместо одной переменной сделать две, но не поняв какие имена им дать, воспользоваться услугой переприсвоения). Там же вопрос к имени этой переменной - немного плохой тон зашивать тип в имя (см. dateString). Аналогия как с интерфейсом и классами - лучше переменная с интерфейсом, чем с конкретным классом (Set лучше чем НashSet или TreeSet).
const GroupedContentDate: Component<IGroupedContentDateProps> = (props) => {
const dateTime = formatISO(props.displayDate);
let dateString = `${formatTime(props.displayDate)} Uhr`; // base date string (used also for today's articles)
dateString = `${formatDate(props.displayDate)}, ${dateString}`;
Убираем let, заменяем имена переменным:
const GroupedContentDate: Component<IGroupedContentDateProps> = (props) => {
const displayDate = formatISO(props.displayDate);
const dateTime = `${formatDate(props.displayDate)}, ${formatTime(props.displayDate)} Uhr`;
Следующая зарисовка не только про window.location (которая повторяется четыре раза), но и про ошибку компоновки - очень не желательно совмещать предикат (который функция, возвращающая булевый результат) с действием (которое почти всегда "сайд-эффект"). Предикат лучше делать "чистой" функцией, так как легче тестить.
async function redirectIfFromAuthentik(): Promise<void> {
if (
window.location.pathname === "/oauth2/idpresponse" &&
window.location.search.includes("code") &&
window.location.search.includes("state")
) {
await userManager.signinCallback();
window.location.replace("/");
}
}
Выносим условие в отдельную функцию (и пишем тесты, если захотим):
type IsFromAuthentikParams = {search: string; pathname: string};
const isFromAuthentik = ({search, pathname}: IsFromAuthentikParams): boolean =>
pathname === "/oauth2/idpresponse" && ["code", "state"].every(x => search.includes(x));
В заключение чуть-чуть про фидбек, который получаем (мы ведь не только ревьюим, да?) на код-ревью - блин, ну какую же чушь иногда пишут люди :) - термины вида "идемпотентный" почему-то триггерят похожие фразы: "мы так сделали в другом файле два дня назад" и т.д. Удачи вам на код-ревью, и делайте хороший продукт, когда вам захочется, а когда не хочется - принимаем фразы на экране не близко к сердцу, потому что комментатор общается с буквами на экране, и в лучшем случае строчками в блокноте.