Здравейте,

решението ми е валидно, но може ли помощ за подобряване на четимостта му? Имам ужасно много повтарящ се код, но за първи път не успявам да го екстрактна в методи. Предполагам, че това се дължи на наличието на "return" и "break" в кода. Успях само четенето на матрицата и принтирането на резултата да ги изнеса в методи, но за жалост само тях. Осъзнавам, че в такова състояние не мога да си кача решението в github.

https://pastebin.com/a6w0vHcL

I just want you to know that I’m deeply grateful for your amazing post! Thanks for the kindness in sharing your expertise to us. visit our website amazon.com/mytv
 

You just had to remove the break; command in the individual movement validations and then the automatic method-extractor was working again (taking care of all the needed variables for the new methods). Hope this helps.

Best,

using System;
using System.Linq;

namespace Miner
{
    class Program
    {
        static void Main(string[] args)
        {
            int rows = int.Parse(Console.ReadLine());
            int cols = rows;
            string[] commandArray = Console.ReadLine().Split().ToArray();
            char[,] matrix = new char[rows, cols];
            ReadTheMatrix(rows, cols, matrix);

            int rowStart = 0;
            int colStart = 0;
            bool foundStart = false;
            for (int row = 0; row < rows; row++) // validate the matrix
            {
                for (int col = 0; col < cols; col++)
                {
                    if (matrix[row, col] != '*'
                        && matrix[row, col] != 'e'
                        && matrix[row, col] != 'c'
                        && matrix[row, col] != 's')
                    {
                        return;
                    }

                    else if (matrix[row, col] == 's' && foundStart == false)
                    {
                        rowStart = row;
                        colStart = col;
                        foundStart = true;
                    }

                    else if (foundStart == true && matrix[row, col] == 's')
                    {
                        return;
                    }
                }
            }

            char start = matrix[rowStart, colStart];
            int currentRow = 0;
            int currentCol = 0;
            bool theEndOfTheRoad = false;
            for (int i = 0; i < commandArray.Length; i++)
            {
                string direction = commandArray[i];

                if (direction == "up") // command is Up
                {
                    if (rowStart - 1 >= 0)
                    {
                        MoveUp(matrix, ref rowStart, ref colStart, out currentRow, out currentCol, ref theEndOfTheRoad);
                    }
                }

                else if (direction == "down") // command is Down
                {
                    if (rowStart + 1 <= rows - 1)
                    {
                        MoveDown(matrix, ref rowStart, ref colStart, out currentRow, out currentCol, ref theEndOfTheRoad);
                    }
                }

                else if (direction == "left") // command is Left
                {
                    if (colStart - 1 >= 0)
                    {
                        MoveLeft(matrix, ref rowStart, ref colStart, out currentRow, out currentCol, ref theEndOfTheRoad);
                    }
                }

                else if (direction == "right") // command is Right
                {
                    if (colStart + 1 < cols)
                    {
                        MoveRight(matrix, ref rowStart, ref colStart, out currentRow, out currentCol, ref theEndOfTheRoad);
                    }
                }

                if (theEndOfTheRoad)
                {
                    break;
                }
            }

            PrintTheResult(matrix, currentRow, currentCol, theEndOfTheRoad);
        }

        private static bool CheckRoad(char[,] matrix, int currentRow, int currentCol, bool theEndOfTheRoad)
        {
            if (matrix[currentRow, currentCol] == 'c')
            {
                matrix[currentRow, currentCol] = '*';
            }

            else if (matrix[currentRow, currentCol] == 'e')
            {
                theEndOfTheRoad = true;
                //break;
            }

            return theEndOfTheRoad;
        }

        private static void MoveUp(char[,] matrix, ref int rowStart, ref int colStart, out int currentRow, out int currentCol, ref bool theEndOfTheRoad)
        {
            currentRow = rowStart - 1;
            currentCol = colStart;

            theEndOfTheRoad = CheckRoad(matrix, currentRow, currentCol, theEndOfTheRoad);

            rowStart = currentRow;
            colStart = currentCol;
        }


        private static void MoveDown(char[,] matrix, ref int rowStart, ref int colStart, out int currentRow, out int currentCol, ref bool theEndOfTheRoad)
        {
            currentRow = rowStart + 1;
            currentCol = colStart;

            theEndOfTheRoad = CheckRoad(matrix, currentRow, currentCol, theEndOfTheRoad);

            rowStart = currentRow;
            colStart = currentCol;
        }

