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

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

Jul 19 '05 #2

In article <2X****************@wards.force9.net>, ms*****@yahoo.co.uk
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.

Jul 19 '05 #3

Michael Strorm

"Martijn Lievaart" <m@rtij.nl.removefromhere.invalid> wrote in message
news:pa****************************@rtij.nl.remove fromhere.invalid...

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
ms*****@yahoo.co.uk

Jul 19 '05 #4

On Sun, 26 Oct 2003 11:53:29 +0000, Michael Strorm wrote:

"Martijn Lievaart" <m@rtij.nl.removefromhere.invalid> wrote in message
news:pa****************************@rtij.nl.remove fromhere.invalid...
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

Jul 19 '05 #5

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.

Jul 19 '05 #6

Michael Strorm

"Jerry Coffin" <jc*****@taeus.com> wrote in message
news:MP************************@news.clspco.adelph ia.net...

In article <2X****************@wards.force9.net>, ms*****@yahoo.co.uk
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

Jul 19 '05 #7

In article <b9*************************@posting.google.com> ,
ma**********@yahoo.com 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[i] & 128) & ((in[i] & 95) > 64) & ((in[i] & 31) <= 26))
stringout += (in[i] | 32);
else stringout += in[i]; //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[i] & 128) & ((in[i] & 95) > 64) & ((in[i] & 31) <= 26))
stringout += (in[i] & (223));
else stringout += in[i]; //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[i] = std::tolower(in[i]);
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::ostringstream 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.

Jul 19 '05 #8

Chris \( Val \)

"Jerry Coffin" <jc*****@taeus.com> wrote in message
news:MP************************@news.clspco.adelph ia.net...
| In article <b9*************************@posting.google.com> ,
| ma**********@yahoo.com 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[i] = std::tolower(in[i]);
| 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[i], 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

Jul 19 '05 #9

Jerry Coffin <jc*****@taeus.com> wrote in message news:<MP************************@news.clspco.adelp hia.net>...

In article <b9*************************@posting.google.com> ,
ma**********@yahoo.com 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!!

Jul 19 '05 #10

In article <bn*************@ID-110726.news.uni-berlin.de>, "Chris \( Val
\)" <ch******@bigpond.com.au> 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.

Jul 19 '05 #11

In article <b9**************************@posting.google.com >,
ma**********@yahoo.com 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.

Jul 19 '05 #12

Agent Mulder

<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

Jul 19 '05 #13

Chris \( Val \)

"Jerry Coffin" <jc*****@taeus.com> wrote in message
news:MP************************@news.clspco.adelph ia.net...
| In article <bn*************@ID-110726.news.uni-berlin.de>, "Chris \( Val
| \)" <ch******@bigpond.com.au> 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

Jul 19 '05 #14

Jerry Coffin <jc*****@taeus.com> wrote in message news:<MP************************@news.clspco.adelp hia.net>...

In article <b9*************************@posting.google.com> ,
ma**********@yahoo.com 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.

Jul 19 '05 #15

In article <b9**************************@posting.google.com >,
ma**********@yahoo.com 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.

Jul 19 '05 #16

Jerry Coffin <jc*****@taeus.com> wrote in message news:<MP************************@news.clspco.adelp hia.net>...

In article <b9**************************@posting.google.com >,
ma**********@yahoo.com 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.

Jul 19 '05 #17

In article <b9**************************@posting.google.com >,
ma**********@yahoo.com 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.

Jul 19 '05 #18

On Thu, 06 Nov 2003 12:51:57 -0800, J. Campbell wrote:

Jerry Coffin <jc*****@taeus.com> wrote in message news:<MP************************@news.clspco.adelp hia.net>...
In article <b9**************************@posting.google.com >,
ma**********@yahoo.com 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

Jul 19 '05 #19

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

Jul 19 '05 #20

Martijn Lievaart <m@rtij.nl.removefromhere.invalid> wrote in message news:<pa****************************@rtij.nl.remov efromhere.invalid>...

