6
\$\begingroup\$

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.

web metrics

\$\endgroup\$
  • \$\begingroup\$ Instead of ADO.NET, use dapper-tutorial.net . \$\endgroup\$ – BCdotWEB Sep 16 at 7:51
  • \$\begingroup\$ Isn't ADO.NET faster? I am familiar with Entity Framework, but consciously decided to go ADO.NET route to also re-sharpen my raw SQL. It had been ages since I had written raw SQL and was actually losing that knwoeldge cause all my projects at work and home were all done with EF as the ORM. \$\endgroup\$ – J86 Sep 16 at 7:54
  • \$\begingroup\$ Dapper isn't EF. You still need to know sql etc., you just don't waste time and effort on writing your own "convert sql data to proper values" code. WRT performance: exceptionnotfound.net/… . \$\endgroup\$ – BCdotWEB Sep 16 at 13:01
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Physical disk or S3? Also, how do I handle storing multiple dimensions of the same image? How should I name the images? SEO friendly names (e.g. bb-king-60x60.png or the artist id from the database? \$\endgroup\$ – J86 Sep 15 at 23:35
  • \$\begingroup\$ @J86 store it on physical disk (on the server side). and about the multiple dimensions, either use naming convention or you could store them in a sub-folder like images/artists/small/someartist.png or you can use some other third-party libraries on Nuget or Javascript to resize the images dynamically. \$\endgroup\$ – iSR5 Sep 15 at 23:46
  • \$\begingroup\$ @J86 storing on physical disk of the server is ok as long as you dont run multiple instances behind a load balancer. Otherwise the image will be stored on the Machine that processed the insert request And unavailable to all the other instances. \$\endgroup\$ – slepic Sep 16 at 4:31
  • \$\begingroup\$ @slepic true, I was thinking of deploying this whole thing on Kubernetes (again for learning). Everything is already dockerised and I have nginx as a proxy server directing traffic to the docker containers. The next step as far as DevOps go will be to deploy it on K8s. So I think S3 would be better, not to mention I get to learn how to store/retrieve programtically to/from AWS S3. \$\endgroup\$ – J86 Sep 16 at 7:03

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.