@nikifovadim
Software Engineer

Я сделал Code Review, может быть я где-то ошибся или у вас есть что добавить?

Вообщем как-то на днях я проходил собеседование в одну компанию как middle+/senior разрабочик и меня попросли сделать Code Review. То как я это сделал им не понравилось. Может быть нужно было дать более развернутое описание почему так нельзя а так можно? Предлогаю Вам взглянуть на код, я закоментировал проблемные места по моему мненю. Может я сделал что-то не так? Или у Вас есть что добвать? Или я не все нашел? Прошу напишите ответ как можно более развернутым.

using System;
using System.Collections.Generic;

//EXTERNAL CODE
//Приведен только для справки, никак нельзя менять, код обфусцирован, исходников нет
public sealed class DataProvider : IDisposable
{
    public extern int LongRunningCalculation(int value, int value2);
    public extern void Dispose();
}

// CODE FOR REVIEW:
class Class2 {
    // Здесь есть какие-то ошибки тут? 
    private readonly object _sync = new object();

    // Переменая должна быть задукументирована что это такое _ht?
    // Предпологаю что нельзя использовать int[] как ключ Почему нельзя? 
    // Может такой словарь иметь ключ и второе значение как список или массив?
    // Думаю да! Вот так Dictionary<int, List<string>>
    // Памятка что значение как массив или список может быть, а может быть оно как object (boxing / unboxing)? Не класс ил strcut?
    // Какой ключ ты предлогаешь использовать?
    // struct (value type) лучше чем class (reference type) и поэтому если ты хочешь написать свой ключ хороший, то во первых ты должен использовать struct, это не ссылочный тип и будет работать быстрее чем класс. 
    private static Dictionary<int[], object> _ht;

    public int GetValue(int index, int index2)
    {
        Init(); // Это должно быть в constructor? Вроде да! А где constructor?
        // Как уже упоминал выше это плохой ключ
        var key = new[] {index, index2};
        // Слишком сложная логика проверки думаю что ее можно вынести в отдельный метод
        if (_ht.ContainsKey(key) & _ht[key].GetType() == typeof(int))
            // casting тип object to int Explicit conversions, каждый раз делать casting это хорошо? Думаю нет (плохой ключ)
            return ((int)_ht[key]);
        else
            // Возвращается тип null а у нас int в сигнатуре метода
            // По моему мнению можно сделать тип nullable в сигнатуре метода но я думаю это плохая практика 
            return null;
    }

    public void Init()
    {
        // Плохой стиль кода не используются if (code ...) { code ... } также и с lock
        if (_ht == null) // try / catch / finaly - я думаю что lock дорогостоящая операция так ли это? (Мой вопрос)
            lock (_sync)
                Create();
        // lock это одна из самых дешевых оперций по синхронизации (мнение Senior и Team Lead)
        // Но под капотом lock - это Monitor реализован с использование  try / catch / finally? (Мой вопрос)
    }

    public static void Create()
    {
        // Мне не неравиться как инициализировали DataProvider
        // Нужно быть внимательным! DataProvider реализует интерфейс Dispose()
        // using var provider = new DataProvider(); // some as using (var ...) {}
        DataProvider provider = new DataProvider();
       
        // Испольование констант в коде как 100 и 12 плохая практика
       // Как вообще это явление называется в коде обычно (использование констант в коде) может быть bad style of code? 
        // Использование двух циклов обезательно или мы можем придумать что-то лучше? Но нам нужны индексы в LongRunningCalculation
        for (int i = 0; i < 100; i++)
        // Цикл начинается с j = 1 
            for (int j = 1; j <= 12; j++)
                // Каждый раз создается new [] это тоже плохо (плохой ключ) 
                _ht[new [] { i, j }] = provider.LongRunningCalculation(i, j);
        
        // if we used for using keyword provider will be disposed here
    }
}
  • Вопрос задан
  • 598 просмотров
Пригласить эксперта
Ответы на вопрос 6
@Sing303
Опишу, как бы комментировал я
public sealed class DataProvider : IDisposable
{
    // nit: Предложил бы названия firstValue, secondValue либо более осмысленные, если возможно
    public extern int LongRunningCalculation(int value, int value2);
    public extern void Dispose();
}

