By Ali Çehreli
[Last month, I published an Append utility that had some interesting properties. It also had some flaws. In an email, Ali did an excellent critique of my code. My responses are in italics - Reg. Charney]
I've read the Tech-Talk column in June 2002 issue of ACCent. I enjoyed reading the article and understand it's usefulness. However, I see a number of places where we can make the code better, and sometimes even correct.
While some of the notes below are unimportant and sometimes reflect only style preferences, I think some are very important. I will go through the lines of the code in the order they were listed in the article.
There is a smaller program at the very end that attempts to do the same thing. I wonder if it has the scaling problems that you were referring to.
| #include <stdio.h> #include <fstream.h> |
The standard C++ versions of the above files are:
| #include <cstdio> #include <fstream> |
The versions used in the original article are pre-standard and at least not aware of namespaces.
This is correct. Unfortunately, not all vendors have done this correctly.
| const int MAXBUF = 1024; |
Some programmers prefer reserving identifiers with all capital letters for macros. Following their advice, I would have introduced the constant as:
| const int maxBuf = 1024; |
The original idea was so that manifest constant values would be readily apparent. For that reason, I continue to capitalize all my manifest constants.
Next,
| class OFile : public
Fbuf { /* ... */ }; |
Public inheritance is used as an is-a relationship where the derived class is a type of the base class. I think OFile is not an FBuf but “is implemented in terms of” Fbuf. That type of relationship is modeled either by a “has-a” relationship or by private inheritance. Besides, FBuf has no public interface that would warrant its public inheritance. Adding the advice of preferring containment of inheritance, I think a better design would be
| class Ofile { FBuf buffer_; /* ... */ }; |
I agree that private inheritance is more appropriate here than public inheritance. However, I was trying to illustrate the concept of using static base class members as a way of sharing data, so I did not want a “has a” relationship.
Next
| class OFile : public
FBuf { ofstream oF; char* oFN; public: OFile(char* oFN_) : oFN(oFN_) { /* ... */ throw oFN; } /* ... */ }; |
The member variable oFN is used to hold the name of the file to be thrown in an error condition. This design relies on the fact that the character buffer that holds the name of the file is still available at the point where the exception is caught.
Even though argv will live longer than any object defined in main, throwing a char * may not work in general. A better approach is to throw std::string and catch with std::string const &. std::string always owns its buffer and need not rely on external data.
Agreed.
Additionally, the exception thrown in the OFile constructor will never be caught in the program, because the variable oF in main, is not constructed within a try block.
This was intentional. If I couldn’t create the output file, there was no reason to go on and I wanted to end the program.
Next,
| oF.open(oFN, ios::app|ios::binary/*,filebuf::sh_none*/); |
Unfortunately there is no standard way of locking a file for single access. filebuf::sh_none is an extension provided by some of the compilers.
This is true, but all the systems that I know of have a similar mechanism to open files exclusively for output.
Next,
| ~OFile() { oF.close(); } |
It is not necessary to call close in this case since the ofstream destructor will be called for oF member anyway. In fact, it is not necessary to define the destructor for OFile because it does not own any explicit resources.
| void write() { if (oF.is_open()) { /* ... */ } |
The check for the file being open in write is unnecessary, because the object would never have existed if the file could not be opened in the constructor. The reason for throwing an exception in the constructor is to avoid having incomplete OFile objects around. If we ever enter write, it must be for a valid object.
| delete [] bp; |
There is not an explicit new[] call in OFile, which corresponds to the line above. This fact points to the possibility that there is an ownership transfer somewhere in the code; but that transfer is not easily visible.
The pointer bp is part of the base class Fbuf. Normally, I always clean up after myself. However, it would have been better to do this in Fbuf’s destructor.
| bp = 0; |
Needing to clear a pointer that points to memory that has just been released is almost always a sign of a buggy program. Why did we need to clear the pointer? Is it because we've realized that we were calling delete[] more than once without an intervening new[] (double-delete)? If so, the bug that causes the double-delete must be identified and fixed.
I strongly disagree here. This is defensive programming. There is no chance that this released memory will be used if the pointer to it is set to null. I always assume that someone else will maintain my code and I don’t want him to trip over this potentially very nasty bug. Think of it as protective redundancy.
| operator bool() |
The reason for this operator is to check for more bytes in the input file. The standard idiom for such operations is to define an operator void * which provides the same functionality without the possibility of silly errors like using the object in arithmetic operations: A bool can be used in arithmetic operations but a void * cannot be.
Agreed. You have a very valid point.
| bp = new char[bSZ]; if (!bp) throw iFN; |
It is not the standard behavior for new to return 0 in case it fails to allocate memory. The standard behavior is to throw std::bad_alloc. For this reason, the program will never reach the line throw iFN: new will either succeed and the if (!bp) block will never be executed, or fail and if (!bp) will never be executed.
There is a no-throw version of new though, but this behavior must be explicitly specified:
| bp = new(nothrow)
char[bSZ]; if (!bp) throw iFN; |
Agreed, but not all vendors do it right.
| printf("\n\nUsage: append in-file1 [ , in-file2 ]..."); |
It may be better to use argv[0] as the name of the program in the message above. This would be more general, in case someone built the application under a different name than append.
A smaller version that might be useful:
| #include <fstream> #include <iostream> int main(int argc, char * argv[])
|
Without the try...catch block, there seems to be a savings of one line. However, I believe your use of the rdbuf() function is problematic here. You are writing from the input buffer without first reading anything. Some implementations read some portion of the file into an internal buffer when the file is first opened for input, but this is not “standard” behavior. rdbuf() may actually return a dummy until the first read is executed successfully. Also, a failure to open any input file terminates the program, while using the try...catch block handles this situation and allows the remaining input files to be read.
By Reg. Charney
When operating as a “lone wolf”, one of the things I miss most is someone to critique my work. For that reason, I like the idea of Code Reviews, Extreme Programming, and Agile Methodology. Ali Çehreli has done me the kindness of reviewing my code from last month any pointing up a number of issues. Many of them are valid and extremely valuable. My responses (excuses ;-) are included in the article.
Amongst our ACCU members, we also have small book review groups. They meet to discuss the latest thinking in programming and design methodology. Much of what Ali has written is reflected in his keeping up-to-date in the current thinking on C++ programming. I must own up to being older and of writing in the “old” style with a sprinkling of some of the new paradigms, like exception handling.
There are two things worth noting here. I have tried to use a variety of compilers to test the code I wrote and I am still having trouble with cross-compiler compatibility for standard C++ code. Second, even such a seemingly small and simple program taught me a lot when I had someone else look at my code.
Thus, if you are interested in increasing your programming and design skills, set up a programming skills group and also actively seek out peer review.
It is with great pleasure that I am able to welcome Accent Technology (www.accent-tech.com) as a sponsor for this newsletter. They provide professional grade IT training to the Silicon Valley. Also, some of you may already know that Accent Technology provides both the ACCU with space for our monthly meetings and the SVLUG with space for their monthly InstallFest. Again, thanks to the terrific folks there and their President, Tom Currier.
Metadata Solutions: Using Metamodels, Repositories, XML, and Enterprise Portals to Generate Information on Demand by Adrienne Tannenbaum,Addison Wesley, ISBN 0-201-71976-2
Recommendation: This is an excellent book and is well worth reading.
As this book's subtitle accurately indicates, this well-written book ranges over considerable topical territory. (However this book generally avoids the commonly related syndromes of useless superficial coverage or endless disjointed details). The central theme and big picture involves using a systematic metalevel conceptual framework for designing, fixing, and managing internet accessible data warehousing systems. (However this book also deals with many other less demanding applications along the way.) This structure leads to a well-organized treatment of many related subtopics that are illustrated with many useful examples. Technical issues are usefully oriented within the context of the business aims (and related concerns) that they ultimately serve, and due diligence is given to people-ware issues.
An adequate technical review of this book requires a long article, which is beyond the scope of this brief review. This book may be regarded as something of a course concerning pragmatic principles of (for lack of better words) "advanced self-describing dynamic data structures". This book contains much information that should be useful for many common types of data centric program development in general and it could also serve as a handbook for dealing with various problems involving metamodels, repositories, XML, and enterprise portals.
Before getting into some of the other topics covered, here is a severely condensed description of the key buzzwords (derived from the book's handy glossary). Metadata describes the formats and characteristics of the base level data; meta-metadata similarly describes meta data (which is useful for generic tool-based processing and accessing). Metamodels are graphic representations of a system of metadata requirements, organized variously according to the perspectives of metadata source, classification, or usage. Meta-metamodels are the counterparts of metamodels for meta-metadata, and these represent metadata solution views for integrating various tools or application-specific metamodels. Everything in the book revolves around this conceptual framework, and many examples and diagrams explain and demonstrate the nature and utility of this framework.
To attract wider interest beyond the apparent target audience suggested by the title, I can summarize some of the topics covered although, I don't think I can do better than the publisher's excellent summary from the back cover, so my heavily edited version of it follows.) The topics include:
While some of the terminology used above may have a "silver bullet" ring to it, the practical issues of metadata solutions for database and data-integration issues are repeatedly emphasized and elaborated.
— Conrad Schneiker
C++ haters point out that even the name of the language contains a bug: "After all, it really should be called ++C, since we only want to use a language after it's been improved." -- Cay Horstmann & Gary Cornell, “Core Java”, Sun Press
—Nate Campi email signature
[Ed. If you come across signatures that you think are humorous, send me (editor@accu-usa.org) a line. Other readers may enjoy them too.]
By Reg. Charney
We have all heard about the Enron, WorldCom and Xerox scandals recently and the massive layoffs these entail. However, the overall picture in the last six months looks more positive than the bad news seems to imply. Figure #1 shows that more jobs have become available since last month and is the most positive it has been, both locally and nationwide.