[CodeJam] Code review, план действий по исправлению
От: Sinix  
Дата: 18.04.16 10:06
Оценка:
Просмотрел код, список будущих исправлений ниже. Замечания приветствуются

UPDATE: all fixed.

1. Memoize<T>
Надо бы сделать кодогенерацию + оставить только одну перегрузку с optional args.
См гадлайны.

2. CollectionsExtensions. Вот тут не надо params?
public static void AddRange<T>([NotNull] this ICollection<T> source, [NotNull] T[] items)


3. EnumerableExtensions.Index() Идея ок, реализация хромает.
IndexedItem не реализует equals/gethashcode/equality operators.
Имя метода — может, что-то типа items.Indexed() ?

4. QueryableExtensions — nameof вместо строк в перегрузках OrderBy()/...

5. Для позаимствованного кода ОБЯЗАТЕЛЕН коммент BASEDON: + url на исходники.
Как минимум для ObjectPools и для файлов в Targeting надо добавить.

6. InterlockedOperations — надо добавить generic update на базе CAS loop,
по аналогии с кодом, что генерит компилятор для подписки событий.

7. Перенёc в соседний топик
Автор: AndrewVK
Дата: 18.04.16
. XNodeExtensions — Required/Optional переименовать в стандартные Get/TryGet/XxxOrDefault.

8. Fn<T> — Identity переименовать в стандартный Self.

В остальном приятно удивлён — код
Отредактировано 19.04.2016 19:34 Sinix . Предыдущая версия . Еще …
Отредактировано 18.04.2016 19:18 Sinix . Предыдущая версия .
Re: [CodeJam] Code review, план действий по исправлению
От: AndrewVK Россия http://blogs.rsdn.org/avk
Дата: 18.04.16 18:17
Оценка: 2 (1)
Здравствуйте, Sinix, Вы писали:

S>1. Memoize<T>

S>Надо бы сделать кодогенерацию

Там всего 5 вариантов. Шаблон будет сопоставимого размера и несопоставимой мутности. Но если надо расширить до 8 аргументов по аналогии с туплами и action/func, то тут уже лучше генерить.

S> + оставить только одну перегрузку с optional args.

S> См гадлайны.

Мне кажется это не совсем наш случай. У нас пока никакой backward compatibility нет в принципе.

S>2. CollectionsExtensions. Вот тут не надо params?

S>
S>public static void AddRange<T>([NotNull] this ICollection<T> source, [NotNull] T[] items)
S>


ИМХО надо.

S>3. EnumerableExtensions.Index() Идея ок, реализация хромает.

S>IndexedItem не реализует equals/gethashcode/equality operators.

А оно надо?

S>Имя метода — может, что-то типа items.Indexed() ?


Because methods are the means of taking action, the design guidelines require that method names be verbs or verb phrases. Following this guideline also serves to distinguish method names from property and type names, which are noun or adjective phrases.

FDG

S>4. QueryableExtensions — nameof вместо строк в перегрузках OrderBy()/...


Поправил

S>6. InterlockedOperations — надо добавить generic update на базе CAS loop,

S>по аналогии с кодом, что генерит компилятор для подписки событий.

+1

S>7. XNodeExtensions — Required/Optional переименовать в стандартные Get/TryGet/XxxOrDefault.


Это, имхо, не совсем тот случай, надо подумать.
... << RSDN@Home 1.0.0 alpha 5 rev. 0 on Windows 8 6.2.9200.0>>
AVK Blog
Re[2]: [CodeJam] Code review, план действий по исправлению
От: Sinix  
Дата: 18.04.16 19:14
Оценка:
Здравствуйте, AndrewVK, Вы писали:

S>>Надо бы сделать кодогенерацию

AVK>Там всего 5 вариантов. Шаблон будет сопоставимого размера и несопоставимой мутности. Но если надо расширить до 8 аргументов по аналогии с туплами и action/func, то тут уже лучше генерить.

Ну вот у меня крутится мысля этот шаблон переиспользовать, например, для хелпера для вывода типов выражений, что-то типа
// Expression<Func<int, int, int>>
var expression = ExpressionHelpers.Func((int x, int y) => x + y); // Имя условное

сделать часть шаблона переиспользуемой — 100% пригодится.

S>> + оставить только одну перегрузку с optional args.

S>> См гадлайны.

AVK>Мне кажется это не совсем наш случай. У нас пока никакой backward compatibility нет в принципе.

Ну так после релиза будет

Иначе начнётся dll hell, когда одна библиотека зависит от CodeJam 1.0.0, вторая — от 1.1.3 и обе версии бинарно несовместимы.



S>>2. CollectionsExtensions. Вот тут не надо params?

AVK>ИМХО надо.
Ок, добавляю.


