Нормально ли я отрефакторил if-else?

Question

[12rbah]

В общем увидел такой код и решил попробовать подрефакторить его. Можете оценить мой код?

////////начальная версия
func MultiSort(tracks []*Track, columns []string) sort.Interface {
	return customSort{
		tracks,
		func(x, y *Track) bool {
			fmt.Println(columns)
			for i := len(columns) - 1; i >= 0; i-- {<code lang="cpp">
				if columns[i] == "Title" && x.Title != y.Title {
					return x.Title < y.Title
				} else if columns[i] == "Artist" && x.Artist != y.Artist {
					return x.Artist < y.Artist
				} else if columns[i] == "Album" && x.Album != y.Album {
					return x.Album < y.Album
				} else if columns[i] == "Year" && x.Year != y.Year {
					return x.Year < y.Year
				} else if columns[i] == "Length" && x.Length != y.Length {
					return x.Length < y.Length
				}
			}
			return true
		},
	}
}

////////моя версия 
type CompRules []CompRule

type CompRule struct{
	name string
	cond bool
	result bool
}

func (d *CompRules)getRulesResult(name string)(bool,bool){
	for _, rule := range *d {
		if rule.name == name && rule.cond {
			return true, rule.result
		}
	}
	return false, false
}

func initRules(x,y *Track)*CompRules{
	return &CompRules{
		{"Title",  x.Title  != y.Title,  x.Title < y.Title},
		{"Artist", x.Artist != y.Artist, x.Artist < y.Artist},
		{"Album",  x.Album  != y.Album,  x.Album < y.Album},
		{"Year",   x.Year   != y.Year,   x.Year < y.Year},
		{"Length", x.Length != y.Length, x.Length < y.Length},
	}
}

func MultiSort(tracks []*Track, columns []string) sort.Interface {
	return customSort{
		tracks,
		func(x, y *Track) bool {
			var r = initRules(x,y)
			for i := len(columns) - 1; i >= 0; i-- {
				if ok, res := r.getRulesResult(columns[i]);ok{
					return res
				}
			}
			return true
		},
	}
}

</code>

Answer 1

[Евгений Мамонов]

Я бы посоветовал оставить начальный вариант, т.к. если сделаете banchmark'и - вы увидите, что он будет быстрее вашей реализации.
Т.к. в начальной реализации нет доп. вызова функции, а в вашей есть getRulesResult.
Чтобы вызвать функцию - нужно положить значения в стек, а по завершению извлечь.
За счёт этого ваша реализация будет работать медленнее.
Но это замечание справедливо только для функций, скорость выполнения которых критична, как сортировка например.

Если бы это была бизнес логика - то тут приоритет был бы уже на понятность кода в первую очередь, чтобы потом не запутаться, и чтобы кто-то потом мог разобраться с кодом.

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

При рефакторинге обычно стараются получить или более понятный/читабельный/расширяемый код, или более производительный.

Но вы однозначно на верном пути, принятые решения и подход мне нравятся, просто у вас не совсем удачная задача для рефакторинга выбрана, решение изначально хорошее )

Обычно, код, который нужно рефакторить выглядит во много раз непонятнее, это может быть простыня на несколько экранов монитора :)), а когда в такой простыне еще и цикломатическая сложность огромная (вложенность if'ов и for'ов большая) - такой код вообще очень трудно понять :)
Вот тут самое то для рефакторинга.

Comments

[12rbah]

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

CompRules{
		{"Title",  x.Title  != y.Title,  x.Title < y.Title},
		{"Artist", x.Artist != y.Artist, x.Artist < y.Artist},
		{"Album",  x.Album  != y.Album,  x.Album < y.Album},
		{"Year",   x.Year   != y.Year,   x.Year < y.Year},
		{"Length", x.Length != y.Length, x.Length < y.Length},
	}

Было бы удобнее чем так

for i := len(columns) - 1; i >= 0; i-- {<code lang="cpp">
        if columns[i] == "Title" && x.Title != y.Title {
          return x.Title < y.Title
        } else if columns[i] == "Artist" && x.Artist != y.Artist {
          return x.Artist < y.Artist
        } else if columns[i] == "Album" && x.Album != y.Album {
          return x.Album < y.Album
        } else if columns[i] == "Year" && x.Year != y.Year {
          return x.Year < y.Year
        } else if columns[i] == "Length" && x.Length != y.Length {
          return x.Length < y.Length
        }
      }
</code>
Хотя разница по сути и не очень большая.

[Евгений Мамонов]

Я бы предпочёл всё же второй вариант, т.к. когда ваш код будет смотреть другой человек - запись вида

{"Title",  x.Title  != y.Title,  x.Title < y.Title},

для него будет не понятной, т.к. тут не очевидно, что есть операция &&, чтобы понять что это значит нужно найти место где эта конструкция используется и изучить как оно работает и только после этого придёт понимание, а глядя на код columns[i] == "Artist" && x.Artist != y.Artist этого делать не нужно.

Я пишу отталкиваясь от своего опыта, я уже шел этим путем и мне тоже казалось, что многие вещи так сделать будет понятнее (мне :))), но это всё до поры, пока в команде не появляется несколько человек - тогда подобный подход становится проблемой. Я бы хотел, чтобы кто-то мне дал такой совет лет 10 назад, это бы сэкономило мне прилично времени ))))

[12rbah]

Евгений Мамонов, спасибо.

Я бы хотел, чтобы кто-то мне дал такой совет лет 10 назад, это бы сэкономило мне прилично времени ))))

подумаю над этим советом.