1
\$\begingroup\$

I wrote this basic CRUD app + simple report so I could practice talking to a db directly. I purposefully avoided repository pattern and Entity Framework and only used a library to show the coming data a bit more readable.

Having that in mind, I'd love to get some feedback, particularly in regards to bad practices that I should avoid in my professional career as developer. You can be brutal, I take criticism well. :)

I'll post just the code for my controller and the rest in this link to my repo:

Here's the link to my repo. https://github.com/cappuccinocodes/CodeTracker1

using System;
using System.Collections.Generic;
using System.Configuration;
using System.Globalization;
using ConsoleTableExt;
using Microsoft.Data.Sqlite;
using Microsoft.EntityFrameworkCore.Migrations.Operations;

namespace CodeTracker1
{
    internal static class CodingController
    {
        static readonly string connectionString = ConfigurationManager.AppSettings.Get("ConnectionString");
        internal static void GetUserCommand()
        {
            Console.WriteLine("\n\nMAIN MENU");
            Console.WriteLine("\nWhat would you like to do?");
            Console.WriteLine("\nType 0 to Close Application.");
            Console.WriteLine("Type 1 to View All Records.");
            Console.WriteLine("Type 2 to Insert Records.");
            Console.WriteLine("Type 3 to Delete Records.");
            Console.WriteLine("Type 4 to Update Records.");
            Console.WriteLine("Type 5 to Generate Reports.");

            string commandInput = Console.ReadLine();

            if (string.IsNullOrEmpty(commandInput)) {
                Console.WriteLine("\nInvalid Command. Please choose an option\n");
                GetUserCommand();
            }

            int command = Convert.ToInt32(commandInput);

            switch (command)
            {
                case 0:
                    Environment.Exit(0);
                    break;
                case 1:
                    Get();
                    break;
                case 2:
                    Post();
                    break;
                case 3:
                    Delete();
                    break;
                case 4:
                    Update();
                    break;
                case 5:
                    Reports.GetReportCommand();
                    break;
                default:
                    Console.WriteLine("\nInvalid Command. Please type a number from 0 to 5.\n");
                    GetUserCommand();
                    break;
            }
        }

        internal static void Post()
        {
            long date = GetDateInput();
            long duration = GetDurationInput();

            using (var connection = new SqliteConnection(connectionString))
            {
                connection.Open();
                var tableCmd = connection.CreateCommand();
                tableCmd.CommandText = $"INSERT INTO coding (date, duration) VALUES ({date}, {duration})";
                tableCmd.ExecuteNonQuery();
                connection.Close();
            }

            string inputDate = new DateTime(date).ToString("dd-MM-yy");
            string inputDuration = new DateTime(duration).ToString("HH:mm");

            Console.WriteLine($"\n\nYour time was logged. Date: {inputDate}; Duration: {inputDuration}.\n\n");

            GetUserCommand();
        }

        internal static void Delete()
        {
            Console.WriteLine("\n\nPlease type the Id of the record would like to delete. Type 0 to return to main menu.\n\n");

            string commandInput = Console.ReadLine();

            if (string.IsNullOrEmpty(commandInput))
            {
                Console.WriteLine("\nYou have to type an Id.\n");
                Delete();
            }

            var Id = Int32.Parse(Console.ReadLine());

            if (Id == 0) GetUserCommand();
          
            using (var connection = new SqliteConnection(connectionString))
            {
                connection.Open();
                var tableCmd = connection.CreateCommand();
                tableCmd.CommandText = $"DELETE from coding WHERE Id = '{Id}'";
                int rowCount = tableCmd.ExecuteNonQuery();

                while (rowCount == 0)
                {
                    Console.WriteLine($"\n\nRecord with Id {Id} doesn't exist. Try Again or type 0 to return to main menu. \n\n");
                    Id = Int32.Parse(Console.ReadLine());

                    if (Id == 0) GetUserCommand();

                    if (rowCount != 0) break;
                }
            }
            GetUserCommand();
        }

        internal static void Get()
        {
            using (var connection = new SqliteConnection(connectionString))
            {
                connection.Open();
                var tableCmd = connection.CreateCommand();
                tableCmd.CommandText = "SELECT * FROM coding";

                List<Coding> tableData = new List<Coding>();

                SqliteDataReader reader = tableCmd.ExecuteReader();

                if (reader.HasRows)
                {
                    while (reader.Read())
                    {
                        tableData.Add(
                        new Coding
                        {
                            Id = reader.GetInt32(0),
                            Date = new DateTime(reader.GetInt64(1)).ToShortDateString(),
                            Duration = new TimeSpan(reader.GetInt64(2)).ToString()
                        });
                    }
                }
                else
                {
                    Console.WriteLine("\n\nNo rows found.\n\n");
                }
                reader.Close();

                Console.WriteLine("\n\n");

                ConsoleTableBuilder
                    .From(tableData)
                    .ExportAndWriteLine();
                Console.WriteLine("\n\n");

                GetUserCommand();
            }
        }

