1
\$\begingroup\$

I am new to c# and decided to write a classic todo app. The implementation of my application now is like this: there is a Task class that describes the task. The List class acts simultaneously as a storage and has methods for creating, deleting, and displaying tasks. How good is this approach, how can you do better (if possible)? I would like a full code review.

There is Program.cs:

using System;

namespace ToDo
{
    class Program
    {
        static void Main(string[] args)
        {
            List list = new List();
            list.AddTask("Task №1");
            list.AddTask("Task №2");
            list.AddTask("Task №3");
            list.AddTask("Task №4");
            list.AddTask("Task №5");
            list.WriteTasks();
            list.RemoveTask(2);
            list.WriteTasks();
        }
    }
}

List.cs:

using System;
using System.Collections.Generic;
using System.Text;

namespace ToDo
{
    class List
    {
        private List<Task> tasks = new List<Task>();
        public void AddTask(string body)
        {
            tasks.Add(new Task() { Body = body });
        }
        public void WriteTasks()
        {
            for (int i = 0; i < tasks.Count; i++)
            {
                Console.WriteLine($"[{i+1}]: {tasks[i].Body}");
            }

            Console.ReadKey();
        }
        public void RemoveTask(int number)
        {
            for (int i = 0; i < tasks.Count; i++)
            {
                if (number == i + 1)
                {
                    tasks.RemoveAt(i);
                }
            }
        }
    }
}

Task.cs:

using System;
using System.Collections.Generic;
using System.Text;

namespace ToDo
{
    class Task
    {
        public string Body { get; set; }
    }
}
\$\endgroup\$
2
\$\begingroup\$

As a very basic application this is fine, however, if the program becomes more complicated and you want to add a data base and asynchronous programming you will run into conflicts with some of the class names. For instance the Task class is supplied by the C# library for asynchronous programming.

If I was going to implement a task list application I would have a few more properties for the task class, such as:

  • Priority
  • Status (not started, started, in progress, completed).
  • Due date
  • Name
  • Description

These are the minimum I would use, some additional properties might be

  • Assigned to
  • Assigned by

if this was a multi-user task list.

\$\endgroup\$
0
1
\$\begingroup\$

I agree with what pacmaninbw says, just a couple more things:

For a task object the minimum I'd have is an Id property (unique, make it a private set and initialize it in the constructor), Name (doesn't have to be unique but might be better from a user perspective), and Description (doesn't need to be unique). If you end up storing this in a database you'll probably index on the Id property.

If you end up adding more properties to the task I'd recommend overriding the ToString() method to print out a more detailed description of the object in your WriteTask method.

I see there's an emphasis on the index of the task (WriteTasks prints out the index of the task, RemoveTask requires an index to remove the task). A user is not going to care about where in the list a task is stored - for example they will most likely want to delete by name, id, etc.

WriteTasks can be simplified to

tasks.ForEach(x => Console.WriteLine(x.Body));

and RemoveTask can be simplified to something like the below, no need for the iteration.

if (number <= list.Count()) {
    list.RemoveAt(number - 1);
}
\$\endgroup\$

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.