Making an alphabatical list grouped by letters

Question

I have a pet project (open source on GitHub) that I built as a learning exercise. It is a lyrics web application built in .NET Core MVC.

On the artists page of the site (view on mobile size viewport, it is only optimised for mobiles so far), I list all artists alphabetically. The artists are grouped by letter.

The code that builds that page is the following:

Controller

[Route("artists")]
public async Task<IActionResult> Index()
{
  IDictionary<char, List<LibraryArtistViewModel>> viewModel = await _artistsService
    .GetAllArtistsAsync();

  return View(viewModel);
}

Service

public async Task<IDictionary<char, List<LibraryArtistViewModel>>> GetAllArtistsAsync()
{
  List<LibraryArtistViewModel> artists = new List<LibraryArtistViewModel>();

  await using NpgsqlConnection connection = new NpgsqlConnection(databaseOptions.ConnectionString);
  await connection.OpenAsync();

  await using NpgsqlCommand command = new NpgsqlCommand("select a.first_name, a.last_name, \"as\".name as primary_slug, ai.data as image_data, count(l.title) as number_of_lyrics from artists a left join artist_images ai on ai.artist_id = a.id inner join artist_slugs \"as\" on \"as\".artist_id = a.id left join lyrics l on l.artist_id = a.id where a.is_approved = true and a.is_deleted = false and \"as\".is_primary = true and l.is_approved = true and l.is_deleted = false group by a.id, \"as\".name, ai.data order by a.first_name asc;", connection);

  await using NpgsqlDataReader reader = await command.ExecuteReaderAsync();

  while (await reader.ReadAsync())
  {
    LibraryArtistViewModel artist = new LibraryArtistViewModel();
    string firstName = Convert.ToString(reader[0]);
    string lastName = Convert.ToString(reader[1]);
    string fullName = textInfo.ToTitleCase($"{firstName} {lastName}");
    string primarySlug = Convert.ToString(reader[2]);
    bool hasImage = reader[3] != System.DBNull.Value;
    int numberOfLyrics = Convert.ToInt32(reader[4]);

    artist.FirstName = firstName;
    artist.LastName = lastName;
    artist.FullName = fullName;
    artist.PrimarySlug = primarySlug;
    artist.HasImage = hasImage;
    artist.NumberOfLyrics = numberOfLyrics;

    artists.Add(artist);
  }

  IDictionary<char, List<LibraryArtistViewModel>> dictionary = BuildDictionary(artists);

  return dictionary;
}

private IDictionary<char, List<LibraryArtistViewModel>> BuildDictionary(List<LibraryArtistViewModel> artists)
{
  List<char> letters = new List<char>();

  IDictionary<char, List<LibraryArtistViewModel>> dictionary =
    new Dictionary<char, List<LibraryArtistViewModel>>();

  foreach (LibraryArtistViewModel artist in artists)
  {
    char firstLetter = char.ToUpper(artist.FirstName[0]);

    if (!letters.Contains(firstLetter))
    {
      letters.Add(firstLetter);

      dictionary.Add(firstLetter, new List<LibraryArtistViewModel>());
    }
  }

  foreach (char letter in letters)
  {
    foreach (LibraryArtistViewModel artist in artists)
    {
      char firstLetter = char.ToUpper(artist.FirstName[0]);

      if (letter == firstLetter)
      {
        List<LibraryArtistViewModel> artistsBeginningWithTheLetter = dictionary[letter];
        artistsBeginningWithTheLetter.Add(artist);
      }
    }
  }

  return dictionary;
}

The code above works, but I feel like it isn't efficient. I feel like there's a better way of doing this.

Also, I am very unsure about the way I serve up images. My images are currently stored in the database as bytes and I have a controller that serves up the image. This seems to be really killing my performance metrics on https://web.dev.

I built this project to learn things. So I'd really love to learn how to do things "properly" even if they are deemed an overkill.

I'd appreciate some pointers on the above and also on how exactly I can improve the artist images situation.

Answer 1

Well, you have a good learning exercise project. here is some notes on the artiest code you've provided :

• You're using string query, which opens SQL Injections. To overcome that, you'll need first to mark your query as const or readonly and end the query with a semicolon (which you did), and ensure that the query itself not having any SQL bugs that might be used in inappropriate way. However, if you want the proper way to manager your database, you can use Object-relational Mapping AKA ORM such as Entity Framework.
• Store images in a psychical disk and store their path (or filename) into the database. Storing the file bytes into the database would impact the system performance as you'll need to read and render the images every time you select them. While if you store the files physically, it would be faster to render. So, create a folder inside the project folder, store the images in that folder (in any structure you want) then store the path of each image in the database. Then, in your application, you just get the path, adjust the path to be viewable to the user. For example, if the stored path images/artists/someartist.png then you just add the current web url to it like https://www.somewebsite.com/images/artists/someartist.png.
• Always use PNG images for web, as png images are more balanced between the quality and size, and would be better option than other types for the web.

In your code, sorting operations should not be done on the presentation layer, in fact, it would be more beneficial if it's moved to early application stages like sorting artists in your case, your application has a core feature to filter artists names in alphabetical order. This means, this functionality needs to be applied to the data before it's even inserted to the database, because it's used as a core feature and not an addon functionality.

There are different ways to do it though, one way is to add a property stores the first name letter like this :

public class LibraryArtistViewModel
{
    public char FirstLetter => FirstName?.Length > 0 ? FirstName.ToUpper()[0] : char.MinValue;

    public string FirstName { get; set; }

    public string LastName { get; set; }

    public string FullName { get; set; }

    public string PrimarySlug { get; set; }

    public bool HasImage { get; set; }

    public int NumberOfLyrics { get; set; }
        
}

Then, you would do this in GetAllArtistsAsync():

  while (await reader.ReadAsync())

      {
        var artist = new LibraryArtistViewModel
        {
            FirstName = reader[0]?.ToString(),
            LastName = reader[1]?.ToString(),
            FullName = textInfo.ToTitleCase($"{reader[0]?.ToString()} {reader[1]?.ToString()}"),
            PrimarySlug = reader[2]?.ToString(),
            HasImage = reader[3] != System.DBNull.Value;
            NumberOfLyrics = int.TryParse(reader[4]?.ToString(), out int number) ? number : 0;  
        };
        
        artists.Add(artist);
      }
    
    var dictionary = artists.GroupBy(x=> x.FirstLetter).ToDictionary(x => x.Key, x => x.ToList()).OrderBy(x=> x.Key);

Though this would give you what you want, it would take out some tiny performance. As I stated before, since your artists are supposed to be sorted, you need to sort it from the time you insert the data, either from the database which would be faster, or on CreateNewArtistAsync action. So, in your Get you won't need to re-sort on every request.