Kiedy rozbijac metody na mniejsze czesci?

Mam problem z tym, ze nie wiem kiedy rozbic metode na mniejsze. Jakos tego nie 'widze'. Czesto robie cos na czuja a potem okazuje sie, ze w ksiazce jest inaczej. Czy jest jakis schemat, ktory mowilby kiedy powinno sie ja rozbijac?

Przyklad:
Kazdy gracz ma karty. Jesli ma grupe 4 kart o tej samej wartosci, to zabiera mu sie je i dodaje 1 punkt. Klasa Deck jest to klasa zestawu kart. Jest w niej metoda PullOutBooks() ktora sprawdza czy gracz ma grupe 4 kart. Jesli tak, to zabiera sie mu ja i zwraca ta grupe.

Sposob 1:

        public IEnumerable<Values> PullOutBooks() {
            List<Values> books = new List<Values>();

            for (int i = 0; i < 12; i++) {
                Values checkingValue = (Values)i;
                if (HasBook(checkingValue)) {
                    books.Add(checkingValue);
                    DealValues(checkingValue);
                }
            }
            return books;
        }
		
        public void DealValues(Values valueToDeal) {
            for (int j = cards.Count - 1; j >= 0; j--) {
                if (cards[j].Value == valueToDeal)
                    Deal(j);
            }
        }		

        public bool HasBook(Values value) {
            int amount = 0;
            for (int i = 0; i < cards.Count; i++) {
                if (cards[i].Value == value)
                    amount++;
            }
            return amount == 4;
        }

Metoda PullOutBooks jest dosc czytelna. Sprawdzanie i wyciaganie zostalo rozbite na osobne metody. Jednak w zadnym innym miejscu te metody nie sa uzywane. Czy jest wiec sens rozbijac sie?

Druga opcja: Jest to to samo co wyzej, tylko metody nie zostaly rozbite.

        internal IEnumerable<Values> PullOutBooks() {
            List<Values> books = new List<Values>();

            for (int i = 0; i < 12; i++) {
                Values checkingValue = (Values)i;
                int amount = 0;
                for (int j = 0; j < cards.Count; j++) {
                    if (cards[j].Value == checkingValue)
                        amount++;
                }
                if (amount == 4) {
                    books.Add(checkingValue);

                    for (int j = cards.Count - 1; j >= 0; j--) {
                        if (cards[j].Value == checkingValue)
                            Deal(j);
                    }
                }
            }
            return books;
        }
```	
		
Ktora wersja jest bardziej poprawna?

mETODE ROZBIJASZ NA MNIEJSZE CZĘŚCI KIEDY NIE SPEŁNIA ZASADY POJEDYNCZEJ ODPOWIEDZIALNOŚCI!

W tym przypadku ciężko mówić o lepszy gorszy. Jest kilka wyznaczników, pierwszy został wspomniany – metoda powinna robić jedną rzecz dostępną z zewnątrz klasy. Jeśli jakaś klasa zewnętrzna miałaby używać tych wyodrębnionych metod to na pewno jest to argument za. Jeśli te metody zostały wyodrębnione ot tak sobie to raczej nie, a jeśli już to na pewno powinny być private lub protected, a nie public. Co innego jeśli metoda robi się bardzo długa i zajmuje ponad 50 linii kodu, wówczas rzeczywiście wydzielenie nazwanych metod może być pomocne. W twoim przypadku metoda jest krótka, więc ciął bym tylko jeśli jedna z mniejszych ma być użyta gdzie indziej.
Poza tym jeśli potrafisz w metodzie wyodrębnić dwa niezależne od siebie kawałki kodu to warto je podzielić (to w sumie to co podał przedmówca).

Tutaj masz kilka słów na ten temat: https://refactoring.guru/extract-method

Generalnie:

• Dzielisz metodę jeżeli kod staje się trudny do zrozumienia. Krótsze metody łatwiej przeczytać i zrozumieć ich działanie.
• Wydzielasz z metody fragmenty kodu, które mogą być użyte w innym miejscu aplikacji.
• Wydzielasz z metody niezależne fragmenty kodu.

@Haskell: właściwie powiedział już wszystko. Ja dorzucę tylko, że jeśli nie jesteś pewien, czy powinieneś metodę wyodrębnić, to lepiej jest mieć więcej krótkich metod, niż mniej długich.

• metoda powinna wykonywać tylko jedną czynność (np. obliczyć jakieś działanie, a nie obliczyć działanie i wyświetlić je na ekranie)
• kod w kilku metodach nie powinien się powtarzać. Gwałci to boleśnie zasadę DRY - Don't Repeat Yourself. Powtarzający się kod (identyczny lub podobny) należy wyodrębnić do innej metody
• metoda staje się długa - tutaj musisz już sam ocenić na podstawie zdrowego rozsądku, czy długa metoda to taka, która ma 20 linijek, czy 40. Jeśli metoda się rozrasta, to jest szansa, że robi za dużo, czyli - patrz punkt 1.

Kolejnym ważnym przypadkiem do wyodrębnienia metody są warunki logiczne. Pisanie w if jakichś szlaczków jest nieczytelne, ale przeniesienie ich do metody, której nazwa opisuje sprawdzane zdarzenie jest bardzo pomocne.

Co do tematu, to drugi kod to jakieś totalne, nieczytelne spaghetti. Pierwszy jest zdecydowanie lepszy. A po ludzku byłoby jakoś tak:

internal IEnumerable<Values> PullOutBooks()
{
    var books = cards.GroupBy(c => c.Value).Where(g => g.Count() == 4).Select(g => g.Key).ToList();
    cards.RemoveAll(c => books.Contains(c.Value));
    return books;
}