Opinions on this code please guys....

[Jenski182]

Is this from a good coder? or a rubbish one? Whichever way, it was paid for his
services, just want to know if its worth while....
Thanks
Jen x




#include <iostream.h>
#include <conio.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <fstream.h>
#include <direct.h>


void main()
{
char current_dir[50],tokens[10][20],input[300]; // for storing current
directory , tokens , and input
int i,j,k;
char x;
char dir_ex[30];
ifstream fin;


cout<<"\n\n\t\t *** COMMAND PROMPT *** \n\n ";

do // repeat till token = EXIT
{
_getcwd(current_dir,50); // gets current directory
cout<<"\n\n\n"<<current_dir;cout<<"> "; // puts it as a prompt
//cin>>input;
cin>>x;
gets(input); // reads command
//tokens[0][0] =NULL;

int length=strlen(input);



j=0;
for(i=0;i< length;i++) // do till end of command
{


k=0;
while( (input== ' ' )&&(i<strlen(input)) ) // parse command to tokens
based upon spaces
{
i++;
}

while( (input!= ' ' )&&(i<strlen(input)) )
{
tokens[j][k] = input;
k++;
i++;
}
tokens[j][k] = NULL;

cout<<"\n Token "<<j+1<<" : "<<tokens[j]; // show token
j++;
}


if( strcmpi(tokens[0],"DIR")==0) // if token is DIR
{
strcat(dir_ex,"DIR ");
strcat(dir_ex,tokens[1]);
strcat(dir_ex,NULL);

system( dir_ex);
cout<<"\n\n\t Listing directory : ";
cout<<" "<<current_dir<<"\n\n";

}



if( strcmpi(tokens[0],"PWD")==0) // if token is PWD
{
cout<<"\n\n\t Current directory : ";
cout<<" "<<current_dir<<"\n\n"; // puts it as a prompt
}





if( strcmpi(tokens[0],"CD")==0) // if token is CD
{
chdir(tokens[1]); // change directory
cout<<"\n\n\t Directory changed to : ";
getcwd(current_dir,50);
cout<<" "<<current_dir<<"\n\n"; // shows changed directory
}



if( strcmpi(tokens[0],"TYPE")==0) // if token is TYPE
{
fin.open(tokens[1]);
if(!fin)
{
cout<<"\n\n\n FILE NOT FOUND ! \n\n ";
}

else
{
cout<<"\n\n\n\t *** PRINTING FILE *** \n\n ";

while(!fin.eof()) // shows file contents
{
fin.read((char*)&x,1);
cout<<x;
}
}
fin.close();
}



if( strcmpi(tokens[0],"EXIT")==0) // if command is exit
{
exit(1);
}

}while(strcmpi(tokens[0],"EXIT")!=0);

}

 

[Ron Natalie]

Is this from a good coder? or a rubbish one? Whichever way, it was paid for his
services, just want to know if its worth while....
Thanks

You were ripped off. This guy obviously has enough knowledge of C to be
dangerous and practically no knowledge of C++.

#include <iostream.h>
#include <conio.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <fstream.h>
#include <direct.h>

void main()

main returns int.

