Re: any suggestion to improve the following code?

On Jan 2, 9:03 pm, Fei Liu <fei....@gmail.com> wrote:


Not deprecated; there was never a "standard" hash_map. And
while most C++ compilers do support some sort of hash table,
there are subtle differences between them.

The next version of the standard will contain an
std::unsorded_map, but until then, you're on your own, at least
if you want to be portable.


It's not hard to write. (There's one at my site, for example.)


unordered_map is presumably an implementation of the draft
standard, and definitly to be preferred if it is available.


How many elements? I've found that for up to a thousand or so,
it really doesn't matter.

Note too that the complexity of a hash map depends on the
quality of the hashing function. Use a bad hashing function,
and it rapidly becomes O(n).


Personally, I don't like the "using namespace". I'd generally
write std:: wherever needed (and alias __gnu_cxx to something
like gcc). And I definitly wouldn't use a "using namespace"
until after all of the include files have been processed.


I'm not familiar with the algorithm, so I'll skip that part.
    [...]


The above seems a bit heavy. What's wrong with just:

    std::ifstream inf( "record_h.txt" ) ;
    if ( inf ) {
        // read file...
    }

Note that the way you read the file in your loop is a recepe for
problems---recompile the code with different options or a
different version of the compiler, and you may read different
values than you wrote. The simplest solution would be to define
a text format for the data, and read and write it as text. This
will also simplify debugging enormously. If you do want to use
a raw memory image (which is probably acceptable since you
haven't lost anything critical if you can't read it), then
prefix it with some sort of signature, which is regenerated
(differently) every time you relink your code.

And of course, you really want this part in a separate function.
Or even a separate class, which encapsulates all of your file
cache management. (If you read and write in the same class,
then it's easy to modify the file format.)


y


I'm not sure what your trying to do here. If you only need two
integers, and you always need two integers, wouldn't it be more
appropriate to pick them up from the command line. Something
like:

    int
    asInt( char const* arg )
    {
        int result ;
        std::istringstream tmp( arg ) ;
        tmp >> result >> std::ws ;
        return tmp && tmp.get() == EOF
            ? result
            : 0 ;
    }

    int
    main( int argc, char** argv )
    {
        if ( argc != 3 ) {
            std::cerr << argv[ 0 ] << ": syntax is: "
                      << argv[ 0 ] << " i j" << std::endl ;
            return EXIT_FAILURE ;
        }
        int i = asInt( argv[ 1 ] ) ;
        int j = asInt( argv[ 2 ] ) ;
        if ( i == 0 || j < i ) {
            std::cerr << argv[ 0 ] << ": illegal values" <<
std::endl ;
            return EXIT_FAILURE ;
        }
        // ...

Note that it's probably best to do this before trying to read
the file with the cached values. No point in reading the file
if all you're going to do is terminate with an error.

Also, of course, if you're doing any amount of programming,
you'll quickly write some generic code to handle the command
line arguments, and use it here.

If you really have to read the two values from std::cin, then
I'd define a somewhat stricter format, say both in one line,
use std::getline() to read the data, and std::istringstream to
parse it. This avoids all problems with handling an error state
in the input or resynchronizing. Something like:

    int i = 0 ;
    int j = 0 ;
    while ( i == 0 ) {
        std::string s ;
        std::getline( std::cin, s ) ;
        if ( ! std::cin ) {
            std::cerr << argv[ 0 ]
                      << ": fatal error on standard in"
                      << std::endl ;
            return EXIT_FAILURE ;
        }
        std::istringstream tmp( s ) ;
        tmp >> i >> j >> std::ws ;
        if ( ! tmp || tmp.get() != EOF || i == 0 || j < i ) {
            std::cerr << argv[ 0 ]
                      << ": illegal input, try again"
                      << std::endl ;
            i = j = 0 ;
        }
    }

    [...]


or it = steps.begin();

Are you sure you didn't want to use a typedef for the type of
the map?


Again, this more or less guarantees that you won't be able to
reliably read the data later, if you recompile the program for
any reasons.


    if ( ! ofs ) {
        std::cerr << argv[ 0 ]
                  << ": fatal error writing cache file"
                  << std::endl ;
        remove( "record_h.txt" ) ;
        return EXIT_FAILURE ;
    }

I'm not sure about the return here---since it's only a cache
file, you might want to just note the error, without aborting
because of it.

The remove, on the other hand, is important; we've only written
part of the file, so it may be corrupt.


More of a style issue, but it's a lot cleaner if you add the
explicit return here:

    return EXIT_SUCCESS ;


You still have way too much in the function main. I'd probably
end up with something like:

    int
    main( int argc, char** argv )
    {
        int i = 0 ;
        int j = 0 ;
        DataMap map ;
        if ( parseArgs( argc, argv, i, j )
                || inputArgs( std::cin, i, j ) ) {
            CacheFileManager cache( filename, map ) ;
            process( dataStructure, i, j ) ;
            outputResults( i, j ) ;
        }
        return exitStatus ;
    }

Note the use of the destructor of CacheFileManager to output the
table.

--
James Kanze (GABI Software) email:james.kanze@gmail.com
Conseils en informatique orient=E9e objet/
                   Beratung in objektorientierter Datenverarbeitung
9 place S=E9mard, 78210 St.-Cyr-l'=C9cole, France, +33 (0)1 30 23 00 34