// nit: сразу бы хотелось видеть уровень доступа и sealed (если класс не планируется наследовать)
// Class2 - дать нормальное имя
// { - перенести на 2ю строку по рекомендациям code style от microsoft (если не принято иных)
class Class2 {
    // Синхронизация не нужна, если убрать метод Init, а Create вызвать в статическом конструкторе
    private readonly object _sync = new object();
    
    // _ht - дать осмысленное название
    // Судя по использованию, value может быть int`ом. Не зачем иметь лишний boxing и проверки на тип
    // _ht статический, значит к нему могут быть обращения из разных потоков, лучше сделать его ConcurrentDictionary
    // Прям сходу не могу сказать, но, возможно, использовал бы какой то другой тип Dictionary <key, key, val> (самописный или существующий), кажется, так было бы быстрее чем массив в ключе
    private static Dictionary<int[], object> _ht; 

    // nit: хотелось бы имена со смыслом
    public int GetValue(int index, int index2)
    {
        // Лишний метод, удалить. Create вызовем в static конструкторе
        Init();
        // Если ключ у нас объект, то необходимо реализовать IEqualityComparer для этого Dictionary (иначе не понятно как по нему искать)
        var key = new[] {index, index2};
        // Проверка на тип не нужна, Dictionary сделаем типа int
        if (_ht.ContainsKey(key) & _ht[key].GetType() == typeof(int))
            // приведение типов больше не нужно
            return ((int)_ht[key]);
        // nit: else не обязателен
        else
            // int не может быть null, будет ошибка, вернуть либо default, либо возвращаемое значение должно быть int?
            return null;
    }

    // Метод удалить, вызовем Create в статическом конструкторе без lock
    public void Init() 
    {
        if (_ht == null)
            lock (_sync)
                Create();
    }
    
    // Нет смысла делать метод public, сделать private
    public static void Create() 
    {
        // nit: и так видно какой тип создаём, можно использовать var
        // Обернуть в using
        DataProvider provider = new DataProvider();
        
        // Тут следует инициализировать значение _ht, т.к. ранее оно нигде не создаётся
        // Не забыть передать реализацию IEqualityComparer в конструктор
        
        // nit: хотелось бы видеть использование фигурных скобок (если не принят иной code style)
        // nit: вместо int можно var
        // i и j, похоже, несут какой то смысл, можно попробовать придумать нормальное название (иначе не понятно почему 100 и 12, их можно в константы класса)
        // nit: возможно можно использовать Parallel.ForEach
        for (int i = 0; i < 100; i++)
            for (int j = 1; j <= 12; j++)
                _ht[new [] { i, j }] = provider.LongRunningCalculation(i, j);
    }
}

А переписал бы так (если не убирать массив в dictionary)
public interface IDataProvider : IDisposable
{
    int LongRunningCalculation(int firstValue, int secondValue);
}

public sealed class DataProvider : IDataProvider
{
    public extern int LongRunningCalculation(int firstValue, int secondValue);
    public extern void Dispose();
}

public sealed class DataProviderService
{
    public DataProviderService(IDataProvider dataProvider)
    {
        _dataProvider = dataProvider;
    }

    private static readonly ConcurrentDictionary<int[], int?> _calculatedCache = new ConcurrentDictionary<int[], int?>(new CalculatedEqualityComparer());
    private readonly IDataProvider _dataProvider;

