473,836 Members | 1,501 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
26 2952
In article <bn************ *@ID-110726.news.uni-berlin.de>, "Chris \( Val
\)" <ch******@bigpo nd.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**********@ya hoo.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
<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 "slicerclas s.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

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


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**********@ya hoo.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. adelphia.net>.. .
In article <b9************ **************@ posting.google. com>,
ma**********@ya hoo.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 entheusiastical ly 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**********@ya hoo.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. adelphia.net>.. .
In article <b9************ **************@ posting.google. com>,
ma**********@ya hoo.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 entheusiastical ly 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:2 6: 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::stringstre am 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

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

Similar topics

37
2118
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
2560
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
1690
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
7270
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
2109
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
1123
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
1322
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
9674
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,...
0
10860
Oralloy
by: Oralloy | last post by:
Hello folks, I am unable to find appropriate documentation on the type promotion of bit-fields when using the generalised comparison operator "<=>". The problem is that using the GNU compilers, it seems that the internal comparison operator "<=>" tries to promote arguments from unsigned to signed. This is as boiled down as I can make it. Here is my compilation command: g++-12 -std=c++20 -Wnarrowing bit_field.cpp Here is the code in...
0
10560
jinu1996
by: jinu1996 | last post by:
In today's digital age, having a compelling online presence is paramount for businesses aiming to thrive in a competitive landscape. At the heart of this digital strategy lies an intricately woven tapestry of website design and digital marketing. It's not merely about having a website; it's about crafting an immersive digital experience that captivates audiences and drives business growth. The Art of Business Website Design Your website is...
1
10604
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
10261
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
6984
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
5659
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
4026
muto222
by: muto222 | last post by:
How can i add a mobile payment intergratation into php mysql website.
3
3116
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.