7
\$\begingroup\$

I have this function that takes a string which represents a html-color and returns the OLE value for it. If the string is null or empty or it can't be parsed, it uses a default value.
However I have to write that one line containing the default value twice and I don't see how I could remove this redundancy in the best possible way.

I know this isn't a big deal but I am curious to see what you guys might suggest for this :)
What do you think is the best possible way to remove this redundancy?

private int GetOleFromHTML(string stringRep)
{
    Color c;
    if (string.IsNullOrWhiteSpace(stringRep))
    {
        c = Color.FromArgb(224, 224, 224);
    }
    else
    {
        try
        {
            c = ColorTranslator.FromHtml(stringRep);
        }
        catch
        {
            c = Color.FromArgb(224, 224, 224);
        }
    }

    return ColorTranslator.ToOle(c);
}
\$\endgroup\$
5
\$\begingroup\$

Since a method has to be executed to get this color, you cannot declare the color as constant. A static readonly field is what comes closest to a const.

private static readonly Color DefaultColor = Color.FromArgb(224, 224, 224);

private int GetOleFromHTML(string stringRep)
{
    Color c;
    if (string.IsNullOrWhiteSpace(stringRep))
    {
        c = DefaultColor
    }
    else
    {
        try
        {
            c = ColorTranslator.FromHtml(stringRep);
        }
        catch
        {
            c = DefaultColor
        }
    }

    return ColorTranslator.ToOle(c);
}

Another approach would be to directly store the OLE color as int.

const int DefaultOleColor = 14737632; // R=224, G=224, B=224

private int GetOleFromHTML(string stringRep)
{
    if (string.IsNullOrWhiteSpace(stringRep))
    {
        return DefaultOleColor;
    }
    try
    {
        return ColorTranslator.ToOle(ColorTranslator.FromHtml(stringRep));
    }
    catch
    {
        return DefaultOleColor;
    }
}

Or calculated explicitly:

const int DefaultOleColor = 256 * (256 * 224 + 224) + 224;

Or with bit shift operations

const int DefaultOleColor = 224 << 16 | 224 << 8 | 224; // My preferred variant.

These expressions can be used to initialize the constant as they can be fully evaluated at compile time.

You can test these variants easily in the Immediate Window of Visual Studio. You must qualify the names with the namespaces for the first variant (System.Drawing.ColorTranslator.ToOle(System.Drawing.Color.FromArgb(224, 224, 224))).


Avoiding repetition is not the only reason for having constants. Constant declarations give a name to otherwise "magic" values. See Magic number (programming) (Wikipedia).

This variant avoids the repetition by restructuring the code, but still uses a constant.

const int DefaultOleColor = 224 << 16 | 224 << 8 | 224;

private int GetOleFromHTML(string stringRep)
{
    if (!String.IsNullOrWhiteSpace(stringRep))
    {
        try
        {
            return ColorTranslator.ToOle(ColorTranslator.FromHtml(stringRep));
        }
        catch
        {
        }
    }
    return DefaultOleColor;
}
\$\endgroup\$
  • \$\begingroup\$ This answer keeps getting better with every edit :) You already have my vote. One question though: do you think the catch could somehow be prevented? \$\endgroup\$ – dfhwze Aug 19 at 14:57
  • 1
    \$\begingroup\$ Here you can see the Reference Source of the ColorTranslator.FromHtml(String) Method. It is quite complex as it accounts for different formats of HTML color. An attempt to avoid these exceptions would be at least as complex. Assuming the exceptions occur rarely, it is not worth the candle. \$\endgroup\$ – Olivier Jacot-Descombes Aug 19 at 15:11
  • \$\begingroup\$ Ah I was afraid it would be too complex. Thanks for this feedback. \$\endgroup\$ – dfhwze Aug 19 at 15:12
  • \$\begingroup\$ I really like the const-int solution. Since this is only used internally at a single point in a small program, I think I will go with this but I also have to give credit to techbots answer because I think if this method were in a library or in a bigger project where it is also used more, their approach seems more clean. Btw nice references and explanations. \$\endgroup\$ – Joelius Aug 20 at 16:42
5
\$\begingroup\$

Another option, though not without caveats, is to reverse the logic. Something like this should work:

private int GetOleFromHTML(string stringRep)
{
    Color c = Color.FromArgb(224, 224, 224);
    if (!string.IsNullOrWhiteSpace(stringRep))
    {
        try
        {
            c = ColorTranslator.FromHtml(stringRep);
        }
        catch
        {
            //Use default color value if the string is invalid.
        }
    }

    return ColorTranslator.ToOle(c);
}
\$\endgroup\$
  • \$\begingroup\$ Good idea. I also thought about this later on but didn't persue the idea because I thought maybe I'd be able to remove the redundancy without creating the default color before I even know if I have to use it or not. The const solution seems to be the middle-ground in this case. \$\endgroup\$ – Joelius Aug 20 at 16:46
5
\$\begingroup\$

You have difficulties getting rid of the repeated code because you're trying to force two different operations into one:

  • parsing html
  • converting the result into OLE color

Start with extracting the default value and make it a field:

static readonly Color DefaultColor = Color.FromArgb(224, 224, 224);

Then create the main API and give it a name that clearly communicates that it does. Now that the parsing has been extrated, you can as simple ternary operators here:

public Color ParseHtmlColorOrDefault(string value, Color defaultColor)
{   
    return     
        !string.IsNullOrWhiteSpace(value) && TryParseHtmlColor(value, out var color)
            ? color
            : defaultColor;
}

The parsing with its try/catch as the TrySomething pattern:

private static bool TryParseHtmlColor(string value, out Color color)
{
    try
    {           
        color = ColorTranslator.FromHtml(value);
        return true;
    }
    catch
    {
        color = Color.Empty;
        return false;
    }
}

Finally, create an extension for the final step of converting Color to OLE color:

public static int ToOle(this Color color) => ColorTranslator.ToOle(color);

Use it like that:

var oleColor = ParseHtmlColorOrDefault("#FFAABB", DefaultColor).ToOle();
\$\endgroup\$
  • 1
    \$\begingroup\$ Great points, thank you. Btw I don't really think you should have added the beginner tag to my question because I like to think that I know C# and I'm not new to the language. I know that there are several ways this redudancy could be remove but I wanted to put this here so I could learn more about what more experienced people than me would consider as the best solution for this (I'm by no means an expert). Also regarding your solution, since in my program this code is only used in one place and not publicly exposed, would you say it's okay to just use the const-int solution of Olivier? \$\endgroup\$ – Joelius Aug 20 at 16:36
  • \$\begingroup\$ @Joelius it was just my gut feeling, this time it was wrong ;-] I removed it... you could have to, it's ok to edit the tags. Sure, you can combine the powers of all answers and build what works best for you. I just didn't want to repeat the other answers. \$\endgroup\$ – t3chb0t Aug 20 at 17:35
  • 1
    \$\begingroup\$ Alright, I'll accept Oliviers answer because I'm going to use something along the lines of his const-int solution. However, I have to give credit to all three answers, all very insightful, non repetitive and well written, thank you all :) \$\endgroup\$ – Joelius Aug 20 at 17:46

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.