    public int? GetValue(int firstValue, int secondValue)
    {
        var isNotSupportedValues = firstValue > 100 || firstValue < 0 || secondValue < 1 || secondValue > 12;
        if (isNotSupportedValues)
        {
            return null;
        }

        var key = new[] { firstValue, secondValue };
        if (!_calculatedCache.TryGetValue(key, out var result))
        {
            result = _dataProvider.LongRunningCalculation(firstValue, secondValue);
            _calculatedCache.TryAdd(key, result);
        }
        
        return result;
    }
}
Ответ написан
insighter
@insighter
-First time? - Huh? (C#, React, JS)
Основная задача код-ревью дать обратную связь на выполненную работу, а не написать сочинение "по мотивам".
Ваши комментарии это не код-ревью, особенно для уровня кандидата которого они ищут.

PS Два момента непонятны.
1.
public extern int LongRunningCalculation(int value, int value2);

Какая семантика у ключевого слова extern в этом случае, подскажет кто?

2. На ревью дается то, что компилируется.
public int GetValue(int index, int index2)

При компиляции выдаст ошибку на строке return null;

По коду - серьезная ошибка с блокировкой, пара не оптимальных моментов, ну и мелочевка с названиями и расположением в классе (статику лучше писать первой).

PS Ах да судя по инициализации словаря, вообще непонятно зачем он используется, когда нужен двухмерный массив.
Ответ написан
@AndromedaStar
.Net - monkey
Странно, что никто не сказал, но тут самая большая проблема в русском языке. Ну серьезно, каждый может ошибаться, писать не всегда грамотно, но тут просто непозволительно ужасный текст. Я понимаю, что может для человека русский не родной язык, но тогда можно написать на английском.
Ответ написан
@Arlekcangp
Разработчик, Лид, Архитектор ПО
Почему никто не спрашивает, что вообще делает class2 ? Т е что бы написать хороший ревью, надо хорошо понимать контекст использования этого кода. В остальном согласен с тем, что уже написали. Код писал студент-недоучка, а ревью - мидл, который не сумел изложить свои мысли. (уж простите, ничего личного). Я честно пытался разобрать что бы делал, прочитав такое ревью на свой код, и осознал что не могу понять примерно половину. Разумеется, в ревью надо объяснять всё как можно подробнее и с учётом того, кто будет читать. Если человек такой код написал, то очевидно, уровень подробностей должен быть максимальным. Более того, наверное имеет смысл сначала указать на главные ошибки, пусть он их исправит, а уже на втором ревью, в случае если он не насажает новых, что весьма и весьма вероятно, писать про имена переменных и код-стайл.
Ну это мой подход. Конкретный интервьюер вполне может его и не разделять. Может у них "галера" и наличие даже одного "подхода" ревьюера - это роскошь. Так что вполне могу допустить, посчитали что "завалил", потому что и сами не в курсе как им оно надо.
Ответ написан
Комментировать
@achird
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;

//EXTERNAL CODE
//Приведен только для справки, никак нельзя менять, код обфусцирован, исходников нет
public sealed class ExternalDataProvider : IDisposable
{
    public extern int LongRunningCalculation(int value, int value2);
    public extern void Dispose();
}

/// <summary>
/// Объект-значение
/// <para>Базовый класс</para>
/// </summary>
public abstract class ValueObject<T> where T : ValueObject<T>
{
    public static bool operator ==(ValueObject<T> value1, ValueObject<T> value2)
    {
        if ((object)value1 == null)
            return (object)value2 == null;

        return value1.Equals(value2);
    }

    public static bool operator !=(ValueObject<T> value1, ValueObject<T> value2)
    {
        return !(value1 == value2);
    }

    public override bool Equals(object obj)
    {
        var valueObject = obj as T;

        if (ReferenceEquals(valueObject, null))
            return false;

        return CompareValues(valueObject);
    }

    public override int GetHashCode()
    {
        return GetValueHashCode();
    }

    /// <summary>
    /// Получить HashCode
    /// </summary>
    /// <returns></returns>
    protected abstract int GetValueHashCode();

    /// <summary>
    /// Сравнить значения
    /// </summary>
    /// <returns></returns>
    protected abstract bool CompareValues(T other);
}

/// <summary>
/// Параметр для DataProvider
/// </summary>
public class Parameter : ValueObject<Parameter>
{
    private Parameter()
    {
        FirstValue = -1;
        SecondValue = -1;
    }
    public Parameter(int firstValue, int secondValue)
    {
        if (firstValue > 100 || firstValue < 0 || secondValue < 1 || secondValue > 12)
            throw new ArgumentException("Is not supported values");
        FirstValue = firstValue;
        SecondValue = secondValue;
    }
    /// <summary>
    /// Первый параметр
    /// </summary>
    public int FirstValue { get; }
    /// <summary>
    /// Второй параметр
    /// </summary>
    public int SecondValue { get; }

    /// <summary>
    /// Default value
    /// </summary>
    public static readonly Parameter Empty = new Parameter();
    protected override bool CompareValues(Parameter other)
    {
        return FirstValue == other.FirstValue && SecondValue == other.SecondValue;
    }
    protected override int GetValueHashCode()
    {
        return (FirstValue, SecondValue).GetHashCode();
    }
}

/// <summary>
/// Результат для DataProvider
/// </summary>
public class Result : ValueObject<Result>
{
    private Result()
    {
        // Default value
        Value = default;
    }
    public Result(int value)
    {
        Value = value;
    }
    /// <summary>
    /// Результат
    /// </summary>
    public int Value { get; }

    /// <summary>
    /// Default value
    /// </summary>
    public static readonly Result Empty = new Result();
    protected override bool CompareValues(Result other)
    {
        return Value == other.Value;
    }
    protected override int GetValueHashCode()
    {
        return Value.GetHashCode();
    }
}

/// <summary>
/// Интерфейс DataProvider
/// </summary>
public interface IDataProvider
{
    public Result GetResult(Parameter parameter);
}

/// <summary>
/// Получить данные из ExternalDataProvider без кеша
/// </summary>
public class DataProvider : IDataProvider
{
    public Result GetResult(Parameter parameter)
    {
        using var externalDataProvider = new ExternalDataProvider();
        return new Result (externalDataProvider.LongRunningCalculation(parameter.FirstValue, parameter.SecondValue));
    }
}

/// <summary>
/// DataProvider с кешированием данных
/// Реализация кеша не имеет принципиального значения 
/// </summary>
public class CacheDataProvider : IDataProvider
{
    private readonly IDataProvider dataProvider;
    private readonly ConcurrentDictionary<Parameter, Result> cache = new();
    public CacheDataProvider(IDataProvider dataProvider)
    {
        this.dataProvider = dataProvider;
    }
    public Result GetResult(Parameter parameter)
    {
        return cache.GetOrAdd(parameter, (p => dataProvider.GetResult(p)));
    }
}


Реализация доступа к DataProvider с использованием ValueObject для параметра и результата. Доступ к данным осуществляется через интерфейс IDataProvider. Реализация кеширования данных через декоратор. Для рабочего кода можно сделать GetResultAsync.
Ответ написан
Комментировать
@nikifovadim Автор вопроса
Software Engineer
Перво наперво напишу, всем спасибо за Ваши ответы!) В общем, по хорошему стоило бы мне учесть все ответы разработчиков на habr и моих друзей. Но я решил пока запостить вот такой ответ на свой вопрос.

