Velocity Reviews

Velocity Reviews (http://www.velocityreviews.com/forums/index.php)
-   C++ (http://www.velocityreviews.com/forums/f39-c.html)
-   -   Re: Non Aggregate Type? (http://www.velocityreviews.com/forums/t276326-re-non-aggregate-type.html)

Kevin Goodsell 08-02-2003 07:01 PM

Re: Non Aggregate Type?
 
Ryan Vilim wrote:

>
> Definitions.h
> ************************************************** *********************
>
> typedef struct series {


What's with the typedef? That shouldn't be there.

>
> char name[40];


I suggest you use std::string instead, unless you have a very good
reason not to.

> bool notify;
> bool autodownload;
> char path[100];


std::string.

> series *next;
> };
>
> class init {
>
> private:
> void interpretline(char currentline[100]);


The '100' here does absolutely nothing. I suggest you lose it, since it
can be misleading. currentline is actually a char* regardless of whether
or not you put a value in the brackets.

> void getline(void);
>
>
> public:
>
> void readconfig(void);
>
> };
>
> #define CONFIGPATH "/etc/watchman/watchman.conf"


Macros are best avoided. One of the following would be better:

const char *configpath = "/etc/watchman/watchman.conf";
const std::string configpath = "/etc/watchman/watchman.conf";

>
> ************************************************** *********************
>
> cls_init.h
>
> ************************************************** *********************
>
> #include "definitions.h"
> #include <stdio.h>


This is a deprecated header. <cstdio> is the replacement.

> #include <iostream.h>


This is not a standard header. Standard C++ uses <iostream>.

>
>
> void init::readconfig(void) {
>
> this->getline();


....or you could just say

getline();

The 'this->' is redundant.

>
> }
>
> void init::getline() {


I notice you are being inconsistent with your use of 'void' in parameter
lists for functions that don't take any arguments. I suggest you decide
on one way of writing it and stick with it. In the case of C++ (not C),
I recommend losing the 'void'.

>
> char c, currentline[100];


I suggest std::string instead of a char array.

> int i;
> FILE * pFile;


I suggest using stream classes instead of old, C-style I/O. It's more
flexible and less error-prone.

Also, declaring variables before they are needed is not recommended.

> pFile=fopen (CONFIGPATH,"r");
>
> if (pFile==NULL) {
>
> perror ("Error opening file");


Shouldn't you do something else here? As it is, if an error occurs your
program will print the error, then continue on its merry way as if
nothing went wrong. In particular, the call to interpretline() below
will use currentline, which has not been initialized.

> }
> else
> {
> do{
>
> c=fgetc (pFile);
> i=c;
>
> if(i==13) {


What's with this magic number? It looks like you want to detect a
newline. Why wouldn't you just say "if (i == '\n')"? Your way is bad for
at least 2 reasons - first, it's more difficult to understand. Second,
it make a non-portable assumption: That the value of the newline
character is 13. C++ implementations are not restricted to any
particular character set.

>
> fgets (currentline, ftell (pFile) , pFile);


This looks very dangerous. What makes you think the return value from
ftell is appropriate here? If ftell(pFile) > sizeof(currentline) you
have very serious problems. The correct thing to do would probably be to
use sizeof(currentline). But a better solution would be to follow the
advice I've been giving you - use C++ I/O streams and std::strings. Then
you get a line this way:

std::ifstream infile(configpath);
// ...
std::string currentline;
getline(infile, currentline);

No worrying about the buffer length or buffer overflows. Much easier,
much safer.

> }
>
> }while((c!=EOF)||(i!=13));


This is a poor way to read an input file. A better way would be
something like this:

while (getline(infile, line))
{
// do something with line
}

This would need to be adapted to your particular needs. I'm not sure
exactly what you want to do though.

> }
>
> this->interpretline(currentline);


Redundant 'this->'.

> }
>
>
> void init::interpretline(char currentline[100]) {


The 100 is meaningless.

>
> int i,a;
> char varname[15];
> char command[100];


Strings are better.

> if(currentline[0]!='<'){
>
> do {
>
> i++;
>
> }while ((i!=strlen(currentline))||(currentline[i]=='='));


What if the string length is 0? This breaks horribly in that case.

>
> if(currentline[i]=='=') {
>
> memcpy(command,currentline,i);
>
> for(a=i; a< strlen(currentline) - i; a++) {
>
> command[a]=currentline[a-i];
>
> }
>
> }
> }


This code could probably be done more cleanly with std::strings. As it
is I have a hard time understanding it and I have very little confidence
in its correctness.

>
> cout<<command<<endl;
> cout<<varname<<endl;
>
> }
>
> ************************************************** *********************
>
> main.cpp
>
> ************************************************** *********************
>
> #include "cls_init.h"
>
> int main(int argc, char *argv[])
> {
>
> init popmem ();


init popmem;

The way you have it declares a function.

>
> popmem.readconfig();
>
> return EXIT_SUCCESS;


Did you #include <cstdlib> for EXIT_SUCCESS?

Overall, I'd say you need to stop fighting modern C++ and start using
it. You've got way too much "C plus C++ features" here, so you are not
using the language effectively and you are making it much more difficult
and error-prone than it needs to be.

-Kevin


Kevin Goodsell 08-02-2003 07:20 PM

Re: Non Aggregate Type?
 
Kevin Goodsell wrote:

> Macros are best avoided. One of the following would be better:
>
> const char *configpath = "/etc/watchman/watchman.conf";


Change that one to

const char *const configpath = "/etc/watchman/watchman.conf";

-Kevin



All times are GMT. The time now is 01:44 AM.

Powered by vBulletin®. Copyright ©2000 - 2014, vBulletin Solutions, Inc.
SEO by vBSEO ©2010, Crawlability, Inc.