# Handling arrow keys for a console menu

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

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{
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{
continue;
}
break;
case Enter:
currencyLeft=currency;
default:
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);
}
PagePlus=0;
}
}

• A plain English description of what this method is meant to do might help you to get better reviews. – mjolka May 28 '15 at 8:38
• @200_success It's not really the arrow key handling, it's mostly everything else. – somebody May 28 '15 at 9:38
• Our site standard is to title questions according to the purpose of the code, not your concern about the code. Otherwise, we will get many questions with generic titles like "Is my function too long?" – 200_success May 28 '15 at 9:39

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.

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.

1. 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.

• Well, I don't really want a conditional operator on the while loop, because I might add multiple return conditions. Also, by refactoring, do you mean splitting into simpler, smaller methods? – somebody May 28 '15 at 8:53
• How is it hard to tell where Select.TurnPage(); is called? I should be okay since VS auto-indents. – somebody May 28 '15 at 8:57
• On my small screen I cannot see the whole method, therefor the intentation is not that helpful anymore. – Tiberiu C. May 28 '15 at 9:03
• And I have XML-doc comments, it shouldn't be hard working out how this method achieves this from there. – somebody May 28 '15 at 9:03
• You can enable word wrap (that's what I do) – somebody May 28 '15 at 9:03

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