S>>3. EnumerableExtensions.Index() Идея ок, реализация хромает.

S>>IndexedItem не реализует equals/gethashcode/equality operators.
AVK>А оно надо?
Я это узнал сложным путём — сделал Distinct по похожей структуре. Угу, результат был немного предсказуем



S>>Имя метода — может, что-то типа items.Indexed() ?

AVK>FDG
Ну, я на своём варианте не настаиваю Если вдруг у кого-то появится предложение получше .Index() — поменяем.



S>>6. InterlockedOperations — надо добавить generic update на базе CAS loop,

AVK>+1
Тож добавляю. Как добавлю — попрошу посмотреть.


S>>7. XNodeExtensions — Required/Optional переименовать в стандартные Get/TryGet/XxxOrDefault.

AVK>Это, имхо, не совсем тот случай, надо подумать.
Понял, перенёc вопрос в соседний топик
Автор: AndrewVK
Дата: 18.04.16
.
Отредактировано 18.04.2016 19:22 Sinix . Предыдущая версия . Еще …
Отредактировано 18.04.2016 19:17 Sinix . Предыдущая версия .
Re[3]: [CodeJam] Code review, план действий по исправлению
От: AndrewVK Россия http://blogs.rsdn.org/avk
Дата: 18.04.16 19:25
Оценка:
Здравствуйте, Sinix, Вы писали:

S>сделать часть шаблона переиспользуемой — 100% пригодится.


Ну, я могу шаблон сделать, расширив до 8 аргументов, а уж переиспользуемую часть ты сам выдирай оттуда.

AVK>>Мне кажется это не совсем наш случай. У нас пока никакой backward compatibility нет в принципе.

S>Ну так после релиза будет
S>Иначе начнётся dll hell, когда одна библиотека зависит от CodeJam 1.0.0, вторая — от 1.1.3 и обе версии бинарно несовместимы.

dll hell начинается, если устраивать расшариваемое хранилище библиотек. Такое актуально для гака, упокой господь его душу, и для плагинов в студии. Насколько это критично для эхотага — ХЗ.
Вобщем, для генеримого кода, наверное, можно и добавить перегрузки вместо опциональных парметров, но вот в абсолют возводить не стоит.

S>>>Имя метода — может, что-то типа items.Indexed() ?

AVK>>FDG
S>Ну, я на своём варианте не настаиваю Если вдруг у кого-то появится предложение получше .Index() — поменяем.

Ну тогда, как минимум, AsIndexed.
... << RSDN@Home 1.0.0 alpha 5 rev. 0 on Windows 8 6.2.9200.0>>
AVK Blog
Re[4]: [CodeJam] Code review, план действий по исправлению
От: Sinix  
Дата: 18.04.16 19:54
Оценка:
Здравствуйте, AndrewVK, Вы писали:
AVK>Ну, я могу шаблон сделать, расширив до 8 аргументов, а уж переиспользуемую часть ты сам выдирай оттуда.
Ок!


AVK>dll hell начинается, если устраивать расшариваемое хранилище библиотек. Такое актуально для гака, упокой господь его душу, и для плагинов в студии. Насколько это критично для эхотага — ХЗ.

Не, я про ситуацию "подтянул две библиотеки из нюгета, одна просит CodeJam 1.0.0, вторая — от 1.1.3" — в общем что-то типа такой ситуации. Ловится-то оно элементарно, через PublicApiAnalyzer, но его ж ещё прикрутить надо


S>>>>Имя метода — может, что-то типа items.Indexed() ?

AVK>Ну тогда, как минимум, AsIndexed.
+1
Re[4]: [CodeJam] Code review, план действий по исправлению
От: Sinix  
Дата: 19.04.16 05:47
Оценка:
Здравствуйте, AndrewVK, Вы писали:

AVK>Ну тогда, как минимум, AsIndexed.

Исправления увидел, спасиб!
Re[4]: [CodeJam] Code review, план действий по исправлению
От: Sinix  
Дата: 19.04.16 19:42
Оценка:
Здравствуйте, AndrewVK, Вы писали:

Добавил params в перегрузку AddRange(T[]) + добавил InterlockedOperations.Update() для lock-free обновления поля.

InterlockedOperations теперь поддерживает все типы, что поддерживают методы в interlocked (вытащил в T4 шаблон), тесты добавил.

Такая просьба — посмотри код в InterlockedOperations Вроде всё правильно, но проверить никогда не помешает.

