Hello. I'm new to the site, I've been lurking a while and recently decided to plunge in and try to learn some OpenGL. I have some experience with c++ (beginner college courses, and messing around on my own), and was curious about trying to get into graphics. Anyway, I've been going through NeHe's tutorials. I'm up to lesson 10 now. I've been tweaking the code to get to know it better and oil my skills a bit (they're rusty from lack of use). Didn't have the glaux header, for instance, so I made a simple PNG loading class instead. Took me a whole weekend to read up on image loading, install the zlib and libpng libraries and write the code to get it working, but it was worth it once the cube loaded with the texture. So it's been fun. But I digress. Sorry. As I said, I'm at lesson 10, and was trying to update the c code in the world loading part of the tutorial to c++, making use of stream libraries and objects. The text file storing the vertex and texture mapping data has a few simple rules, - lines starting with // are comments and should be ignored. - there can be empty lines. - first line of valid data should read: "NUMPOLLIES nn", where nn is an int. - the rest of the data is formatted as lines of 5 ints each, holding the u, v, x, y and z coordinates. The update I did is basically: Thing is, I'm not sure that this is a good way of doing it. I've got the program working (need to slow it down, but it works), what I was hopiong you could help me with is noticing any obvious errors. Way I saw it, there are two parts to parsing the file: get the number of triangles, and then read the vertex data. That's why I put two loops. On the first loop, I enter the line into a string. If the first character is a slash, I decide it is a comment and skip it. If it is empty I also skip it. I keep skipping line until I find one that is neither empty nor starts with a slash. If the not-empty line starts with "numpollies" I feed a substring of the line to a stringstream object (ss). The substring would start from the end of NUMPOLLIES, so it should only contain whitespace and a number. I then feed it to the numtriangles variables, and once done I exit the loop. If I didn't exit the loop, it would keep going until I read the whole file and then abort the program. So then I enter the second loop. It is basically the same, but since I'm not looking for any keywords I just check for empty lines or comments, and anything that passes that filter is sent to ss and back out to the variables. The thing that bothers me is that I don't really like extracting a line from one stream, putting it in a string, doing the checking, then sending it back to another stream to load my variables. But I didn't see that I could do the necessary checks (comments, empty lines, keyword) any other way. Streams don't have find() methods and strings don't have formatted output methods. Is there something I'm missing? Thanks for reading this far, and I'll appreciate any help. [Edited by - Kian on February 26, 2010 7:12:15 PM]

Thank you very much for the replies. They're very helpful. I'll clean up the code a bit based on your suggestions. It is good to know I got the general idea right, at least.

Regarding file handling, I understand file input/output takes a lot of time compared to everything else. Is it best to try to read a file in one go (or in chunks as big as you can manage without exhausting your memory for large files) and then work with the contents from memory, or to do several inputs as you need them?

About some of the comments;

// Why return anything if you're just going to hard-code 0 or abort?

It was something of a hackish job. I was more interested in getting it working. All the aborts where also a way of getting some quick feedback. If it just hung, I didn't know what was failing. If it aborted, I knew where it might have been failing. As for why to return 0, I tend to avoid void functions. On a more serious project I would remove the aborts, and might either make it void or provide more return options.

// You should really add some error checking

I really should. But as I mentioned above, I didn't want to waste time thinking of error possibilities and adding a ton of error checking code. My priority was to find a good way of loading the data from the file first, get it running second. I knew the file was going to be well formatted, so error checking was the lowest priority.

// filein will close automatically at the end of this function

I prefer being explicit. Just the way I was taught, I guess. Is it actually detrimental to the program in any way if you close the file manually as oppossed to leaving it up to the function to do it when it is done?

// You could also load your objects using Daabli

I found links to a couple of libraries that offered that functionality while I researched a solution, but I wanted to solve the problem on my own first. File I/O is something basic that needs to be dealt with, and I needed the practice. That said, I'll look into it for future projects. No need to reinvent the wheel when there's a car available.

0) Be consistent about indentation. A tab is not four, nor eight, nor any number of spaces, no matter what your editor tells you or what you tell it.