        private static void MoveRight(char[,] matrix, ref int rowStart, ref int colStart, out int currentRow, out int currentCol, ref bool theEndOfTheRoad)
        {
            currentRow = rowStart;
            currentCol = colStart + 1;

            theEndOfTheRoad = CheckRoad(matrix, currentRow, currentCol, theEndOfTheRoad);

            rowStart = currentRow;
            colStart = currentCol;
        }

        private static void MoveLeft(char[,] matrix, ref int rowStart, ref int colStart, out int currentRow, out int currentCol, ref bool theEndOfTheRoad)
        {
            currentRow = rowStart;
            currentCol = colStart - 1;

            theEndOfTheRoad = CheckRoad(matrix, currentRow, currentCol, theEndOfTheRoad);

            rowStart = currentRow;
            colStart = currentCol;
        }

        private static void PrintTheResult(char[,] matrix, int currentRow, int currentCol, bool theEndOfTheRoad)
        {
            if (theEndOfTheRoad == true)
            {
                Console.WriteLine($"Game over! ({currentRow}, {currentCol})");
            }

            else
            {
                int countCoals = 0;
                foreach (var cell in matrix)
                {
                    if (cell == 'c')
                    {
                        countCoals++;
                    }
                }

                if (countCoals == 0)
                {
                    Console.WriteLine($"You collected all coals! ({currentRow}, {currentCol})");
                }

                else if (countCoals > 0)
                {
                    Console.WriteLine($"{countCoals} coals left. ({currentRow}, {currentCol})");
                }
            }
        }

        private static void ReadTheMatrix(int rows, int cols, char[,] matrix)
        {
            for (int row = 0; row < rows; row++)
            {
                char[] currentLine = Console.ReadLine().Split().Select(char.Parse).ToArray();
                for (int col = 0; col < cols; col++)
                {
                    matrix[row, col] = currentLine[col];
                }
            }
        }
    }
}

@Axiomatik,

thanks again. 

Yes, this is all that I want for this exercise- to extract every direction in own method.

And next time I will do my best to extract only the "break" in other method and then try to refactor the code.

All the best!

@Elena123456

Изнасянето на повтаряща се логика в сходни по своята същност методи не е решение на проблема.

Това е вариант на кода, при който повторяемостта е избегната без да се създават нови методи (опитал съм се да запазя структурата на решението):

using System;
using System.Linq;

namespace Miner
{
    internal static class Program
    {
        private static void Main()
        {
            var matrixSize = int.Parse(Console.ReadLine());
            var commands = Console.ReadLine().Split().ToArray();
            var matrix = ReadTheMatrix(matrixSize);

            var currentRow = 0;
            var currentCol = 0;
            for (var row = 0; row < matrixSize; row++)
            {
                for (var col = 0; col < matrixSize; col++)
                {
                    if (matrix[row, col] != 's') continue;
                    currentRow = row;
                    currentCol = col;
                }
            }

            var theEndOfTheRoad = false;
            foreach (var command in commands)
            {
                var nextRow = currentRow;
                var nextCol = currentCol;
                switch (command)
                {
                    case "up":
                        nextRow--;
                        break;
                    case "down":
                        nextRow++;
                        break;
                    case "left":
                        nextCol--;
                        break;
                    case "right":
                        nextCol++;
                        break;
                }

                if (!IsInBounds(nextRow, nextCol, matrixSize)) continue;

                currentRow = nextRow;
                currentCol = nextCol;
                
                if (matrix[currentRow, currentCol] == 'c')
                {
                    matrix[currentRow, currentCol] = '*';
                }
                else if (matrix[currentRow, currentCol] == 'e')
                {
                    theEndOfTheRoad = true;
                    break;
                }
            }

            var coalsLeft = CountCoalsInMatrix(matrix);
            PrintTheResult(coalsLeft, currentRow, currentCol, theEndOfTheRoad);
        }

        private static bool IsInBounds(int row, int col, int matrixSize)
        {
            return row >= 0 && row < matrixSize && col >= 0 && col < matrixSize;
        }

        private static void PrintTheResult(int coalsLeft, int lastRow, int lastCol, bool theEndOfTheRoad)
        {
            if (theEndOfTheRoad)
            {
                Console.WriteLine($"Game over! ({lastRow}, {lastCol})");
                return;
            }

            Console.WriteLine(coalsLeft == 0
                ? $"You collected all coals! ({lastRow}, {lastCol})"
                : $"{coalsLeft} coals left. ({lastRow}, {lastCol})");
        }

