Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I have a simple custom attribute MessageDeliveryAttribute that has one string filed. Also I have several classes that mark with this attribute like:

[MessageDeliveryAttribute("MessegaToDb")]

In the app.config are settings like:

  <appSettings>
    <add key="MessageToConsole" value="true"/>
    <add key="MessageToDb" value="true"/>
    <add key="MessageToService" value="false"/>
    <add key="MessageToFile" value="false"/>
  </appSettings>

I'm trying to implement class that will read all keys with value "true" from app.config, find all classes that marked with attribute

[MessageDeliveryAttribute("value of key from app.config where value = true")]

and then create a list with instance of these classes.
Here are working code, but I need your suggestions how to improve that?

public static List<IMessageDelivery> GetMessagesDelivery()
{
    var instances = new List<IMessageDelivery>();

    // list of settings from app.config where value is true
    List<string> settingValues = ConfigurationManager.AppSettings.AllKeys
        .Where(key => ConfigurationManager.AppSettings[key].Equals("true"))
        .Select(value => value)
        .ToList();


    foreach (var types in GetTypesWithMessageDeliveryAttribute())
    {
        var attributeValue = GetAttributeValue(types);
        if (settingValues.IndexOf(attributeValue) >= 0)
        {
            instances.Add(Activator.CreateInstance(types) as IMessageDelivery); 
        }                 
    }

    return instances;
}
// Get all classes with my custom attribute
public static IEnumerable<Type> GetTypesWithMessageDeliveryAttribute()
{
    var assembly = Assembly.GetExecutingAssembly();
    return from type in assembly.GetTypes()
           where type.IsDefined(typeof(MessageDeliveryAttribute), false)
           select type;
}
// get value of attribute
public static string GetAttributeValue(Type t)
{
    var messageDeliveryAttribute = (MessageDeliveryAttribute)Attribute.GetCustomAttribute(t, typeof(MessageDeliveryAttribute));

    return messageDeliveryAttribute != null ? messageDeliveryAttribute.DeliveryPointName : string.Empty;
}
share|improve this question
up vote 1 down vote accepted

I'm not sure what feedback specifically you're looking for. There are minor formatting and naming questions, but only two things I would specifically draw attention to.

The first thing is that you're enumerating all the settings in the App.config, but you're only interested in settings that relate directly to message delivery. You should name-space your setting keys, and filter the settings before evaluating them.

<appSettings>
    <add key="MessageDelivery.MessageToConsole" value="true"/>
    <add key="MessageDelivery.MessageToDb" value="true"/>
    <add key="MessageDelivery.MessageToService" value="false"/>
    <add key="MessageDelivery.MessageToFile" value="false"/>
  </appSettings>

and

List<string> settingValues = ConfigurationManager.AppSettings.AllKeys
        .Where(key => ConfigurationManager.AppSettings[key].Equals("true"))
        .Where(key => key.StartsWith("MessageDelivery."))
        .Select(value => value.Replace("MessageDelivery.",""))
        .ToList();

Secondly, avoid using two different forms of Linq expression building:

List<string> settingValues = ConfigurationManager.AppSettings.AllKeys

        .Where(key => ConfigurationManager.AppSettings[key].Equals("true"))
        .Select(value => value)
        .ToList();

and

return from type in assembly.GetTypes()
           where type.IsDefined(typeof(MessageDeliveryAttribute), false)
           select type;
share|improve this answer
    
Thanks for your advices – MegaTron Nov 24 '15 at 8:32

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.