• Hannes (unregistered)

    I'm a fri-st?

  • Hannes (unregistered)

    TRWTF is that "O(n²)" is being displayed as "O(n2)", making this loop quite less expensive.

  • thridy (unregistered)

    @Hannes You meant "Am I fri-st?"

  • Damon (unregistered)

    Made me think WTF for convoluated logic, with no comments. So if the interest rate for a given year is not defined, we just use the highest year or what?

  • Foo AKA Fooo (unregistered) in reply to Hannes

    I don't know yet. I'm still iterating through all comment IDs from 1 to 473649, looking up each comment on this web site until I get to the first one of today's article. Then I'll know if it's yours.

    O(n^2) instead of O(n) is bad enough, but rather common. O(n^2) instead of O(1) is really WTFy.

  • gleemonk (unregistered)

    Are C# dicts ordered in some way? It's not clear to me what Last() should do on a dictionary. Return the entry with the highest key? The last entered element?

  • Hannes (unregistered) in reply to thridy

    I was iterating through all letters in "I am frist?" and this is what my "logarithm" came up with. Wonder where it got the ' from though.

  • (nodebb)

    Arrays? Bahaha... Are you still using Solaris or something? Arrays are soooo old, you have to use dictionaries for everything because it is totes the latest fashion in coding (takes a sip of latte)

  • Alistair (unregistered)

    It is not that bad - it is mapping years to interest rates, not interest rates to years.

  • SolePurposeOfVisit (unregistered)

    Unless this company has been around for a hundred years or more, the algorithmic cost of this thing is of no consequence whatsoever. To take an extreme example, you can even brute-force the Traveling Salesman problem without much pain, so long as n < 10 or so. Let's not encourage premature optimization, shall we?

    Having said which, this code is almost defiantly ugly. No question: it should die in a fire.

  • bvs23bkv33 (unregistered)

    who is cow orcer? its about orcs or orcas?

  • Slurpee (unregistered) in reply to gleemonk

    No, the order of elements in a Dictionary is non-deterministic in C# - It's using a hash set under the covers, but it is IEnumerable, so .Last() will return the item at the "right side" of the underlying tree. There is an OrderedDictionary class if you actually need the best of both worlds, but its less performant.

  • Quite (unregistered) in reply to bvs23bkv33

    What's the matter with you? Never seen a lady ork a cow before?

  • Erik (unregistered) in reply to SolePurposeOfVisit

    While I agree that the algorithmic cost is a non issue the fact that dictionaries are unordered is a serious problem - because you selecting the interest in an arbitrary year that came no later than the given operatingYear

  • Bert (unregistered)

    So, they wanted .Item instead of .ElementAt? Sounds like TRWTF is the method names on Dictionary.

  • Peter (unregistered)

    This might be the single worst piece of code ever posted on this site. The WTF to line ratio is incredible!!

  • Erik (unregistered) in reply to Bert

    If the article is correct and they where guaranteed a rate at every year they wanted yearToInterestRates[operatingYear]. If they needed to account for the possibility of years where the rate was missing, and they wanted to assume that in this case the previous rate applies then they wanted to use an OrderedDictionary

  • Anonymous Coward (unregistered)

    The REAL WTF is that the cost of a Dictionary lookup is not O(1). It's O(log n).

  • iWantToKeepAnon (unregistered) in reply to Alistair

    Alistair (unregistered) It is not that bad - it is mapping years to interest rates, not interest rates to years.

    Right, that was my thought. "map interest rates to years" means:
    $m->{5.5%} => [1980, 1997, 2014];

  • Herby (unregistered)

    I can see it now:

    New (non WTF) worker: Boss, this code is terrible, I want to make it better.

    Boss: Does the code work? What is the risk of changing it? What happens if it is wrong?

    New guy: Yes, it works, I don't know what the risk is. It is pretty simple, I think I can get it right.

    Boss: Sorry, too much risk. It works, leave it alone.

    New guy: But, but but.... (SIGH)

    Another WTF code segment carved in stone.

  • M (unregistered) in reply to Anonymous Coward

    Au contraire, dictionary lookup can be (and usually is) done via hash tables which really do have O(1) performance. I thought this was pretty close to magic when I first heard about it, but it's true and in fact pretty straightforward. You can implement dictionary lookup with O(log n) tree search, but there's not a lot of point unless you care about the order of items.

  • Dan (unregistered) in reply to M

    It's still not O(1). No matter how a dictionary is implemented, you still have to worry about the case where the first item you find due to your hash isn't the correct one, and then you have to search some more. This effectively makes the search O(f(n)) where f(n) is some function based on your hash table scheme. The pure hash table would be O(n) worst case. The bucket table would be closer to O(log(n)) depending on how many buckets there are and how it decides to expand the bucket count.

  • Lazerbaems (unregistered)

    I just wrote a test console app which verifies that this only works so long as the dictionary is initialized in order from year zero. At least on my machine. Also, even with that initialization order, the function still returns results such that FindInterestRate(year, rates) == rates[Math.Min(year+1, YEAR_COUNT-1)]

  • Paul Neumann (unregistered)

    Check the code one more time. It's not compatible with using an array. It is returning the entry value prior to the first entry where the requested operationYear is less than key - 1. An array would not allow you to skip and shuffle operationYears in order to get entirely new results.

  • M (unregistered) in reply to Dan

    Generally all that's done during creation of the dictionary. The actual search itself should be done with a hash chosen (during the creation of the dictionary) to have no collisions, and as you correctly note this creation process will not be constant time. However, once that's done, subsequent searches will be O(1) until further modification of the dictionary.

  • Sh= (unregistered)

    1. This is designed to cope with sparse data (not all years have rates); without storing nonsense values in the dictionary (you can't do this cleanly in a C# array; unless you have an array of nullable?)

    2. This is designed to give the interest rate in effect in a particular year; even if that year does not have an explicitly defined interest rate; in which case it gives the most recent interest rate. Be sure about the 'goals' of you code, it looks like this was written by someone that knew a tad more about the situation (and what was likely to happen down the track) than you did, when dealing with flaky and incomplete data is a requirement.

    A simple array does NOT have this behaviour. Don't be so quick to judge others.

    1. Programmer time is more important. What's the actual run time for this? Does that matter? Shouldn't you be chasing after actual bugs instead?

    private static double FindInterestRate(int operationYear, Dictionary<int, double> yearToInterestRates) //where 0 is the first year { // Search backwards for the most recently defined year while (operationYear > 0 && !yearToInterestRates.HasKey[operationYear]) { operationYear--; }

    if (operationYear < 0)
    return 0;
else {
    return yearToInterestRates[operationYear];
}

    }

  • Sh= (unregistered) 
    private static double FindInterestRate(int operationYear, Dictionary<int, double> yearToInterestRates) //where 0 is the first year
{
    // Search backwards for the most recently defined year
    while (operationYear > 0 && !yearToInterestRates.HasKey[operationYear]) {
        operationYear--;
    }

    if (operationYear < 0)
        return 0;
    else {
        return yearToInterestRates[operationYear];
    }
}
  • ooOOooGa (unregistered)

    If memory serves me correctly, Dictionary objects in C# can be indexed like arrays. So performance aside, wouldn't the non-WTF function be something along the lines of: private static double FindInterestRate(int operationYear, Dictionary<int, double> yearToInterestRates) //where 0 is the first year { return yearToInterestRates[operationYear]; }

    Which probably isn't worth implementing as a separate function call.

    So yeah. With a 100% ratio of lines-of-code to WTF, this probably at least ties for highest.

  • Sh= (unregistered) in reply to ooOOooGa

    ooOOooGa: Depends -- if it truly is a 1:1 mapping from year to interest rate, then yes, don't even bother with the method call. BUT this is clearly written to answer a different question - "what is the interest rate in effect in operationYear, even if that interest rate was defined in a previous year and has not been explicitly set for the year in question."

    Given that extra complexity, a method call is (in my opinion) justified; especially since the .Net CLR's JIT compiler might inline it anyway.

  • siciac (unregistered)

    I see this all the time: {v : k for k, v in some_dict.items()}.get(whatever, 'default')

    Sometimes it might be a micro-optimization, but I think people genuinely don't realize that object construction isn't free. I've seen smart people do this in a loop.

    If it really makes more sense to scan the dictionary, at least be lazy as possible, e.g. next((k for k, v in some_dict.items() if k == whatever), 'default')

  • Bert (unregistered) in reply to Erik

    Yeah, array[index] is sugar for array.Item(index). Seems like they wouldn't even think to account for a missing year, because what years are ever missing, right?

  • Alistair (unregistered) in reply to Bert

    If the period spans BCE to CE, there should be no year 0.

  • John S (unregistered) in reply to Sh=

    Are you saying there's some years that don't have an interest rate?

  • (nodebb) in reply to SolePurposeOfVisit

    "defiantly ugly"?

    Is that someone who refuses to wear make-up, brush their hair or bathe?

  • Daniel (unregistered)

    My company hired some outsourced programmers to write an API. On a previous project, I taught them to use Dictionaries instead of 2 arrays when mapping keys to value. On this project, they needed a collection with the index of each element (which is exactly what an array provides). Instead, the API they wrote required users to pass in a Dictionary<int, string>, with the index as the key...

  • (nodebb) in reply to campkev

    They deliberately use the makeup to highlight their acne.

  • Gustav (unregistered)

    IMHO another just as big WTF is not returning an error or raising an exception for inputing invalid data, i.e. years for which no interest rate was recorded. Also combined with having completely inconsistent range checking. For years < 0 the interest was 0, but for years in the future, interest is the same as the last recorded year.

    I have seen too many cases of code trying to halfheartedly salvage broken input and in the process just causing more damage upstream, when they really should have failed hard instead.

  • Bert (unregistered) in reply to Alistair

    Well, I was joking, but the epoch in use is the company's first year of operation.

  • jport (unregistered) in reply to Anonymous Coward

    btree the dictionaries!

  • siciac (unregistered) in reply to Daniel

    Instead, the API they wrote required users to pass in a Dictionary<int, string>, with the index as the key...

    If OO actually worked, arrays could just implement that interface.

  • jerzyszczur (unregistered)

    False. It is not my name. It is my comment, though.

Leave a comment on “Dictionary Definition of a Loop”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article