Сделайте ревью кода

Сделал рефакторинг. Какая работа была проведена:

1. разделил на методы, вынес многие в BaseManager

2. упростил названия переменных

3. перенёс все try-catch в базовый класс BaseManager

4. сгруппировал методы по доступу (вверху private, далее publiс)

5. было:

   protected RestClient GetApiClient()
   {
       var client = new RestClient
       {
           BaseUrl = new Uri(_baseApiServer)
       };
       return client;
   }
   

   стало:

   protected RestClient ApiClient
   {
       get
       { 
           return new RestClient
           {
               BaseUrl = new Uri(_baseApiServer)
           };
       }
   }
   

6. Было

   private async Task<string> GetObjVal(string groupId)
   

   стало:

   private async Task<JSON> GetObjValue(string groupId)
   

   теперь видно, что метод возвращает JSON. Для это пришлось ввести класс JSON, а в нём поле Value

7. Вынес константы:

   private const string getFormsAPI = "anketa.list";
   private const string getFormAPI = "anketa.get";
   private const string requestName = "request";
   private const string idKey = "id";
   private static class API
   {
       public static string GetForms { get { return "anketa.list"; } } // ! const бессмысленно? Раз отсутствует set
       public static string GetForm = "anketa.get"; 
   }
   

   мотив - чтобы избавится от префикса API в константах.

8. Как лучше?

   Parameter request = ....
   

   или лучше

   Parameter requestParam
   

   или

   requestP
   

   чтобы было понятно, что это параметр. Или по типу можно понять, наведя мышкой?

9. В UserName.cs я бросаю исключение, как мне посоветовали. Но везде по коду ошибки пишутся в лог. Согласно стилю предыдущего кода мне тоже надо в лог писать?

10. JSON.cs, RootObject.cs, UserName.cs поместил в папку Model

11. Избавился от dynamic

Если какие-то пункты некорректны, исправьте, пожалуйста. + напишите что ещё исправить

public class BaseManager
{
    private static readonly string _baseApiServer = "https://api.sendsay.ru/clu206"; // ! подчёркивания
    protected const string getGroupsAPI = "group.list";
    protected readonly IApiConfig ApiConfig;
    protected readonly ILogger Logger;
    protected CancellationToken Token { get { return new CancellationTokenSource().Token; } }
    public BaseManager(IApiConfig config)
    {
        ApiConfig = config;
        Logger = config.Logger;
    }
    protected IRestRequest GetBaseRequest(string action)
    {
        IRestRequest request = new RestRequest(_baseApiServer, Method.POST);
        request.AddParameter("apiversion", "100");
        request.AddParameter("json", "1");
        UserName username = new UserName(ApiConfig.UserName);
        string json = JsonConvert.SerializeObject(
                      new
                      {
                          one_time_auth = new
                          {
                              login = username.Login,
                              sublogin = username.Sublogin,
                              passwd = ApiConfig.Password
                          },
                          action = action
                      });
        request.AddParameter("request", json);
        return request;
    }
    protected RestClient ApiClient
    {
        get
        { 
            return new RestClient
            {
                BaseUrl = new Uri(_baseApiServer)
            };
        }
    }
    protected string AddObjectToParam(Parameter param, string key, string value)
    {
        value = key == "obj" ? value : '"'+value+'"';
        string paramVal = param.Value.ToString();
        return paramVal.Insert(paramVal.Length - 1, ",\"" + key + "\":" + value + "");
    }
    protected async Task<IRestResponse> GetResponseAsync(IRestRequest request)
    {
        IRestResponse response = null; // !
        try
        {
            response = await ApiClient.ExecuteTaskAsync(request, Token);
            return response;
        }
        catch (Exception e)
        {
            Logger?.Error($"Response getting error: {e.Message}");
        }
        if (response.StatusCode != HttpStatusCode.Created && response.StatusCode != HttpStatusCode.OK)
        {
            Logger?.Error($"Server return error: {response.StatusCode}");
        }
        return null;
    }
    protected T GetDeserialized<T>(string content)
    {
        try
        {
            return JsonConvert.DeserializeObject<T>(content);
        }
        catch (Exception e)
        {
            Logger?.Error($"Deserialization error: {e.Message}. Probably API structure has changed");
        }
        return (T)(object)null; // !
    }
}


    public class SubscriberManager : BaseManager, ISubscriberManager
    {
        private FieldManager fieldMng;
        private const string getMembersAPI = "member.set";
        private const string emailKey = "email";
        private const string newbieConfirmKey = "newbie.confirm";
        private const string newbieLetterConfirmKey = 
             "newbie.letter.confirm";
        private const string objKey = "obj";
        public SubscriberManager(IApiConfig config) : base(config)
        {
            fieldMng = new FieldManager(ApiConfig);
        }
        private async Task<JSON> GetObjValue(string groupId)
        {
            StringBuilder objVal = new StringBuilder("{");
            IEnumerable<FormWithQuests> formsWithQuests = await 
             fieldMng.GetFormsWithQuestsAsync();
            foreach (FormWithQuests formWithQuests in formsWithQuests)
            {
                IEnumerable<Quest> quests = formWithQuests.Quests;
                if (!quests.Any()) continue;

objVal.Append('"').Append(formWithQuests.Id).Append('"').Append(":{");
                for (int i = 0; i < quests.Count(); i++)
                {
                    Quest quest = quests.ElementAt(i);
                    objVal.Append('"').Append(quest.Id).Append('"')
                          .Append(':')
                          .Append('"').Append(quest.Name).Append('"');
                    if (i < quests.Count() - 1)
                    {
                        objVal.Append(',');
                    }
                }
                objVal.Append("},");
            }
            objVal.Append("\"-group\": {\"" + groupId + "\":\"1\"}}");
            return new JSON(objVal.ToString());
        }

        public async Task<int> ValidateRequestAsync()
        {
            try
            {
                IRestResponse resp = await GetResponseAsync(GetBaseRequest(getGroupsAPI)); // ! уже есть try-catch внутри
                return 200;
            }
            catch (Exception e)
            {
                Logger?.Error(e);
                return (int)HttpStatusCode.ServiceUnavailable;
            }
        }

            public async Task<Person> AddAsync(Subscriber subscriber)
            {
                Parameter request = GetBaseRequest(getMembersAPI).Parameters.FirstOrDefault(x => x.Name == "request"); // ! requestP
                request.Value = AddObjectToParam(request, emailKey, subscriber.Email);
                request.Value = AddObjectToParam(request, newbieConfirmKey, subscriber.OptIn ? "1" : "0");
                if (subscriber.OptIn)
                    request.Value = AddObjectToParam(request, newbieLetterConfirmKey, subscriber.TemplateId.ToString());
                request.Value = AddObjectToParam(request, objKey, (await GetObjValue(subscriber.GroupId)).Value);
                IRestResponse response = await GetResponseAsync(GetBaseRequest(getMembersAPI));
                SubscriberResponse data = GetDeserialized<SubscriberResponse>(
response.Content);
            return data.Member;
        }
    }