{
char current_dir[50],tokens[10][20],input[300]; // for storing current

Ugh! What are all these hard coded sizes?

_getcwd(current_dir,50); // gets current directory

Poor maintainablility, what happens when someone decides 50 really isn't
a reasonable limit on the current_dir size? What happens if someone only
changes it in one place.

cout<<"\n\n\n"<<current_dir;cout<<"> "; // puts it as a prompt
//cin>>input;

Leaves crap in the code inidicating that he was clueless about how to get
what he wanted.

cin>>x;

Why do we read this and throw it away? Do you never want to see the
first char on the line?

gets(input); // reads command

GETS should NEVER EVER EVER EVER EVER BE USED.
Can I state that loudly enough. NEVER! There's no check here
to see if the string writes off the end of the buffer.

Further, there's no check for error (or even EOF) which is quite
possible.

//tokens[0][0] =NULL;

Again left code in there...commented out indicating his stupidity.

int length=strlen(input);

Wow, finally he gets to the point of finally actually initializing
a variable. This should have been done for all the vars abaove.

while( (input== ' ' )&&(i<strlen(input)) ) // parse command to tokens
based upon spaces

Highly inefficeint. First, he's calling strlen over and over again even though it
doesn't ever change (he did compute the value and stick it in a variable length
above, so why didn't he at least use that?). Second, when i is at the end of
the string, the input will be zero (this is the definition), so it won't be ' '
so the loop will end even without the test against strlen. Further.

Also, it's confusing to use variables like i, j, and k without a clue as to what
their purpose is.

tokens[j][k] = input;

Nothing checks to see that j and k aren't exceeding their allowed range.

strcat(dir_ex,"DIR ");

UNDEFINED BEHAVIOR. Very bad, might crash or do other bizarre
things. dir_ex has not yet been set to anything. strcat() expects it to hold a
null terminated string already.

strcat(dir_ex,tokens[1]);
strcat(dir_ex,NULL);

This is allso undefined behavior and stupid. The above call is not necessary , dir_ex
is already a null terminated array. Strcat wants a pointer to another char array, passing
it a NULL is UNDEFINED BEHAVIOR (again a crash or worse).

if( strcmpi(tokens[0],"PWD")==0) // if token is PWD

tokens[0] can't be PWD if it was DIR above, this should be else if...

while(!fin.eof()) // shows file contents
{
fin.read((char*)&x,1);

NO NO NO...This is NOT how you check for EOF when reading
file. The eof only is set after the read fails.

while(fin.read((char&)&x, 1)) cout << x;
would be a correct loop.

Even the above is horrendously ineffiient and stupid. Also not very
symetrical. Why use unformatted I/O to read x but formated to write
x. Again, it demonstrates a lot of confusion on the part of the programmer.

if( strcmpi(tokens[0],"EXIT")==0) // if command is exit
{
exit(1);
}

}while(strcmpi(tokens[0],"EXIT")!=0);

exit(1) is guaranteed never to return, so there is no way that the while test above ever is false.

Not only does this person not know C or C++ adequately, he doesn't seem to understand
how to program at all. His gaffs and clutter go far beyond lack of knowledge of C++.

If this code actually ran, it's purely coincidental.

 

[jeffc]

Is this from a good coder? or a rubbish one? Whichever way, it was paid for his
services, just want to know if its worth while....

Well it depends on his constraints - the compiler to use, etc. It looks OK,
but it looks like a C programmer only dipping his toes in C++ code. I would
say that his "EXIT" strategy is a bit dubious.

 

[Ron Natalie]

Well it depends on his constraints - the compiler to use, etc. It looks OK,
but it looks like a C programmer only dipping his toes in C++ code. I would
say that his "EXIT" strategy is a bit dubious.

It's lousy C code too. In addition to the horrendous control constructs and
inefficiencies, there are over a half a dozen invocations of undefined behavior
even in the C constructs.

 

[Brian Genisio]

Is this from a good coder? or a rubbish one? Whichever way, it was paid for his
services, just want to know if its worth while....
Thanks
Jen x




#include <iostream.h>
#include <conio.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <fstream.h>
#include <direct.h>


void main()
{
char current_dir[50],tokens[10][20],input[300]; // for storing current
directory , tokens , and input
int i,j,k;
char x;
char dir_ex[30];
ifstream fin;


cout<<"\n\n\t\t *** COMMAND PROMPT *** \n\n ";

do // repeat till token = EXIT
{
_getcwd(current_dir,50); // gets current directory
cout<<"\n\n\n"<<current_dir;cout<<"> "; // puts it as a prompt
//cin>>input;
cin>>x;
gets(input); // reads command
//tokens[0][0] =NULL;

int length=strlen(input);



j=0;
for(i=0;i< length;i++) // do till end of command
{


k=0;
while( (input== ' ' )&&(i<strlen(input)) ) // parse command to tokens
based upon spaces
{
i++;
}

while( (input!= ' ' )&&(i<strlen(input)) )
{
tokens[j][k] = input;
k++;
i++;
}
tokens[j][k] = NULL;

cout<<"\n Token "<<j+1<<" : "<<tokens[j]; // show token
j++;
}


if( strcmpi(tokens[0],"DIR")==0) // if token is DIR
{
strcat(dir_ex,"DIR ");
strcat(dir_ex,tokens[1]);
strcat(dir_ex,NULL);

system( dir_ex);
cout<<"\n\n\t Listing directory : ";
cout<<" "<<current_dir<<"\n\n";

}



if( strcmpi(tokens[0],"PWD")==0) // if token is PWD
{
cout<<"\n\n\t Current directory : ";
cout<<" "<<current_dir<<"\n\n"; // puts it as a prompt
}





if( strcmpi(tokens[0],"CD")==0) // if token is CD
{
chdir(tokens[1]); // change directory
cout<<"\n\n\t Directory changed to : ";
getcwd(current_dir,50);
cout<<" "<<current_dir<<"\n\n"; // shows changed directory
}



if( strcmpi(tokens[0],"TYPE")==0) // if token is TYPE
{
fin.open(tokens[1]);
if(!fin)
{
cout<<"\n\n\n FILE NOT FOUND ! \n\n ";
}

else
{
cout<<"\n\n\n\t *** PRINTING FILE *** \n\n ";

while(!fin.eof()) // shows file contents
{
fin.read((char*)&x,1);
cout<<x;
}
}
fin.close();
}



if( strcmpi(tokens[0],"EXIT")==0) // if command is exit
{
exit(1);
}

}while(strcmpi(tokens[0],"EXIT")!=0);

}

Before someone looses a job over this, what was the context? Was it an
intern? If so, cut them some slack... What was the assignment? What did
you want the program to do?

This program implements an exteremely minimal shell, with very little
capabilitiy : dir, pwd, cd, type and exit. It doesnt really let you do
much... but is that what you wanted? Did you get something that met the
requirements for the job?

Did you pay a professional, or a student? I think this information is
useful, before anyone gets in trouble.

Brian

 

[Martijn Lievaart]

Is this from a good coder? or a rubbish one? Whichever way, it was paid
for his services, just want to know if its worth while....

<snip>

I saw lots of bad style, and some errors. For a beginner not bad, for a
pro, absolute rubbish.

Just my opinion of course.

M4

 

[jeffc]

It's lousy C code too. In addition to the horrendous control constructs and
inefficiencies, there are over a half a dozen invocations of undefined behavior
even in the C constructs.

Well, I was hedging my bets with "depends on constraints" - sometimes people
cut and paste other examples of similar code, because in some environments,
it's the fastest way to get ugly things to work (who knows exactly what the
guy's assignment was, or who he was working with). Also, you are speaking
from an "ivory tower" perspective, whereas I'm saying that kind of code is
not uncommon in the real world - I've seen worse. I don't mean I like the
looks of it.

 

[jeffc]

Before someone looses a job over this, what was the context? Was it an
intern? If so, cut them some slack... What was the assignment? What did
you want the program to do?

This program implements an exteremely minimal shell, with very little
capabilitiy : dir, pwd, cd, type and exit. It doesnt really let you do
much... but is that what you wanted? Did you get something that met the
requirements for the job?

Another way to phrase my point to Ron.

 

[Ron Natalie]

from an "ivory tower" perspective, whereas I'm saying that kind of code is
not uncommon in the real world - I've seen worse. I don't mean I like the
looks of it.

I'm not speaking of an Ivory Tower. I'm the lead engineer in a software company.
Anybody who wrote that shit here would be out on their ear. The program doesn't
even look like it would work anyhow. It does two things that are almost certainly
going to puke in most instances.

 

[Thore Karlsen]

Is this from a good coder? or a rubbish one? Whichever way, it was paid for his
services, just want to know if its worth while....

It's terrible code that should get someone fired. But I have a sneaking
suspicion somebody paid someone else (another student, from the looks of
it) to do a homework assignment?

 

[jeffc]

Also, you are speaking
I'm not speaking of an Ivory Tower. I'm the lead engineer in a software company.
Anybody who wrote that shit here would be out on their ear.

They must have all got jobs around here after, I guess.

 

[red floyd]

It's terrible code that should get someone fired. But I have a sneaking
suspicion somebody paid someone else (another student, from the looks of
it) to do a homework assignment?

That was my suspicion as well, based on the (apparent) requirements of
the program. The OP got ripped off when he paid someone to do his homework.

Maybe the coder deliberately did it wrong?

 

[Derek]

It's terrible code that should get someone fired. But I have a
sneaking suspicion somebody paid someone else (another student,
from the looks of it) to do a homework assignment?

Or the folks in this NG just did someone's homework by answering a
"find all the things wrong with this code" question.

 

[jeffc]

Or the folks in this NG just did someone's homework by answering a
"find all the things wrong with this code" question.

That would be a great troll - absolutely irresistible.

 

[Thore Karlsen]

Or the folks in this NG just did someone's homework by answering a
"find all the things wrong with this code" question.

If that's the case, that's pretty clever, and he deserves some credit
for that.

 

[Fat Bastard]

Or the folks in this NG just did someone's homework by answering a
"find all the things wrong with this code" question.

Yes, it definitely is pretty clever. I kinda chuckled a bit.

 

[Rolf Magnus]

Maybe the coder deliberately did it wrong?

First, I thought the posting was a troll posting, since all the things
most commonly marked as "evel" are done in that code, but then I too
thought it could be what you suspect above.

 

[Jenski182]

Before someone looses a job over this, what was the context? Was it an

intern? If so, cut them some slack... What was the assignment? What did
you want the program to do?

To write a simple command line interface, and it was to be written using VC++
(6.0). However, it compiled in VC with 0 Errors and 2 warnings, and didnt
function properly. The exe file he sent me worked fine (suprise, suprise)

This program implements an exteremely minimal shell, with very little
capabilitiy : dir, pwd, cd, type and exit. It doesnt really let you do
much... but is that what you wanted? Did you get something that met the
requirements for the job?

ish. Her covered the right functions, but the some didnt work properly:
1) the dir command didnt distinguish between, files and folders, didnt display
file/folder info either, e.g. size
2) the wild card of directroy listing didnt work properly.... eg. dir *.txt
listed all files, not text ones