        private static int CountCoalsInMatrix(char[,] matrix)
        {
            return matrix.Cast<char>().Count(cell => cell == 'c');
        }

        private static char[,] ReadTheMatrix(int matrixSize)
        {
            var matrix = new char[matrixSize, matrixSize];
            for (var row = 0; row < matrixSize; row++)
            {
                var currentLine = Console.ReadLine().Split().Select(char.Parse).ToArray();
                for (var col = 0; col < matrixSize; col++)
                {
                    matrix[row, col] = currentLine[col];
                }
            }

            return matrix;
        }
    }
}

За сравнение ето и едно решение с помощен клас.

@MartinBG, благодаря за решенията. Отбелязвам, че първото Ви решение е доста приятно за четене с последователна логика. Може ли да попитам защо не е решение подобряването на четимостта чрез изнасянето на повтаряща се логика в сходни методи? Много ли влияе на пърформанса?  И щом не е решение, това означава ли, че винаги ако имаме сходни методи, то трябва да се опитаме да пренапишем задачата с нова логика?

@Elena123456

Може ли да попитам защо не е решение подобряването на четимостта чрез изнасянето на повтаряща се логика в сходни методи? Много ли влияе на пърформанса?  И щом не е решение, това означава ли, че винаги ако имаме сходни методи, то трябва да се опитаме да пренапишем задачата с нова логика?

В случая визирах решението, при което бяха създадени 4 метода с по 5 реда всеки, като само един от тези редове е различен, т.е. имаме 80% повтаряемост на кода в методите. Тези 4 метода спокойно могат да бъдат заменени от един единствен, като разликата се подава през аргументите:

        private static void Move(char[,] matrix, ref int rowStart, ref int colStart, int currentRow, int currentCol, ref bool theEndOfTheRoad)
        {
            theEndOfTheRoad = CheckRoad(matrix, currentRow, currentCol, theEndOfTheRoad);

            rowStart = currentRow;
            colStart = currentCol;
        }


//// извикване така:



                if (direction == "up") // command is Up
                {
                    if (rowStart - 1 >= 0)
                    {
                        currentRow = rowStart - 1;
                        currentCol = colStart;
                        Move(matrix, ref rowStart, ref colStart, currentRow, currentCol, ref theEndOfTheRoad);
                    }
                }

Но проблемите с този метод са много по-сериозни: има твърде много параметри, промeня стейта на външни за него обекти, върши повече от едно нещо.

На практика, методи като този са по-вредни за четимостта и качеството на кода, отколкото всичко да е в един метод, защото прикриват страничните си ефекти и дебъгването ще е кошмарно.

При писането на методи/функции винаги трябва да се стремим към създаването на т.н. Pure Functions. Разбира се, това не винаги е възможно и понякога изисква повече обмисляне, но е цел, която трябва да бъде преследвана и умение, което трябва да бъде усвоено.

Относно пърформанса - това е последното нещо за което трябва да се мисли при създаването на програми. Функционалнстта, надежността и четимостта са най-важни.

Изнасянето на част от кода в метод, който да бъде извикан на няма да доведе до забавяне на приложението. Не мисля, че ще има осезаемо забавяне дори и при XXXXXX извиквния в секунда, но правилният подход е първо програмата да се напише коректно и четимо, a при съмнения за проблеми с производителността, да се тества с profiler, с чиято помощ ще се открие къде точно е това забавяне. Едва след това трябва да се мисли как да се подобри производителността на кода, отговорен за забавянето.

Благодаря, че ми обърнахте внимание за качеството на кода. Ясно осъзнавам, че при работа в екип добрата четимост е от първостепенно значение. Ще се опитам да пренапиша решението на тази задача още днес, така че да е с по-добра четимост и ще го покажа отново. Извинявайте, че Ви отнемам толкова много от времето.

Поздрави!

Успях да подобря четимостта на кода си с решаването на задачата напълно наново, без да използвам старите решения. Все още не мога да повярвам, че се отървах от близо 100 реда излишен код.   Отново много благодаря за това упражнение, споделения код и за акцента върху качеството. За мен всичко беше много полезно, но си беше и предизвикателство. Много  моля, който има време да погледне за тест номер 6 и 7- RunTime Error и на двата тест, и 80/100 в Judge, като за момент си помислих, че ще ми даде 0 точки.