Можно было бы добавить в этот код еще документацию и что-то еще. Типо так ...
/// <summary>
    /// Project status
    /// </summary>
    public enum ProjectStatus
    {
        NotStarted,
        Active,
        Completed
    }


Может быть и расписать параметр и методы, что они возвращают или принимают. Как-то вот так ...
/// <summary>
        /// Get project by id
        /// </summary>
        /// <param name="id">Id of project</param>
        /// <param name="cancellationToken"></param>
        /// <returns><see cref="ProjectResponse"/></returns>
        [HttpGet]
        [Route("projects/{id}")]
        [ProducesResponseType(typeof(ProjectResponse), 200)]
        [ProducesResponseType(400)]
        [ProducesResponseType(500)]
        public async Task<ProjectResponse> GetProject(long id, CancellationToken cancellationToken)
        {
            return await _projectService.GetByIdAsync(id, cancellationToken);
        }


Не совсем хороший пример документации так, как например можно было бы подробнее расписать что он возвращает. Ну как Microsoft делает))) Весь код выше был показан только как пример)))

Собственно вот как мне объяснил или показал мой наставник, как наверное стоило бы сделать по хорошему. Мне нужно еще самому этот код позапускать и все проверить / посмотреть, но сделаю это я после прогулки :) Может быть за это время у кого-то появиться какая-нибудь критика, вопросы, предложения и прочие :)
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Threading.Tasks;
using System.Threading;
 
//EXTERNAL CODE
//Приведен только для справки, никак нельзя менять, код обфусцирован, исходников нет
public sealed class DataProvider : IDisposable
{
    public int LongRunningCalculation(int value, int value2) 
    { 
        Thread.Sleep(200); // milliseconds
        if ((value + value2) % 3 == 0) // 1 of 3 throws exceptions
            throw new ArgumentNullException();
        else
         return value + value2; 
    }
    public void Dispose() { }
}
public class Calculator : IDisposable
{
    private readonly Range rangeI = 0..10;
    private readonly Range rangeJ = 1..10;
 
    private readonly IDataProvider dataProvider;
 
    private readonly ConcurrentDictionary<Key, int> results = new();
 
    private readonly ConcurrentDictionary<Key, Exception> exceptions = new();
 
