Update todo/24 update todo #25

18-F-cali · 2021-08-12T17:14:21Z

closes #24

2. If DueDate is not given we show empty in ouput.

Refactored Validate Commands.

…ther-Discord/EasyList into UpdateTodo/24-UpdateTodo

valincius · 2021-08-13T19:43:53Z

src/EasyList/Program.cs

 {
    class Program
    {
+        public static ITodoService _todoService = Factory.CreateTodoServiceDB();


Rename to something like TodoService and make it a readonly property.

valincius · 2021-08-13T19:48:30Z

src/EasyList/Validate.cs

+using System.Linq;
+
+public static class Validate
+{


I wouldn't make this static. You should create a new instance of the validator each time you want to run it.

I not able to understand the reasoning.

The Validate class validates one object. It does so by having a field called errors. Now the errors field is not for every validated item but rather for the one being validated right now. So you use one Validator to validate an item, then you use another validator for another. Because every validator that runs has different errors due to another object meaning there should never be just one list of errors.

valincius · 2021-08-13T19:49:18Z

src/EasyList/Validate.cs

+
+    public static bool DueDate(string? input)
+    {
+        if (DateTimeOffset.TryParse(input, out DateTimeOffset tempDate))


This will return true if I put in an invalid date

chrisK00 · 2021-08-15T08:49:54Z

src/EasyList/Validate.cs

+
+public static class Validate
+{
+    static List<string> errors = new List<string>();


missing access modifier and underscore prefix

Anequit · 2021-08-19T14:18:03Z

src/EasyList/ParseAdd.cs

            string label = GetData("add", args, positions) ?? string.Empty;
            string description = GetData("-d", args, positions) ?? string.Empty;
            string dueDate = GetData("-t", args, positions) ?? string.Empty;
-            string priority = GetData("-p", args, positions) ?? "${TodoPriority.Low}";


How did this get committed? Was the code not debugged?

Yup, Maybe Expecting only a silly string change I didn't check after this commit.

Anequit · 2021-08-19T14:19:27Z

src/EasyList/Program.cs

    class Program
    {
-        public static ITodoService _todoService = Factory.CreateTodoServiceDB();
+        public static ITodoService TodoService => Factory.CreateTodoServiceDB();


Correct naming convention.

This is a field and not a Property so _todoService ? @Anequit

It is actually a property. This is shorthand for a read-only property.

Why is the service a static instance and inside program.cs if its not being used here?

Why is the service a static instance and inside program.cs if its not being used here?

So that is becomes very easy to identify where we declared it at one place.

It is actually a property. This is shorthand for a read-only property.

@valincius I meant before changing it. As you suggested last time i changed it to a read only property here

Why is the service a static instance and inside program.cs if its not being used here?

So that is becomes very easy to identify where we declared it at one place.

What do you mean with "identify"? I have mentioned this before. You have set up a dependency inversion system with the factories, i would stick with that. You should first of all use encapsulation which is part of the OOP principles. Secondly there is no point in making a class non-static just to make it static afterwards. Also always try to keep Program.cs as clean as possible, it is only supposed to be used as the starting point of your application.

Makes sense

Anequit · 2021-08-19T14:23:51Z

src/EasyList/TodoMenu.cs

-                        break;
+                        {
+                            Console.WriteLine("Exiting...");
+                            Environment.Exit(Environment.ExitCode = 0);


Environment.Exit(0); does the same thing

Anequit · 2021-08-19T14:26:54Z

src/EasyList/TodoMenu.cs

            {
                var action = Prompt.Select<TODOMENU>("Welcome to EasyList!");
                //Refactor this such that adding new should not modify this input layer
                switch (action)


And if statement would be a bit cleaner. Especially when you have logic in every case.

What do you think about this ? @Anequit

Anequit · 2021-08-19T14:27:37Z

src/EasyList/TodoServiceDB.cs

        public void UpdateTodo(Todo todo, TodoUpdate command)
        {
+            var _todoValidate = new Validate();
            switch (command)


If statement would be better here too.

Anequit · 2021-08-19T14:28:29Z

src/EasyList/TodoServiceDB.cs

-                                todo.Label = newLabel!;
-                            }
+                            todo.Label = newLabel;
+                            _todoRepository.UpdateTodo(todo);


This line is being repeated in every case.

Anequit

I think most of the switch statements would work better as if statements.

chrisK00 · 2021-08-19T17:26:00Z

disable
enable?

#nullable enable

src/EasyList/DataModels/Todo.cs (Line 47)

chrisK00

hmu on dc if you want and we can go through the stuff together

chrisK00 · 2021-08-19T17:18:33Z

src/EasyList/Program.cs

    class Program
    {
-        public static ITodoService _todoService = Factory.CreateTodoServiceDB();
+        public static ITodoService TodoService => Factory.CreateTodoServiceDB();


Why is the service a static instance and inside program.cs if its not being used here?

chrisK00 · 2021-08-19T17:30:23Z

src/EasyList/TodoMenu.cs

        {
-            ITodoService _todoService = Factory.CreateTodoServiceDB();
-            while(true)
+            var _todoValidate = new Validate();


The class is Todo specific and should be called TodoValidator, i think a name for the field should be the same name as the class

chrisK00 · 2021-08-19T17:32:20Z

src/EasyList/TodoMenu.cs

+                                    DueDate = DateTimeOffset.TryParse(parsedAdd["duedate"], out DateTimeOffset tempDate) ? tempDate : null,
+                                    Priority = Enum.Parse<TodoPriority>(parsedAdd["priority"])
+                                };
+                                Program.TodoService.AddTodo(newTodo);


TodoService should be in this class instead

chrisK00 · 2021-08-19T17:34:41Z

src/EasyList/TodoServiceDB.cs

+
+                case TodoUpdate.Priority:
+                    {
+                        todo.Priority = Prompt.Select<TodoPriority>("Select new Priority."); ;


service should not interact with the user, thats a job for the menu

chrisK00 · 2021-08-19T17:41:32Z

src/EasyList/Utility.cs

+{
+    public static class Utility
+    {
+        public static IEnumerable<int> ToIntIds(this string[] input)


This is not bound to ids so could use a more generic name

chrisK00 · 2021-08-19T17:44:29Z

src/EasyList/Validate.cs

+            }
+            Console.Clear();
+            var choice = Prompt.Confirm("Continue with Valid Ids ? ");
+            Thread.Sleep(1500);


dont bully my cpu :(

chrisK00 · 2021-08-19T17:45:20Z

src/EasyList/Validate.cs

+{
+    public class Validate
+    { 
+        private static bool IsErrorFree(IEnumerable<string> errors)


Method name implies it checks if the todo is error free but thats not what it does

What do you suggest ?

First of all this method should be "PrintErrors" because the if errors.any can be checked outside the code. This method does two things, it checks if there are any errors and then prints all the errors at the same time it returns a boolean. If you do something like if(todoValidator.IsValid){todoValidator.PrintErrors} it would make a bit more sense and also follow principles such as SRP in this case the method should only have one responsibility, one job.

valincius · 2021-09-09T14:25:24Z

Any update on this one? Looks like there haven't been any commits in 21 days

18-F-cali added 4 commits August 12, 2021 11:02

1. Changed time fields to store as UTC.

c872dfb

2. If DueDate is not given we show empty in ouput.

Added "Quit" option to Input Menu.

5d7c6db

Changed output Table Format to MarkDown.

9f9589c

Implemented Update Commands.

b75304d

Refactored Validate Commands.

18-F-cali self-assigned this Aug 12, 2021

18-F-cali requested review from Anequit and chrisK00 and removed request for Anequit August 12, 2021 17:15

chrisK00 requested review from valincius and removed request for chrisK00 August 12, 2021 17:41

18-F-cali requested review from Anequit and chrisK00 August 12, 2021 17:53

18-F-cali and others added 7 commits August 12, 2021 23:35

Implemented Update Commands.

2f07ce3

Refactored Validate Commands.

Implmented Utility

3d338b9

Merge branch 'master' into UpdateTodo/24-UpdateTodo

876b1fe

Merge branch 'master' into UpdateTodo/24-UpdateTodo

d851887

Merge branch 'UpdateTodo/24-UpdateTodo' of https://github.com/Code2Ge…

88d0cac

…ther-Discord/EasyList into UpdateTodo/24-UpdateTodo

Delete EasyList.Tests.csproj

479f610

Delete UnitTest1.cs

0f1c464

valincius suggested changes Aug 13, 2021

View reviewed changes

changed _todoService to TodoService property.

9cf2a9a

chrisK00 reviewed Aug 15, 2021

View reviewed changes

Updated Validation Logic

936a3c2

Anequit reviewed Aug 19, 2021

View reviewed changes

chrisK00 requested changes Aug 19, 2021

View reviewed changes

Update todo/24 update todo #25

Are you sure you want to change the base?

Update todo/24 update todo #25

Uh oh!

Conversation

18-F-cali commented Aug 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

18-F-cali Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anequit left a comment

Choose a reason for hiding this comment

Uh oh!

chrisK00 commented Aug 19, 2021

Uh oh!

chrisK00 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valincius commented Sep 9, 2021

Uh oh!

Reviewers

18-F-cali Aug 20, 2021 •

edited

Loading