1
\$\begingroup\$

I have question regarding best practices. I've decided instead having config inside either app.config or xml file to have my config file inside SQLite file database. This config will contain basic information for specific class. Config will be managed from SQLite database browser. My question is is it fine assuming that config information is in single db file to have still call to it directly from for instance Email class (this is just example class i want to use the same way in other classes required default config information)? I know it's db call but in fact it's also call to config default values therefore i assume it's fine to have it like this. Nevertheless would like to get your opinion. Look below:

public class Email : IEmail
    {
        public string To { get; private set; }
        public string Cc { get; private set; }
        public string Body { get; private set; }
        public string SmtpIp { get; private set; }
        public string Subject { get; private set; }
        public string LogPath { get; private set; }
        public string FromName { get; private set; }
        public string Signature { get; private set; }
        public string FromAddress { get; private set; }

        public void ResolveConfig(string memberEntryName = "Default")
        {
            IDbManager dbMansdager = new DbManager(ConnectionDbType.SqlLite);

            LogPath = dbMansdager.GetDataTable("SELECT Value FROM Config WHERE Member='Path'", CommandType.Text).Rows[0][0].ToString();
            To = dbMansdager.GetDataTable($"SELECT To FROM Email WHERE Member='{memberEntryName}'", CommandType.Text).Rows[0][0].ToString();
            Body = dbMansdager.GetDataTable($"SELECT Body FROM Email WHERE Member='{memberEntryName}'", CommandType.Text).Rows[0][0].ToString();
            SmtpIp = dbMansdager.GetDataTable($"SELECT SmtpIp FROM Email WHERE Member='{memberEntryName}'", CommandType.Text).Rows[0][0].ToString();
            Subject = dbMansdager.GetDataTable($"SELECT Subject FROM Email WHERE Member='{memberEntryName}'", CommandType.Text).Rows[0][0].ToString();
            Signature = dbMansdager.GetDataTable($"SELECT Signature FROM Email WHERE Member='{memberEntryName}'", CommandType.Text).Rows[0][0].ToString();
            FromName = dbMansdager.GetDataTable($"SELECT FromName FROM Email WHERE Member='{memberEntryName}'", CommandType.Text).Rows[0][0].ToString();
            FromAddress = dbMansdager.GetDataTable($"SELECT FromAddress FROM Email WHERE Member='{memberEntryName}'", CommandType.Text).Rows[0][0].ToString();
            Cc = dbMansdager.GetDataTable($"SELECT Cc FROM Email WHERE Member='{memberEntryName}'", CommandType.Text).Rows[0][0].ToString();
        }

        /// <summary>
        /// Default values comming from database from Email table nevertheless you can override some values in method arguments
        /// memberEntryName is entry in Email table that information for particural email will be used default set is default entry
        /// If new entry is needed it has to be added to Email table and Member's name has to be specified in method argument
        /// To and CC in table if more email required all has to be separated by ;
        /// </summary>
        /// <param name="memberEntryName"></param>
        /// <param name="customBody"></param>
        /// <param name="customSubject"></param>
        /// <param name="attachementsPaths"></param>
        /// <returns></returns>
        public async Task Send(string memberEntryName = "Default", string customBody = "", string customSubject = "", IEnumerable<string> attachementsPaths = null)
        {
            ResolveConfig(memberEntryName);
            try
            {
                using (var smtp = new SmtpClient(SmtpIp))
                {
                    smtp.UseDefaultCredentials = true;

                    using (var mail = new MailMessage())
                    {
                        mail.IsBodyHtml = true;
                        mail.Priority = MailPriority.High;
                        mail.From = new MailAddress(FromAddress, FromName);

                        if (To != null && !string.IsNullOrEmpty(To))
                        {
                            var mails = To.Split(';');
                            foreach (var m in mails)
                                mail.To.Add(m);
                        }

                        if (Cc != null && !string.IsNullOrEmpty(Cc))
                        {
                            var mails = Cc.Split(';');
                            foreach (var m in mails)
                                mail.CC.Add(m);
                        }

                        mail.Subject = !string.IsNullOrEmpty(customSubject) ? customSubject : Subject;
                        mail.Body = !string.IsNullOrEmpty(customBody) ? customBody : Body;

                        if (attachementsPaths != null)
                        {
                            foreach (var attachment in attachementsPaths)
                            {
                                if (File.Exists(attachment))
                                    mail.Attachments.Add(new Attachment(attachment));
                            }
                        }
                        await smtp.SendMailAsync(mail);
                    }
                }
            }
            catch (Exception ex) { throw new NotSupportedException(ex.ToString()); }
            finally
            {
                if (attachementsPaths != null)
                {
                    foreach (var attachment in attachementsPaths)
                    {
                        if (File.Exists(attachment))
                            File.Delete(attachment);
                    }
                }
            }
        }
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ I have rolled back your last edit. Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Heslacher Dec 13 '18 at 11:21
  • \$\begingroup\$ @Heslacher ahh sorry forgive .. \$\endgroup\$ – dev Dec 13 '18 at 11:22
  • \$\begingroup\$ no problem. You are new here so you couldn't know. \$\endgroup\$ – Heslacher Dec 13 '18 at 11:23
  • \$\begingroup\$ @Heslacher thanks, can you respond to my question at the top to your answer please \$\endgroup\$ – dev Dec 13 '18 at 11:25
2
\$\begingroup\$
IDbManager dbMansdager = new DbManager(ConnectionDbType.SqlLite);  

Small typo here. It should be named dbManagershould't it?

  • ResolveConfig() is a public method hence you should validate the passed methodparameter. One could pass null and your queries would throw an exception.

  • Send() you are using using statements for disposable objects which is good but you could do better by stacking the usings like so

    using (var smtp = new SmtpClient(SmtpIp))
    using (var mail = new MailMessage())
    {
        smtp.UseDefaultCredentials = true;
    
    
    }  
    

    This will save one level of indentation.

  • It seems you expect that To may be null or empty. You can simplify the check by only calling string.IsNullOrEmpty() or much better string.IsNullOrWhiteSpace(). But wait, if To is either null or empty calling SendMailAsync() would throw an ArgumentNullException.

  • Finally this could be a problem because the finally will be executed no matter if an exception had been thrown or not. Meaning the attachment will be deleted. This will happen e.g the smtp-server is down as well. Is this the desired behaviour ?
  • Omitting braces {} whould be avoided althought they might be optional. Omitting braces can lead to hidden and therefor hard to find bugs.

Is it fine approach to call config like this generally in that example class?

If you want to make quick adjustments to some config this isn't the way you should go because changing e.g a xml-file just needs an editor but changing some db entries adds a lot of overhead. It also depends on the use case. If you consider to make mass-mailings then the db version would be better.

\$\endgroup\$
  • \$\begingroup\$ Yes that was a typo and yes ResolveConfig should be private because if user want to override some staff he can do it from Send method by specifying memberEntryName. I also agree with all other propositions however main question is: Is it fine approach to call config like this generally in that example class? \$\endgroup\$ – dev Dec 13 '18 at 11:23
  • \$\begingroup\$ Can you answer please? \$\endgroup\$ – dev Dec 13 '18 at 11:50
  • \$\begingroup\$ See my edit (at the end) \$\endgroup\$ – Heslacher Dec 13 '18 at 11:54
  • \$\begingroup\$ but it is in fact db version. I would say modifying it is very same as changing in xml file. Nevertheles is comparable. Hpowever looking at designing model itself it's acceptable right? I mean putting callers like i did by IDbManager interface it's fine from the class itself rigth? The same way as i would call xml file. I am asking only about the approach itself calling from Email for xml config file or db config file. Hope you get my point \$\endgroup\$ – dev Dec 13 '18 at 13:22
  • \$\begingroup\$ You may better ask this question at softwareengineering.stackexchange.com but make sure to read their help-center first. \$\endgroup\$ – Heslacher Dec 13 '18 at 13:24

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.