473,705 Members | 7,112 Online
Bytes | Software Development & Data Engineering Community
+ Post

Home Posts Topics Members FAQ

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

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
ms*****@yahoo.c o.uk
Jul 19 '05 #1
26 2931
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.forc e9.net>, ms*****@yahoo.c o.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
"Martijn Lievaart" <m@rtij.nl.remo vefromhere.inva lid> wrote in message
news:pa******** *************** *****@rtij.nl.r emovefromhere.i nvalid...
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.c o.uk
Jul 19 '05 #4
On Sun, 26 Oct 2003 11:53:29 +0000, Michael Strorm wrote:
"Martijn Lievaart" <m@rtij.nl.remo vefromhere.inva lid> wrote in message
news:pa******** *************** *****@rtij.nl.r emovefromhere.i nvalid...
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\fil ename.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

"Jerry Coffin" <jc*****@taeus. com> wrote in message
news:MP******** *************** *@news.clspco.a delphia.net...
In article <2X************ ****@wards.forc e9.net>, ms*****@yahoo.c o.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************ *************@p osting.google.c om>,
ma**********@ya hoo.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::si ze_type s_t;

std::string lcase(std::stri ng 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::stri ng in) {
std::string ret;

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

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

std::transform( in.begin(), in.end(),
std::back_inser ter<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::str ing s, int width = 80) {
std::string ret;
std::fill_n(std ::back_inserter <std::string>(r et),
(width-s.size())/2,
' ');
return ret+s+"\n";
}

Or perhaps:

std::string center(std::str ing s, int width = 80) {
std::ostringstr eam 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

"Jerry Coffin" <jc*****@taeus. com> wrote in message
news:MP******** *************** *@news.clspco.a delphia.net...
| In article <b9************ *************@p osting.google.c om>,
| ma**********@ya hoo.com says...

[snip]

Hi Jerry.

| typedef std::string::si ze_type s_t;
|
| std::string lcase(std::stri ng 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. adelphia.net>.. .
In article <b9************ *************@p osting.google.c om>,
ma**********@ya hoo.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...anyw ay...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'l l 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

This thread has been closed and replies have been disabled. Please start a new discussion.

Similar topics

37
2095
by: Eric | last post by:
There is a VB.NET critique on the following page: http://www.vb7-critique.741.com/ for those who are interested. Feel free to take a look and share your thoughts. Cheers, Eric. Ps: for those on comp.programming, this may be off topic, but I've posted there because the critique was part of a discussion in that group.
19
2544
by: TC | last post by:
Are there any good sites or forums for a web critique? I went to alt.html.critique and it's pretty dead.
2
421
by: Rv5 | last post by:
Let me start out by saying this is actually c++ code, but I couldn't get anyone on the c++ newsgroup to respond, and Id really like opinions. The code works fine, so Im not looking for syntax help. Im more interested in general programming practice and style critiques. I think it is good code, but Ive said that and been wrong before. Despite it not being pure C, Im hoping someone can still help me. One thing to keep in mind if you do...
8
1687
by: G Patel | last post by:
I wrote the following program to remove C89 type comments from stdin and send it to stdout (as per exercise in K&R2) and it works but I was hoping more experienced programmer would critique the layout/style/etc. Any comments will be helpful. Thank you. /* **************************************************** C89/90 COMMENT REMOVER
188
7183
by: christopher diggins | last post by:
I have posted a C# critique at http://www.heron-language.com/c-sharp-critique.html. To summarize I bring up the following issues : - unsafe code - attributes - garbage collection - non-deterministic destructors - Objects can't exist on the stack - Type / Reference Types
26
2093
by: JB | last post by:
Hi All, This is my first go at a pure CSS web site and I'd like your comments and suggestions please. The URL is http://www.waukeshapumps.com/ Comments on CSS, design and content very welcome. Thanks in advance,
2
1116
by: winston | last post by:
I wrote a Python program (103 lines, below) to download developer data from SourceForge for research about social networks. Please critique the code and let me know how to improve it. An example use of the program: promptpython download.py 1 240000 The above command downloads data for the projects with IDs between 1
2
1316
by: matt | last post by:
this is my first program in this language ever (besides 'hello world'), can i get a code critique, please? it's purpose is to read through an input file character by character and tally the occurrence of each input character. it seems to compile and run, so i'm looking for the opinions of old-timers here plz. /* * File: occurrenceTally.cpp * Author: matthew *
0
8768
marktang
by: marktang | last post by:
ONU (Optical Network Unit) is one of the key components for providing high-speed Internet services. Its primary function is to act as an endpoint device located at the user's premises. However, people are often confused as to whether an ONU can Work As a Router. In this blog post, we’ll explore What is ONU, What Is Router, ONU & Router’s main usage, and What is the difference between ONU and Router. Let’s take a closer look ! Part I. Meaning of...
0
8690
by: Hystou | last post by:
Most computers default to English, but sometimes we require a different language, especially when relocating. Forgot to request a specific language before your computer shipped? No problem! You can effortlessly switch the default language on Windows 10 without reinstalling. I'll walk you through it. First, let's disable language synchronization. With a Microsoft account, language settings sync across devices. To prevent any complications,...
1
9034
by: Hystou | last post by:
Overview: Windows 11 and 10 have less user interface control over operating system update behaviour than previous versions of Windows. In Windows 11 and 10, there is no way to turn off the Windows Update option using the Control Panel or Settings app; it automatically checks for updates and installs any it finds, whether you like it or not. For most users, this new feature is actually very convenient. If you want to control the update process,...
0
8979
tracyyun
by: tracyyun | last post by:
Dear forum friends, With the development of smart home technology, a variety of wireless communication protocols have appeared on the market, such as Zigbee, Z-Wave, Wi-Fi, Bluetooth, etc. Each protocol has its own unique characteristics and advantages, but as a user who is planning to build a smart home system, I am a bit confused by the choice of these technologies. I'm particularly interested in Zigbee because I've heard it does some...
0
7895
agi2029
by: agi2029 | last post by:
Let's talk about the concept of autonomous AI software engineers and no-code agents. These AIs are designed to manage the entire lifecycle of a software development project—planning, coding, testing, and deployment—without human intervention. Imagine an AI that can take a project description, break it down, write the code, debug it, and then launch it, all on its own.... Now, this would greatly impact the work of software developers. The idea...
0
5933
by: conductexam | last post by:
I have .net C# application in which I am extracting data from word file and save it in database particularly. To store word all data as it is I am converting the whole word file firstly in HTML and then checking html paragraph one by one. At the time of converting from word file to html my equations which are in the word document file was convert into image. Globals.ThisAddIn.Application.ActiveDocument.Select();...
0
4440
by: TSSRALBI | last post by:
Hello I'm a network technician in training and I need your help. I am currently learning how to create and manage the different types of VPNs and I have a question about LAN-to-LAN VPNs. The last exercise I practiced was to create a LAN-to-LAN VPN between two Pfsense firewalls, by using IPSEC protocols. I succeeded, with both firewalls in the same network. But I'm wondering if it's possible to do the same thing, with 2 Pfsense firewalls...
2
2491
muto222
by: muto222 | last post by:
How can i add a mobile payment intergratation into php mysql website.
3
2083
bsmnconsultancy
by: bsmnconsultancy | last post by:
In today's digital era, a well-designed website is crucial for businesses looking to succeed. Whether you're a small business owner or a large corporation in Toronto, having a strong online presence can significantly impact your brand's success. BSMN Consultancy, a leader in Website Development in Toronto offers valuable insights into creating effective websites that not only look great but also perform exceptionally well. In this comprehensive...

By using Bytes.com and it's services, you agree to our Privacy Policy and Terms of Use.

To disable or enable advertisements and analytics tracking please visit the manage ads & tracking page.