1
\$\begingroup\$

following code below works fine but feels kind of a mess. Any one has a better way to do this?

I am creating a basic "Enter Pin" to unclick screen on an app. So if the user enters the 1st digit, then ellipse1 should be filled. And if the user enters the 2nd digit, then ellipse1 and ellipse2 should be filled. And if the user enters the 3rd digit, then ellipse1, ellipse2, and ellipse3 should get filled. And so on

in view class, I have 4 ellipses, representing 4 digit pin. If user enters a digit, ellipse color get filled.

in viewmodel, I am a method that check if digit was pressed. If it is then fill ellipse color

in helperclass, I am just checking if ellipse is transparent (not pressed), or fill color (key was pressed)

view class

 <Ellipse Fill="{Binding EllipseFill1}" />
 <Ellipse Fill="{Binding EllipseFill2}" />
 <Ellipse Fill="{Binding EllipseFill3}" />
 <Ellipse Fill="{Binding EllipseFill4}" />

ViewModel class

  List<int> CreatePin = new List<int>; //4 digit pin is store here
  public void OnKeypadTapCommand(string value)
    {
        //Set color of Pin
        EllipseFill1 = helperCLass.SetColorPin(CreatePin, Brush.White, 1);
        EllipseFill2 = helperCLass.SetColorPin(CreatePin, Brush.White, 2);
        EllipseFill3 = helperCLass.SetColorPin(CreatePin, Brush.White, 3);
        EllipseFill4 = helperCLass.SetColorPin(CreatePin, Brush.White, 4);
    }

Helperclass

    public Brush SetColorPin(List<int> valueList, Brush value, int PinNumber)
    {
        //Pin#1
        if (PinNumber == 1) 
        {
            if (valueList.Count == 0)
                return Brush.Transparent;
            else
                return value;
        }
        //Pin#2
        else if (PinNumber == 2)
        {
            if (valueList.Count == 0 || valueList.Count == 1)
                return Brush.Transparent;
            else
                return value;
        }
        //Pin#3
        else if (PinNumber == 3)
        {
            if (valueList.Count == 3 || valueList.Count == 4)
                return value; 
            else
                return Brush.Transparent;
        }
        //Pin#4
        else if (PinNumber == 4)
        {
            if (valueList.Count == 4)
                return value;
            else
                return Brush.Transparent;
        }
        else
        {
            return Brush.Transparent;
        }
        
    }
\$\endgroup\$
1
\$\begingroup\$

Switching the order of the nested if makes it harder to spot the pattern from the code, but I think you just need this:

public Brush SetColorPin(List<int> valueList, Brush value, int PinNumber)
{
    if (PinNumber < 1 || PinNumber > 4) 
    {
        return Brush.Transparent
    }

    return PinNumber <= valueList.Count ? value : Brush.Transparent;
}

For PinNumber 1,2 you return Brush.Transparent in the true branch of the inner if. For 3,4 you return value.

\$\endgroup\$
2
  • \$\begingroup\$ Or even return (PinNumber < 1 || PinNumber > 4 || PinNumber > valueList.Count) ? Brush.Transparent : value; \$\endgroup\$ Oct 14 at 20:41
  • 1
    \$\begingroup\$ @TobySpeight - yeah, that would work too. I kept them separate as I think of the first if as a precondition check for the main logic. We could not do it at all and support PINs of different lengths. \$\endgroup\$
    – RobH
    Oct 15 at 6:49
0
\$\begingroup\$

you could change the helper method to use switch if you're using C# 7.0 or higher then you can do this :

switch(PinNumber)
{
    case 1 when valueList.Count == 0:           
    case 2 when valueList.Count == 0 || valueList.Count == 1:
    case 3 when valueList.Count == 3 || valueList.Count == 4:
    case 4 valueList.Count == 4:
        return value;
    default: 
        return Brush.Transparent;
}

if not, then you can focus on readability by doing something like this :

var isPin1 = PinNumber == 1 && valueList.Count != 0; 
var isPin2 = PinNumber == 2 && valueList.Count == 0 || valueList.Count == 1; 
var isPin3 = PinNumber == 3 && valueList.Count == 3 || valueList.Count == 4; 
var isPin4 = PinNumber == 4 && valueList.Count == 4;

return isPin1 || isPin2 || isPin3 || isPin4 ? value : Brush.Transparent;
\$\endgroup\$
1
  • \$\begingroup\$ I guess that XAML Style.Triggers/DataTrigger may be useful because handling View components (Brush) in View Model is probably a MVVM violation. \$\endgroup\$
    – aepot
    May 17 at 0:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.