1
\$\begingroup\$

I am using Xamarin.Forms, Microcharts and SQLite.NET to create a mobile app. The SQLite.NET database stores details about books (book ID and entry date - the date it was entered in the system).

The bar chart displays the number of books entered this week on each day - from Monday to Sunday.

However, this implementation seems inefficient. Additionally, as DateTime fields in the database don't have an equivalent of DateTime.Date property in .NET the query checks between two dates to get the count for each day.

using SkiaSharp;
using Xamarin.Forms;
using Xamarin.Forms.Xaml;
using Microcharts;
using ChartEntry = Microcharts.ChartEntry;

public GraphDisplayPage()
{
     InitializeComponent();
     DrawChart();
}

void DrawChart()
{
     var monday = DateTime.Today.AddDays(-(int)DateTime.Today.DayOfWeek + (int)DayOfWeek.Monday);
     int mondayBookCount = App.Database.GetDailyCount(monday);

     var tuesday = DateTime.Today.AddDays(-(int)DateTime.Today.DayOfWeek + (int)DayOfWeek.Tuesday);
     int tuesdayBookCount = App.Database.GetDailyCount(tuesday);

     var wednesday = DateTime.Today.AddDays(-(int)DateTime.Today.DayOfWeek + (int)DayOfWeek.Wednesday);
     int wednesdayBookCount = App.Database.GetDailyCount(wednesday);

     var thursday = DateTime.Today.AddDays(-(int)DateTime.Today.DayOfWeek + (int)DayOfWeek.Thursday);
     int thursdayBookCount = App.Database.GetDailyCount(thursday);

     var friday = DateTime.Today.AddDays(-(int)DateTime.Today.DayOfWeek + (int)DayOfWeek.Friday);
     int fridayBookCount = App.Database.GetDailyCount(friday);

     var saturday = DateTime.Today.AddDays(-(int)DateTime.Today.DayOfWeek + (int)DayOfWeek.Saturday);
     int saturdayBookCount = App.Database.GetDailyCount(saturday);

     var sunday = DateTime.Today.AddDays(-(int)DateTime.Today.DayOfWeek + (int)DayOfWeek.Sunday);
     int sundayBookCount = App.Database.GetDailyCount(sunday);

     List<ChartEntry> entries = new List<ChartEntry>
     {
          new ChartEntry(mondayBookCount)
          {
               Label = "Monday",
               ValueLabel = mondayBookCount.ToString(),
               Color = SKColor.Parse("#004daa")
           },
           new ChartEntry(tuesdayBookCount)
           {
               Label = "Tuesday",
               ValueLabel = tuesdayBookCount.ToString(),
               Color = SKColor.Parse("#004daa")
           },
           new ChartEntry(wednesdayBookCount)
           {
                Label = "Wednesday",
                ValueLabel = wednesdayBookCount.ToString(),
                Color = SKColor.Parse("#004daa")
            },
            new ChartEntry(thursdayBookCount)
            {
                Label = "Thursday",
                ValueLabel = thursdayBookCount.ToString(),
                Color = SKColor.Parse("#004daa")
            },
            new ChartEntry(fridayBookCount)
            {
                 Label = "Friday",
                 ValueLabel = fridayBookCount.ToString(),
                 Color = SKColor.Parse("#004daa")
            },
            new ChartEntry(saturdayBookCount)
            {
                 Label = "Saturday",
                 ValueLabel = saturdayBookCount.ToString(),
                 Color = SKColor.Parse("#004daa")
            },
            new ChartEntry(sundayBookCount)
            {
                 Label = "Sunday",
                 ValueLabel = sundayBookCount.ToString(),
                 Color = SKColor.Parse("#004daa")
            }
         };

       chartView.Chart = new BarChart { Entries = entries, LabelTextSize = 32f, LabelOrientation = Orientation.Horizontal, ValueLabelOrientation = Orientation.Horizontal, Margin = 20 };
}