        internal static void Update()
        {
            Console.WriteLine("\n\nPlease type Id of the record would like to update. Type 0 to return to main manu.\n\n");

            string commandInput = Console.ReadLine();

            if (string.IsNullOrEmpty(commandInput))
            {
                Console.WriteLine("\nYou have to type an Id.\n");
                Update();
            }

            var Id = Int32.Parse(commandInput);

            if (Id == 0) GetUserCommand();

            using (var connection = new SqliteConnection(connectionString))
            {
                connection.Open();

                var checkCmd = connection.CreateCommand();
                checkCmd.CommandText = $"SELECT EXISTS(SELECT 1 FROM coding WHERE Id = {Id})";
                int checkQuery = Convert.ToInt32(checkCmd.ExecuteScalar());

                if (checkQuery == 0)
                {
                    Console.WriteLine($"\n\nRecord with Id {Id} doesn't exist.\n\n");
                    GetUserCommand();
                }

                long date = GetDateInput();
                long duration = GetDurationInput();

                var tableCmd = connection.CreateCommand();
                tableCmd.CommandText = $"UPDATE coding SET date = {date}, duration = {duration} WHERE Id = {Id}";
                tableCmd.ExecuteNonQuery();

                string inputDate = new DateTime(date).ToString("dd-MM-yy");
                string inputDuration = new DateTime(duration).ToString("HH:mm");

                Console.WriteLine($"\n\nYour time was logged: date({inputDate}), duration({inputDuration}).\n\n");
                connection.Close();
            }

            GetUserCommand();
        }

        internal static long GetDateInput()
        {
            Console.WriteLine("\n\nPlease insert the date: (Format: dd-mm-yy). Type 0 to return to main manu.\n\n");
            DateTime result;

            string dateInput = Console.ReadLine();

            if (dateInput == "0") GetUserCommand();

            bool success = DateTime.TryParseExact(dateInput, "dd-MM-yy", new CultureInfo("en-US"), DateTimeStyles.None, out result);

            if (success)
            {
                var parsedDate = DateTime.ParseExact(dateInput, "dd-MM-yy", new CultureInfo("en-US"));
                long date = parsedDate.Ticks;
                return date;
            }

            Console.WriteLine("\n\nNot a valid date. Please insert the date with the format: dd-mm-yy.\n\n");
            return GetDateInput();
        }

        internal static long GetDurationInput()
        {
            Console.WriteLine("\n\nPlease insert the duration: (Format: hh:mm). Type 0 to return to main manu.\n\n");
            TimeSpan timeSpan;

            string durationInput = Console.ReadLine(); 

            if (durationInput == "0") GetUserCommand();
       
            bool success = TimeSpan.TryParseExact(durationInput, "h\\:mm", CultureInfo.InvariantCulture, out timeSpan);

            if (success)
            {
                var parsedDuration = TimeSpan.Parse(durationInput);
                long date = parsedDuration.Ticks;
                if (date < 0)
                {
                    Console.WriteLine("\n\nNegative Time Not allowed.\n\n");
                    GetDurationInput();
                }
                return date;
            }

            Console.WriteLine("\n\nNot a valid time.\n\n");
            return GetDurationInput();
        }
    }
}
\$\endgroup\$
1

1 Answer 1

1
\$\begingroup\$

Your code is very neat, and I see that you tried to take care of the basics like disposing of the connection. My main concern is the recursive call to GetUserCommand.

Do not use recursion where you could have a simple loop!

I would suggest that you change GetUserCommand to the following:

internal static void GetUserCommand()
{
    bool closeApp = false;
    while (closeApp == false)
    {
        Console.WriteLine("\n\nMAIN MENU");
        Console.WriteLine("\nWhat would you like to do?");
        Console.WriteLine("\nType 0 to Close Application.");
        Console.WriteLine("Type 1 to View All Records.");
        // ...
        Console.WriteLine("Type 5 to Generate Reports.");

        string commandInput = Console.ReadLine();
        if (string.IsNullOrEmpty(commandInput))
        {
            Console.WriteLine("\nInvalid Command. Please choose an option\n");
            continue;
        }

        int command = Convert.ToInt32(commandInput);
        switch (command)
        {
            case 0:
                closeApp = true;
                break;
            case 1:
                Get();
                break;
            // ...
            default:
                Console.WriteLine("\nInvalid Command. Please type a number from 0 to 5.\n");
                break;
        }
    }
}

In Get() you have a ticking time bomb:

internal static void Get()
{
    using (var connection = new SqliteConnection(connectionString))
    {
        connection.Open();
        ...
        GetUserCommand();
    }
}

This sort of thing has spoiled many good programmers' weekends/sanity. Here you never dispose of the connection and can eventually either run out of stack space or database handles / resources.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ @BCdotWEB, That is correct, the problem in Get() is the recursive call to GetUserCommand before using (var connection… is out of the scope. \$\endgroup\$
    – jdt
    Nov 1, 2021 at 13:30
  • \$\begingroup\$ @BCdotWEB, see what happens in this example: TIO \$\endgroup\$
    – jdt
    Nov 1, 2021 at 14:36
  • 1
    \$\begingroup\$ In this example TIO, the connection object is disposed of properly, but the unchecked recursive call still leads to a stack overflow. \$\endgroup\$
    – jdt
    Nov 1, 2021 at 14:53
  • \$\begingroup\$ Thanks a lot for your feedback! I'll have a closer look. \$\endgroup\$ Nov 4, 2021 at 9:06
  • 1
    \$\begingroup\$ Holy ghost! You have just changed my life. Haha. I finally understand the importance of while loops 🤣 \$\endgroup\$ Nov 5, 2021 at 2:58

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.