Good practices for a class

I was working in a class, but there are a lot of things that I’m not sure if they’re a good practice or not. For example, the => instead of the { }. When should I use it? Also, I’m not pretty sure about the keywords const and readonly. For example, uriImage and key it’s something that won’t change. So, I assume it’ll be const. Nevertheless, summonerName or currentVersion despite they won’t change, they will be initialized later. That’s why I assumed they should be readonly.

Another thing that I was wondering is the Key Api. What’d be the correct way of using a Key api? They could do reversing to the program and take the keyApi?

All good practice tips, and so on, are welcome, thank you very much in advance.

class GameApi
{

   private const    string uriImage = "http://opgg-static.akamaized.net/images/lol/champion/";
   private const    string key = "XXX-XXXXX-XXXXXXXX-XXXXXX-XXXXXX";
   private readonly string summonerName;
   private readonly string currentVersion;
        
   private ChampionListStatic championList;
   private Summoner summonerObject;
   private RiotApi  api;
    
   public GameApi(string _summonerName)
   {
       api = RiotApi.GetDevelopmentInstance(key);
       summonerName = _summonerName;

      try
      {
         // gets currentVersion so I can work with the GetAllAsync() method.
         // Save all champions in championList to work with it in the GetImageUri method.

         currentVersion = api.DataDragon.Versions.GetAllAsync().Result.First();
         summonerObject = api.Summoner.GetSummonerByNameAsync(RiotSharp.Misc.Region.Euw, _summonerName).Result;
         championList   = api.DataDragon.Champions.GetAllAsync(currentVersion).Result;

      } catch (RiotSharpException) { // Exception bla bla }
    }

   public IEnumerable<CurrentGameParticipant> getEnemyTeam()
   {
      // Get the 10 participants in the current game.
      List<CurrentGameParticipant> currentMatch = 
      api.Spectator.GetCurrentGameAsync(Region.Euw, summonerObject.Id).Result.Participants;
      
      // Get the ID team that summonerName belongs to.
      long teamId = currentMatch.Where(p => p.SummonerName == summonerName).First().TeamId;
 
      // Return the (5) participants that doens't belong to the same team than summonerName.    
      return currentMatch.Where(p => p.TeamId != teamId);
   }

   public string getImageUri(long championId) =>
      // Return the uri to load in the pictureBox.ImageLocation propriety
      uriImage + championList.Champions.Where(p => p.Value.Id == championId).First().Value.Image.Full;
}
```

=> instead of the { }

This is personal preference. My preference, for example, is to generally use { } because I like it more, stylistically. I will say if it’s more than a line, using => (again, in my opinion) becomes hard to read

I’m not pretty sure about the keywords const and readonly

const is used for compile time constants and readonly are for things that are set when the object is constructed. This can be either on a field or property that is initialized (ie set) where it is declared OR set in a constructor. You also can’t use const with objects that you have to new. For example, you can’t do private const List<string> _someList = new List<string>().

From what I see here, championList, summonerObject, and api can all be readonly.

correct way of using a Key api

Depends on what you’re doing. Since you’re asking about it, it sounds like you are deploying this somewhere (and possibly have this code in a repo somewhere that is available to others). You should not have the secret stored in the repo. Going in depth for what you should do is a bit out of scope for an answer here.

good practice tips

Since this is a small snapshot of your application, I’m going to make a few guesses.

You’re abusing async calls here quite a bit by just putting .Result on the end of each call. The reason you need to do that is you can’t make the constructor async. Since you don’t need any of that information that you’re getting in the constructor in the constructor itself, you can do what’s called “Lazy Load” the information later. Essentially, you check if you have the object before you use it (if it’s not null) and, if you don’t, you get it.

Making this change pushes all of the api calls to getEnemyTeam, which should be async already (public async Task<IEnumerable<CurrentGameParticipant>> getEnemyTeam()) and change lines where you use .Result to use await instead.

From the sound of the class GameApi, it’s not meant for a single summoner but a single instance could check any number of summoners. Instead of passing a single summoner to the constructor, you could pass it in to whatever method you would need. Same goes for the region, for example. Instead of hardcoding it to a single region, you could pass it in (but if it’s for your own use and that’s your region, so be it).

You don’t need to do separate Where and First clauses here: currentMatch.Where(p => p.SummonerName == summonerName).First().TeamId. It can become currentMatch.First(p => p.SummonerName == summonerName).TeamId. As an aside, what if you pass in a different casing of summonerName than what’s expected? Instead of using ==, you can do summonerName.Equals(p.SummonerName, StringComparison.OrdinalIgnoreCase).

The way you are getting the image URI, you could consider using a Dictionary. Also, here again, you can combine the Where and First methods. I also, personally, would break up that method to use { } and split it to multiple lines for readability.