1) Don't comment obvious things. Comments are a warning flag that something tricky is going on. If you repeatedly warn that something tricky is going on when it isn't, you run the risk of becoming the proverbial boy who cried wolf.

2a) Write the function in such a way that it you can tell it what SECTOR to initialize, instead of just assuming one.

There are two possibilities for this. One is to decide that the purpose of this function is to create a SECTOR; i.e. there's no particular reason a SECTOR has to exist (in an uninitialized state) before the file has loaded. In this case, you can rewrite the function as a constructor. This is the approach I have shown.

The other way is to decide that the purpose is to modify an existing SECTOR, in which case you would pass that SECTOR in as a parameter, by reference.

2b) Similarly, write the function in such a way that you can tell it what file to load from, instead of just assuming one. This is straightforward: add a parameter for the file name.

3) The purpose of the return value of a function is to return something meaningful. If you have nothing meaningful to return, then don't. The 'void' return type exists for a reason.

Since I'm writing the function as a constructor, there can't be a return type anyway. This is not a problem.

4) Handle errors at the point where you have the information needed to handle them. The calling code might have a better idea of what to do if the file isn't found or whatever. Maybe it doesn't, but it's still the calling code's responsibility. So report the error, by throwing an exception. (If you wrote the function to modify an existing SECTOR, you could return a status code instead; but exceptions are generally considered cleaner.)

5) There is no reason to call .close() manually on a file stream in normal cases. The underlying file will be closed automatically by the destructor of the fstream object.

6) Don't specify the 'in' file mode for an fstream; instead, just use an ifstream (which is always in in-mode). The file mode flags are provided for when you need to do something special (e.g. open for both input and output, or use a different mode such as appending, or specify binary file mode...). Similarly, use the constructor instead of .open(), when you can.

7) Don't do your own memory management. Instead of maintaining a length count and a dynamically allocated array, replace both with a std::vector instance (which keeps track of its length automatically).

8) Simplify your logic. Recognize impossible cases. For example, a line that contains "NUMPOLLIES" can't possibly be empty, and in particular a string that begins with "NUMPOLLIES" can't also begin with '/'.

9) Don't hard-code magic numbers, such as the length of the magic word "NUMPOLLIES". There is a simpler solution here: use the stringstream to parse the entire string.

10) Scope variables as tightly as possible, and initialize them with initial values, where possible. And when you do initialize, use a value that's useful. Initializing 'numtriangles' to 0 is not useful because we are going to overwrite it anyway, and we could overwrite it with 0. But initializing it to -1 is useful, because we are supposed to overwrite it with a non-negative value, and this helps us simplify the "did we read a good value?" logic.

11) You can often simplify code (reduce indentation) by turning conditions into "guard clauses". That is, instead of saying "if (something) { lots of code }", you say "if (not something) { GTFO of here }; lots of code" - and then the 'lots of code' is not indented. In the current case, instead of doing part of the 'vert' loop conditionally on reading a non-empty line, I set up a separate inner loop to read until we get a non-empty line, and then process it unconditionally. This also allows for using the 'for' loop construct in the natural way.

12) You need error checking in case data for a given triangle is corrupt, or in case you run out of file data while reading the triangles.

13) Don't use 'ss << line' to try to put the whole line into a stringstream, because it won't work - it will only read the first word. A stringstream is a stream, after all - the usual rules for reading apply. (That is wrong, but the design advice still applies:) But again, you can avoid this by re-creating the stringstream object, which happens naturally when you scope your variables properly.

14) Read into data members of a struct directly, instead of copying from local variables. To avoid repeated code to refer to the struct, alias it using a reference. (Better yet, write a helper function to overload operator<< for the struct; I can show this upon request.)



[Edited by - Zahlman on February 26, 2010 7:27:16 PM]

Quote:Original post by Zahlman
13) Don't use 'ss << line' to try to put the whole line into a stringstream, because it won't work - it will only read the first word. A stringstream is a stream, after all - the usual rules for reading apply. But again, you can avoid this by re-creating the stringstream object, which happens naturally when you scope your variables properly.

Using << on strings with spaces will work with std::stringstream. Were you thinking of >> ?