2
\$\begingroup\$

I tried in this project to apply what I learned from OOP, using classes and properties, please advise for any best practice or any improvement.

Conclusion: I have a text file "ToDoList.txt" which has the following syntax Task,status for example: Shopping,0 cooking,1 pick up kids from school,2 do something,0

And I have two classes:

  • TaskData class which only has properties and it will return the status in text(Completed or not completed)
  • FilterData class which is going to filter the data according to the status

Main Program class:

namespace ExtractingDataFromTextFile
{
    class Program
    {
        static void Main()
        {
            //Save Results of text data and declaring the text file to 
            //Read Data from.

            TaskData textData = new TaskData();

            string path = "ToDoList.txt";

            //Create a string to read all data in file
            string[] allData = File.ReadAllLines(path);

            //Assign each data to TextData object properties
            foreach (var task in allData)
            {
                PrintData(textData, task);
            }
            FilterData filterData = new FilterData();

            //Filter to show Completed Tasks only
            string[] completedTasks = filterData.ShowCompleted(allData);
            Console.WriteLine("\nFilter only completed tasks:");
            foreach (var task in completedTasks)
            {
                PrintData(textData, task);   
            }

            //Filter to show uncompleted Taks only
            string[] unCompletedTasks = filterData.ShowUnCompleted(allData);
            Console.WriteLine("\nFilter only Un-completed tasks:");
            foreach (var task in unCompletedTasks)
            {
                PrintData(textData, task);
            }

            Console.ReadLine();
        }


        //This method will print data to the console
        private static void PrintData(TaskData textData, string task)
        {
            string[] lineData = task.Split(',');
            textData.Name = lineData[0];
            textData.Status = lineData[1];
            Console.WriteLine($"Task: {textData.Name}, Status:[{textData.Status}]");
        }
    }
}

TaskData class:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace ExtractingDataFromTextFile
{
    class TaskData
    {
        private string _status;
        public string Name { get; set; }
        public string Status
        {
            get {return _status; }
            //The setter will convert status from number to text
            //0 --> Not completed
            //1 --> Completed
            set
            {
                if (value == "0")
                {
                    _status = "Not completed";
                }
                else if(value =="1")
                {
                    _status = "Completed";
                }
                else
                {
                    _status = "Unknown";
                }
            }
        }
    }
}

FilterData class

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace ExtractingDataFromTextFile
{
    class FilterData
    {
        private List<string> _filterData = new List<string>();

        //This function will filter the list to show only completed tasks
        public string[] ShowCompleted(string[] allData)
        {
            _filterData = new List<string>(); //Recreate the list to delete old filter
            foreach (var line in allData)
            {
                string[] lineData = line.Split(',');
                if (lineData[1] == "1")
                {
                    //Completed task
                    _filterData.Add($"{lineData[0]},{lineData[1]}");
                }
            }
            return _filterData.ToArray();
        }

        //This function will filter the list to show only un-completed tasks
        public string[] ShowUnCompleted(string[] allData)
        {
            _filterData = new List<string>(); //Recreate the list to deleted old filter
            foreach (var line in allData)
            {
                string[] lineData = line.Split(',');
                if (lineData[1] == "0")
                {
                    //Un-Completed task
                    _filterData.Add($"{lineData[0]},{lineData[1]}");
                }
            }
            return _filterData.ToArray();
        }
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Please see What to do when someone answers. I have rolled back Rev 5 → 3. \$\endgroup\$
    – Mast
    May 14 '19 at 19:53
  • \$\begingroup\$ @Mast Thank you, I followed the link and answered my question. \$\endgroup\$
    – AMJ
    May 14 '19 at 20:03
2
\$\begingroup\$

In PrintData method you are assigning values to TaskData object but you don't seem to do anything with it besides just printing it out.

I would parse the file into a List at the beginning and then work with this collection in later code.

private static List<TaskData> ParseTaskData(string[] allData)
{
    var tasksData = new List<TaskData>();

    foreach (var task in allData)
    {
        string[] lineData = task.Split(',');

        tasksData.Add(new TaskData()
        {
            Name = lineData[0],
            // Before Enum: Status = lineData[1]
            Status = (Status)int.Parse(lineData[1].ToString())
        });
    };

    return tasksData;
}

Also your filter methods can then work as one-liners with lambda. I would be tempted here to split property Status into Status and StatusReadable. Make Status an Enum for ease of filtering.

public List<TaskData> ShowUnCompletedFromList(List<TaskData> allData)
{
    return allData.Where(w => w.Status == "Not completed").ToList();
}

Enum solution (I changed the ParseTaskData above):

internal enum Status
{
    NotCompleted = 0,
    Completed = 1
}

And TaskData then becomes:

internal class TaskData
{
    public string Name { get; set; }

    public Status Status { get; set; }

    public string StatusDescription
    {
        get
        {
            if (Status == Status.NotCompleted)
            {
                return "Not completed";
            }
            else if (Status == Status.Completed)
            {
                return "Completed";
            }
            else
            {
                return "Unknown";
            }
        }
    }
}

Filtering is now based on Enum and not magic strings:

public List<TaskData> ShowUnCompletedFromList(List<TaskData> allData)
{
    // With Magic string: return allData.Where(w => w.Status == "Not completed").ToList();
    return allData.Where(w => w.Status == Status.NotCompleted).ToList();
}

Try it out with Enum solution:

// Parse all tasks to List.
var tasksData = ParseTaskData(allData);

// Show items in List<>.
Console.WriteLine(">> All tasks:");

foreach (var task in tasksData)
{
    Console.WriteLine($"Task: {task.Name} is {task.StatusDescription}.");
}

Console.WriteLine(">> Uncompleted tasks:");
foreach (var uncompletedTask in filterData.ShowUnCompletedFromList(tasksData))
{
    Console.WriteLine($"Task: {uncompletedTask.Name} is {uncompletedTask.StatusDescription}.");
}

// Actually no need for a filter method, this one-liner does it the same as above.
Console.WriteLine(">> Uncompleted tasks short:");
filterData.ShowUnCompletedFromList(tasksData).ForEach(e => Console.WriteLine($"Task: {e.Name} is {e.StatusDescription}."));
\$\endgroup\$
8
  • \$\begingroup\$ Hi, thanks for reviewing, can your explain more about Make Status an Enum for ease of filtering how enum is easier to filter. \$\endgroup\$
    – AMJ
    May 13 '19 at 6:53
  • 1
    \$\begingroup\$ @AMJ I'm on my phone, I'll get back to you regarding first comment when I get to a PC. Regarding your second comment - yes, if you would need to print the data often or perhaps 'prettify' it etc. then go for a separate method. The Main method will be cleaner and you would promote code reusage (DRY). \$\endgroup\$
    – Iztoksson
    May 13 '19 at 10:41
  • 1
    \$\begingroup\$ @AMJ I changed to code to implement an Enum. Let me know if you have more questions. \$\endgroup\$
    – Iztoksson
    May 13 '19 at 17:57
  • 1
    \$\begingroup\$ Wow thanks a lot, using enum really was a game changer made the code easier to understand! Also I used Task.Name instead of Task[0] which look better as well. Is it possible to post my reviewed code here ? i dont know if the rules allows that. \$\endgroup\$
    – AMJ
    May 13 '19 at 19:21
  • 1
    \$\begingroup\$ @AMJ I'm not sure about the rules for posting modified code. If you keep the original question as-is and edit your solution below that, it would probably be ok. \$\endgroup\$
    – Iztoksson
    May 13 '19 at 20:08

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.