Project finished!

[santisica29]

Please let me know what I can improve for my next project and what I am doing well.

I appreciate feedback on my use of Separation of Concerns.

[nwdorian]

Hey @santisica29👋,

Great work on your Coding Tracker project submission! 🎉

I have performed a code review. Review/ignore any comments as you wish.

---

Please take care of issues marked with 🔴, as they block my approval. 🚫

Feel free to reach out if you have any questions or if you'd like to collaborate on any of these points.

---

Please edit your code to fix these issues then push the changes to your fork. The code will get updated in the PR and I will be notified to check again. ☺️

Best regards,
@nwdorian

[nwdorian]

🔴 Project doesn't build

🐞 Removing using System.Configuration; causes errors so I can't run the project locally. Codacy can have false-positives with certain code, if it suggests to delete something that is required for your project to work, you can ignore it and leave a note for reviewers.

💡A good rule of thumb is to always build and run your code before committing it to source control. This way you will make sure to have a backup of a working copy.

[nwdorian]

🟡 Base class

💡Nice to see that you tried to implement inheritance by using an abstract class. I do feel like these methods belong elsewhere, probably in Helpers.cs, since the CodingController.cs already does lots of other things apart from just displaying prompts.

[nwdorian]

🔴Separation of concerns

➡️ Extract the code for database interaction with SQL into a separate class - I suggest adding it into Data folder in a class called DatabaseMethods.cs or something similar. Then you can call these methods from the controller and reduce some lines of code. This approach also helps with reusability.

[santisica29]

Are you speaking about the GetReport and GetReportOfTotalAndAvg or more lines of code? I don't understand completely how to separate them from the codingController class could you give me an example?

[nwdorian]

For example AddSession method

after you get the input from user and create a new coding session I'd like to see a method call like this

var affectedRows = databaseMethods.CreateSession(session);

this method would be extracted into DatabaseMethods.cs class and would probably look something like this

public int CreateSession(CodingSession newSession)
{
    using var connection = new SqliteConnection(DatabaseInitializer.GetConnectionString());
    var sql =
            @$"INSERT INTO {DatabaseInitializer.GetDBName()} (startTime, endTime, duration)
               VALUES (@StartTime, @EndTime, @Duration)";

    var affectedRows = connection.Execute(sql, new { StartTime = newSession.StartTime, EndTime = newSession.EndTime, Duration = newSession.Duration.ToString() });
    
    return affectedRows;
}

To use it in the controller you would just create a new instance of the DatabaseMethods.cs class:

internal class CodingController
{
    private readonly DatabaseMethods databaseMethods = new();
    
    // ... rest of the code

[nwdorian]

@santisica29

I update the code in my example as I noticed some mistakes (sorry, it's hard to write without IDE support 😄 )

Let me know when you are done with all changes.

[santisica29]

Oh ok I think I get it now, I should move the parts that comunicate with the database to the DatabaseMethod class to keep the separation of concerns and then just call those methods from my CodingController class. Let me see if I can do it. Thanks for the example it's very clear!

[santisica29]

I have a quick question. What are the advantages and disadvantages of keeping the new DatabaseMethods class static, as opposed to creating an instance of it and using that? I made my DatabaseInitializer class static so I could call its methods directly, but I'm unsure whether this is considered a bad practice.

[nwdorian]

In general we should not use static methods at all because they don't align with OOP principles. With that said, static classes and methods certainly have their place in the language, for example when defining extension methods. The way I personally approach it, is to make nothing static and only if the compiler gives me a hint that says something like "you can make this static" I consider it.

[nwdorian]

While we are on this refactoring topic, make sure to also move all other methods that have SQL in them to the DatabaseMethods class. There's a few of them at the bottom of the controller class that only interact with database so it would make sense to have them together with the ones you will exctract in the new class.

[santisica29]

Perfect, on it!

[nwdorian]

🟢 Enums

⭐ Good job on using enums instead of magic strings. Look into the [DisplayName] attribute and use a converter with Spectre.Console to show a human friendly version. e.g. Add Coding Session.

[santisica29]

Thanks for the info I ended up using [Description] instead since it look easier to understand and apply.

With the help of chatgpt end up creating a method that gets the description attribute of a enum and then i've used UseConverter to display that text instead of the default enum.ToString().

I think I've already used the UseConverter in a similar manner (showing the names of the author and name of the book in a previous project) but i forgot about it, it was a good refresher thanks!

[nwdorian]

🔴 Missing Requirement

🐞 As per this projects requirements, you are required to have your validation logic in a separate class Validation.cs (you will thank yourself when you get to the Unit Testing project).

[nwdorian]

🟠 Database in Repo

💡 You shouldn't be committing your database file to source control. Add it to the .gitignore.