using System;
using System.Linq;
namespace Miner
{
    class Program
    {
        static void Main(string[] args)
        {
            int rows = int.Parse(Console.ReadLine());
            int cols = rows;
            string[] commandArray = Console.ReadLine().Split().ToArray();
            char[,] matrix = new char[rows, cols];
            int rowStart = 0;
            int colStart = 0;
            for (int row = 0; row < rows; row++)
            {
                char[] currentLine = Console.ReadLine().Split().Select(char.Parse).ToArray();
                for (int col = 0; col < cols; col++)
                {
                    matrix[row, col] = currentLine[col];
                    if (matrix[row, col] == 's')
                    {
                        rowStart = row;
                        colStart = col;
                    }
                }
            }
            int nextRow = 0;
            int nextCol = 0;
            int countCollectedCoals = 0;
            int countTotalCoals = 0;
            countTotalCoals = CheckTotalCoals(matrix, countTotalCoals);
            for (int i = 0; i < commandArray.Length; i++)
            {
                string command = commandArray[i];
                switch (command)
                {
                    case "up":
                        nextRow = rowStart - 1;
                        nextCol = colStart;
                        break;
                    case "down":
                        nextRow = rowStart + 1;
                        nextCol = colStart;
                        break;
                    case "left":
                        nextRow = rowStart;
                        nextCol = colStart - 1;
                        break;
                    case "right":
                        nextRow = rowStart;
                        nextCol = colStart + 1;
                        break;
                }
                if (nextRow >= 0 && nextCol < cols)
                {
                    if (matrix[nextRow, nextCol] == 'e')
                    {
                        Console.WriteLine($"Game over! ({nextRow}, {nextCol})");
                        return;
                    }
                    else if (matrix[nextRow, nextCol] == 'c')
                    {
                        countCollectedCoals++;
                        matrix[nextRow, nextCol] = '*';
                    }
                    rowStart = nextRow;
                    colStart = nextCol;
                }
            }
            if (matrix.Cast<char>().Contains('c') == false)
            {
                Console.WriteLine($"You collected all coals! ({rowStart}, {colStart})");
            }
            else
            {
                Console.WriteLine($"{countTotalCoals - countCollectedCoals} coals left. ({rowStart}, {colStart})");
            }
        }

        private static int CheckTotalCoals(char[,] matrix, int countTotalCoals)
        {
            foreach (char symbol in matrix)
            {
                if (symbol == 'c')
                {
                    countTotalCoals++;
                }
            }
            return countTotalCoals;
        }
    }
}

@Elena123456  

Успях да подобря четимостта на кода си с решаването на задачата напълно наново, без да използвам старите решения. Все още не мога да повярвам, че се отървах от близо 100 реда излишен код.   Отново много благодаря за това упражнение, споделения код и за акцента върху качеството. За мен всичко беше много полезно, но си беше и предизвикателство. Много  моля, който има време да погледне за тест номер 6 и 7- RunTime Error и на двата тест, и 80/100 в Judge, като за момент си помислих, че ще ми даде 0 точки.

Проблемът с тестове 6 и 7 най-вероятно е защото при някоя команда се излиза от матрицата заради непълна проверка:

if (nextRow >= 0 && nextCol < cols) // не проверява за nextRow < rows и nextCol >= 0

Оправеното решение, като си позволих и малко  рефакторинг:

using System;
using System.Linq;