On Thu, 06 Nov 2003 12:51:57 -0800, J. Campbell wrote:
Jerry Coffin <jc*****@taeus.com> wrote in message news:<MP************************@news.clspco.adelp hia.net>...
In article <b9**************************@posting.google.com >,
ma**********@yahoo.com 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.

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. :-)
Martijn, thanks for the reply.

Sorry about the case errors with compilation...It's a hard error to
catch when there is no (local) penalty for making it (on my DOS
machine).

A quick look on some random chosen files.

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

Is this a style issue? I moved all #includes to the header because I
read in a different thread (while trying to understand appropriate use
of headers), "put everything that will be needed for the class in it's
header", which makes some sense. Is there a reason to put the
#includes in the *.cpp file? I'm not arguing a point, just trying to
learn ;-)
- 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.
Any pointers to where I can learn about using <iosfwd> instead of
<iostream>?

Can you demonstrate how to use <iosfwd> rather than <iostream> to
avoid code bloat in the following, trivial example?

#include <iostream>
using namespace std;
int main(){
cout << "Thanks Martijn!" << endl;
return 0;
}

- 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.
Thanks for the advice...I'll need to ponder this a bit before it is
clear...I suspect it may not be until I start writing code for GUI
use. However, it does make sense. If I understand, you are saying
that I should remove *all* commands that output information to the
screen from this function, because it makes the function less
reusable?

- 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.
Thanks for the advice

HTH
M4

BTW, What's HTH?? "Hate to harp"?

Jul 19 '05 #21

Jerry Coffin <jc*****@taeus.com> wrote in message news:<MP************************@news.clspco.adelp hia.net>...

In article <b9**************************@posting.google.com >,
ma**********@yahoo.com 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.
Thanks for the time. Sorry to keep dragging this out. I agree that
outputting to stream would offer more flexibility. I made it as is
because if the string of the hash (a "gettable value")is printed it
can contain non-printable characters...and I wanted the class to have
an in-built way to *display* itself to the user. So...you'd advise
outputting the hex-formated output to a stringstream then pass that
back from the function as a string? Makes sense but makes it slightly
harder to use, since the user would then have 2 different ways of
getting the hash...the displayable but not usable form, and the usable
but not displayable form. I was trying to avoid that by making a
"show" and a "get" function, rather than 2 different "get" functions.
Anyway, I realize that I'm novice enough that I'm quite likely trying
to justify a poor design decision. Any further thoughts on this?

As-is, your HashS class looks to me like it's a function in hiding (so
to speak).
good point. I think I'll change it, since there is no reason to keep
the object once it has returned it's value, and the:
class instance(data);
instance.hash();
motif that needs to be used with the current setup is awkward. a
simple:
string hashfunction(data);
function would be less awkward to use.
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.
What's the better way? I was basically trying to avoid parsing the
filespec multiple times, or passing so many strings (eg path + root +
ext) into functions...seemed easier (and less error-pront) to pass the
class.

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.
Well...I was writing PRNG's with names that were descriptive (to me
anyway) of how they worked. Slicerclass happened to be a pretty good
one at generating solid random data that passes all tests for
randomness that I'm aware of. I decided to keep the PRNG as a
stand-alone class that spits out random data, rather than to write it
as an encrypter that accept files and spit out encrypted files.
(other than having a more useful name like My_PRNG) do you have a
suggestion about how to use the PRNG? Would you write a "super-class"
that includes all the functionality of a PRNG, and all the
functionality of the hasher, and all the functionality of the
encrypter, and the associated helper functions? I thought that I had
essentially done this with my "driver" function, while keeping my PRNG
class (slicer_class) usable as a general purpose PRNG.

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.
Hey...I'll take free criticisim where I can get it...I appreciate it
;-)
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.

