Problem
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; }
}
}
Solution
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.
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);
}