а что тут такого? Предлагаете писать фабрику для каждого типа? Или может мапы строить и потом по рефлексии создавать колонку? Всё одинаково плохо для такого простого примера. KIS как он есть.
Здравствуйте, sergii.p, Вы писали:
SP>а что тут такого? Предлагаете писать фабрику для каждого типа? Или может мапы строить и потом по рефлексии создавать колонку?
Ну, хотя бы так:
private static DataFrameColumn CreateColumn(Type kind, string[] columnNames, int columnIndex)
{
var colName = GetColumnName(columnNames, columnIndex);
return kind switch
{
typeof (bool) => new BooleanDataFrameColumn(colName),
typeof (int) => new Int32DataFrameColumn(colName),
...
_ => throw new NotSupportedException()
};
}
Здравствуйте, Codealot, Вы писали:
C>Всякое я раньше видел, но это как-то особенно пробрало.
Я не понял... тебя "пробрало", но при этом ты тупо фэйспалмишь, ничего не объясняя. И главное, не предлагая правильной альтернативы! Походу ты сам пока джун и увидел тупо простыню кода и считаешь, что это неправильно. Хотя после 10 лет ковыряния в "интыпрайзе" и чёрт-ногу-сломит фабриках и "шаблонах четырёх", ты придёшь к одному выводу — "чем проще выглядит код, тем лучше". Лучше для сопровождения, для отладки, для объяснения другим, для документации и т.п.
В принципе, этот код неплох, но я бы сделал чутка короче: словарь "тип -> конструктор колонки" и далее constr[type].Invoke... (ну или Activator.CreateInstance).
Здравствуйте, Ночной Смотрящий, Вы писали:
SP>>а что тут такого? Предлагаете писать фабрику для каждого типа? Или может мапы строить и потом по рефлексии создавать колонку? НС>Ну, хотя бы так:
Здравствуйте, Teolog, Вы писали:
T>если GetColumnName(columnNames, columnIndex) выдаст исключение, оно трегернет первым до проверки типа колонки. Причем уже в другом методе.
Здравствуйте, yenik, Вы писали:
G>>3) Тип эксепшена не тот — это не имеет значения на самом деле. Это usage\design exeptions, их не надо ловить. Достаточно чтобы эксепшен сказал что с кодом не так.
Y>Этак можно и NullReferenceException бросать, работать-то будет.
Нельзя. NRE это runtime exeption, а не usage. Человек, увидев NRE, не поймет что произошло.
K>Ну так вы и не понимаете именно в силу вот таких new NotSupportedException!! K>Ошибки всегда надо сопровождать локльным контекстом и/или кратким адекватным комментом — "при создании колонки использован неизвестный тип ДаОткудаТыВзялсяТварь".
Я как понял, то устроили порку библиотечному коду.
Так, вот, все исключения типа: NotSupportedException, ArgumentException и тд. — это исключения, возникающие из-за нарушения контракта.
В отлаженном и хорошо написанном пользовательском продукте, использующим данную библиотеку, все эти исключения не должны возникнуть.
Так, что пользователь не должен увидить библиотечные исключения. Так, что throw new ArgumentNullException(nameof({myParameter})) — это норм для библиотечного кода
Ну и какой в этом смысл. Фабрика нужна, если набор динамический. А для статического набора — ну единственный минус, такое линейный поиск имеет сложность O(N), а хеш-таблица имеет сложность O(1). Но скорей всего реально разницы не будет.
4>однократно прописать перед блоком if-ов, на производительность не повлияет, но будет меньше копипасты.
От этого поведение чуток измениться, если GetColumnName(columnNames, columnIndex) выдаст исключение, оно трегернет первым до проверки типа колонки. Причем уже в другом методе.
Вероятно это и послужило причиной того что сначала проверка, а потом уже GetColumnName
else
{
throw new NotSupportedException(nameof(kind));
}
Это ублюдство работает до поры до времени, пока прогер сидит локально в CreateColumn(Type kind, string[] columnNames, int columnIndex) и ему "всё понятно". Но когда исключение вылезет эдак на 10 уровней выше, удивлённая индусятина обнаружит "not supported ComplexType" и даже не поймёт, кто кого и зачем должен поддерживать. А ведь ИМЕННО ЭТИ ошибки вылезают у юзера в самый ненужный момент и всё, что предлагает микрософт — "отошлите нам годичную телеметрию, МЫ САМИ НИ ЧЕРТА НЕ ПОНИМАЕМ В СВОЁМ КОДЕ!".
Ну так вы и не понимаете именно в силу вот таких new NotSupportedException!!
Ошибки всегда надо сопровождать локльным контекстом и/или кратким адекватным комментом — "при создании колонки использован неизвестный тип ДаОткудаТыВзялсяТварь".
Собственно, MS потому и прячет всё своё говно под коврик, что они сами не понимают, что за проблема у юзера. Писали бы они нормальные комменты к иксепшынам, распоследний юзер мог бы им НОРМАЛЬНО рапортовать: "ребят, вот такой месыдж, чините!", а не отправлять мегабайты телеметрии, чтобы обезьяна копалась весь день в логах. Аштрисёт!! (ц)
А чем этот код плох?
1) Можно было был if превратить в switch, но видимо код был написан до появления этой фичи в языке.
2) Заменить if\switch на "фабрики" или просто таблицу — сильно просадит перформанс. Тем более сейчас мэтчинг оптимизирован под популярные типы (мы же знаем что переходы вперед предсказываются как ложные).
3) Тип эксепшена не тот — это не имеет значения на самом деле. Это usage\design exeptions, их не надо ловить. Достаточно чтобы эксепшен сказал что с кодом не так.
Остается только GetColumnName не надо вызывать в каждой ветке, можно вызвать до if. Скорее всего тупо копипаста, в целом неопасная, так как ни вызов GetColumnName, ни его содержимое меняться не будет.
Здравствуйте, Ночной Смотрящий, Вы писали:
НС>Ну, хотя бы так:
Этот код, небось, копируется со времен царя Гороха, когда switch выглядел немногим лучше if else if
Здравствуйте, sergii.p, Вы писали:
SP>а что тут такого? Предлагаете писать фабрику для каждого типа? Или может мапы строить и потом по рефлексии создавать колонку? Всё одинаково плохо для такого простого примера. KIS как он есть.
Предлагаю помнить про DRY и не копипастить GetColumnName(columnNames, columnIndex) в каждую строку. Да и вообще идея тащить в метод весь массив имен, где он нафиг не нужен — она очень странная, мягко говоря.
Странно, что такие азы надо объяснять.
Здравствуйте, sergii.p, Вы писали:
SP>а что тут такого? Предлагаете писать фабрику для каждого типа? Или может мапы строить и потом по рефлексии создавать колонку? Всё одинаково плохо для такого простого примера. KIS как он есть.
Для примитивных типов, которые там примерно все, можно использовать Type.GetTypeCode.
Если нам не помогут, то мы тоже никого не пощадим.
G>3) Тип эксепшена не тот — это не имеет значения на самом деле. Это usage\design exeptions, их не надо ловить. Достаточно чтобы эксепшен сказал что с кодом не так.
Этак можно и NullReferenceException бросать, работать-то будет. Нет, если ты выкладываешь код с пространством имён Microsoft, то надо соблюдать соглашения о кодировании Microsoft. Тут похоже, что автор "угадал" назначение NotSupportedException по названию, вместо того, чтобы заглянуть в документацию. А это неправильно.
G>Остается только GetColumnName не надо вызывать в каждой ветке, можно вызвать до if. Скорее всего тупо копипаста, в целом неопасная, так как ни вызов GetColumnName, ни его содержимое меняться не будет.
G>>>3) Тип эксепшена не тот — это не имеет значения на самом деле. Это usage\design exeptions, их не надо ловить. Достаточно чтобы эксепшен сказал что с кодом не так.
Y>>Этак можно и NullReferenceException бросать, работать-то будет. G>Нельзя. NRE это runtime exeption, а не usage. Человек, увидев NRE, не поймет что произошло.
Значит, тип имеет значение. И надо следовать соглашениям.
Y>>Нет, если ты выкладываешь код с пространством имён Microsoft, то надо соблюдать соглашения о кодировании Microsoft. Тут похоже, что автор "угадал" назначение NotSupportedException по названию, вместо того, чтобы заглянуть в документацию. А это неправильно. G>Может они stack overflow читали https://stackoverflow.com/questions/12669805/when-to-use-invalidoperationexception-or-notsupportedexception
Даже там не сказано делать, как сделал Прашантх Говиндраджан.
Здравствуйте, yenik, Вы писали:
J>>Посмотрел по внимательнее, а он вообще что нить кроме typeof(string) вернуть может? Чё то как то совсем плохо
Y>А переменная nbline — что она считает? Если номер строки, то для разных колонок получится разный номер строки.
Ну в принципе да, номер значащей строки. Для них важно определить первая это значащая строка или нет. По мне все это должно выглядеть примерно так: (+/- опечатки, пишу без IDE)
private static Type GuessKind(int col, List<string[]> read)
{
Type previousType = null;
foreach (var line in read)
{
if (col >= line.Length)
throw new FormatException(string.Format(Strings.LessColumnsThatExpected, nbline + 1));
string val = line[col];
if (string.IsNullOrEmpty(val) || string.Equals(val, "null", StringComparison.OrdinalIgnoreCase))
{
continue;
}
var suggestType = bool.TryParse(val, out _) ? typeof(bool)
: float.TryParse(val, out _) ? typeof(float)
: DateTime.TryParse(val, out _) ? typeof(DateTime)
: typeof(string);
if (previousType == null)
{
previousType = suggestType;
}
if (previousType != suggestType || suggestType == typeof(string))
{
return typeof(string);
}
}
return previousType ?? typeof(string);
}
Но на самом деле — самое страшное, что у них походу нету тестов. Ибо простейший тест бы показал, что типы колонок неверно определяются.
А если будет 10000 разных типов? По крайней мере switch-case оптимизируется до выбора по таблице.
Но почему бы kind не свести к индексу в таблице сразу и не выбрать. Ну или мапу построить,
всяко O(log(N)) сильно лучше, чем O(N). Но O(1) вообще же лучше (выбор из таблицы). Я понимаю,
что до трёх десятков элементов это не сильно принципиально... Но вот Ctrl-C/Ctrl-V по всему коду
одно и то же 100500 раз подряд. Почему один раз в переменную не сохранить, а потом её везде
не подставить. Это какой-то треш.
И наконец зачем 100500 вариантов руками расписывать для каждого типа, если там реализация
отличается только типом. Int16DataFrameColumn.cs, Int32DataFrameColumn.cs -- это ж обезьяна
какая-то писала. Ну вот я бы шаблон какой-то бы придумал, чтоб функций само нагенерило,
а не через Ctrl-C/Ctrl-V (потому, что потом если где-то ошибка например, то её теперь
по всем 100500 файлам выискивать и править? а если интерфейс поменять -- проще повеситься).
Я в C# вообще не силён, но по аналогии с C++... А в голом C бы руками сделал какую-то
обобщённую функцию, которая с почти любым типом умеет работать (ну может ей бы ширина
типа передавалась, например).
Здравствуйте, fk0, Вы писали:
fk0>Int16DataFrameColumn.cs, Int32DataFrameColumn.cs -- это ж обезьяна fk0>какая-то писала. Ну вот я бы шаблон какой-то бы придумал, чтоб функций само нагенерило, fk0>а не через Ctrl-C/Ctrl-V (потому, что потом если где-то ошибка например, то её теперь fk0>по всем 100500 файлам выискивать и править? а если интерфейс поменять -- проще повеситься).
Скорее всего, одного генерика там бы за глаза хватило. Но это уже высший пилотаж.
Если нам не помогут, то мы тоже никого не пощадим.
Ну вот я бы шаблон какой-то бы придумал, чтоб функций само нагенерило,