Well, since I built authentication into the system, the user *needs*
to know if he correctly unencrypted the file. While I agree that
telling him that he got it correct would make a dictionary attack
easier (if I understand the attack that you imply), I thought it
wasn't really an issue since I wrote the prog so that such an attack
would be costly at circa 1+ second / try. At this rate(using a single
modern computers) it would take something like 150 million years to
test all 8-character passwords (92 characters (u-case, l-case, nums &
punctuation)), and over a trillion yrs if a 10 char pass is used...
(in case you are reading and say, "bull...all 8 char combinations can
be generated in *much* less time, I need to tell you that the program
uses an initialization vector whereby each PRNG is stepped forward as
fast as the computer can muster for at least 0.5 seconds before it is
used. And 2 independent PRNGs must be initialized to unencrypt.)

Thanks, SO MUCH, for the input. Last week when I posted the code, my
use of header files was whacked...I was including *.cpp files into the
main-containing file and compiling the whole thing into a single
object file...You helped kick my ass to at least get me stumbling down
a smoother road.

Joe

Jul 19 '05 #22

lilburne

J. Campbell wrote:

Martijn Lievaart <m@rtij.nl.removefromhere.invalid> wrote in message news:<pa****************************@rtij.nl.remov efromhere.invalid>...

A quick look on some random chosen files.

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

Is this a style issue? I moved all #includes to the header because I
read in a different thread (while trying to understand appropriate use
of headers), "put everything that will be needed for the class in it's
header", which makes some sense. Is there a reason to put the
#includes in the *.cpp file? I'm not arguing a point, just trying to
learn ;-)

The recommendation is to #include all the types that are
required by a header but no more.

Take the following header files:

fred.h
======

class Fred {
};
wilma.h
=======

#include "fred.h"

class Wilma {
Fred partner;
};
pebbles.h
=========

#include "wilma.h"

class Pebbles {
Fred father;
Wilma mother;
};

now if ever Wilma decides that Fred is superflous to
requirements and removes the include of fred.h then pebbles
will no longer know about her father. So pebbles.h should
include fred.h not rely on the indirect inclusion from
wilma.h That is what is meant by "put everything that will
be needed for the class in it's header".

Having made the changes so that you now have:

wilma.h
=======

class Wilma {
};
pebbles.h
=========

#include "wilma.h"
#include "fred.h"

class Pebbles {
Fred father;
Wilma mother;
};

milkman.h

#include "wilma.h"

class Milkman {
Wilma customer;
};

if wilma.h also included pebbles.h because it was used in
wilma.cpp then the Milkman would become dependent on both
pebbles and fred too.

Jul 19 '05 #23

On Fri, 07 Nov 2003 06:53:21 -0800, J. Campbell wrote:

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. :-)
Sorry about the case errors with compilation...It's a hard error to
catch when there is no (local) penalty for making it (on my DOS
machine).

Yes I know. I found out the hard way when porting 300KLOC. That was not
fun, but a good opporunity to better my scripting skills.

A quick look on some random chosen files.

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

Is this a style issue? I moved all #includes to the header because I
read in a different thread (while trying to understand appropriate use
of headers), "put everything that will be needed for the class in it's
header", which makes some sense. Is there a reason to put the
#includes in the *.cpp file? I'm not arguing a point, just trying to
learn ;-)

Decoupling. It makes maintenance easier in the end. Include in the header
only what you need to compile the header. Put in the cpp file what you
need to compile that cpp file. It also lessens compile times (except when
you have a broken precompiled header implementation).

- 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.

Any pointers to where I can learn about using <iosfwd> instead of
<iostream>?

Ahhh, Google? Scott Meyers "Effective STL" and Jossutis "The standard C++
Library" come to mind. Thos are books every C++ programmer should have
anyhow.
Can you demonstrate how to use <iosfwd> rather than <iostream> to avoid
code bloat in the following, trivial example?