+ есть мысль добавить хелпер для bool-флагов, что-то типа
        //A simple method that denies reentrancy.
        static bool UseResource()
        {
            //0 indicates that the method is not in use.
            if(0 == Interlocked.Exchange(ref usingResource, 1))
            {
                Console.WriteLine("{0} acquired the lock", Thread.CurrentThread.Name);

                //Code to access a resource that is not thread safe would go here.

                //Simulate some work
                Thread.Sleep(500);

                Console.WriteLine("{0} exiting lock", Thread.CurrentThread.Name);

                //Release the lock
                Interlocked.Exchange(ref usingResource, 0);
                return true;
            }
            else
            {
                Console.WriteLine("   {0} was denied the lock", Thread.CurrentThread.Name);
                return false;
            }
        }
    }

из MSDN..

Юзинг тут не подходит в принципе, остаётся такой вариант:
InterlockedOperations.NonReentrant(ref intFlagField, ()=>SomeCode()); // тип флага только int, на всякий уточняю :)
, пойдёт?

Имя метода мне не нравится, есть идея как сделать лучше — велкам!
Отредактировано 19.04.2016 20:05 Sinix . Предыдущая версия .
Re[5]: [CodeJam] Code review, план действий по исправлению
От: AndrewVK Россия http://blogs.rsdn.org/avk
Дата: 19.04.16 20:00
Оценка: 2 (1)
Здравствуйте, Sinix, Вы писали:

S>Такая просьба — посмотри код в InterlockedOperations Вроде всё правильно, но проверить никогда не помешает.


Посмотрел. Вроде все нормально. Я только не понял зачем вторая перегрузка Update. Чтобы избежать замыкания?
... << RSDN@Home 1.0.0 alpha 5 rev. 0 on Windows 8 6.2.9200.0>>
AVK Blog
Re[6]: [CodeJam] Code review, план действий по исправлению
От: Sinix  
Дата: 19.04.16 20:08
Оценка:
Здравствуйте, AndrewVK, Вы писали:

AVK>Посмотрел. Вроде все нормально. Я только не понял зачем вторая перегрузка Update. Чтобы избежать замыкания?

Ага. У нас когда-то это было узким местом — из-за кучи мусора забивался GC0.
И я там update написал, про хелпер для бул-флага — глянь плиз.

На будущее — буду отдельным вопросом дописывать
Re[5]: [CodeJam] Code review, план действий по исправлению
От: AndrewVK Россия http://blogs.rsdn.org/avk
Дата: 19.04.16 20:26
Оценка:
Здравствуйте, Sinix, Вы писали:

S>+ есть мысль добавить хелпер для bool-флагов, что-то типа


А от него есть практическая польза в сравнении с фреймворковской SpinLock? Там fast path по похожей схеме работает.
... << RSDN@Home 1.0.0 alpha 5 rev. 0 on Windows 8 6.2.9200.0>>
AVK Blog
Re[6]: [CodeJam] Code review, план действий по исправлению
От: Sinix  
Дата: 19.04.16 21:08
Оценка:
Здравствуйте, AndrewVK, Вы писали:


S>>+ есть мысль добавить хелпер для bool-флагов, что-то типа


AVK>А от него есть практическая польза в сравнении с фреймворковской SpinLock? Там fast path по похожей схеме работает.


Ну можно тот же хелпер поверх SpinLock навернуть.
Тут главное не реализация, а сценарий "попробовать захватить флаг, получилось — сделать что-то и освободить флаг".

Как пример, где это полезно — обработчики PropertyChanged. Обычно если обработчик изменяет то же свойство, что пришло в e.PropertyName, то вызывать повторно обработчик не надо. Чтобы не городить хитрых проверок — оборачиваем код в NonReentrant() и оно просто работает

Если оно никому не надо — выкидываем, как AppSwitch
Автор: Sinix
Дата: 14.04.16
например.
Re[7]: [CodeJam] Code review, план действий по исправлению
От: AndrewVK Россия http://blogs.rsdn.org/avk
Дата: 19.04.16 21:40
Оценка:
Здравствуйте, Sinix, Вы писали:

S>Ну можно тот же хелпер поверх SpinLock навернуть.


Есть уже хелпер, оператор lock называется.

S>Тут главное не реализация, а сценарий "попробовать захватить флаг, получилось — сделать что-то и освободить флаг".


Такой сценарий нужен только внутри низкоуровневых lock free алгоритмов, имхо. А в прикладном коде нужно что то более высокоуровневое.
... << RSDN@Home 1.0.0 alpha 5 rev. 0 on Windows 8 6.2.9200.0>>
AVK Blog
Re[8]: [CodeJam] Code review, план действий по исправлению
От: Sinix  
Дата: 20.04.16 05:49
Оценка:
Здравствуйте, AndrewVK, Вы писали:

AVK>Есть уже хелпер, оператор lock называется.

Не, я ж говорю, что важна не блокировка, а защита от попытки повторного выполнения, в том числе в том же потоке.
Не нужна — не добавляем
 
Подождите ...
Wait...
Пока на собственное сообщение не было ответов, его можно удалить.