Какие твердые принципы нарушены?(Что я должен сделать, чтобы решить проблемы с этим контроллером?)

Question

недавно у меня было собеседование на работу, которое интервьюер дал мне кусок кода для решения проблемы нарушения принципов SOLID, но я не являюсь опытным программистом и из-за недостатка знаний не смог найти никаких проблем, теперь япрошу вас помочь мне

что не так с этим фрагментом кода?

using System.Data.SqlClient;
using System.Linq;
using System.Web.Mvc;
using System.IO;
namespace InterviewTest.Controllers
{
    public class EMailController : Controller
    {
        const string logFile = "log.txt";

        // this is a method which takes an id or a username after searching in 
       //database this method sends an email to the searched id or username 
       //then this operation Is stored in a log file...

        public ActionResult Index()
        {
            string connectionString = "Server=myServerAddress;Database=myDataBase;User Id=myUsername;Password = myPassword;";

            SqlConnection con = new SqlConnection(connectionString);

            con.Open();

            SqlCommand cmd = new SqlCommand("select * from tblUsers where id = " + int.Parse(Request.QueryString["id"]) + " or username = '" + Request.QueryString["username"] + "'" , con);
            SqlDataReader reader = cmd.ExecuteReader();

            reader.Read();

            string email = reader.GetString(2);

            emailer.Instance.send(email);

            FileStream fs = System.IO.File.OpenWrite("c:\\myapp\\" + logFile);
            StreamWriter sw = new StreamWriter(fs);

            sw.Write(DateTime.Now.ToString() + " sent email to " + email);

            fs.Close();

            return View();
        }

        // This is a class Which is responsible for sending email
        public class emailer
        {
            private static emailer instance = null;

            public static emailer Instance
            {
                get {
                    if (instance == null)
                    {
                        instance = new emailer();
                    }
                    return instance;
                }
            }

            internal void send(string email) {
                try {
                    // Suppose this piece of code has been implemented
                }
                catch (Exception ex)
                {
                    Console.Write(ex.ToString());
                }
            }
        }
    }
}

Answer 1

(я предполагаю, что вы знаете принципы наизусть, поэтому я не буду объяснять, что они означают)

Единственная ответственность: потому что контроллер заботится о получении данных из базы данных (что обычно делается черезхранилище или комбо-сервис-репозиторий), а также отправка электронной почты, которая должна быть в службе, используемой в контроллере.

Принцип открытого-закрытого: класс отправителя электронной почты реализован в том же месте, что ипоэтому контроллер явно не открыт для расширения и закрыт для модификации

Разделение интерфейса: вообще не используется

Инверсия зависимостей: вообще не используется, например, класс репозитория и отправка электронной почтысервис должен быть скрыт за интерфейсами (т.е. IEmailSender, IMyDataRepository), а контроллер должен использовать тех, кто не знает / не заботится о точной реализации.Еще лучше, если его использовать вместе с внедрением зависимостей -> контроллер получит экземпляры классов, реализующих эти интерфейсы в конструкторе, используя Unity, SimpleInjector и т. Д.

Лисков: иерархия классов, интерфейсы и т. Д. Не используются.

Если бы мне пришлось реализовать что-то вроде этого:

public class EmailController : Controller
{
  // Interface segregation applied
  private IEmailSendingService emailService;
  private IUserService userService;
  private ILoggingService loggingService

  // Dependency inversion principle / Dependency injection applied
  public EmailController(IEmailService mailSrvc, IUserservice usrSvc, ILoggingService log)
  {
      this.emailService = mailSrvc;
      this.userService = usrSvc;
      this.loggingService = log;
  }

  public ActionResult SendEmail()
  {
    try
    {
      var emailAddress = this.userService.GetEmail(...);
      // validate email address, maybe through another service
      if(Validator.ValidateEmail())
      {
          // Single responsibility applied
          this.emailService.SendEmail(emailAddress);
      }          
    }
    catch(MailNotFoundException ex)
    {
      this.loggingService.LogError("Email address not found for xy user");
      return NotFound();
    }
    catch(EmailSendingFailedException ex)
    {
      this.loggingService.LogError("Could not send email because xyz");
      // return internalservererror etc.
    }
    catch(Exception ex)
    {
      this.loggingService.LogError("...");
    }          

      // return whats needed
  }
}

Пример не безупречен, но вы можете понять его суть:)