-2
\$\begingroup\$

I have a class with some properties. In the class I have a method that is "collecting" all the properties in a specific instance of the class to a dictionary, with the properties name being the dictionary key and the properties value being the dictionary value. Later in the class I have a method where I read text from a file into a string. This textfile includes the properties name with a template value. And in my method I want to replace the template value with the actual value that the properties of the instance contains. For this I have a struct that contains a method that is using LINQ to find a specific property and store that value into another dictionary. This dictionary will then contain the values that the template values in the file will be replaced with.

The thing is that the method in my struct is using a lot of LINQ queries just stacked one after another. I found it to be badly written code to just reuse hardcoded lines like this and I would like to find a way to "automate" the method a little bit more.

To show you more precisely what I mean here is my code:

First the class:

public class Files
{
    //All the properties in the class:
    public string Type { get; set; }
    public string FileName { get; set; }
    public string InternalName { get; set; }
    public string DdsName { get; set; }
    public string ColorName{ get; set; }
    public string FlashName{ get; set; }
    public string HexName{ get; set; }
    public string Number{ get; set; }
    public string TxtName { get; set; }
    public string Price { get; set; }
    public string Unlock { get; set; }
    public string Stock { get; set; }
    public string Base { get; set; }
    public string BaseLocked { get; set; }
    public string R_base { get; set; }
    public string B_base { get; set; }
    public string G_base { get; set; }
    public string brush { get; set; }
    public string Mask { get; set; }
    public string R_brush_color { get; set; }
    public string G_brush_color { get; set; }
    public string B_brush_color { get; set; }
    public string G_brush_locked { get; set; }
    public string B_brush_locked { get; set; }
    public string DdsDestpath { get; set; }
    public string DdsSourcepath { get; set; }

    //Method in the class that is collecting all the properties Note that 
    //this method //and the CreateDefaultPart() method is in a class while the 
   //DictionaryReplacements() is in a struct.

   //Collects all properties in the class and adds them to a dictionary
      public dynamic DictionaryOfProperties()
    {
        var dictionary = new Dictionary<string, string>();
        var properties = typeof(Files).GetProperties();

        for (int i = 0; i < properties.Length; i++)
            dictionary.Add(properties[i]
                .Name.ToString(), properties[i]
                .GetValue(this, null)?.ToString());
        return dictionary;
    }

//Here is the method that is using the DictionaryReplacements method.
    public virtual void CreateDefaultPart()
        {
            var propInfo = DictionaryOfProperties(); //This method is getting all the current objects properties and returns a dictionary with the properties names and values
            var replacements = new Replacements(propInfo); //The struct that has the DictionaryReplacements() method 
            string destPath = Path.Combine(@"C:\PathToDest", FileName);
            string sourcePath = Path.Combine(@"C:\PathToTemplateFile", "files", "cars", "main_part.txt");
            string text = File.ReadAllText(sourcePath);

            Regex regex = new Regex(string.Join("|", replacements.DictionaryReplacements().Keys.Select(x => Regex.Escape(x))));
            string replaced = regex.Replace(text, m => replacements.DictionaryReplacements()[m.Value]);

            File.WriteAllText(destPath, replaced);
        }
}

And here is the struct:

 struct Replacements
        {
            #region Fields
            //Private field
            Dictionary<string, string> properties;
            #endregion

            #region Constructor/Destructor
            //Constructor
            /// <summary>
            /// Creates a instance of the struct
            /// </summary>
            public Replacements(Dictionary<string, string> properties)
            {
                this.properties = properties;
            }
            #endregion
    /// <summary>
    /// Returns a dictionary with keys from a template file that will be replaced with the properties in the baseFile class
    /// </summary>
    /// <param name="properties">Dictionary with all properties from base class</param>
    /// <returns></returns>
    public Dictionary<string, string> DictionaryReplacements()
    {
        string internalName = properties.Where(name => name.Key == "InternalName").Select(name => name.Value).FirstOrDefault();
        string stock = properties.Where(name => name.Key == "Stock").Select(name => name.Value).FirstOrDefault();
        string brush = properties.Where(name => name.Key == "Brush").Select(name => name.Value).FirstOrDefault();
        string mask = properties.Where(name => name.Key == "Mask").Select(name => name.Value).FirstOrDefault();
        string r_brush_color = properties.Where(name => name.Key == "R_brush_color").Select(name => name.Value).FirstOrDefault();
        string g_brush_color = properties.Where(name => name.Key == "G_brush_color").Select(name => name.Value).FirstOrDefault();
        string b_brush_color = properties.Where(name => name.Key == "B_brush_color").Select(name => name.Value).FirstOrDefault();
        string base_b = properties.Where(name => name.Key == "Base").Select(name => name.Value).FirstOrDefault();
        string r_brush_locked = properties.Where(name => name.Key == "R_brush_locked").Select(name => name.Value).FirstOrDefault();
        string g_brush_locked = properties.Where(name => name.Key == "G_brush_locked").Select(name => name.Value).FirstOrDefault();
        string b_brush_locked = properties.Where(name => name.Key == "B_brush_locked").Select(name => name.Value).FirstOrDefault();
        string baseLocked = properties.Where(name => name.Key == "BaseLocked").Select(name => name.Value).FirstOrDefault();

        var DictionaryMaskReplacements = new Dictionary<string, string>()
        {
            {"[internalName]", internalName },
            {"[stock]", stock },
            {"[brush]", brush },
            {"[mask_temp]", mask },
            {"[red_brush_color]", r_brush_color },
            {"[green_brush_color]", g_brush_color },
            {"[blue_brush_color]", b_brush_color },
            {"[base]", base_b },
            {"[red_brush_locked]", r_brush_locked },
            {"[green_brush_locked]", g_brush_locked  },
            {"[blue_brush_locked]", b_brush_locked },
            {"[base_locked]", baseLocked }
        };
        return DictionaryMaskReplacements;
    }
    }