    public Calculator(DataProvider externProvider)
    {
        dataProvider = new CustomProvider(externProvider);
    }
 
    public async Task Calculate() /// async!!!
    {
        var tasks = new ConcurrentBag<Task>();
 
        for (var i = 0; i < rangeI.End.Value - rangeI.Start.Value; i++)
            for (var j = 0; j < rangeJ.End.Value - rangeJ.Start.Value; j++)
            {
                var key = new Key(i, j);
                tasks.Add(Task.Run(async () =>
                {
                    try //try-get
                    {
                        var result = await dataProvider.LongRunningCalculationAsync(key.FirstValue, key.SecondValue);
                        results.TryAdd(key, result);
                    }
                    catch (Exception ex) // try-get
                    {
                        exceptions.TryAdd(key, ex);
                    }
                }));
            }
 
        await Task.WhenAll(tasks);
    }
 
    public int GetValue(int first, int second)
    {
        if (results.TryGetValue(new Key(first, second), out int value))
        {
            return value;
        }
        else
        {
            if(exceptions.TryGetValue(new Key(first, second), out Exception ex))
                throw ex;
        }
        return 0;
    }
 
    public void Dispose() // обязательно реализовать
    {
        if (dataProvider != null) // проверка на null
            dataProvider.Dispose();
    }
}
 
public interface IDataProvider : IDisposable
{
    Task<int> LongRunningCalculationAsync(int firstValue, int secondValue);
}
 
public class CustomProvider : IDataProvider
{
    private readonly DataProvider _provider;
 
    public CustomProvider(DataProvider provider)
    {
        _provider = provider;
    }
 
    public void Dispose()
    {
        if (_provider != null)
            _provider.Dispose();
    }
 
    public Task<int> LongRunningCalculationAsync(int firstValue, int secondValue)
    {
        var tsc = new TaskCompletionSource<int>();
 
        Task.Run(() =>
        {
            try
            {
                var result = _provider.LongRunningCalculation(firstValue, secondValue);
                tsc.SetResult(result);
            }
            catch (Exception e)
            {
                tsc.SetException(e);
            }
        });
 
        return tsc.Task;
 
        // no, because Exception
        return Task.Run(() => _provider.LongRunningCalculation(firstValue, secondValue));
    }
}
 
readonly public struct Key // struct immutable
{
    public readonly int FirstValue;
    public readonly int SecondValue;
 
    public Key(int first, int second)
    {
        FirstValue = first;
        SecondValue = second;
    }
 
    public override int GetHashCode() // override !!!
    {
        return (FirstValue.GetHashCode() * 1_000_000 + SecondValue.GetHashCode()) % 1_000_000_000;
    }
 
    public override bool Equals(object obj) // override !!!
    {
        var other = (Key)obj;
        return GetHashCode() == other.GetHashCode();
    }
 
    public override string ToString() // не обязательно, но поскольку у нас ключи закрытые - сделаем
    {
        return FirstValue.ToString() + " " + SecondValue.ToString();
    }
}


class Program
{
    static void Main(string[] args)
    {
        using var calculator = new Calculator(new DataProvider());
        calculator.Calculate().Wait();
        try
        {
            var r = calculator.GetValue(1, 1);
            Console.WriteLine("1: 1: " + r);
            r = calculator.GetValue(1, 2);
            Console.WriteLine("1: 1: " + r);
            r = calculator.GetValue(1, 3);
            Console.WriteLine("1: 1: " + r);
        }
        catch (Exception ex)
        {
            Console.WriteLine(ex.Message);
        }
        Console.ReadLine();
    }
}


Добавлю от себя, что будь это допустим не интервью, а реальный проект то Code Review наверное бы делал не один я, а много разработчиков (команда). Ну после интервью я показав этот код своим друзьям, своему наставнику и выложив его на habr. Так же Code Review, как писали уже выше нужно делать в несколько подходов и добавлю от себя что его должен делать не один человек, что бы найти все возможные проблемы. В общем я соглашусь с мнением Александр то что компания возможно "галера" и добавлю что интервью прошло не так хорошо, мы договаривались на 2ч, а на деле оказалось 1ч с чем-то плюс мину. Только их HR / рекрутер показалась мне компетентной и она мне понравилось, а интервью г**** :)
Ответ написан
Ваш ответ на вопрос

Войдите, чтобы написать ответ

Войти через центр авторизации
Похожие вопросы