Book.cs

   public class Book : INotifyPropertyChanged
   {
        [PrimaryKey, AutoIncrement]
        public int ID { get; set; }
        private DateTime bookSaveTime;
        public DateTime BookSaveTime
        {
            get
            {
                 return bookSaveTime;
             }
             set
             {
                 if (bookSaveTime != value)
                 {
                     bookSaveTime= value;
                     OnPropertyChanged("BookSaveTime");
                 }
              }
         }
    
         public event PropertyChangedEventHandler PropertyChanged;
         protected virtual void OnPropertyChanged(string propertyName)
         {
              var changed = PropertyChanged;
              if (changed != null)
              {
                  PropertyChanged(this, new PropertyChangedEventArgs(propertyName));
              }
         }
   }

SQLiteDatabase.cs:

static SQLiteConnection database;
public const string DbFileName = "SQLite.db3";
public string CurrentState;

public SQLiteDatabase()
{
            try
            {
                database = DependencyService.Get<ISQLiteService>().GetConnection(DbFileName);
                database.CreateTable<Book>();

                CurrentState = "Database created";

            }
            catch (SQLiteException ex)
            {
                CurrentState = ex.Message;
            }
}

public int GetDailyCount(DateTime day)
{
      var dayAfterCurrentDay = day.AddDays(1); 
      return database.ExecuteScalar<int>("SELECT COUNT(*) FROM Book WHERE bookSaveTime> ? AND bookSaveTime< ?;", day, dayAfterCurrentDay);   
}

Is it possible to improve this?

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I would think the DrawChart method could be shorter by applying DRY. \$\endgroup\$
    – Rick Davin
    Feb 9, 2021 at 21:32

1 Answer 1

2
\$\begingroup\$

In this review let me try to focus on a single thing: eliminating duplicates.

What is the problem with duplicates?

  1. The size of your code base could become quite lengthy. It ruins legibility.
  2. If you need to change one thing on the functionality, which affects all duplicates, then you have to do that in all occurrences.
  3. If you need to extend your datasource then it is appealing to use copy-paste, which is error-prone. With Ctrl+C & Ctrl+V you might forget to change an important property/method call/whatever which is duplicate specific.

How can I eliminate this?

  • The short answer: By loosening the coupling between data and functionality.
  • In this particular case your implementation executes the same functionality over and over again against different data.
  • So, first you have to identify which piece of functionality is executed multiple times:
    • Here I have used {xyz} to indicate placeholders, which changes for each duplicate.
var {dayOfWeekDate} = DateTime.Today.AddDays(-(int)DateTime.Today.DayOfWeek + (int){dayOfWeekEnumCalue});
int {dayOfWeekBookCount} = App.Database.GetDailyCount({dayOfWeekDate});

{dayOfWeekEnumValue}

  • You can get all values of DayOfWeek by using the Enum.GetValues functionality.
var days = (DayOfWeek[]) Enum.GetValues(typeof(DayOfWeek));
  • We can iterate through this collection to get each day:
foreach (var day in days)
{
    var {dayOfWeekDate} = DateTime.Today.AddDays(-(int)DateTime.Today.DayOfWeek + (int)day);
    var {dayOfWeekBookCount} = App.Database.GetDailyCount({dayOfWeekDate});
}

{dayOfWeekDate}

  • This variable is scoped for the foreach. So, here we can use whatever name we want.
    • We can name it by describing what data is stored on it.
    • It is better than naming it how do we want to use it. If we need to extend functionality and we need to use this variable in multiple method calls then the naming could become problematic.
foreach (var day in days)
{
    var specificDayAtMidnight = DateTime.Today.AddDays(-(int)DateTime.Today.DayOfWeek + (int)day);
    var {dayOfWeekBookCount} = App.Database.GetDailyCount(specificDayAtMidnight);
}

{dayOfWeekBookCount}

  • Here we are enlarging the scope of our interest.
    • Where and how do we want to use this data?
new ChartEntry({dayOfWeekBookCount})
{
   Label = "Monday",
   ValueLabel = {dayOfWeekBookCount}.ToString(),
   Color = SKColor.Parse("#004daa")
},
  • As we can see it is day specific. So, we have to bind the day and the book count.
    • This can be easily done by using for example a Dictionary:
Dictionary<string, int> dailyBookCounts =  new Dictionary<string, int>();
  • Here the key is the name of the day and value is the book count.
  • We should populate this collection inside our foreach loop:
foreach (var day in days)
{
    var specificDayAtMidnight = DateTime.Today.AddDays(-(int)DateTime.Today.DayOfWeek + (int)day);
    var bookCount = App.Database.GetDailyCount(specificDayAtMidnight);
    dailyBookCounts.Add(day.ToString("G"), bookCount);
}

entries

  • With these data in our hand we are able to eliminate the duplicates of the ChartEntry instance creation.
List<ChartEntry> entries = new List<ChartEntry>();
foreach (var dailyBookCount in dailyBookCounts)
{
    entries.Add(new ChartEntry(dailyBookCount.Value)
    {
        Label = dailyBookCount.Key,
        ValueLabel = dailyBookCount.Value.ToString(),
        Color = SKColor.Parse("#004daa")
    });
}
  • Depending on the C# version we can take advantage of deconstruction:
foreach (var (nameOfTheDay, bookCountOfTheDay) in dailyBookCounts)
{
    entries.Add(new ChartEntry(bookCountOfTheDay)
    {
        Label = nameOfTheDay,
        ValueLabel = bookCountOfTheDay.ToString(),
        Color = SKColor.Parse("#004daa")
    });
}

How does my code look like after these modifications?

//Extract
var days = (DayOfWeek[]) Enum.GetValues(typeof(DayOfWeek));
var dailyBookCounts =  new Dictionary<string, int>();
foreach (var day in days)
{
    var specificDayAtMidnight = DateTime.Today.AddDays(-(int)DateTime.Today.DayOfWeek + (int)day);
    var bookCount = App.Database.GetDailyCount(specificDayAtMidnight);
    dailyBookCounts.Add(day.ToString("G"), bookCount);
}

//Transform
var entries = new List<ChartEntry>();
foreach (var (nameOfTheDay, bookCountOfTheDay) in dailyBookCounts)
{
    entries.Add(new ChartEntry(bookCountOfTheDay)
    {
        Label = nameOfTheDay,
        ValueLabel = bookCountOfTheDay.ToString(),
        Color = SKColor.Parse("#004daa")
    });
}

//Load
chartView.Chart = new BarChart
{
    Entries = entries, 
    LabelTextSize = 32f, 
    LabelOrientation = Orientation.Horizontal, 
    ValueLabelOrientation = Orientation.Horizontal, 
    Margin = 20
};
  • As you can see I've added 3 lines of comment. I've used here the Extract, Transform and Load (or in short ETL) concept to separate the phases of your method. This improves legibility by helping the maintainer of your code where to locate a particular piece of functionality.

Is there something else which could improved?

  • Yes, there are two major concerns which could be addressed:
    • Async I/O
    • Batch operation

Async I/O

  • Whenever you perform a database query by calling the App.Database.GetDailyCount method you are blocking the execution of your code until the data retrieval finishes. Which is not bad, but your Thread does not do anything just waits for the response. So, you are basically wasting valuable computation resources.
  • If you would use async (non-blocking) I/O calls then while the database performs the query your Thread will be freed and it could be able to execute any other code. So, this can improve your system throughput (by allowing to perform CPU based calculation while the application is waiting for an I/O operation to complete).

Batch operation

  • In your current implementation you perform 7 individual database calls. Which means there are 7 round trips between your app and database. It is not a problem on your dev machine because most probably both runs on the same machine. But in case of production environment they are likely separated and running on different machines. Which means there is a network latency which should be considered as well.
  • By allowing to perform a batch operation on the database side you can reduce the number of round trips to 1 which means that the network latency can become negligible.

Your code would look something like:

var startDates = new List<DateTime>();
foreach (var day in days)
{
    startDates.Add(DateTime.Today.AddDays(-(int)DateTime.Today.DayOfWeek + (int)day));
}

Dictionary<string, int> dailyBookCounts = await App.Database.GetWeeklyCountAsync(startDates);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.