3
\$\begingroup\$

I'm new programmer and I'm working on Xamarin MVVM app and I have a pin view like

enter image description here

So, basically I have numbers from 0-9 if you pick one number its visible then if you pick a second one first one changed to * and I store all numbers into string called PinCode

ViewModel Code:

  public string PinCode { get; set; } = string.Empty;

        private async Task<bool> SelectedButton(Button button)
        {
            //If button is a number then
            if (button.Text != null)
            {
                if (PinCode.Length < 4)
                {
                    PinCode = PinCode + button.Text;

                    //Assign every number field text depending of PinCode length
                    if (PinCode.Length == 1)
                    {
                        PinNumberOne = button.Text;
                    }
                    else if (PinCode.Length == 2)
                    {
                        PinNumberTwo = button.Text;
                        PinNumberOne = "*";
                    }
                    else if (PinCode.Length == 3)
                    {
                        PinNumberThree = button.Text;
                        PinNumberOne = "*";
                        PinNumberTwo = "*";
                    }
                    else if (PinCode.Length == 4)
                    {
                        PinNumberFour = button.Text;
                        PinNumberOne = "*";
                        PinNumberTwo = "*";
                        PinNumberThree = "*";
                    }
                }
            }
            // if it's backspace button then
            else
            {
                PinCode = PinCode.Remove(PinCode.Length - 1);

                if (PinCode.Length == 3)
                {
                    PinNumberFour = "_";
                }
                else if (PinCode.Length == 2)
                {
                    PinNumberFour = "_";
                    PinNumberThree = "_";
                }
                else if (PinCode.Length == 1)
                {
                    PinNumberFour = "_";
                    PinNumberThree = "_";
                    PinNumberTwo = "_";
                }
                else if (PinCode.Length == 0)
                {
                    PinNumberFour = "_";
                    PinNumberThree = "_";
                    PinNumberTwo = "_";
                    PinNumberOne = "_";
                }
            }
            return true;
        }

As you can see, I have a lot of repeated code and I know it's possible to improve this method much better, but I can not find the way. If you guys have an idea of a recursion or how can I refactor this code to do something much clear and clean I really appreciate it. Regards

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here (since many users want to refactor repeated code). Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jul 24 at 19:49
  • \$\begingroup\$ Put PinNumber* in an array. Change the array values by index. Iterate the array by loop. \$\endgroup\$ – Alexander Petrov Jul 24 at 21:16
  • \$\begingroup\$ Can you show me how can I iterate and replace that length... I know how can I iterate an array but I don't see how can I use in code instead if and else conditions @AlexanderPetrov \$\endgroup\$ – Jesus Jul 24 at 21:31
3
\$\begingroup\$

This is a common problem. Sometimes it's tricky to generalise a process even when its iterative in nature.

By keeping track of both:

  • the location of the user's cursor (cursor_position), and
  • an iterable data structure storing which characters are shown (pinNumbers),

it is possible to implement this behaviour without so much repeated code. This gave the desired behaviour after some thorough testing (in my imagination).

public string PinCode { get; set; } = string.Empty;

private int cursor_position = -1;
private string[] pinNumbers = new string[] {"_", "_", "_", "_"};

    private async Task<bool> SelectedButton(Button button)
    {
        //If button is a number then
        if (button.Text != null)
        {
            if ( cursor_position < 3 )
            {
                PinCode = PinCode + button.Text;

                if ( cursor_position > 0 )
                {
                    pinNumbers[cursor_position] = "*";
                }

                cursor_position += 1;
                pinNumbers[cursor_position] = button.Text;
            }
        } else { // apparently backspace?
            if ( cursor_position >= 0 )
            {
                PinCode = PinCode.Remove(PinCode.Length - 1);
                pin_numbers[cursor_position] = "_";
                cursor_position -= 1;
            }
        }

        pinNumberOne   = pinNumbers[0]; // update actual shown values with array
        pinNumberTwo   = pinNumbers[1]; // you could probably take out the "pinNumberX" middleman
        pinNumberThree = pinNumbers[2]; // but I dont know what relation these variables have to
        pinNumberFour  = pinNumbers[3]; // the rest of your code

        return true;
    }

Depending on how you are assigning values to the actual rendered characters you may be able to remove the first four of the last five lines. You likely could just refer the rendered components directly to the values in the pinNumbers array.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ @yamfox missed a semicolon on line 3, and missing closing braces on line 38. \$\endgroup\$ – iSR5 Jul 26 at 14:36
0
\$\begingroup\$

You really need to store the original values into an int array or collection. The reason behind that is you don't need to recasting the values every now and then. Keep the original values in their correct form, and then cast them to string or any other type whenever needed. If you just keep going doing the same concept (using strings on everything) you'll end up having many parts that need to be refactored, and probably it will impact the overall performance.

Your Pin Code can be simplified by using string[] for the code mask, and List<int> for the actual integers.

public string PinCode { get; set; } = string.Empty;

private string[] _codeMask = new string[] { "_", "_", "_", "_" };

private List<int> _pinNumber = new List<int>(4); // limit list size to 4 elements, if you remove it would dynamic size. 

private async Task<bool> SelectedButton(Button button)
{
    if(PinCode == 4) { return true; }
        
    // validate input
    if(int.TryParse(button.Text, out int number) && (number < 10 && number > 0))
    {
       if(_pinNumber.Count == 4) { return; } 
        
        _pinNumber.Add(number);
        
        var maskPosition = _pinNumber.Count - 2;
        
        var numberPosition = _pinNumber.Count - 1;
     
         _codeMask[numberPosition] = number.ToString();                                        
            
        if(maskPosition != -1)
        { 
          _codeMask[maskPosition] = "*";
        }
        
        PinCode = String.Join(' ', _codeMask);              
    }
    
    return false;
}

Then just use the _codeMask or _pinNumber to assign their values to any releated variable. But I would suggest you get rid of PinNumberOne , PinNumberX variables, and use _pinNumber[x] directly. would be more appropriate and much maintainable.

| improve this answer | |
\$\endgroup\$

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.