Lazy by Palezehvat · Pull Request #21 · Palezehvat/HomeworksCSharp

Palezehvat · 2023-09-25T11:31:10Z

No description provided.

YuriUfimtsev

И еще одно замечание: условие про отсутствие дублирования тестов на общее поведение для двух реализаций. Можно воспользоваться атрибутом TestCaseSource

YuriUfimtsev · 2023-09-29T17:06:02Z

Lazy/Lazy/FunctionsForTests.cs

+/// </summary>
+public static class FunctionsForTests
+{
+    public static volatile int howMutchFunctionBeenCalled = 0;


YuriUfimtsev · 2023-09-29T17:07:49Z

Lazy/Lazy/ILazy.cs

+public interface ILazy<T>
+{
+    /// <summary>
+    /// Getting the created object


Тогда уж
The class implements, Gets the created object

YuriUfimtsev · 2023-09-29T17:09:29Z

Lazy/Lazy/MultiThreadLazy.cs

+
+public class MultiThreadLazy<T> : ILazy<T>
+{
+    private Func<T> functionForLazy;


Лучше просто supplier

YuriUfimtsev · 2023-09-29T17:12:51Z

Lazy/Lazy/MultiThreadLazy.cs

+    private Object objectLock = new ();
+    private volatile bool initialized = false;
+    private T resultSuppiler;
+    private Exception exceptionSuppiler = default;


К предупреждению компилятора стоит прислушаться, exceptionSupplier при инициализации класса null-ом становится

YuriUfimtsev · 2023-09-29T17:14:19Z

Lazy/Lazy/MultiThreadLazy.cs

+    /// Constructor for storing the object creation function
+    /// </summary>
+    public MultiThreadLazy(Func<T> function)
+    {


Тоже важное замечание компилятора, которое также отсылает к условию "supplier вправе вернуть null"

YuriUfimtsev · 2023-09-29T17:42:05Z

Lazy/TestsForLazy/TestsForLazy.cs

+{   
+    [Test]
+    public void SimpleExample()
+    {


Тест с тремя+ Assert-ами не есть хорошо, особенно, если они все друг с другом не связаны. Надо разбить на более мелкие тесты с говорящими названиями

Я понял, но тогда будут отдельные тесты для многопоточности и одно поточности, я не очень понимаю, как применить TestCaseSource, если запуск многопоточных тест и однопоточных сильно отличается

Речь больше про то, чтобы разделить тесты на два типа:

Тест(тесты) на корректную работу многопоточного Lazy и отсутствие гонок

Тест(тесты) на корректную работу Lazy, когда к нему обращается всего один поток. И такого рода тесты надо запускать на двух вариантах Lazy, используя TestCaseSource для передачи объектов разных типов

YuriUfimtsev · 2023-09-29T17:43:58Z

Lazy/TestsForLazy/TestsForLazy.cs

+        Assert.That(FunctionsForTests.FunctionForLazyWithCounter(), Is.Not.EqualTo(singleThreadLazy.Get()));
+        FunctionsForTests.howMutchFunctionBeenCalled = 0;
+        var multiThreadLazy = new MultiThreadLazy<int>(() => FunctionsForTests.FunctionForLazyWithCounter());
+        var arrayThreads = new Thread[Environment.ProcessorCount];


Вдруг Environment.ProcessorCount вернет 1? :)
На такой случай лучше вручную потоков 10 создать, в крайнем случае будет симуляция многопоточности

YuriUfimtsev · 2023-09-29T17:47:52Z

Lazy/TestsForLazy/TestsForLazy.cs

+                }
+                catch (InvalidOperationException)
+                {
+                    Assert.True(true);


Какая-то максимально бесполезная строчка :)
Никакой проверки в Assert не происходит, и в то же время если исключение не бросится, тест не упадет. Надо исправить

YuriUfimtsev · 2023-09-29T17:49:38Z

Lazy/TestsForLazy/TestsForLazy.cs

+    [Test]
+    public void ThreadRaceTest()
+    {
+        var multiThreadLazy = new SingleThreadLazy<int>(() => FunctionsForTests.FunctionForLazyWithCounter());


Что-то здесь не так

Это проверка на гонку, проверяю, что при реализации в однопоточном режим использование многопоточности приводит к искажению результата, как по другому проверить, что нету гонок я не знаю

Проверка на отсутствие гонок предполагает запуск многопоточного Lazy несколькими потоками и проверку, что все потоки получили один и тот же результат. Кстати, можно использовать ManualResetEvent для увеличения шанса одновременного запрашивания потоками значения Lazy.
В однопоточном режиме должно приводить к искажению, да, но это не часть функциональности системы, которую мы реализуем и хотим поддерживать, проверять тестами

YuriUfimtsev · 2023-09-29T17:59:00Z

Lazy/TestsForLazy/TestsForLazy.cs

+                value = multiThreadLazy.Get(); 
+                lock (value)
+                {
+                    if ((int)value != 1)


К этому тесту тоже много вопросов :)

Неочевидно, чему должно быть равно value. Порядок тестов иметь значения не должен, а static-поле меняется в нескольких тестах. Поэтому, кстати, лучше его static-ом вообще не делать, а для каждого теста создавать свой объект с полем определенного значения. А то запустятся два теста и упадут из-за неупорядоченных проверок одного и того же поля на равенство разным значениям.

Этот тест обречен на вечный успех

Сравнение лучше делать более определенным. Если мы конкретное число ждем, то его и писать

Тут скорее попытка поймать, что в однопоточной реализации при использовании многопоточности появляется гонка потоков, а сказать конкретно в какой момент value будет не равно 1 и чему именно
невозможно. Понимаю, что тест не самый хороший, но по-другому, как это проверить я не знаю
.

Выше написал, но главное, что мы хотим проверить не наличие гонки в однопоточном режиме, а отсутствие гонок в многопоточном

YuriUfimtsev · 2023-10-01T22:11:55Z

Lazy/Lazy/ILazy.cs

+namespace Lazy;
+
+/// <summary>
+/// The class implements, Gets the created object


Suggested change

/// The class implements, Gets the created object

/// The class implements an object whose function is called only once.

YuriUfimtsev · 2023-10-01T22:12:42Z

Lazy/Lazy/ILazy.cs

+public interface ILazy<T>
+{
+    /// <summary>
+    /// Getting the created object


Suggested change

/// Getting the created object

/// Gets the created object.

YuriUfimtsev · 2023-10-01T22:17:29Z

Lazy/Lazy/MultiThreadLazy.cs

+                    {
+                        resultSuppiler = supplier();
+                        supplier = null;
+                    }


Компилятор остался неуслышанным :(

YuriUfimtsev · 2023-10-01T22:19:59Z

Lazy/Lazy/MultiThreadLazy.cs

+                    if (exceptionSuppiler != default)
+                    {
+                        throw exceptionSuppiler;
+                    }


Копипаст остался. Может ли exceptionSupplier != default и пройти мимо catch? Если нет, то нужна ли эта проверка, или можно кидать исключение в другом блоке?

YuriUfimtsev · 2023-10-01T22:21:56Z

Lazy/Lazy/SingleThreadLazy.cs

@@ -0,0 +1,46 @@
+namespace Lazy;


В этот раз аналогично

YuriUfimtsev · 2023-10-01T22:50:27Z

Lazy/Lazy/FunctionsForTests.cs

+/// </summary>
+public class FunctionsForTests
+{
+    public volatile int howMuchFunctionBeenCalled = 0;


public-поле -- это не про .NET

YuriUfimtsev · 2023-10-01T22:54:49Z

Lazy/TestsForLazy/TestsForLazy.cs

+                bool isExceptionWasThrown = false;
+                try
+                {
+                    value =  multiThreadLazy.Get();
+                }
+                catch (InvalidOperationException)
+                {
+                    isExceptionWasThrown = true;
+                }
+                Assert.True(isExceptionWasThrown);


Suggested change

bool isExceptionWasThrown = false;

try

{

value = multiThreadLazy.Get();

}

catch (InvalidOperationException)

{

isExceptionWasThrown = true;

}

Assert.True(isExceptionWasThrown);

Assert.Throws<InvalidOperationException>(() => multiThreadLazy.Get());

Тогда и value не нужно

YuriUfimtsev · 2023-10-01T23:03:41Z

Lazy/TestsForLazy/TestsForLazy.cs

+
+    [Test]
+    public void ExampleWithExceptionWithMultiThreadLazy()
+    {


На самом деле, от многопоточного теста нужна только проверка на отсутствие гонок, а проверка на корректность работы произойдет и в однопоточных тестах. Так что будет вполне достаточно трех тестов ниже

YuriUfimtsev · 2023-10-01T23:17:51Z

Lazy/TestsForLazy/TestsForLazy.cs

+        {
+            arrayThreads[i] = new Thread(() => 
+            { 
+                Assert.That(correctResult, Is.EqualTo(multiThreadLazy.Get())); 


Assert.That в разных потоках -- не лучший вариант, как минимум из-за сложностей отладки. Лучше собрать значения потоков в массив и по нему пробежаться в конце в главном потоке, сравнивая значения с помощью Assert.That

YuriUfimtsev · 2023-10-01T23:23:08Z

Lazy/Lazy/FunctionsForTests.cs

+    /// Function that throws InvalidOperationException
+    /// </summary>
+    /// <returns></returns>
+    public int FunctionWithInvalidOperationException()


Функция себя не оправдывает, int и не собирается возвращать :)
Лучше вернуть int в конце, но чтобы компилятор не ругался, добавить строчку, выполнение которой вызовет исключение (типо деления на ноль)

YuriUfimtsev · 2023-10-09T18:56:33Z

Lazy/Lazy/MultiThreadLazy.cs

                    {
-                        throw exceptionSuppiler;
+                        supplier = null;
+                        GC.Collect();


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

Просто явно через GC.Collect(); его не вызывать, среда выполнения .NET сама это сделает, когда посчитает нужным

YuriUfimtsev · 2023-10-09T18:59:47Z

Lazy/Lazy/SingleThreadLazy.cs

-                supplier = null;
+                if (supplier == null)
+                {
+                    resultSuppiler = default;


Нагляднее будет просто null написать. default для nullable типа в точности null и вернет

И resultSupplier при инициализации и так null-ом станет, нет смысла его еще раз обнулять

YuriUfimtsev · 2023-10-09T19:05:00Z

Lazy/TestsForLazy/TestsForLazy.cs

            arrayThreads[i] = new Thread(() => 
            {
-                Assert.That(result, Is.EqualTo(multiThreadLazy.Get()));
+                Assert.That(Equals(multiThreadLazy.Get(), 1));


Есть Assert.That и Assert.Equals. Надо что-то одно выбрать.
Например, Assert.That(multiThreadLazy.Get(), Is.EqualTo(1));

YuriUfimtsev · 2023-10-09T19:06:55Z

Lazy/TestsForLazy/TestsForLazy.cs

            arrayThreads[i] = new Thread(() => 
            { 
-                Assert.That(correctResult, Is.EqualTo(multiThreadLazy.Get())); 
+                Assert.That(Equals(multiThreadLazy.Get(), 1)); 


Аналогично

Assert.That в разных потоках -- не исправлено.

Хорошо, а как получить значения из потоков и класть их в массив?

Пример с презентации

YuriUfimtsev · 2023-10-09T19:08:53Z

Lazy/TestsForLazy/FunctionsForTests.cs

@@ -22,6 +22,7 @@ public int FunctionForLazyWithCounter()
    /// <returns></returns>
    public int FunctionWithInvalidOperationException()


Функция теперь свое название не оправдывает, exception другой.
И точно ли нужна отдельная переменная для единицы?

Переменная нужна, т.к. при её отсутствии появляется error и сборка невозможна

YuriUfimtsev

В первом коммите "Часть ревью" исправили Assert.That(Equals(...)), а в следующих двух обратно вернули :)
И имя функции надо сменить. Либо указать DivideByZeroException, либо просто Exception. Но никак не InvalidOperationException

palorel added 4 commits September 21, 2023 15:41

Основной функционал, частичные тесты

d089905

Недоделанные тесты

d2023b9

Доделал тесты

a1eed46

Добавил комментарии

f9f3ba9

YuriUfimtsev reviewed Sep 29, 2023

View reviewed changes

Ревью

7f3b5f7

YuriUfimtsev reviewed Oct 1, 2023

View reviewed changes

palorel added 2 commits October 7, 2023 16:09

Ревью

914f984

Забытый файл

41fc3c9

YuriUfimtsev reviewed Oct 9, 2023

View reviewed changes

palorel added 3 commits October 10, 2023 14:30

Часть ревью

93a167e

Часть изменений

d40eae5

Ревью

acfaf47

YuriUfimtsev reviewed Dec 21, 2023

View reviewed changes

Ревью

a325505

	/// The class implements, Gets the created object
	/// The class implements an object whose function is called only once.

		@@ -22,6 +22,7 @@ public int FunctionForLazyWithCounter()
		/// <returns></returns>
		public int FunctionWithInvalidOperationException()

Conversation

Palezehvat commented Sep 25, 2023

Uh oh!

YuriUfimtsev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YuriUfimtsev Sep 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YuriUfimtsev Sep 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YuriUfimtsev Sep 30, 2023 •

edited

Loading

YuriUfimtsev Sep 29, 2023 •

edited

Loading