Рефакторинг. Должен ли я оставлять избыточным, если дело

Question

Я исправлял ошибку на этом очень длинном и подробном методе в C# и продолжал находить эти избыточные if по всему месту.

эс

//var objParam = new SqlParameter(); var cmd = new SqlCommand(); var myVar = new Foo(); 
objParam = cmd.Parameters.Add("@SomeParam", SqlType.NVarchar, 40); 
if(myVar.SomeParam.Length > 4) 
    objParam.Value = myVar.SomeParam.Substring(0, 40); 
else 
    objParam.Value = myVar.SomeParam; 

эти if s повторяются миллион раз везде, и всегда SqlType.NVarChar.

Мой вопрос в том, должен ли я оставить их? Я полагаю, это не делает код медленнее или что-то еще, но методы более длинные, и это не помогает читаемости.

Я думаю, что все они должны быть заменены

objParam = cmd.Parameters.Add("@SomeParam", SqlType.NVarchar, n); 
objParam.Value = myVar.SomeParam.Substring(0, n); 

с n число символов, очевидно.

Примечание: Я знаю, что это VB6-эск способ добавления параметров очень старый, но я стараюсь моим босс не замечать весь маленький рефакторинг я делаю на этих старых программах;)

Answer 1

Создать этот метод :

private static string getSomeParam(var myVar) => 
    myVar.SomeParam.Length > 40 ? myVar.SomeParam.SubString(0, 40) 
           : myVar.SomeParam; 

И затем использовать его так:

//var objParam = new SqlParameter(); var cmd = new SqlCommand(); var myVar = new Foo(); 
objParam = cmd.Parameters.Add("@SomeParam", SqlType.NVarchar, 40); 
objParam.Value = getSomeParam(myVar); 

Это добавит readablity, а также позволяет легко редактировать этот метод в будущем. Поэтому, если вы решите сделать 40 постоянным maxLength или что-то в этом месте.

Если вы хотите изменить значение myVar в getSomeParam():

private static string getSomeParam(var myVar, var out objParam) => 
    objParam.Value = myVar.SomeParam.Length > 40 ? myVar.SomeParam.SubString(0, 40) 
               : myVar.SomeParam; 

И называть это так:

objParam = cmd.Parameters.Add("@SomeParam", SqlType.NVarchar, 40); 
getSomeParam(myVar, out objParam); 

Answer 2

Когда вы одинаково выглядящие if S, которые выполняют значимую работу, создание вспомогательного метода - хорошая идея. Мало того, что это сделает ваш код короче и легче читать, но и устранить потенциально вредное копирование, прекрасно проиллюстрированное опечаткой в ​​вашем вопросе:

if(myVar.SomeParam.Length > 4) // <<= This const should be 40, not 4 
    objParam.Value = myVar.SomeParam.Substring(0, 40); 
else 
    objParam.Value = myVar.SomeParam; 

Сделать помощник, который выглядит следующим образом:

private static SqlParameter AddNvarchar(SqlCommand cmd, string name, int size, string val) { 
    var res = cmd.Parameters.Add(name, SqlType.NVarchar, size); 
    if(val.Length > size) 
     res.Value = val.Substring(0, 40); 
    else 
     res.Value = val; 
    return res; 
} 

Теперь вы можете заменить задания многоканальных с этим:

var objParam = AddNvarchar(cmd, "@SomeParam", 40, myVar.SomeParam); 

Answer 3

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

К сожалению, это не так, как работает метод подстроки C# - если вы дадите слишком короткую строку, он выкинет исключение ArgumentOutOfRangeException. Таким образом, проверки не являются избыточными.Другой способ, чтобы написать это было бы:

objParam.Value = myVar.SomeParam.Substring(0, Math.Max(40, SomeParam.Length));