Processing selected and deselected elements

Question

I think that this code is very bad because it does not agree with DRY principle. Can you advise me how to improve it?

public void WhatSelected(object paramaters)
 {
 string ElementToProcess = BASM.FindElement(paramaters, true);
 SeleсtedCurrent.Add(ElementToProcess);
 int i = (from x in AssetSelectorCollection where x.IsChecked == true select x).Count();
 if (i >= state)
 {
 foreach (BinaryAssetSelector item in AssetSelectorCollection)
 {
 if (item.IsChecked == false) { item.IsEnable = false; item.IsChecked = false; }
 }
 }
 if (state != 1)
 {
 foreach (BinaryAssetSelector item in AssetSelectorCollection)
 {
 if (item.IsChecked == true && item.IsEnable == false)
 {
 item.IsEnable = true;
 }
 }
 }
 Messenger.Default.Send<BinarySelectorCommunicator>(new BinarySelectorCommunicator { SelectedIN = SeleсtedCurrent });
 }
 public void WhatDeSelected(object param)
 {
 string ElementToProcess = BASM.FindElement(param, false);
 SeleсtedCurrent.Remove(ElementToProcess);
 int i = (from x in AssetSelectorCollection where x.IsChecked == true select x).Count();
 if (i <= state)
 {
 foreach (BinaryAssetSelector item in AssetSelectorCollection)
 {
 if (item.IsEnable == false)
 {
 item.IsEnable = true;
 }
 }
 }
 if (state != 1 && i == 1)
 {
 foreach (BinaryAssetSelector item in AssetSelectorCollection)
 {
 if (item.IsChecked == true)
 {
 item.IsEnable = false;
 }
 }
 }
 Messenger.Default.Send<BinarySelectorCommunicator>(new BinarySelectorCommunicator { SelectedIN = SeleсtedCurrent });
 }

• Simon Forsberg: To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question

Answer 1

Variable names:

Names like i, param, parameters, state don't have a useful meaning, not for you, nor for others who are reading/reviewing your code. Use meaningful names for your variables, this is better for readability and maintainability.

---

You have four different actions, based on the four different conditions regarding state and i:

1. i>= state
2. state != 1
3. i <= state
4. state != 1 && i == 1

Is there a reason why they are in separate methods? The only overlapping part is that in two conditions i can be the same as state (1 and 3). If this is a mistake, you can place all the code in one method, if the checks are correct and you want to keep both methods, also fine. I'll base my answer on the correctness of your code and keep two methods.

---

In both the methods you perform two foreach loops based on a check. This means that if AssetSelectorCollection has 200 items, you loop 400 times, which is not smart. Reverse the code like this:

foreach (BinaryAssetSelector item in AssetSelectorCollection)
{
 if (i >= state)
 {
 if (item.IsChecked == false)
 {
 item.IsEnable = false;
 item.IsChecked = false;
 }
 }
 if (state != 1)
 {
 if (item.IsChecked == true && item.IsEnable == false)
 {
 item.IsEnable = true;
 }
 }
}

Now you only have one loop over the collection, which will increase performance, certainly over a bigger collection. This must be applied in both methods.

---

Redundant checking:

Following code:

if (item.IsChecked == false)
{
 item.IsEnable = false;
 item.IsChecked = false;
}

contains redundant checks. The IsChecked property is a boolean and thus needs no evaluation against true or false in the condition, plus you un-check the item when it is already unchecked, no need to do that twice. Rewrite it as follows:

if (!item.IsChecked)
{
 item.IsEnable = false;
}

and even shorter:

item.IsEnable = item.IsChecked

Note that this last line will enable the item when it is checked, not only disable the item when it is unchecked. This logic can also be applied to all the if statements in the code.

---

Example of the first method, reviewed:

public void WhatSelected(object paramaters)
{
 string ElementToProcess = BASM.FindElement(paramaters, true);
 SeleсtedCurrent.Add(ElementToProcess);
 var i = (from x in AssetSelectorCollection where x.IsChecked == true select x).Count();
 foreach (BinaryAssetSelector item in AssetSelectorCollection)
 {
 if (i >= state)
 {
 if (!item.IsChecked)
 {
 item.IsEnable = false;
 }
 }
 if (state != 1)
 {
 if (item.IsChecked && !item.IsEnable)
 {
 item.IsEnable = true;
 }
 }
 }
 Messenger.Default.Send<BinarySelectorCommunicator>(new BinarySelectorCommunicator { SelectedIN = SeleсtedCurrent });
}

Edit:

Since you're not doing anything between the two if statements you can place the condition of the second statement inside the first one with an && operator:

public void WhatSelected(object paramaters)
{
 string ElementToProcess = BASM.FindElement(paramaters, true);
 SeleсtedCurrent.Add(ElementToProcess);
 var i = (from x in AssetSelectorCollection where x.IsChecked == true select x).Count();
 foreach (BinaryAssetSelector item in AssetSelectorCollection)
 {
 if (i >= state && !item.IsChecked)
 {
 item.IsEnable = false;
 }
 if (state != 1 && (item.IsChecked && !item.IsEnable))
 {
 item.IsEnable = true;
 }
 }
 Messenger.Default.Send<BinarySelectorCommunicator>(new BinarySelectorCommunicator { SelectedIN = SeleсtedCurrent });
}