AnswerBun.com

Refactor multiple if-else conditions when condition is a minor change

Code Review Asked by Jesus on January 2, 2022

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

2 Answers

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.

Answered by iSR5 on January 2, 2022

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.

Answered by yarnfox on January 2, 2022

Add your own answers!

Related Questions

CSS Alphabetizer

2  Asked on December 2, 2021

 

Hacking Minigame

1  Asked on December 2, 2021 by aguywhocodes

     

Parse a simple key=value config file in C

6  Asked on November 28, 2021 by sedmaister

       

Rails model for aggregate data instead of a real table

1  Asked on November 28, 2021 by knejad

   

Battleship game in C

1  Asked on November 25, 2021 by mrplow

   

QTableView implementation

1  Asked on November 25, 2021 by 19172281

     

upload multiple images and resize

1  Asked on November 25, 2021 by retqio

         

RFC 1951 “Inflate” (de-compression)

0  Asked on November 25, 2021 by george-barwood

   

Generalised NxN Sudoku solver using heap

1  Asked on November 25, 2021 by srt1104

   

Created a paperfold like effect

0  Asked on November 25, 2021 by nico-shultz

       

(Improved) get-release npm module

1  Asked on November 25, 2021

         

Simple Flask REST API to a MongoDB

0  Asked on November 21, 2021 by amos-baron

         

Hit the target number

0  Asked on November 19, 2021 by mixopteryx

   

Pizza Order System

1  Asked on November 19, 2021 by chris-jaurigue

     

Ask a Question

Get help from others!

© 2023 AnswerBun.com. All rights reserved. Sites we Love: PCI Database, MenuIva, UKBizDB, Menu Kuliner, Sharing RPP, SolveDir