#include <iostream>
using namespace std;
int main(){
cout << "Thanks Martijn!" << endl;
return 0;
}
No, you need <iostream> to compile this, you would use <iosfwd> in headers
where you only need the forward declarations to classes like ostream. E.g:

-- file.h

#include <iosfwd>

class X {
public:
void print(std::ostream&);
};
Thanks for the advice...I'll need to ponder this a bit before it is
clear...I suspect it may not be until I start writing code for GUI use.
However, it does make sense. If I understand, you are saying that I
should remove *all* commands that output information to the screen from
this function, because it makes the function less reusable?
Exactly.
BTW, What's HTH?? "Hate to harp"?

www.acronymfinder.com is your friend.

HTH,
M4

Jul 19 '05 #24

In article <b9**************************@posting.google.com >,
ma**********@yahoo.com says...

[ ... ]

Thanks for the time. Sorry to keep dragging this out. I agree that
outputting to stream would offer more flexibility. I made it as is
because if the string of the hash (a "gettable value")is printed it
can contain non-printable characters...and I wanted the class to have
an in-built way to *display* itself to the user.
Separating "display" from "store" is perfectly fine, and irrelevant to
what I'm talking about here.
So...you'd advise
outputting the hex-formated output to a stringstream then pass that
back from the function as a string?
I suppose you could do that, but it's not what I had in mind at all. I
was thinking of something a bit simpler. Right now, showhex looks
roughly like this:

void HashS::showhex() {

std::cout << various_stuff;
}

I was talking about changing that to something like this:

void HashS::showhex(std::ostream os) {

os << various_stuff;
}
Makes sense but makes it slightly
harder to use, since the user would then have 2 different ways of
getting the hash...the displayable but not usable form, and the usable
but not displayable form. I was trying to avoid that by making a
"show" and a "get" function, rather than 2 different "get" functions.
Anyway, I realize that I'm novice enough that I'm quite likely trying
to justify a poor design decision. Any further thoughts on this?
It's open to some question, but it's not the question I was asking. A
hash _is_ a numeric value, so it might also be reasonable to just have a
function to get the value (probably as some user-defined long-integer
type) and then separately have functions to deal with formatting that
type for display, storage, etc. That's a bit beyond what I had in mind
at the moment though.
good point. I think I'll change it, since there is no reason to keep
the object once it has returned it's value, and the:
class instance(data);
instance.hash();
motif that needs to be used with the current setup is awkward. a
simple:
string hashfunction(data);
function would be less awkward to use.
I quite agree -- it looks (to me) like the only other time you use the
hash object after creation is the display function we discussed above.
It looks to me like you may have had something somewhat different in
mind to start with, but it ended up being simplified out of existence
before all was said and done.

One thing that might be worth considering is defining your own type to
hold the hash value -- it doesn't look to me like a string is the ideal
type for the job.

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.

What's the better way? I was basically trying to avoid parsing the
filespec multiple times, or passing so many strings (eg path + root +
ext) into functions...seemed easier (and less error-pront) to pass the
class.

