Handling arrow keys for a console menu

Question

I have a relatively long method, and I'm wondering:

1. How readable is it?
2. Is a method this long okay, or will it be bad later on?

Note: This method enables users to select from a console menu - UpArrow and DownArrow change option, and LeftArrow and RightArrow change amount.

Here is the method (from here):

    public static List<Item> Spend(string[] text,List<Item> items,int currency,out int currencyLeft,string singular=null,string textZero=null){
        Initiate(items.Count);
        var maxNameLength=items.Max(n=>n.Name.Length);
        var sellIsEnabled=items.All(v=>v.Value>=0);
        var maxCurrencyLength=Math.Max(items.Aggregate(currency,(i,j)=>j.Value).Length()+text.Take(3).Aggregate(0,(i,j)=>i+j.Length)+
            Math.Max(0,(singular=singular??text[1].RemoveLast()).Length-text[1].Length),textZero?.Length??0);
        var line=Top+1;
        var totalItems=items.Select(i=>new Item(i)).ToList();
        while(true){
            switch(Input){
                case UpArrow:
                    Option--;
                    Index--;
                    break;
                case DownArrow:
                    Option++;
                    Index++;
                    break;
                case LeftArrow:
                    if(totalItems[Option].Amount-items[Option].Amount>0){
                        totalItems[Option].Amount--;
                        currency+=items[Option].Cost;
                    } else if(sellIsEnabled&&totalItems[Option].Amount>0){
                        totalItems[Option].Amount--;
                        currency+=items[Option].Value;
                    } else{
                        Input=ReadKey(true).Key;
                        continue;
                    }
                    break;
                case RightArrow:
                    if(currency-items[Option].Cost>=0){
                        totalItems[Option].Amount++;
                        currency-=items[Option].Cost;
                    } else if(sellIsEnabled&&currency-items[Option].Value>=0&&totalItems[Option].Amount-items[Option].Amount<0){
                        totalItems[Option].Amount++;
                        currency-=items[Option].Value;
                    } else{
                        Input=ReadKey(true).Key;
                        continue;
                    }
                    break;
                case Enter:
                    currencyLeft=currency;
                    return totalItems;
                default:
                    Input=ReadKey(true).Key;
                    continue;
            }
            if(Input==LeftArrow||Input==RightArrow){
                totalItems[Option].Amount.WriteAt(maxNameLength+3,line,maxLength:totalItems[Option].Amount.Length()+2);
                (currency!=0||textZero==null?text[0]+(currency==0?"no":currency.ToString())+(currency==1?singular:text[1])+text[2]:textZero).WriteAt(0,Top+MaxLines+2,maxCurrencyLength);
            }
            Maths.RMod(ref Option,TotalOptions);
            Select.TurnPage();
            if(ThePage.Was(Turned)){
                var maxAmountLength=totalItems.Max(t=>t.Amount.Length());
                var top=Top;
                foreach(var i in Indices){
                    items[i].Name.WriteAt(2,++top,maxNameLength);
                    totalItems[i].Amount.WriteAt(maxNameLength+3,top,maxAmountLength);
                }
                for(var i=Indices.Count;i<MaxLines;i++) "".WriteAt(2,++top,1+maxNameLength+maxAmountLength);
            }
            if(Input==UpArrow||Input==DownArrow){
                ' '.WriteAt(Left,line);
                '>'.WriteAt(Left,line=Index+Top+1);
            }
            Input=ReadKey(true).Key;
            PagePlus=0;
        }
    }

Answer 1

There's a lot about this method that makes it very difficult to follow... Let's start with the signature:

public List<Item> Spend(/* ... */)

So, based on the name and return type I'm going to "Spend" something(?) and get a list of items. To me, I find it had to believe this method is going to have a single responsibility straight away. The parameters only confirm the suspicion:

string[] text, List<Item> items, int currency, out int currencyLeft, string singular = null, string textZero = null

Wow. So I'm passing in some "text", a list of items, an amount of currency and optionally a "singular" string (no idea what that is) and a textZero string (again no idea) AND I'm getting an output of how much currency I have left. This is ringing alarm bells - why am I passing in text to a function that lets me spend money... Why is it an array? Why am I passing in items? At the very least I know this method is: showing info to the user, changing how much currency I have and probably changing what items I own. That's 3 separate responsibilities already and we haven't even got into the method yet.

Let's start on that:

var maxCurrencyLength=Math.Max(items.Aggregate(currency,(i,j)=>j.Value).Length()+text.Take(3).Aggregate(0,(i,j)=>i+j.Length)+
        Math.Max(0,(singular=singular??text[1].RemoveLast()).Length-text[1].Length),textZero?.Length??0);

That should be a method by itself! It's basically the most unreadable variable declaration I've ever seen (sorry if that sounds harsh).

Then there's this:

var totalItems=items.Select(i=>new Item(i)).ToList();

Okay, you're deep cloning the items (I would guess) - no indication of why and you've made me try to figure out what you're doing.

while (true) {
    switch (Input)

Most people writing C# prefer braces on new lines. What is Input? A property that gets input from the user?

I could go on but that's not very constructive. What you need to do, is go back to the drawing board and come up with some classes to encapsulate all the different things going on here.

public class Shop 
{
    public IList<Item> GetAvailableItems() 
    {
        // ..
    }

    public int GetSellItemPrice(Item item)
    {
        // ..
    }
}

You'll need other classes too. Like a Player with an ItemBag (for storing items) and a Wallet (for storing money). Potentially, Transaction objects to pass into Shops/Wallets to change the amount of money etc.

So to answer your question: the method length is the least of your worries, you need to break the problem up into smaller pieces and make each piece responsible for a single thing.

Answer 2

1. How readable is it?

A short answer is no is not, for example is hard for me to tell in which block is this Select.TurnPage(); called, is either hard to tell when that infinite loop : while(true) is stoped.

There is a metric called Cyclomatic Complexity which counts how many paths your method can have depending on the arguments. For your method here this metric is more than 20 (I counted manually). Now for a good method would be a CC of 1-4, a tolerated method should't pass 10 CC, what passes 10 needs refactoring.

2. Is a method this long okay, or will it be bad later on?

Other people would have a hard time to understand what exactly does this method, you will forget as well, and then only a Devine entity will know.

Answer 3

The method size is probably ok, however it is hard to read and understand mainly because of:

• lack of comments
  • what is the purpose of this methods?
  • what does it do?
  • what are the parameters and how they are related to each other?
• too many chained operations and long lines