5
\$\begingroup\$

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;
        }
    }
\$\endgroup\$
  • 2
    \$\begingroup\$ A plain English description of what this method is meant to do might help you to get better reviews. \$\endgroup\$ – mjolka May 28 '15 at 8:38
  • \$\begingroup\$ @200_success It's not really the arrow key handling, it's mostly everything else. \$\endgroup\$ – somebody May 28 '15 at 9:38
  • \$\begingroup\$ 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?" \$\endgroup\$ – 200_success May 28 '15 at 9:39
8
\$\begingroup\$

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.

\$\endgroup\$
4
\$\begingroup\$
  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.

  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.

\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – somebody May 28 '15 at 8:53
  • \$\begingroup\$ How is it hard to tell where Select.TurnPage(); is called? I should be okay since VS auto-indents. \$\endgroup\$ – somebody May 28 '15 at 8:57
  • \$\begingroup\$ On my small screen I cannot see the whole method, therefor the intentation is not that helpful anymore. \$\endgroup\$ – Tiberiu C. May 28 '15 at 9:03
  • \$\begingroup\$ And I have XML-doc comments, it shouldn't be hard working out how this method achieves this from there. \$\endgroup\$ – somebody May 28 '15 at 9:03
  • \$\begingroup\$ You can enable word wrap (that's what I do) \$\endgroup\$ – somebody May 28 '15 at 9:03
-1
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ I have xml-doc comments, which (I hope) fixes point 1. Also, doesn't purpose mean what something does? \$\endgroup\$ – somebody May 28 '15 at 8:49
  • \$\begingroup\$ It would be good if you add the xml-doc to the question then. I probably should have worded it clearer. By purpose I mean how it is intended to be used in your program. In simple cases there is no need to add comments what a method does as it is clear from the code. However it is not the case with the code you provided. \$\endgroup\$ – kostya May 28 '15 at 9:05
  • \$\begingroup\$ @Somebody If you follow tiberiu.corbu's advice and refactor the method into smaller methods, you can write self-describing code and omit comments to explain what your code is doing. The method name itself will be the description. \$\endgroup\$ – Torsten May 28 '15 at 9:19
  • \$\begingroup\$ Yeah, but I don't like methods that are only used once. (I have to admit though, much if this code is reused elsewhere) \$\endgroup\$ – somebody May 28 '15 at 9:22
  • \$\begingroup\$ Most of the time code is split into smaller methods to improve readability, not to reuse these methods. Make smaller methods private to show your intent that they are not meant to be reused. \$\endgroup\$ – kostya May 28 '15 at 9:25

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.