It depends -- as-is, you might about as well have a single function that
manipulates data in a normal struct. For a class to make much sense,
the class should encapsulate _something_. As-is, your class has almost
no intelligence, no encapsulation, etc.
Well...I was writing PRNG's with names that were descriptive (to me
anyway) of how they worked. Slicerclass happened to be a pretty good
one at generating solid random data that passes all tests for
randomness that I'm aware of. I decided to keep the PRNG as a
stand-alone class that spits out random data, rather than to write it
as an encrypter that accept files and spit out encrypted files.
(other than having a more useful name like My_PRNG) do you have a
suggestion about how to use the PRNG? Would you write a "super-class"
that includes all the functionality of a PRNG, and all the
functionality of the hasher, and all the functionality of the
encrypter, and the associated helper functions?
No -- rather the contrary. I'd probably split things up quite a bit
more finely. Right now, your random number generator has one function
that's really related to generating a random number, which is good, but
also has things to give direct access to the key (almost certainly NOT
good) getting an init vector and system size (apparently both of which
are somehow related to repeating a sequence, but only in some rather
ill-defined way.

I'd have a couple of classes. First would be an PRNG state. It would
incorporate enough to be able to store a state of the PRNG, and restore
it later -- rather than your get_system_size, get_init_vector, etc.,
being stored (and manipulated?) by the user in some unknown way, have a
PRNG state that can be created, stored, assigned, and used to create a
stream.

Separate from that, I'd have the encryptor that combines the random
stream with the data to produce the encrypted stream.
I thought that I had
essentially done this with my "driver" function, while keeping my PRNG
class (slicer_class) usable as a general purpose PRNG.
Perhaps I missed something, but right now the separation doesn't look
very clean, and your encryption seems to know quite a bit about the
internals of the PRNG, to the point that you probably could not (for one
example) plug in some totally different PRNG at a moment's notice.
Well, since I built authentication into the system, the user *needs*
to know if he correctly unencrypted the file. While I agree that
telling him that he got it correct would make a dictionary attack
easier (if I understand the attack that you imply), I thought it
wasn't really an issue since I wrote the prog so that such an attack
would be costly at circa 1+ second / try.

That would sort of apply in the case of a dictionary attack, but really
would not to a dictionary attack. A PRNG is basically an algorithmic
way of creating a large, but still finite, sequence of numbers. The
seed to the PRNG simply selects a point in that sequence at which to
start using numbers.

When you do your second of looping, you still end up selecting _some_
point in that sequence. If I skip over the second of looping, I pick
some point in the sequence as well. If I'm doing a brute-force attack,
my plan is to simply look at _every_ starting point in the sequence
until I find the one you used -- and the fact that I got there by a
somewhat different route than you did is utterly irrelevant.

IOW, a brute force attack has no reason whatsoever to use one second per
candidate key.

I haven't tried to figure out the details of a dictionary attack, but in
your bounce(), you seem to use only linear operations. Assuming that's
correct, any number of iterations can be collapsed down to a single,
constant-time operation, meaning your 1-second loop can be reduced to a
matter of a few nanoseconds.

Anyhow, this is getting wildly off-topic, so I'll leave it alone for
now.

--
Later,
Jerry.

The universe is a figment of its own imagination.

Jul 19 '05 #25

In article <bo*************@ID-203936.news.uni-berlin.de>,
li******@godzilla.net says...

[ ... ]

The recommendation is to #include all the types that are
required by a header but no more.

Anybody who says anything about "the recommendation" on this point
clearly hasn't a clue of what he's talking about.

There are at least two entirely separate theories on this: one says that
no header should include any other header -- if a header needs to use
types from other headers, you include those headers before you include
this one. This style was (and to a lesser extent remains) quite common
on, for one example, UNIX and similar OSes. Just for example, many of
the sys/* header require that you include sys/types.h first. The same
is true in the Windows world, where most other system-oriented headers
require that you include windows.h first.

Another theory runs that a header should be independent, so you can
include it without explicitly including any other headers first; any
other headers needed for it to work correctly, it should include itself.

There is, to some extent, a middle ground between these as well: make a
header independent by including only forward declarations of other types
needed for that header to work. This is arguably cleaner than either
extreme, but almost certainly the most difficult to do well also -- in
particular, it more or less requires generating one complete header, and
another containing only the forward declarations. Theoretically, this
saves you from including a lot of unnecessary "stuff" (especially extra
names that could conflict) from the full-blown header. IMO, it's rarely
worth it: reasonable use of namespaces removes the conflict, so at best
you're talking about going to a fair amount of extra work to reduce
compile times. A header large enough for this to make a real difference
is a good clue that you have other problems. This is usually roughly
equivalent to giving a band-aid and an aspirin to somebody who's just
stepped on a land-mine.

--
Later,
Jerry.

The universe is a figment of its own imagination.

Jul 19 '05 #26