public class SendSayManager : ISendSayManager
{
    public IApiConfig ApiConfig { get; set; }
    public IListManager Lists { get; set; }
    public ISubscriberManager Subscribers { get; set; }
    public IFieldManager Fields { get; set; }
    public IEmailTemplateManager EmailTemplates { get; set; }
    public SendSayManager(IApiConfig apiConfig)
    {
        ApiConfig = apiConfig;
        Lists = new ListManager(ApiConfig);
        Subscribers = new SubscriberManager(ApiConfig);
        Fields = new FieldManager(apiConfig);
        EmailTemplates = new EmailTemplateManager(apiConfig);
    }
}

public class ListManager : BaseManager, IListManager
{
    public ListManager(IApiConfig config) : base(config)
    {
    }
    public async Task<IList<Group>> GetAllAsync()
    {
        IRestResponse response = await GetResponseAsync(GetBaseRequest(getGroupsAPI));
        ListResult lists = GetDeserialized<ListResult>(response.Content);
        return lists.List;
    }
}

public class FieldManager : BaseManager, IFieldManager
{
    private static class API
    {
        public static string GetForms { get { return "anketa.list"; } } // ! const бессмысленно? Раз отсутствует set
        public static string GetForm = "anketa.get"; 
    }
    private const string getFormsAPI = "anketa.list";
    private const string getFormAPI = "anketa.get";
    private const string requestName = "request";
    private const string idKey = "id";
    public FieldManager(IApiConfig config) : base(config)
    {
    }
    private async Task<string[]> GetFormIdsAsync(string apiMethod)
    {
        IRestResponse formsResp = await GetResponseAsync(GetBaseRequest(apiMethod));
        string[] formIds = GetDeserialized<FormList>(formsResp.Content).List.Select(x => x.Id).ToArray();
        return formIds;
    }
    private async Task<IEnumerable<QuestObj>> GetQuestsFromJSON(string formId)
    {
        IRestRequest readFormRequest = GetBaseRequest(getFormAPI);
        Parameter request = readFormRequest.Parameters.FirstOrDefault(x => x.Name == requestName);
        request.Value = AddObjectToParam(request, idKey, formId);
        IRestResponse readFormResp = await GetResponseAsync(readFormRequest);
        IEnumerable<QuestObj> questsFromJSON = GetDeserialized<RootObject>(readFormResp.Content).Obj.Quests.Select(x => x.Value);
        return questsFromJSON;
    }
    public async Task<IList<Quest>> GetAllAsync()
    {
        IList<Quest> quests = new List<Quest>();
        foreach (string formId in await GetFormIdsAsync(API.GetForms))
        {
            foreach (QuestObj quest in await GetQuestsFromJSON(formId))
            {
                quests.Add(new Quest(quest.Id, quest.Name));
            }
        }
        return quests;
    }
    public async Task<IEnumerable<FormWithQuests>> GetFormsWithQuestsAsync()
    {
        IList<FormWithQuests> formsWithQuests = new List<FormWithQuests>();
        foreach (string formId in await GetFormIdsAsync(API.GetForms))
        {
            IList<Quest> quests = new List<Quest>();
            foreach (QuestObj quest in await GetQuestsFromJSON(formId))
            {
                quests.Add(new Quest(quest.Id, quest.Name));
            }
            formsWithQuests.Add(new FormWithQuests(formId, quests));
        }
        return formsWithQuests;
    }
}

class JSON
{
    public string Value { get; set; }
    public JSON(string value)
    {
        Value = value;
    }
}

public class RootObject
{
    [JsonProperty("obj")]
    public Obj Obj { get; set; }
}
public class Obj
{
    [JsonProperty("quests")]
    public Dictionary<string, QuestObj> Quests { get; set; }
}
public class QuestObj
{
    [JsonProperty("name")]
    public string Name { get; set; }
    [JsonProperty("id")]
    public string Id { get; set; }
}


    class UserName
    {
        private const char delimeter = '|';
        public string Login { get; set; }
        public string Sublogin { get; set; }
        public UserName(string username)
        {
            if (string.IsNullOrEmpty(username))
            {
                throw new ArgumentException("Username is not filled", nameof(username));
            }
            string[] logins = username.Split(delimeter);
            if (logins.Length == 2)
            {
                Login = logins[0];
                Sublogin = logins[1];
            }
            else
            {
                throw new ArgumentException($"The parameter {nameof(username)} has incorrect content.");
            }
        }
    }