Rob, I think the problem is that you are assuming human beings are rational/reasonable. Where I am currently, I have to fill out a form and get authorization to fix a simple memory leak. The code base is several million lines of C++, suffering from 20 years of technical debt. Since I already have a reputation for being "too critical about code quality" which causes my input to get knocked down a level or two, I have to bite my tongue a lot. It's a grand learning experience, but I'll be glad when I figure out what the lesson is!

So if I have this straight, in your example:

if(UsernameTextBox.Text == "Manager" & PasswordTextBox.Text == "Maintenance")

because both arguments are boolean, the '&' is effectively acting just like a '&&' except for being trivially less efficient because it is always doing both of the string compares. I know in this case you were merely quoting previous post using '&', but even if the non-short-circuit behaviour would be useful sometime, I'd avoid it because it just looks wrong to me. We're in a mixed C++/C# environment here, and I have to be on the lookout for misused '&'s in the code as it is. Allowing for false positives is not in the cards here. That said, I think you got a raw deal.

Yes and if the enum values are all disjoint - like they are for a set of flags or possibly for a field in a binary structure - then the code to validate the value would essentially have to do a search every time an enum is assigned. That's a tax I would not want to have to pay just because other developers can't be trusted. (As if I were perfect...) And the enum-as-flag idiom comes from C++ where the only way to declare a const int value in a class scope was to use an enum. Bonus info: The compiler I use (Microsoft C++) lets you declare the size of the integer underlying the enum. So for case where I need more than 32 flags, I can write:

enum MyBigEnum : __int64 {
MBE_Unknown = 0,
MBE_QualityFlag = 0x0000000001,
MBE_EnergyFlag = 0x0000000002,
// ...
MBE_MyLastFlag = 0x4000000000,
};

-- T

I hate to admit it, but I've found code like that here from a previous developer. It's also in C#, but the delimiter is a newline. Maybe you have our old developer?

public string FormatBarContent(string foo1,
string foo2, string f003,
// ...
string foo14,
)
{
StringBuilder sb=new StringBuilder();
sb.Append(foo1);
sb.Append("\n");
sb.Append(foo2);
sb.Append("\n");
// ...
sb.Append(foo14);
// sb.Append("\n");
return sb.ToString();
}

And some of the foo's have been converted from numbers to strings. The routine that gets this string immediately Split()'s it and parses the numbers back into numbers. None of the data ever needs to leave the current thread. Don't they know what a struct is? -- T

Megidolaon wrote:

What is the point of

    if (node.Attributes\["Publish"\].InnerText.Trim().Contains(s))
    {
        return;
    }

A check for containting a single letter and then doing nothing?

No, that loop checks if the node text contains one of the non-translatable strings and if so, bails.

Megidolaon wrote:

Maybe the XML contains trailing spaces? Unless you trim them, the XML Nodes' InnerText wouldn't equal the DictionaryEntries with same

You are correct, however within a given call to TranslateNode(), the xml node stays the same. Until it gets translated from the dictionary. Oh crap, there's yet another bug in there that I hadn't noticed - it can double translate. Anyway, the phrase node.Attributes["Publish"].InnerText.Trim() should be invariant, and I assume it takes some processing. So a line like:

string xmlText = node.Attributes\["Publish"\].InnerText.Trim();

and then refering to that string wold have been more efficient,and easier for me to read, I think. -- T

qualitychecker wrote:

More safe and maintainable code: ------------------------------------ bool res = false; if (null == dt) else if (null == dt.Rows) else if (dt.Rows.Count < 0) else res = (1 == (int)dt.Rows[0]["Number"]); return res; ------------------------------------

Oops - hate to say this, but if dt.Rows.Count == 0 - i.e. the table is empty - then that code will throw an array-out-of-bounds exception. You meant else if (dt.Rows.Count <= 0), right? -- T

Oddly enough, I just got rid of some code that looked a bit like that...

bool InitRoutine()
{
bool failed=false;

ValType val;

HRESULT ans = GetValue1(val);
if (ans==S\_OK)
    {
    globalVal1 = val;
    }
else
    {
    failed = failed || true;
    }

ans = GetValue2(val);
if (ans==S\_OK)
    {
    globalVal2 = val;
    }
else
    {
    failed = failed || true;
    }

ans = GetValue3(val);
if (ans==S\_OK)
    {
    globalVal3 = val;
    }
else
    {
    failed = failed || true;
    }

ans = GetValue4(val);
if (ans==S\_OK)
    {
    globalVal4 = val;
    }
else
    {
    failed = failed || true;
    }


return !failed;
}

Simply putting an if (nonTranslatable.Count == 0) block around the code in the NonTranslatable property took the run time down from >75 minutes to ~30 seconds. (Fun fact: During the 75 minutes of processing, those repeated Add() calls caused the program to take up about 25MB of extra memory. That's a lot of strings to loop through and compare repeatedly. :omg: )

PIEBALDconsult wrote:

And how about removing the .ToString and .Equals ?

Not to mention all the calls to Trim() inside the loop. Or does the runtime optimize those out as invariant? I ended up just dumping all their code and using my own. To add another wrinkle they wrote it in VS 2008, and we are still on 2005 until we get this release out, so I couldn't even build their solution. Besides there were some other issues. Not only was it still 10x slower than my rewrite, but the output wasn't quite correct. The problem is that there was also a non-XML text file that needed to be parsed - imagine taking a Windows .rc file and extracting all the strings that can be translated from it and putting them into an Excel file. They weren't being careful enough with their regular expressions, so some things were getting missed and some extra things were being found. Using .net 2.0 and a Dictionary<string,> worked fine for me. By making the regular expressions more correct, I didn't need the NonTranslatable array at all. (The two loops I showed were actually in two separate routines, but I wanted to keep my post relatively compact.) I've vented - I feel better now. -- T

Our marketing department hired a new web developer, and they were given a task to write some code for our department. Here are a couple excerpts:

// Names have been changed to protect the guilty...
//
namespace OMG
{
static class TranslationClass
{
static ArrayList nonTranslatable = new ArrayList();

    public static ArrayList NonTranslatable
    {
        get
        {
            nonTranslatable.Add("1st String");
            nonTranslatable.Add("2nd String");
            // ...
            nonTranslatable.Add("30th String");
            return nonTranslatable;
        }
    }

    private SortedList translationDictionary = new SortedList();

    // more stuff here to fill the translationDictionary

    public void TranslateNode(XmlNode node)
    {
        foreach (string s in NonTranslatable)
        {
            if (node.Attributes\["Publish"\].InnerText.Trim().Contains(s))
            {
                return;
            }
        }

        foreach (DictionaryEntry de in translationDictionary)
        {
            if (de.Key.ToString().Equals(node.Attributes\["Publish"\].InnerText.Trim()))
                node.Attributes\["Publish"\].InnerText
                        = translationDictionary\[node.Attributes\["Publish"\].InnerText.Trim()\].ToString();
        }
    }
}

}

When I asked them to run their code on the sample dataset, I was told they'd need to run it over-night. Looking at their code, I could see why. So I wrote my own version of the utility, and timed them versus each other. Their program took an hour and a quarter to process a 2MB text file; mine took 2 seconds. The biggest problem was that every time that NonTranslatable was used, new, redundant elements were being added to it, and the loop took longer and longer to run. Also they were looping through the dictionary instead of using something like ContainsKey(). Argh!