Re: Non Aggregate Type?

Discussion in 'C++' started by Kevin Goodsell, Aug 2, 2003.

  1. 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=='='));


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

    >
    > if(currentline=='=') {
    >
    > 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, Aug 2, 2003
    #1
    1. Advertising

  2. 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
    Kevin Goodsell, Aug 2, 2003
    #2
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Ryan Vilim

    Re: Non Aggregate Type?

    Ryan Vilim, Aug 2, 2003, in forum: C++
    Replies:
    1
    Views:
    4,400
    Artie Gold
    Aug 2, 2003
  2. Christian Christmann

    non-aggregate type bool

    Christian Christmann, Mar 25, 2005, in forum: C++
    Replies:
    3
    Views:
    1,753
    Donovan Rebbechi
    Mar 25, 2005
  3. Alden Pierre
    Replies:
    3
    Views:
    349
    Ben Pope
    Apr 5, 2006
  4. Timothee Groleau

    non-aggregate type error?

    Timothee Groleau, Apr 27, 2006, in forum: C++
    Replies:
    2
    Views:
    713
    Timothee Groleau
    Apr 27, 2006
  5. rickman
    Replies:
    5
    Views:
    427
    rickman
    Mar 30, 2013
Loading...

Share This Page