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);
}
}
}
}
}
-
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– Heslacher2018年12月13日 11:21:30 +00:00Commented Dec 13, 2018 at 11:21
-
\$\begingroup\$ @Heslacher ahh sorry forgive .. \$\endgroup\$dev– dev2018年12月13日 11:22:40 +00:00Commented Dec 13, 2018 at 11:22
-
\$\begingroup\$ no problem. You are new here so you couldn't know. \$\endgroup\$Heslacher– Heslacher2018年12月13日 11:23:44 +00:00Commented Dec 13, 2018 at 11:23
-
\$\begingroup\$ @Heslacher thanks, can you respond to my question at the top to your answer please \$\endgroup\$dev– dev2018年12月13日 11:25:19 +00:00Commented Dec 13, 2018 at 11:25
1 Answer 1
IDbManager dbMansdager = new DbManager(ConnectionDbType.SqlLite);
Small typo here. It should be named dbManager
should't it?
ResolveConfig()
is apublic
method hence you should validate the passed methodparameter. One could passnull
and your queries would throw an exception.Send()
you are usingusing
statements for disposable objects which is good but you could do better by stacking theusing
s like sousing (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 benull
or empty. You can simplify the check by only callingstring.IsNullOrEmpty()
or much betterstring.IsNullOrWhiteSpace()
. But wait, ifTo
is eithernull
or empty callingSendMailAsync()
would throw anArgumentNullException
.Finally
this could be a problem because thefinally
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.
-
\$\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– dev2018年12月13日 11:23:47 +00:00Commented Dec 13, 2018 at 11:23
-
\$\begingroup\$ Can you answer please? \$\endgroup\$dev– dev2018年12月13日 11:50:21 +00:00Commented Dec 13, 2018 at 11:50
-
\$\begingroup\$ See my edit (at the end) \$\endgroup\$Heslacher– Heslacher2018年12月13日 11:54:31 +00:00Commented Dec 13, 2018 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– dev2018年12月13日 13:22:03 +00:00Commented Dec 13, 2018 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– Heslacher2018年12月13日 13:24:14 +00:00Commented Dec 13, 2018 at 13:24