Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C++ > Re: Non Aggregate Type?

Reply
Thread Tools

Re: Non Aggregate Type?

 
 
Kevin Goodsell
Guest
Posts: n/a
 
      08-02-2003
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

 
Reply With Quote
 
 
 
 
Kevin Goodsell
Guest
Posts: n/a
 
      08-02-2003
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

 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
"Non-static aggregate with multiple choices has non-static otherschoice." rickman VHDL 5 03-30-2013 11:06 PM
non-aggregate type bool Christian Christmann C++ 3 03-25-2005 07:14 PM
Vector of structures: non-aggregate error koperenkogel C++ 3 03-24-2005 06:19 PM
non-aggregate error Charles Jamieson C++ 6 07-14-2004 08:42 PM
Re: Non Aggregate Type? Ryan Vilim C++ 1 08-02-2003 03:51 AM



Advertisments