3
\$\begingroup\$

I just wrote this and don't like how bulky it is, also given the fact I will have to add at least another if statement.

I was going to switch it to a case statement but wanted to check if there were even better ways to reduce the clutter.

string emailLetterPath = Server.MapPath("~/emails/rejected.htm");

if (jobApplicantAndJob.Jobs.Title == "Store Sales Manager" || jobApplicantAndJob.Jobs.Title == "Sales Representative")
{
     emailLetterPath = Server.MapPath("~/emails/TC1RejectedP3.htm");
}

if (jobApplicantAndJob.Jobs.Title == "Outside Sales Professional")
{
     emailLetterPath = Server.MapPath("~/emails/TC2RejectedP3.htm");
}

The default option should be the rejected.htm.

\$\endgroup\$
7
\$\begingroup\$

I was going to switch it to a case statement but wanted to check if there were even better ways to reduce the clutter.

You could load the mappings from job titles to emails into a (static) dictionary, and then do a dictionary look-up (using the default value if it's not in the dictionary).

The Dictionary<string,string> can be initialized with a collection initializer.

Using a (data-driven) dictionary instead of a (hard-coded) switch statement has the additional benefit that users can configure/maintain the behaviour by editing a configuration file (e.g. a tab-delimited 'CSV file' of title/email pairs) or database, instead of editing code.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I'm pleased to see I'm not the only person who feels that the generic term CSV should include tab-separated files. \$\endgroup\$ – David Harkness Feb 7 '14 at 4:26
4
\$\begingroup\$

I'd at least shift it into its own function and then reorder the clauses:

string getEmailBaseFileForJobTitle(string title) {    
  if (title == "Outside Sales Professional") 
     return "TC2RejectedP3";
  if (title == "Store Sales Manager" || title == "Sales Representative")
     return "TC1RejectedP3";
  return "rejected";
}

Then your function will look like:

string emailLetterPath = Server.MapPath("~/emails/" +
         getEmailBaseFileForJobTitle(jobApplicantAndJob.Jobs.Title) +
         ".htm");

You could move the more into the function too if you really wanted.

Another option would be to make it a member function on the JobApplicantAndJob class.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ That's a fairly overkill method name, and not even PascalCase. \$\endgroup\$ – Magus Feb 6 '14 at 16:17
2
\$\begingroup\$

I would go with a rule generation solution if this is liable to grow.

EmailRules _rules;
string emailLetterPath = GetEmailPath();
string GetEmailPath()
{
    string path = rejectedPath;
    EmailRule rule = _rules.GetActingRule(path);

    if(rule != null) path = rule.Path;

    return path; 
}



void CreateEmailRules()
{
  _rules = new EmailRulesImpl();
  _rules.add(new ManagerOrRepresentative(),TC1Path);
  _rules.add(new OutsideSales(),TC2Path);
}

This is just a primer, allows the separation of rule creation from the actual parsing code. Does allow the extensibility of making smaller rules that you could combine to make aggregate rules etc...

Finally it means you can make rules sets by instantiating different rule managers depending on situations.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ For a simple, short method like the one in the quesion, this seems way too overengineered to me. YAGNI. \$\endgroup\$ – svick Feb 18 '14 at 15:02

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.