I have an awful lot of hardcoded LINQ queries stacked one after another in the DictionaryReplacements() method that you can see in the struct and I would really want to reduce my queries because it does not feel very productive and it feels like it is badly written code when you repeat the same code over and over like in this example. I would really like it if anyone could point me to a solution where I do not need so many queries. There must be some way I can "automate" the code, perhaps with loops?.

properties that you see in the DictionaryReplacements() method is the dictionary that holds the return results from the listOfProperties() method

Example of the text file:

{
   _internalname:                  [internalName]
   _stock:                         [stock] 
   _mask_temp:                     [mask_temp]
   _red_brush_color :              [red_brush_color] 
    etc...
}

The name in the brackets is the "template value" that will be replaced with the actual values

\$\endgroup\$
  • 1
    \$\begingroup\$ This isn't any better than your last question :-\ \$\endgroup\$ – t3chb0t Oct 29 '18 at 20:29
  • \$\begingroup\$ To those voting to close: there does seem to be sufficient context here (OP is substituting placeholders in a file based on property values). \$\endgroup\$ – Pieter Witvoet Oct 30 '18 at 10:20
  • 2
    \$\begingroup\$ @PieterWitvoet as stated in the delted question (1:1 the same) OP has provided edited code that wasn't even validated in an IDE and cotained errors. Besides OP has recived a very helpuf answer despite the poor quality of the question. OP could actually have accepted it. I don't understand why they didn't do it. It was quite good. \$\endgroup\$ – t3chb0t Oct 30 '18 at 10:24
  • \$\begingroup\$ @t3chb0t: deleted questions are invisible to <10K users... and the code shown here does compile (despite reading very much like an XY problem). Oh well. \$\endgroup\$ – Pieter Witvoet Oct 30 '18 at 10:43
3
\$\begingroup\$

So your actual goal is to substitute placeholders in a file, based on property values. The code you have shown is a lot more complicated than it needs to be.

First, to address your immediate question:

  • In DictionaryReplacements, properties is a dictionary, so you can perform key lookups directly. There's no need for those Where(..).Select(..).FirstOrDefault() constructions at all.
  • For that key lookup, you may want to use the nameof operator: properties[nameof(Files.InternalName)]. The benefit should be immediately clear: it reveals that Files is missing a R_brush_locked property.
  • But instead of creating an intermediary dictionary, you could use a Files instance directly: {"[internalName]", files.InternalName}, so you don't need to use reflection anymore.
  • You might as well create that dictionary in a method in your Files class, there's no need for that Replacements struct at all.
  • Inside CreateDefaultPart, you're calling DictionaryReplacements each time your regex finds a match. Just call it once and store the result in a local variable to prevent doing a lot of duplicate work.

Other notes:

  • It's good to see some documentation, but try documenting the 'why' instead of the 'what'. Things like 'Creates an instance of the struct' are already obvious from the code itself.
  • CreateDefaultParts isn't a very descriptive name - there's no hint that this will create a file, or that it needs a template file, or what the 'default' in the name applies to.
  • I'd also expect that method to accept file paths as arguments, instead of having them hardcoded.
  • As for the destination filename, having that as a property is confusing, because all properties are used to substitute values. That's not clearly documented, and it's easy to forget to give it a value. A method parameter on the other hand gives a clear signal: 'hey, you need to specify a destination filename!'.
  • Why is the return type of DictionaryOfProperties dynamic? There's no obvious reason for that, and it does come with additional runtime costs and a loss of compile-time checks. Unless you're doing interop with a dynamic language you'll rarely if ever need to use dynamic.
  • Inside DictionaryOfProperties, there's some odd newline placement inside that for loop. If you want to split it up across multiple lines, at least keep each argument on the same line. Also, I'd add braces here to prevent confusion. Finally, PropertyInfo.Name is already a string - no need to call ToString() on it.
\$\endgroup\$

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.