namespace Miner
{
    class Program
    {
        static void Main(string[] args)
        {
            int rows = int.Parse(Console.ReadLine());
            int cols = rows;
            string[] commandArray = Console.ReadLine().Split().ToArray();
            char[,] matrix = new char[rows, cols];
            int rowStart = 0;
            int colStart = 0;
            for (int row = 0; row < rows; row++)
            {
                char[] currentLine = Console.ReadLine().Split().Select(char.Parse).ToArray();
                for (int col = 0; col < cols; col++)
                {
                    matrix[row, col] = currentLine[col];
                    if (matrix[row, col] == 's')
                    {
                        rowStart = row;
                        colStart = col;
                    }
                }
            }

            foreach (string command in commandArray)
            {
                int nextRow = rowStart;
                int nextCol = colStart;

                switch (command)
                {
                    case "up":
                        nextRow--;
                        break;
                    case "down":
                        nextRow++;
                        break;
                    case "left":
                        nextCol--;
                        break;
                    case "right":
                        nextCol++;
                        break;
                }

                if (nextRow < 0 || nextRow >= rows || nextCol < 0 || nextCol >= cols) continue;

                rowStart = nextRow;
                colStart = nextCol;

                switch (matrix[nextRow, nextCol])
                {
                    case 'e':
                        Console.WriteLine($"Game over! ({nextRow}, {nextCol})");
                        return;
                    case 'c':
                        matrix[nextRow, nextCol] = '*';
                        break;
                }
            }

            int countCoalsLeft = matrix.Cast<char>().Count(symbol => symbol == 'c');

            Console.WriteLine(countCoalsLeft == 0
                ? $"You collected all coals! ({rowStart}, {colStart})"
                : $"{countCoalsLeft} coals left. ({rowStart}, {colStart})");
        }
    }
}

Няколко съвета:

• Декларирайте си променливите там, където са необходими и по възможност ги инициализирайте на момента. Например nextRow и nextCol няма причина да съществуват извън тялото на цикъла. Това подобрява четимостта и намалява рисковете от бъгове (например невалидна стойност, останала от предходна итерация).
• Използвайте foreach вместо for цикли винаги, когато е възможно (т.е. не ви трябва индекса) - по кратко и ясно е
• Използвайте LINQ вместо цикли когато е възможно (на практика това е почти винаги) - още по-кратко и минимизира рисковете от бъгове в логиката на кода ни
• switch е по-четим от if/else, когато кодът за всеки case е до 2-3 реда
• има случаи, когато използването на тернанрен оператор подобрява четимоста в сравнение с if/else - например за отпечатването на резултата
• Силно препоръчвам да пробвате Rider като IDE за работа със C# - изключително мощен редактор с много полезни функционалности и подсказки по кода. На пракика е като персонален ментор и може много да се научи за C#, докато се работи с него.

 @MartinBG

Приемам забележката за използването на foreach вместо for,  ако не са ни необходими индексите, както и че няма нужда да декларирам текущия ред и колона извън тялото на foreach цикъла.

Найстина печатането накрая е доста по-четимо без for и else, а с ? и : .

Ако накрая чрез LINQ ще намеря останалото количесто от coals, то няма нужда от цял метод за намирането още в началото на общото им количесто.

Ще си позволя да отбележа, че само тук:

 if (nextRow < 0 || nextRow >= rows || nextCol < 0 || nextCol >= cols) continue;

си предпочитам положителния case- ако реда и колоната са в границите да вляза в проверките за пътя- дали е  'е' или 'с'.

И предлагам да се замени nextRow с currentRow и nextCol с currentCol. След като  играча стъпи на текущата клетка, тя вече става стартова и навсякъде  в проверките да ни фигурира вече rowStart и colStart, както впрочем си е и при печатането накрая.

                rowStart = currentRow;
                colStart = currentCol;

                switch (matrix[ rowStart, colStart])

                {
                    case 'e':

                        Console.WriteLine($"Game over! ({rowStart}, {colStart})");

                        return;
                    case 'c':

                        matrix[rowStart, colStart] = '*';

                        break;
                }

И финалната версия на кода, която ще си кача в github ще е това- https://pastebin.com/a6w0vHcL  От близо 200 реда код, вече е на 75. И благодаря за съветите и отделеното време.

А колкото за Rider,  попаднах на интересна документация- https://www.jetbrains.com/rider/compare/rider-vs-visual-studio/ , която сравнява VS с Rider и с VS с ReSharper. Виждам, че Rider има  quick-fixes  и за SQL и SQL editing tools inside string literals in C#, VB.NET, JavaScript, TypeScript, etc. И че Rider поддържа това- Visualize, compare, revert changes in the editor.

Но Rider няма regular expression validator, както и че било малко по-трудно навигирането в Solution-" Object Browser gives me this with one click. In Rider I need to navigate in Solution tree and find it. "

Минал е само месец, откакто се наслаждавам на преминаване си от MonoDevelop към VS и найстина много добре трябва да обмисля преминаването към Rider. Особено като се има предвид, че няма безплатна версия, но ако Rider е по- юзър френдли, по-бърз и би могъл да ми помогне при SQL заявките, то може би найстина е по-удачния вариант.

Поздрави!

Ели