I made a Code Review, maybe I made a mistake somewhere or do you have something to add?

Question

ASP.NET

In general, one of these days I was interviewing in one company as a middle + / senior developer and I was asked to do a Code Review. They didn't like the way I did it. Maybe it was necessary to give a more detailed description of why this is not possible, but is it possible? I suggest you take a look at the code, I commented out the problem areas in my opinion. Maybe I did something wrong? Or do you have something to add? Or did I not find everything? Please write your answer as detailed as possible.

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
    }
}

Answer 1

I will describe how I would comment

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);
    }
}

And I would rewrite it like this (if you do not remove the array in the 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;
    }
}

Answer 2

The main task of a code review is to give feedback on the work done, and not to write an essay "based on".
Your comments are not code reviews, especially for the level of candidate they are looking for.
PS Two points are not clear.
one.

public extern int LongRunningCalculation(int value, int value2);

What is the semantics of the extern keyword in this case, who can tell?
2. What is compiled is given for review.

public int GetValue(int index, int index2)

When compiling, it will give an error on the line return null;
According to the code - a serious error with blocking, a couple of not optimal moments, well, small things with names and location in the class (it's better to write statics first).
PS Oh yes, judging by the initialization of the dictionary, it's not at all clear why it is used when a two-dimensional array is needed.

Answer 3

It is strange that no one said, but here is the biggest problem in the Russian language. Well, seriously, everyone can make mistakes, writing is not always correct, but this is just an unacceptably terrible text. I understand that Russian may not be a native language for a person, but then you can write in English.

Answer 4

Why is no one asking what class2 does anyway? That is, in order to write a good review, you need to understand the context of using this code well. Other than that, I agree with what has already been said. The code was written by a dropout student, and the review was written by a middle who failed to express his thoughts. (Sorry, nothing personal). I honestly tried to make out what I would do after reading such a review of my code, and realized that I could not understand about half. Of course, in a review, you need to explain everything in as much detail as possible and taking into account who will be reading it. If a person wrote such code, then obviously, the level of detail should be maximum. Moreover, it probably makes sense to first point out the main mistakes, let him fix them, and already at the second review, if he does not plant new ones, which is very, very likely, write about variable names and code-style.
Well, this is my approach. A particular interviewer may well not share it. Maybe they have a "galley" and the presence of even one "approach" of the reviewer is a luxury. So I can quite admit, they considered that they "flunked", because they themselves do not know how they need it.

Answer 5

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)));
    }
}

Implementing DataProvider access using ValueObject for parameter and result. Data is accessed through the IDataProvider interface. Implementing data caching through a decorator. For working code, you can do GetResultAsync.

Answer 6

First things first, I'll write, thank you all for your answers!) In general, it would be good if I took into account all the answers of the developers on habr and my friends. But I decided to post this answer to my question for the time being.
It would be possible to add more documentation to this code and something else. Like so...

/// <summary>
    /// Project status
    /// </summary>
    public enum ProjectStatus
    {
        NotStarted,
        Active,
        Completed
    }

Maybe paint the parameter and methods, what they return or accept. Something like that ...

/// <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);
        }

Not a very good example of documentation in the way that, for example, it would be possible to describe in more detail what it returns. Well, how Microsoft does it))) All the code above was shown only as an example)))
Actually, this is how my mentor explained or showed me how it should probably be done for good. I still need to run this code myself and check / see everything, but I will do it after a walk :) Maybe during this time someone will have some criticism, questions, suggestions and others :)

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();
    }
}

I’ll add on my own that if this wasn’t an interview, but a real project, then Code Review would probably have been done not by me alone, but by many developers (team). Well, after the interview, I showed this code to my friends, my mentor and posted it on habr. Just as Code Review, as already mentioned above, needs to be done in several approaches and I will add on my own that it should be done by more than one person in order to find all possible problems. In general, I agree with Alexander 's opinion that the company is probably a "galley" and I will add that the interview did not go so well, we agreed on 2 hours, but in reality it turned out to be 1 hour with something plus a mine. Only their HR / recruiter seemed competent to me and I liked her, but the interview was d **** :)