Thread: Please suggest a better algorithm..

Originally Posted by manasij7479

Wouldn't

Code:

for(VI it1=st[temp.v1[1]].begin();it1!=st[temp.v1[1]].end();it1++)

look more cumbersome if written as

Code:

for ( VI it1 = st[ temp.v1[1]].begin() ; it1 != st[temp.v1[1]].end() ;  it1++)

?

Absolutely not. It provides visual separation of tokens. It makes it much easier to see where the three parts of the for head begin and end.
Although you're overdoing it a bit. I'd write it like this:

Code:

for (VI it1 = st[temp.v1[1]].begin(); it1 != st[temp.v1[1]].end(); it1++)

Whitespace placement is a matter of taste, really, but I don't know anyone who thinks that removing as much as the language allows is a good idea.

Of course, you get a more effective readability improvement by factoring out that horrible nested indexing. Since you're using <array> I'm going to assume your compiler supports auto.

Code:

    for (c = 10; (c * c) < 1000; c++)
    {
        temp.v1 = itotr(c * c);
        auto& a1 = st[temp.v1[1]];
        for (VI it1 = a1.begin(); it1 != a1.end(); it1++)
        {
            temp.v2=*it1;
            auto& a2 = st[temp.v1[2]];
            for(VI it2 = a2.begin(); it2 != st[temp.v1[2]].end(); it2++)
            {
                temp.v3=*it2;
                if(temp.v2[2]==temp.v3[1])
                {
                    mc++;
                    storage.push_back(temp);
                }
            }
        }
    }

That still leaves your horrid naming. What is up with this?

Code:

class Tr //triad

Seriously? Is your need to save three keystrokes per use of this class so big that you can't just call the class Triad? It's members are called disp() (What's wrong with display()? Or print(), which is shorter, and closer to typical computer terminology?), v1, v2, and v3. What are v1, v2 and v3? For that matter, what is a triad?

Code:

itotr // int to triad row

You're basically confusing comments with names. In the above snippet, the comment should be the name, and the actual comment should say something like, "Given a three-digit number, return an array containing the three digits."
Then add an assertion to the function that validates that the number is actually in the expected range.

Why is storage not called "results"? Every variable stores something. The special thing about this one is that it stores the actual results of your program.
What is mc? Inspection of your program tells me that it is the result count. Call it resultCount. And then remove it, because you realize that it is always equal to results.size().

Typedefs would also help. A typedef of array<int, 3> to Row or TriadRow would go a long way towards making the type vector<array<int, 3>>[10] palatable. Of course, giving st a name that indicates what it is (a map from first digit to possible rows, try "possibleRowsByFirstDigit" or "rowsStartingWith") would go even further.

Basically, your code is extremely cryptic because you want it to be compact. That's not a good habit.

understand your first code snippet

I didn't post the first snippet to be thoroughly read... The sentence above it says that it was to show that the code looks really clumsy.

Stick to n-column rule EmacsWiki: Eighty Column Rule.

Could this be implemented with less than 3 levels of indentations?...+ It shows far less than 80 cols in my ide...with font set to 18..

Whitespace placement is a matter of taste

In my case.. a habit ..after using a netbook for a year.. Maybe I'll grow out of it when permanently using a larger screen..

I consider netbooks to be unusable for programming. But I guess you can't always choose your tools.

Programming on netbook?! Lol.

They are usable....at least when programming is a hobby....as in my case.....

As you can see, even on such a low level, they are ususable. You aren't even able to read 80-column lines.

You're basically confusing comments with names. In the above snippet, the comment should be the name, and the actual comment should say something like, "Given a three-digit number, return an array containing the three digits."
Then add an assertion to the function that validates that the number is actually in the expected range.

Why is storage not called "results"? Every variable stores something. The special thing about this one is that it stores the actual results of your program.
What is mc? Inspection of your program tells me that it is the result count. Call it resultCount. And then remove it, because you realize that it is always equal to results.size().

Typedefs would also help. A typedef of array<int, 3> to Row or TriadRow would go a long way towards making the type vector<array<int, 3>>[10] palatable. Of course, giving st a name that indicates what it is (a map from first digit to possible rows, try "possibleRowsByFirstDigit" or "rowsStartingWith") would go even further.

Basically, your code is extremely cryptic because you want it to be compact. That's not a good habit.

Thanks, I was really adhering to some bad principles.

Could you elaborate on the "factoring out the indent" part ?
according to wikipedia... auto seems to declare 'callable' types ...Wth is that ?

Basically auto means that the compiler determines the type of the variable based on the initializer. It's a new feature in C++11 and thus only supported by very new compilers.

Without auto, it would look like this:

Code:

array<int, 3>& a1 = st[temp.v1[1]];

Correctly applying spaces means that to most of us it should look like this:

Code:

for (VI it1 = st[temp.v1[1]].begin(); it1 != st[temp.v1[1]].end(); ++it1)

You don't just put them in random places. Removing whitespace prior to review is ridiculously back-to-front.

Edit: Hmm, I should have noticed the second page before posting.

Originally Posted by manasij7479

Correct me if I'm wrong:
Does

Code:

for (VI it1 = st[temp.v1[1]].begin(); it1 != st[temp.v1[1]].end(); it1++)

become

Code:

for (auto it1 : st[temp.v1[1]] )

?

Not quite. First, of course, your compiler has to support the for-range loop, and MSVC, at least, doesn't yet. (GCC and Clang do.)
Second, the declaration in the for-range loop is for the element type, not the iterator type, so the name it1 is no longer appropriate, and you should probably use a reference. The auto in your code expands to array<int, 3>, and you probably don't want to copy the array. So you would do

Code:

for (const auto& row : st[temp..v1[1]])

With const auto row, auto resolve to array<int, 3>, so the code is equivalent to

Code:

for (const array<int, 3> row : st[...])

Since this row is not a reference, there will be a copy. That's just how C++ works.