2016-08-29 6 views
0

Я думаю, что название говорит все это,Есть ли более эффективный способ, чтобы написать этот код

вот код :)

private void listBoxComponents_MouseDoubleClick(object sender, MouseEventArgs e) 
    { 
     object item = listBoxComponents.SelectedItem; 
     string name = listBoxComponents.GetItemText(item); 

     if (e.Button == MouseButtons.Left) 
     { 
      for (int i = 0; i < terrains.Count; i++) 
      { 
       if (terrains[i].name == name) 
        terrains[i].EnableBoundingBox(true); 
       else 
        terrains[i].EnableBoundingBox(false); 
      } 
     } 
    } 

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

Спасибо :).

+0

что тип данных «Территории»? это Перечисляемо? –

+0

Это общий список – tdkr80

+0

@ tdkr80 Рассмотрите вопрос об этом здесь: http://codereview.stackexchange.com/ – random

ответ

1

Сам код в порядке. LINQ не обязательно более эффективен, несмотря на то, что иногда имеет меньшие символы кода (во всяком случае, он несет дополнительные накладные расходы, но обычно он удаляется во время компиляции IL, и я склонен обнаруживать, что LINQ будет менее читаемым, чем традиционные блоки/циклы) ,

Если вы используете LINQ, оно заканчивается компиляцией до чего-то похожего на то, что вы написали, так что на самом деле это просто синтаксический сахар/код-конфеты.

Код читаемый, и вы не тратите ресурсы внутри цикла ... что еще вы хотите?

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

3

Возможно, ваш вопрос лучше подходит для сайта SO SOODE, поскольку он работает, но вы хотите его оптимизировать.

Предлагаю вам изучить другую структуру данных для terrains. Использование его в качестве массива не очень эффективно. Если вы создаете словарь в свойстве name, вы избежите постоянных итераций для определенной именованной местности.

ОБНОВЛЕНИЕ: Поскольку мое первоначальное предложение ошибочно, я придумываю другое.

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

Ваш код будет выглядеть примерно так:

if (e.Button == MouseButtons.Left) 
    { 
     terrains.highlight(name); 
    } 

Реализация функции highlight может быть оптимизирована без побочных эффектов на другие части вашего кода.

+0

Я согласен с вашей точкой в ​​том, что идентификатор лучше подходит для Codereview. В своем комментарии он сказал, что это общий список, а не массив. – Marv

+0

Зачем нужен словарь здесь лучше? По-прежнему существует параметр «false» для всех случаев, отличных от имен. – Maarten

+0

@Maarten Вы на самом деле правы, я, очевидно, перестал читать в цикле 'for'. Я буду перефразировать это предложение. – thst

6

Вы не указано, что именно вы имеете в виду под «более эффективным», так что я буду просто взять на себя смелость интерпретировать это как «легче читать и тип»:

Вы можете заменить свой цикл:

for (int i = 0; i < terrains.Count; i++) 
{ 
    if (terrains[i].name == name) 
     terrains[i].EnableBoundingBox(true); 
    else 
     terrains[i].EnableBoundingBox(false); 
} 

с этим одним:

foreach (var terrain in terrains) 
{ 
    terrain.EnableBoundingBox(terrain.name == name); 
} 

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

Кстати, лучше сравнить строки, используя метод string.Equals(string, string, StringComparison), потому что это делает более ясным, какую культуру и чувствительность к регистру следует использовать для сравнения.

Как и в другом случае, вы упомянули LINQ. LINQ был бы плохо подходит здесь; позвольте мне вкратце объяснить, почему: теоретически возможно написать пользовательский оператор LINQ ForEach, который принял бы делегата Action<T>. Он может быть реализован как цикл foreach, который вызывает делегат для каждого элемента в исходной последовательности. Затем можно написать следующую Однострочник:

terrains.ForEach(terrain => terrain.EnableBoundingBox(terrain.name == name)); 

На мой взгляд, простой foreach петля на самом деле более легко читаемым, даже если он занимает несколько строк кода. Но самое главное, помните, что означает LINQ: «Language-Integrated Query». Выражения LINQ должны запрос данные, а не изменить он (то есть имеет побочные эффекты). Однако пользовательский оператор ForEach будет касаться побочных эффектов; поэтому он не соответствует парадигме LINQ и не включен в Framework.

(В качестве последнего замечания, мой ответ в основном обзор кода. Если это то, что вы хотели, возможно, Code Review SE сайт, возможно, был более подходящим местом, чтобы разместить Ваш вопрос.)

+1

Это, вероятно, даже более эффективно с точки зрения производительности на низком уровне, так как в нем нет ветвей. Однако код, стоящий за «EnableBoundingBox», в любом случае будет доминировать во время выполнения: D – stefan

+0

имя - это строка, а EnableBoundingBox - bool. Я не думаю, что это сработало бы – tdkr80

+0

@ tdkr80: сравнение строк с '==' или 'string.Equals' даст' bool', конечно, это работает. – stakx

Смежные вопросы