Прошел бы у вас такой код Code Review? - .NET

Привет всем!

Есть кусок кода

public IProduct CreateProduct( string specification )
{
  if ( specification == null || ( specification = specification.Trim() ) == "" )
    return null;
  
  // Далее строим продукт по гарантировано непустой потриманной спецификации
  // ....

  return product;
}

Собственно, вопрос в названии темы.

Спасибо.

Здравствуйте, SergASh, Вы писали:

Нет.

= 4.0

public IProduct CreateProduct(string specification)
{
    if (string.IsNullOrWhitespace(specification))
    {
        return null;
    }

    return product;
}

< 4.0

public IProduct CreateProduct(string specification)
{
    if (
        (specification == null) ||
        (specification.Trim().Length == 0))
    {
        return null;
    }

    return product;
}

Вообще делать Trim надо бы на момент ввода и валидации этой спецификации, чтобы туда изначально попали корректные данные

А дальше пользоваться String.IsNullOrEmpty()

Здравствуйте, adontz, Вы писали:

A>< 4.0
A>

A>public IProduct CreateProduct(string specification)
A>{
A>    if (
A>        (specification == null) ||
A>        (specification.Trim().Length == 0))
A>    {
A>        return null;
A>    }

A>    return product;
A>}
A>

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

Чем проверка длины на 0 лучше сравнения с пустой строкой?

Здравствуйте, belnetmon, Вы писали:

B>Вообще делать Trim надо бы на момент ввода и валидации этой спецификации, чтобы туда изначально попали корректные данные

B>А дальше пользоваться String.IsNullOrEmpty()

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

Здравствуйте, SergASh, Вы писали:

SAS>Это не эквивалентный код, поскольку после проверки не остается усеченной строки. То есть Trim придется выполнять второй раз, чего хотелось бы избежать.

Да, действительно. Тогда два if.

SAS>Чем проверка длины на 0 лучше сравнения с пустой строкой?

http://msdn.microsoft.com/en-us/library/ms182279%28VS.80%29.aspx

Здравствуйте, adontz, Вы писали:

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

Здравствуйте, SergASh, Вы писали:

SAS>Привет всем!

SAS>Есть кусок кода
SAS>

SAS>public IProduct CreateProduct( string specification )
SAS>{
SAS>  if ( specification == null || ( specification = specification.Trim() ) == "" )
SAS>    return null;
  
SAS>  // Далее строим продукт по гарантировано непустой потриманной спецификации
SAS>  // ....

SAS>  return product;
SAS>}
SAS>

SAS>Собственно, вопрос в названии темы.

SAS>Спасибо.

public IProduct CreateProduct( string specification )
{
  if (!IsValidSpecification(specification))
  {
    return null;
  }
  
  specification = specification.Trim();

  // Далее строим продукт по гарантировано непустой потриманной спецификации
  // ....

  return product;
}

Здравствуйте, SergASh, Вы писали:

SAS>Можете озвучить конкретную причину? Я так понимаю, либо из-за того, что вносится изменение в аргумент метода, либо потому, что проверка условия имеет побочный эффект. Что по-вашему было бы более серьезным нарушением?

Проверка условия имеет побочный эффект. Это главное. Проверка == "" была бы выявлена автоматически.

Здравствуйте, SergASh, Вы писали:

SAS>Есть кусок кода
SAS>

SAS>public IProduct CreateProduct( string specification )
SAS>{
SAS>  if ( specification == null || ( specification = specification.Trim() ) == "" )
SAS>    return null;
SAS>

Я бы сразу озадачился вопросом — а нафига нам продукт который null? Исключение или null object всяко предпочтительнее.
Так что 10% что прошел и 90% что нет.
А содержимое if побоку — мысль ясна, большего и не надо.

Здравствуйте, SergASh, Вы писали:

SAS>Собственно, вопрос в названии темы.
Неа, т.к. метод должен или падать при недопустимых значениях, или (в случае редких исключений) должен выглядеть так:

public bool TryCreate(string specification, out IProduct product)

Критерий очень простой: чем меньше WPF/sec у пользователя вашего API, тем лучше. Высший пилотаж — если пользователь делает то, что ему нужно, вообще не заглядывая в документацию.

Здравствуйте, Sinix, Вы писали:

S>Критерий очень простой: чем меньше WPF/sec у пользователя вашего API, тем лучше. Высший пилотаж — если пользователь делает то, что ему нужно, вообще не заглядывая в документацию.

Крутая опечатка.

Здравствуйте, adontz, Вы писали:

A>Крутая опечатка.
Ага, прям по Фрейду. У меня как раз сейчас WPF/sec все нормы перешёл.

Здравствуйте, SergASh, Вы писали:

SAS>Привет всем!

SAS>Есть кусок кода
SAS>

SAS>public IProduct CreateProduct( string specification )
SAS>{
SAS>  if ( specification == null || ( specification = specification.Trim() ) == "" )
SAS>    return null;
  
SAS>  // Далее строим продукт по гарантировано непустой потриманной спецификации
SAS>  // ....

SAS>  return product;
SAS>}
SAS>

SAS>Собственно, вопрос в названии темы.

SAS>Спасибо.

Нет, не прошел бы. Надо либо так

public IProduct CreateProduct( string specification )
{
  if(specification.IsNullOrWhitespace())
  {
        throw new InvalidArgumentException("specification");
  }
  
  // Далее строим продукт по гарантировано непустой потриманной спецификации
  // ....

  return product;
}

Либо так:

public IProduct CreateProduct( string specification )
{
  if(specification.IsNullOrWhitespace())
  {
        return new NullProduct();
  }
  
  // Далее строим продукт по гарантировано непустой потриманной спецификации
  // ....

  return product;
}

Где для 3-го фреймворка IsNullOrWhitespace — это extension метод.

... << RSDN@Home 1.2.0 alpha 4 rev. 1476>>

Здравствуйте, SergASh, Вы писали:

SAS>Собственно, вопрос в названии темы.

В коде есть три спорных момента:

1) Возвращение null при недопустимом аргументе — отношусь положительно
2) Изменение значения аргумента — т.к. производится в первых строчках метода, то тоже отношусь положительно
3) Совмещение проверки условия с присвоением значения — отношусь плохо, предпочел бы два условия