Did you pay a professional, or a student? I think this information is
useful, before anyone gets in trouble.

someone who claimed to be a "proffessional"

Jen

 

[Y2KYZFR1]

Is this from a good coder? or a rubbish one? Whichever way, it was paid for his
services, just want to know if its worth while....
Thanks
Jen x




#include <iostream.h>
#include <conio.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <fstream.h>
#include <direct.h>


void main()
{
char current_dir[50],tokens[10][20],input[300]; // for storing current
directory , tokens , and input
int i,j,k;
char x;
char dir_ex[30];
ifstream fin;


cout<<"\n\n\t\t *** COMMAND PROMPT *** \n\n ";

do // repeat till token = EXIT
{
_getcwd(current_dir,50); // gets current directory
cout<<"\n\n\n"<<current_dir;cout<<"> "; // puts it as a prompt
//cin>>input;
cin>>x;
gets(input); // reads command
//tokens[0][0] =NULL;

int length=strlen(input);



j=0;
for(i=0;i< length;i++) // do till end of command
{


k=0;
while( (input== ' ' )&&(i<strlen(input)) ) // parse command to tokens
based upon spaces
{
i++;
}

while( (input!= ' ' )&&(i<strlen(input)) )
{
tokens[j][k] = input;
k++;
i++;
}
tokens[j][k] = NULL;

cout<<"\n Token "<<j+1<<" : "<<tokens[j]; // show token
j++;
}


if( strcmpi(tokens[0],"DIR")==0) // if token is DIR
{
strcat(dir_ex,"DIR ");
strcat(dir_ex,tokens[1]);
strcat(dir_ex,NULL);

system( dir_ex);
cout<<"\n\n\t Listing directory : ";
cout<<" "<<current_dir<<"\n\n";

}



if( strcmpi(tokens[0],"PWD")==0) // if token is PWD
{
cout<<"\n\n\t Current directory : ";
cout<<" "<<current_dir<<"\n\n"; // puts it as a prompt
}





if( strcmpi(tokens[0],"CD")==0) // if token is CD
{
chdir(tokens[1]); // change directory
cout<<"\n\n\t Directory changed to : ";
getcwd(current_dir,50);
cout<<" "<<current_dir<<"\n\n"; // shows changed directory
}



if( strcmpi(tokens[0],"TYPE")==0) // if token is TYPE
{
fin.open(tokens[1]);
if(!fin)
{
cout<<"\n\n\n FILE NOT FOUND ! \n\n ";
}

else
{
cout<<"\n\n\n\t *** PRINTING FILE *** \n\n ";

while(!fin.eof()) // shows file contents
{
fin.read((char*)&x,1);
cout<<x;
}
}
fin.close();
}



if( strcmpi(tokens[0],"EXIT")==0) // if command is exit
{
exit(1);
}

}while(strcmpi(tokens[0],"EXIT")!=0);

}

complete and utter crap you were robbed!

 

[Brian Genisio]

It's terrible code that should get someone fired. But I have a sneaking
suspicion somebody paid someone else (another student, from the looks of
it) to do a homework assignment?

Hmmmm.... If that is the case, it needs to be looked at differently.
For instance, if I (a computing professional, though new to the c++ vs
c) were being paid to do someone's homework for school, (big IF... I
think it is morally wrong) I would likely make it look like
non-professional code. I took the approach of professional standard
code in a masters assignment, and became the over-achiever-kiss-ass who
got extra credit, where no one else got any... My code was the freak
code, and it stood out in the croud. In other words, I would do what
worked, not worry about error checking, robust sys-calls,
maintainability, portability, etc...

Especially since a student is not likely to pay me my per-hour rate that
I get at work

Brian