Nasty code...but please critique it anyway :-)

Discussion in 'C++' started by Michael Strorm, Oct 25, 2003.

  1. Hi!
    I posted a message a while back asking for project suggestions, and decided
    to go with the idea of creating an adventure game (although it was never
    intended to be a 'proper' game, rather an excuse to write- and learn- some
    C++).

    To cut a long story short, I wrote a fair chunk of it, but realised that
    it's... not very good. Okay, it's my first "proper" C++ program, so that's
    no big deal, but I don't want to waste more time working on a program that
    should be rewritten from scratch (I want to start reading 'Accelerated C++'
    instead). What *would* be interesting would be to hear what other people
    think about the (compilable, but not properly working) code.

    It's a long program, spread over many files, so I haven't posted them here.
    The URL is
    http://www.mstrorm.free-online.co.uk/
    Yes, I know the code isn't particularly well laid out or commented- but I
    hope it's clear enough.

    The design of the program is that a Controller class controls the flow of
    events and oversees everything. A base-class 'Noun' includes common behavior
    and is extended to give us game locations, items (i.e. physical objects),
    Beings (i.e. game characters, further subclassed for the Player object which
    represents the human player) and exits.
    Given a command such as
    eat chocolate
    or
    go north
    the controller finds the Noun-subclass-object representing chocolate (an
    Item) or north (an Exit)- that object then does the action "eat" or "north"
    respectively. Assuming the Noun-subclass supports that action, it can
    respond appropriately (e.g. the Exit object with id=="north" would update
    the player's location in its "go" method to wherever the exit pointed to).
    Note that action methods (or, more specifically, functions) are static
    functions held in a vector of pointers. I don't like the way that the vector
    of function-pointers is initialized, but it seemed the best way at the time.

    Criticisms of the large-scale design, or basic programming would be equally
    useful. I think there's plenty to criticize about both, as you can see from
    the comments I left in (though I like the subclassing of Nouns into
    different object types).

    Anyway, feedback would be appreciated....

    --

    Michael Strorm
    Michael Strorm, Oct 25, 2003
    #1
    1. Advertising

  2. On Sat, 25 Oct 2003 15:38:05 +0100, Michael Strorm wrote:

    > It's a long program, spread over many files, so I haven't posted them here.
    > The URL is
    > http://www.mstrorm.free-online.co.uk/
    > Yes, I know the code isn't particularly well laid out or commented- but I
    > hope it's clear enough.


    Better create a zip and/or tarball, that's much easier to download.

    HTH,
    M4
    Martijn Lievaart, Oct 25, 2003
    #2
    1. Advertising

  3. Michael Strorm

    Jerry Coffin Guest

    In article <2Xvmb.620$9.net>,
    says...

    [ ... ]

    > The design of the program is that a Controller class controls the flow of
    > events and oversees everything.


    IMO, you've put too much into your Controller class -- a search on
    Google (in the newsgroups) for "God class" should turn up some
    interesting reading on how to de-centralize the control a bit.

    > A base-class 'Noun' includes common behavior
    > and is extended to give us game locations, items (i.e. physical objects),
    > Beings (i.e. game characters, further subclassed for the Player object which
    > represents the human player) and exits.


    IMO, "Noun" is so general as to be of little use. Looking through
    Noun.hpp, it has (for example) a getLocation and setLocation -- but
    looking at Locn.hpp, a Locn _is_ a Noun. This doesn't make a lot of
    sense to me.

    > Given a command such as
    > eat chocolate
    > or
    > go north
    > the controller finds the Noun-subclass-object representing chocolate (an
    > Item) or north (an Exit)- that object then does the action "eat" or "north"
    > respectively. Assuming the Noun-subclass supports that action, it can
    > respond appropriately (e.g. the Exit object with id=="north" would update
    > the player's location in its "go" method to wherever the exit pointed to).


    I don't think this is really a very good way of structuring things. A
    person is giving a command to their player, so the player object should
    receive the command. The player object should probably have a parser
    object to figure out what the command means. Based on that, it should
    check its environment and try to figure out a way to carry out the
    command.

    I don't think it makes much sense for a location to derive from the same
    base as an item -- and unless you want (for example) to allow for the
    possibility of a player carrying around another player like it would
    carry around a sword, goblet, etc., it probably doesn't make much sense
    for a player and a goblet to derive from the same base either.

    If I were doing this, at the top level I'd have some locations and some
    beings. Each location contains a collection of exits, another of items,
    and possibly a set of attributes (e.g. a particular room might be dark
    so the player can't see in it). Each location will also have a
    collection of beings that are within that room at the present time.

    Each being has a current location, a collection of items it's carrying,
    probably some other attributes to describe its strength, talents,
    skills, etc.

    A player is a being that also have a command parser so it can take
    orders. The commands go directly to the player, which then uses its
    command parser to figure out what it is. Based on its current
    environment (skills, strength, environment, etc.) it tries to figure out
    a way to carry out the command its received. e.g. if you say "go
    north" it looks at the collection of exits in its current location, and
    sees whether there's an exit to the north. If so, moves to that
    location (updating its current location and updating the locations'
    collections of what they contain).

    I also wouldn't hard-code the commands to which a player can respond.
    Instead, the player data file contains data telling about what commands
    a player can respond to, and what the effects of that command are.
    Likewise, each item would have a set of commands that it responds to (or
    enables) and the effects of those as well. Just for example, you might
    decide that a player can fly if he has a talent for a particular type of
    magic OR if he is wearing a particular cloak. This would enable a
    player to respond to a command to go up that would otherwise not be
    allowed.

    This would make a relatively easy to add (for example) new food items to
    your game, each with different effects on strength, reaction speed, and
    so on. Likewise with adding new weapons, monsters, etc.

    --
    Later,
    Jerry.

    The universe is a figment of its own imagination.
    Jerry Coffin, Oct 25, 2003
    #3
  4. "Martijn Lievaart" <> wrote in message
    news:p...
    > Better create a zip and/or tarball, that's much easier to download.


    Sorry- should have mentioned that there's a zip of the whole lot somewhere
    in there!

    --

    Michael Strorm
    Michael Strorm, Oct 26, 2003
    #4
  5. On Sun, 26 Oct 2003 11:53:29 +0000, Michael Strorm wrote:

    > "Martijn Lievaart" <> wrote in message
    > news:p...
    >> Better create a zip and/or tarball, that's much easier to download.

    >
    > Sorry- should have mentioned that there's a zip of the whole lot somewhere
    > in there!


    Looked at the listing and missed it. Damn! Sorry about that.

    M4
    Martijn Lievaart, Oct 26, 2003
    #5
  6. Michael Strorm

    J. Campbell Guest

    I, too, would like feedback on my code.
    I'd appreciate feedback on:

    1) my use header files
    2) the inclusion of namespace std
    3) the general organization of the program
    4) anything else that you might care to comment on

    The source is located in J_Crypt.zip at:
    http://home.bellsouth.net/p/PWP-brightwave

    J_Crypt a file encrypter/unencrypter that is based on a PRNG I
    designed (contained in slicerclass.h and slicerclass.cpp). This
    project was an excuse to get started with c++.

    To use the program, you "drop" a file onto the executable and follow
    the menu to encrypt or unencrypt the file. The program is set up to
    work with dos-style paths:
    drive:\path\filename.extension

    if another system is used, filespec_class.h and filespec_class.cpp
    will need to be modified to appropriately parse the commandline.

    Thanks for taking the time to look at this.
    J. Campbell, Oct 27, 2003
    #6
  7. "Jerry Coffin" <> wrote in message
    news:...
    > In article <2Xvmb.620$9.net>,
    > says...
    >
    > [ ... ]
    >
    > > The design of the program is that a Controller class controls the flow

    of
    > > events and oversees everything.

    >
    > IMO, you've put too much into your Controller class -- a search on
    > Google (in the newsgroups) for "God class" should turn up some
    > interesting reading on how to de-centralize the control a bit.


    I'll take a better look for that later on; it sounds interesting (and pretty
    useful), but I wanted to reply to this post first. It definitely helped when
    I blocked the obvious religious words BTW. :)

    > IMO, "Noun" is so general as to be of little use. Looking through
    > Noun.hpp, it has (for example) a getLocation and setLocation -- but
    > looking at Locn.hpp, a Locn _is_ a Noun. This doesn't make a lot of
    > sense to me.


    I agree; I'd thought about that myself. However, a location, like an Exit,
    Item or Being, can be the 'noun' in the typical 'verb noun' or 'verb noun
    preposition noun2' input forms (eg "look room"), and to make it a non-noun
    would mean rewriting/rehashing of existing code with additional handling
    required, plus twice as much maintenance. That's why I went with the noun
    solution even though, as you say, there are some inconsistencies, such as a
    location having a location. The other thing I wanted to avoid was runaway
    child classes for every minor variation (which would have solved the
    inelegance above at the expense of more classes). I'm not disagreeing with
    your criticism, only trying to explain why it was written that way.

    >
    > > Given a command such as
    > > eat chocolate
    > > or
    > > go north
    > > the controller finds the Noun-subclass-object representing chocolate (an
    > > Item) or north (an Exit)- that object then does the action "eat" or

    "north"
    > > respectively. Assuming the Noun-subclass supports that action, it can
    > > respond appropriately (e.g. the Exit object with id=="north" would

    update
    > > the player's location in its "go" method to wherever the exit pointed

    to).
    >
    > I don't think this is really a very good way of structuring things. A
    > person is giving a command to their player, so the player object should
    > receive the command. The player object should probably have a parser
    > object to figure out what the command means. Based on that, it should
    > check its environment and try to figure out a way to carry out the
    > command.


    My reasoning here is that the player doesn't need to know about everything
    it can carry out an action on- of course, dependency is hard to avoid (if
    player eats an apple, the apple might have to know about the player if
    player is a special case of being.... this could go on).

    > I don't think it makes much sense for a location to derive from the same
    > base as an item -- and unless you want (for example) to allow for the
    > possibility of a player carrying around another player like it would
    > carry around a sword, goblet, etc., it probably doesn't make much sense
    > for a player and a goblet to derive from the same base either.


    Perhaps I'm thinking too hard about simplifying the parser. It just seemed
    very OO to let an object determine its own behaviour rather than what was
    carrying out the action- making it easier to expand.

    > If I were doing this, at the top level I'd have some locations and some
    > beings. Each location contains a collection of exits, another of items,
    > and possibly a set of attributes (e.g. a particular room might be dark
    > so the player can't see in it). Each location will also have a
    > collection of beings that are within that room at the present time.


    Yeah, this is where I can see the point you made about the god-class above.
    I'm not sure that the controller needs to be able to handle that kind of
    thing- it wasn't part of the design I put that much thought into, on
    reflection.
    Question... would pointers in both directions (Player having *Locn and Locn
    having *Player) be a good idea? This is what you seem to suggest below...

    > Each being has a current location, a collection of items it's carrying,
    > probably some other attributes to describe its strength, talents,
    > skills, etc.


    > I also wouldn't hard-code the commands to which a player can respond.
    > Instead, the player data file contains data telling about what commands
    > a player can respond to, and what the effects of that command are.
    > Likewise, each item would have a set of commands that it responds to (or
    > enables) and the effects of those as well. Just for example, you might
    > decide that a player can fly if he has a talent for a particular type of
    > magic OR if he is wearing a particular cloak. This would enable a
    > player to respond to a command to go up that would otherwise not be
    > allowed.


    Ideally, I'd like to have very little hard-coded. The way I wrote it was
    intended to be a compromise between code-flexibility and hard-coding
    (hard-coding making it easier to customise behaviour, but not being as
    elegant or convenient).

    > This would make a relatively easy to add (for example) new food items to
    > your game, each with different effects on strength, reaction speed, and
    > so on. Likewise with adding new weapons, monsters, etc.


    Would it be possible to add custom code for each item, or just vary a few
    (pre-decided) parameters?
    IIRC Java can load classes dynamically, but even if ISO-C++ can do this, I
    don't think my knowledge is good enough to do that yet.

    With the existing code, what I don't like about my customised Noun behaviour
    in particular is (a) The C-style nature of the code, and (b) Use of static
    functions (since, apparently, a static function within the class still has a
    different signature depending on the class, that is, function of type foobar
    has type Noun::foobar if defined within Noun, and Being::foobar if defined
    within Being, leading to PITA compatibility problems and my kludgey
    solution).

    Anyway, interesting answers, thanks!

    - MS
    Michael Strorm, Oct 27, 2003
    #7
  8. Michael Strorm

    Jerry Coffin Guest

    In article <>,
    says...
    > I, too, would like feedback on my code.
    > I'd appreciate feedback on:
    >
    > 1) my use header files


    Not really very good -- each of your .cpp files should include the
    header declaring the class(es) for which it contains implementation.
    As-is, I'm not at all sure how you got some of the .cpp files to compile
    at all.

    > 2) the inclusion of namespace std


    Well, you certainly don't make what I'd consider optimal use of the
    standard anyway. Consider (for one example) these three functions:

    string lcase(string in){
    string stringout;
    for(int i = 0; i < in.size(); ++i)
    if(!(in & 128) & ((in & 95) > 64) & ((in & 31) <= 26))
    stringout += (in | 32);
    else stringout += in; //character wasn't a letter...dont change
    return stringout;
    }

    string ucase(string in){
    string stringout;
    for(int i = 0; i < in.size(); ++i)
    if(!(in & 128) & ((in & 95) > 64) & ((in & 31) <= 26))
    stringout += (in & (223));
    else stringout += in; //character wasn't a letter...dont change
    return stringout;
    }

    These are barely understandable at all. I guess they're intended to
    convert entire strings so all letters are upper case or all characters
    are lower case. For that sort of job, I'd do something like this:

    typedef std::string::size_type s_t;

    std::string lcase(std::string in) {
    std::string ret;

    s_t len= in.size();

    ret.resize(len);

    for (s_t i=0; i<len; ++i)
    ret = std::tolower(in);
    return ret;
    }

    and similarly for ucase. Better yet, you could use std::transform
    instead of an explicit loop:

    std::string lcase(std::string in) {
    std::string ret;

    std::transform(in.begin(), in.end(),
    std::back_inserter<std::string>(ret), std::tolower);
    return ret;
    }

    std::string ucase(std::string in) {
    std::string ret;

    std::transform(in.begin(), in.end(),
    std::back_inserter<std::string>(ret), std::toupper);
    return ret;
    }

    Likewise, with this:

    string center(string s){
    int p = s.size();
    string temp;
    if(s.size() < 80){
    for(int i = 0; i < (39 - (p/2)); ++i)
    temp += ' ';
    }
    return (temp + s + "\n");
    }

    First of all, this assumes you're only ever going to center things over
    an 80 column area, which is pretty inflexible. Second, it does that
    with less than, shall we say, the greatest of aplomb. I think I'd use
    something like this:

    std::string center(std::string s, int width = 80) {
    std::string ret;
    std::fill_n(std::back_inserter<std::string>(ret),
    (width-s.size())/2,
    ' ');
    return ret+s+"\n";
    }

    Or perhaps:

    std::string center(std::string s, int width = 80) {
    std::eek:stringstream os;

    os << std::setw(width - (s.size()/2)) << s << "\n";
    return os.str();
    }

    > 3) the general organization of the program


    It looks to me like you need to work on generalizing things. Just for
    example, your "gun" does't just read input -- it also reports errors (to
    standard output) so it can only ever be used interactively.

    It's usually better to have one part that does only reading, and if it
    fails, reports the failure only in a return code, by throwing an
    exception, etc. (I.e. only to other parts of the program, NOT directly
    to the user). Then, a separate part handles the direct interaction with
    the user, based on the feedback from the internal function.

    This generally improves portability (and often readability), and makes
    each easier to transplant into other situations with a total rewrite.

    As far as overall organization, it may simply be that I'm tired right
    now, but I couldn't follow it at all. As I said above, I'm not even
    quite sure how some of it compiles...

    --
    Later,
    Jerry.

    The universe is a figment of its own imagination.
    Jerry Coffin, Oct 28, 2003
    #8
  9. "Jerry Coffin" <> wrote in message
    news:...
    | In article <>,
    | says...

    [snip]

    Hi Jerry.

    | typedef std::string::size_type s_t;
    |
    | std::string lcase(std::string in) {
    | std::string ret;
    |
    | s_t len= in.size();
    |
    | ret.resize(len);
    |
    | for (s_t i=0; i<len; ++i)
    | ret = std::tolower(in);
    | return ret;
    | }

    [snip]

    It is very rare that I see anyone using locales
    with 'std::tolower()', etc. For example, the
    above might have been implemented as follows:

    # include <locale>
    std::tolower( in, std::locale() );

    ....but the real question(s) are:

    Why do many people choose not to use the locale
    version ? - an loaded question, I know :).

    Is there no advantage to the locale version ?

    I was always under the impression that the locale
    version offered better safety, due to the possible
    use of different character sets being manipulated.

    Comments ?

    Thanks.
    Chris Val
    Chris \( Val \), Oct 28, 2003
    #9
  10. Michael Strorm

    J. Campbell Guest

    Jerry Coffin <> wrote in message news:<>...
    > In article <>,
    > says...
    > > I, too, would like feedback on my code.
    > > I'd appreciate feedback on:
    > >
    > > 1) my use header files

    >
    > Not really very good -- each of your .cpp files should include the
    > header declaring the class(es) for which it contains implementation.
    > As-is, I'm not at all sure how you got some of the .cpp files to compile
    > at all.
    >


    Thanks...I knew I wasn't quite doing it right...So...I should be
    including *.cpp files that have the *.h declarations included at the
    top rather than including the *.h files with the *.cpp definitions
    included at the bottom. This is clear.

    > > 2) the inclusion of namespace std

    >
    > Well, you certainly don't make what I'd consider optimal use of the
    > standard anyway. Consider (for one example) these three functions:


    It's ironic...you focused on "non-important" helper functions that I
    wrote to get input or for text formatting. I agree that your methods
    are better. I wrote the case-functions some time back while looking
    at the ascii table layout. It was an excercise to see what to_upper
    and to_lower really involved...anyway...I'm not making excuses.
    Thanks for taking the time, and getting me straightened out on the
    header-file stuff.

    > > 3) the general organization of the program

    >
    > It looks to me like you need to work on generalizing things.


    Thanks. I agree.

    > As far as overall organization, it may simply be that I'm tired right
    > now, but I couldn't follow it at all.


    Don't bame your tiredness...I'll accept the responsibility...and work
    on improving readability.

    > As I said above, I'm not even
    > quite sure how some of it compiles...


    It acctually compiled fine with dev-c++. I think that my reversing
    the *.h and *.cpp included causes confusion. Still, I have
    function/class declarations, followed by their
    definitions/implementations...

    Anyway...Thanks a bunch for you time and help. I have much to
    learn...and it's really useful to get human feedback rather than the
    only feedback coming from the compiler!!
    J. Campbell, Oct 28, 2003
    #10
  11. Michael Strorm

    Jerry Coffin Guest

    In article <bnls4h$11vrm9$-berlin.de>, "Chris \( Val
    \)" <> says...

    [ ... ]

    > Why do many people choose not to use the locale
    > version ? - an loaded question, I know :).


    I can't speak for anybody else, but in my case, because it's extra work
    (albeit not a great deal).

    > Is there no advantage to the locale version ?


    There is an advantage under some circumstances, but most people don't
    encounter those circumstances very often. In code like I was posting,
    it would only have been an unnecessary distraction in any case.

    > I was always under the impression that the locale
    > version offered better safety, due to the possible
    > use of different character sets being manipulated.


    Less a matter of safety than versatility. If you don't pass a locale,
    then they default to a single global locale. When/if you have to deal
    simultaneously with input in a number of different character sets, then
    an explicit locale becomes very handy. Until or unless you encounter
    that, it's of little real consequence.

    --
    Later,
    Jerry.

    The universe is a figment of its own imagination.
    Jerry Coffin, Oct 28, 2003
    #11
  12. Michael Strorm

    Jerry Coffin Guest

    In article <>,
    says...

    [ ... ]

    > It's ironic...you focused on "non-important" helper functions that I
    > wrote to get input or for text formatting.


    This was largely because in those cases the names of the functions made
    it fairly clear what they were intended to do, so it didn't take much to
    rewrite them. I leave it to you to understand your own encryption
    algorithm sufficiently to apply the same principles to it.

    [ ... ]

    > Anyway...Thanks a bunch for you time and help. I have much to
    > learn...and it's really useful to get human feedback rather than the
    > only feedback coming from the compiler!!


    'tis true: compilers are pretty short on advice about style
    (intentionally so, to a large extent).

    --
    Later,
    Jerry.

    The universe is a figment of its own imagination.
    Jerry Coffin, Oct 28, 2003
    #12
  13. Michael Strorm

    Agent Mulder Guest

    <J. Campbell>
    > I, too, would like feedback on my code.
    > http://home.bellsouth.net/p/PWP-brightwave

    </>

    Hey Joe,

    I like your code. I only gave it a brief look, since I am
    busy programming on something else, but I'll give you
    my comments:

    -You have a funny style. Files with only one function in
    it. That is ok. Try everything.

    -Stick to having one file in every project (or directory) called
    main.cpp. It contains the main routine, as expected, but also has
    a special importance in program organisation. Your
    'main.cpp' is called J_Cript.cpp. That is too cryptic a name.

    -Merge the .h files and the .cpp files. Put one class declaration
    + the method definitions in one file. You can inline everything if you
    like (I do it only), but that is un-C++-ish. You can also define
    the methods just below the class declarations, still in the same file.

    -Step 3. You now have a 1-to-1 relation between a file and
    a class. The extension of this file is arbitrary. It is both a .h
    file with a declaration and a .cpp file containing the definitions.
    Extend it with h.

    -In file main.cpp, write out the #included .h files like this:

    #include <iostream>
    #include <iomanip>
    #include <fstream>
    #include <string>

    using namespace std;

    const string version = "J-Crypt 01.0.0.2";

    #include "AlignInt_class.h"
    #include "describe.h"
    #include "Encrypt.h"
    #include "Filespec_class.h"
    #include "hashSclass.h"
    #include "JKC_functions.h"
    #include "J_Crypt.h"
    #include "slicerclass.h"
    #include "unencrypt.h"

    int main(int argc, char* argv[]){
    cout << "\n" << center(version) << center("2003 Soft Corp.") << endl;
    if(argc < 2){describe(); wait(); return 0;}
    ....

    -You now have all your class enumarated in file main.cpp. You can
    remove all and every #include in any other file. Really. This has some
    major advantages (and a drawback).

    Advantages:
    - You can remove all and every #include in any other file
    - You have a list of all the classes used by your program ready in main.cpp
    - You are forced to have the hierarchy of your objects right, thus reinforcing
    the design.

    Drawback:
    - Everything gets compiled all the time.


    That is how I enforce rigidity in my programs. Try it your way.

    -X
    Agent Mulder, Oct 28, 2003
    #13
  14. "Jerry Coffin" <> wrote in message
    news:...
    | In article <bnls4h$11vrm9$-berlin.de>, "Chris \( Val
    | \)" <> says...

    Hi Jerry.

    | > Why do many people choose not to use the locale
    | > version ? - an loaded question, I know :).
    |
    | I can't speak for anybody else, but in my case, because it's extra work
    | (albeit not a great deal).
    |
    | > Is there no advantage to the locale version ?
    |
    | There is an advantage under some circumstances, but most people don't
    | encounter those circumstances very often. In code like I was posting,
    | it would only have been an unnecessary distraction in any case.
    |
    | > I was always under the impression that the locale
    | > version offered better safety, due to the possible
    | > use of different character sets being manipulated.
    |
    | Less a matter of safety than versatility. If you don't pass a locale,
    | then they default to a single global locale. When/if you have to deal
    | simultaneously with input in a number of different character sets, then
    | an explicit locale becomes very handy. Until or unless you encounter
    | that, it's of little real consequence.

    Thank you for clarifying that, it makes sense :).

    Cheers.
    Chris Val
    Chris \( Val \), Oct 29, 2003
    #14
  15. Michael Strorm

    J. Campbell Guest

    Jerry Coffin <> wrote in message news:<>...
    > In article <>,
    > says...
    > > I, too, would like feedback on my code.
    > > I'd appreciate feedback on:
    > >
    > > 1) my use header files

    >
    > Not really very good -- each of your .cpp files should include the
    > header declaring the class(es) for which it contains implementation.
    > As-is, I'm not at all sure how you got some of the .cpp files to compile
    > at all.
    >


    I must be pretty dense...I had the hardest time understanding what I
    was doing wrong. I think I get it now. Will you take a look at the
    (revised) code from my first link in this thread and tell me if my use
    of headers is now more or less normal?
    int main(){} is located in j_crypt.cpp

    thanks again for the response.
    J. Campbell, Nov 4, 2003
    #15
  16. Michael Strorm

    Jerry Coffin Guest

    In article <>,
    says...

    [ ... ]

    > I must be pretty dense...I had the hardest time understanding what I
    > was doing wrong. I think I get it now. Will you take a look at the
    > (revised) code from my first link in this thread and tell me if my use
    > of headers is now more or less normal?


    If I still had the link handy I would, but I don't...

    --
    Later,
    Jerry.

    The universe is a figment of its own imagination.
    Jerry Coffin, Nov 6, 2003
    #16
  17. Michael Strorm

    J. Campbell Guest

    Jerry Coffin <> wrote in message news:<>...
    > In article <>,
    > says...
    >
    > [ ... ]
    >
    > > I must be pretty dense...I had the hardest time understanding what I
    > > was doing wrong. I think I get it now. Will you take a look at the
    > > (revised) code from my first link in this thread and tell me if my use
    > > of headers is now more or less normal?

    >
    > If I still had the link handy I would, but I don't...


    http://home.bellsouth.net/p/PWP-brightwave

    Sorry I didn't include the link before...I didn't want people to think
    that I was entheusiastically pimping my novice code, and the link was
    just up the thread, at least on my newsreader ;-) Anyway, thanks for
    responding.
    J. Campbell, Nov 6, 2003
    #17
  18. Michael Strorm

    Jerry Coffin Guest

    In article <>,
    says...

    Well, I've taken a look at the code, and I still think it could use some
    work. One obvious point would be that there are quite a few places that
    use std::cout (for one example) directly (e.g. HashS::showhex and
    Slicer::SeedMe).

    IMO, these should take a stream as a parameter, and use that stream,
    rather than always using std::cout and std::cin.

    As-is, your HashS class looks to me like it's a function in hiding (so
    to speak). File_spec looks a bit similar -- you basically just
    construct a File_spec from a string, and then use (public, no less) data
    members of the object. I see little advantage to this design.

    It's also somewhat confusing that encrypt.cpp contains only a driver,
    and the encryption proper is done in slicer_class -- the latter name, in
    particular, gives not even a hint as to the real use or point of the
    class.

    Though I've been _trying_ to stick to commenting on how the code is
    written, I can't help pointing out that your design for the file format
    makes a ciphertext-only attack _dramatically_ easier than it should be.
    To get any hope of security, you want to make it as difficult as
    possible to even guess at whether a decryption was successful or not --
    if at all possible, you'd like almost any key to produce something
    that's reasonable, so it's as difficult as possible for the attacker to
    decide whether a given key is correct. Your design does the opposite:
    it tells him immediately whether his guess at a key was correct.

    --
    Later,
    Jerry.

    The universe is a figment of its own imagination.
    Jerry Coffin, Nov 7, 2003
    #18
  19. On Thu, 06 Nov 2003 12:51:57 -0800, J. Campbell wrote:

    > Jerry Coffin <> wrote in message news:<>...
    >> In article <>,
    >> says...
    >>
    >> [ ... ]
    >>
    >> > I must be pretty dense...I had the hardest time understanding what I
    >> > was doing wrong. I think I get it now. Will you take a look at the
    >> > (revised) code from my first link in this thread and tell me if my use
    >> > of headers is now more or less normal?

    >>
    >> If I still had the link handy I would, but I don't...

    >
    > http://home.bellsouth.net/p/PWP-brightwave
    >
    > Sorry I didn't include the link before...I didn't want people to think
    > that I was entheusiastically pimping my novice code, and the link was
    > just up the thread, at least on my newsreader ;-) Anyway, thanks for
    > responding.


    g++ -c -o J_Crypt.o J_Crypt.cpp
    In file included from J_Crypt.h:9,
    from J_Crypt.cpp:1:
    hashSclass.h:6:30: alignint_class.h: No such file or directory
    In file included from J_Crypt.cpp:1:
    J_Crypt.h:10:30: filespec_class.h: No such file or directory
    J_Crypt.h:15:23: encrypt.h: No such file or directory
    In file included from J_Crypt.h:16,
    from J_Crypt.cpp:1:
    unencrypt.h:9:26: HashSclass.h: No such file or directory
    unencrypt.h:13:30: filespec_class.h: No such file or directory
    In file included from J_Crypt.h:16,
    from J_Crypt.cpp:1:
    unencrypt.h:15: `Filespec' was not declared in this scope

    (snip a lot more errors due to missing headers)

    make: *** [J_Crypt.o] Error 1

    Compilation exited abnormally with code 2 at Fri Nov 7 10:18:11

    It really is better to watch your case. :)

    A quick look on some random chosen files.

    - hashSclass.h
    You only need <string>, move all the other #includes to the C++ files.

    - J_Crypt.h
    A file that includes all other headers. A style issue, but I don't like
    it. Also, including <iostream> may lead to code bloat and longer
    compilation times. Include it only where needed.

    - Encrypt.cpp
    Looks good. Good comments, very readable. Only thing is that you do user
    interaction here. Better to decouple this. A way to do that would be to
    pass a const ostream& or a function pointer to a function that can write
    strings. The latter technique makes sure you can use the same encrypt
    routine in a GUI program as well.

    - menu.cpp
    * Get text by line and parse that, good!
    * You may want to use a std::stringstream to parse the integer, else just
    use atoi.
    * Why not print the menutext the first time from menu1 as well?
    Inconsistent.
    * You may want to flush the output stream after printing menutext, in
    trhis function you cannot be sure it is terminated with a newline.

    HTH
    M4
    Martijn Lievaart, Nov 7, 2003
    #19
  20. On Fri, 07 Nov 2003 10:30:26 +0100, Martijn Lievaart wrote:

    > - J_Crypt.h
    > A file that includes all other headers. A style issue, but I don't like
    > it. Also, including <iostream> may lead to code bloat and longer
    > compilation times. Include it only where needed.


    Forgot to say: Use <iosfwd> in headers instead.

    HTH,
    M4
    Martijn Lievaart, Nov 7, 2003
    #20
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Rv5

    Code Critique Please

    Rv5, Nov 16, 2003, in forum: C++
    Replies:
    3
    Views:
    354
    Benny Hill
    Nov 16, 2003
  2. gorda
    Replies:
    16
    Views:
    763
    Karl Heinz Buchegger
    Jul 29, 2004
  3. Adrian

    Code critique please

    Adrian, Oct 30, 2004, in forum: C++
    Replies:
    2
    Views:
    340
    Adrian
    Oct 31, 2004
  4. Zygmunt Krynicki
    Replies:
    5
    Views:
    419
    Zygmunt Krynicki
    Sep 19, 2003
  5. Rv5

    Code Critique Please

    Rv5, Nov 16, 2003, in forum: C Programming
    Replies:
    2
    Views:
    329
    -berlin.de
    Nov 16, 2003
Loading...

Share This Page