Здравствуйте, Undying, Вы писали:

U>1) Возвращение null при недопустимом аргументе — отношусь положительно
А я — наоборот, отрицательно. И МС — тоже:

Do validate arguments passed to public, protected, or explicitly implemented members. Throw System.ArgumentException, or one of its derived classes, if the validation fails.

This guideline does not require that the validation code be in the publicly visible member. It is acceptable to pass the arguments to an internal method that handles the validation.

И, для редких исключений

Consider the TryParse pattern for members that may throw exceptions in common scenarios to avoid performance problems related to exceptions.
...

Do provide an exception-throwing member for each member using the TryParse pattern.
...

Впрочем, враги прокрались и туда:

Return null for extremely common error cases. For example, Open returns null if the file is not found, but throws an exception if the file is locked.

U>2) Изменение значения аргумента
Копейки.

3) Совмещение проверки условия с присвоением значения — отношусь плохо, предпочел бы два условия
+1

Здравствуйте, Sinix, Вы писали:

U>>1) Возвращение null при недопустимом аргументе — отношусь положительно
S>А я — наоборот, отрицательно. И МС — тоже:

Если по условию задачи отсутствие значения является валидным состоянием для объекта, то от возвращения null'а никуда не деться, чтобы там не писала MC. Исключение, когда типом значения является коллекция, тогда обычно лучше вернуть пустую коллекцию.

S>

S>Do validate arguments passed to public, protected, or explicitly implemented members. Throw System.ArgumentException, or one of its derived classes, if the validation fails.

Кидание ArgumentException для штатной ситуации делает метод просто таки бесполезным, т.к. при его использовании приходиться строить логику на исключениях, что плохо по многим причинам.

S>

S>Consider the TryParse pattern for members that may throw exceptions in common scenarios to avoid performance problems related to exceptions.

А смысл какой в TryParse, если отсутствие значения это валидное состояние объекта?

Здравствуйте, Undying, Вы писали:

U>Если по условию задачи отсутствие значения является валидным состоянием для объекта, то от возвращения null'а никуда не деться.
Итак, null — это валидный результат для Create() ?

Обращаю ваше внимание, что у ТС какой-то вспомогательный код, а не часть модели предметной области. Так что условие задачи тут явно притянуто за уши.

U>Кидание ArgumentException для штатной ситуации делает метод просто таки бесполезным, т.к. при его использовании приходиться строить логику на исключениях, что плохо по многим причинам.
Ну да. Поэтому рекомендуется использовать явный контракт аля TryParse.

U>А смысл какой в TryParse, если отсутствие значения это валидное состояние объекта?
Мы ходим по кругу

Здравствуйте, Sinix, Вы писали:

S>Итак, null — это валидный результат для Create() ?

S>Обращаю ваше внимание, что у ТС какой-то вспомогательный код, а не часть модели предметной области. Так что условие задачи тут явно притянуто за уши.

Допустим, что IProduct это составная часть некоего объекта Parent, причем отсутствие значения для IProduct является валидным состоянием для Parent. Соответственно сейчас я могу написать так:

Parent parent = new Parent(CreateProduct(specification));

А сколько строк кода тут будет при использовании TryCreate? И какую пользу эти лишние строки принесут?

Здравствуйте, Undying, Вы писали:

U>Допустим, что IProduct это составная часть некоего объекта Parent, причем отсутствие значения для IProduct является валидным состоянием для Parent. Соответственно сейчас я могу написать так:

U>Parent parent = new Parent(CreateProduct(specification));
U>А сколько строк кода тут будет при использовании TryCreate? И какую пользу эти лишние строки принесут?

Да, для такого юз-кейза следование рекомендациям принесёт только вред.

Однако у ТС метод Create — public. И, соответственно, может использоваться в весьма неожиданными способами. Так что в долгосрочной перспективе ещё неясно кто прав

	От:	SergASh
	Дата:	23.10.10 12:09
	Оценка:

От:	adontz	http://adontz.wordpress.com/
Дата:	23.10.10 12:13
Оценка:

	От:	belnetmon
	Дата:	23.10.10 12:14
	Оценка:	1 (1) +1

	От:	SergASh
	Дата:	23.10.10 12:21
	Оценка:

	От:	SergASh
	Дата:	23.10.10 12:22
	Оценка:

	От:	Undying
	Дата:	25.10.10 05:59
	Оценка:	1 (1) +1

	От:	mrTwister
	Дата:	24.10.10 12:35
	Оценка: