Handling arrow keys for a console menu

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

• How readable is it?

• 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 Spend(string[] text,List 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&

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

public List 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 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 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.

StackExchange Code Review Q#92029, answer score: 8