What’s Wrong With This Code
Not many people responded to last week's puzzle, so I'm going to leave it open for another week. In the mean time, take a crack at this.
Here's something many of us would have written in college. Take a look at the code and see if you can spot what's wrong before reading on. Note that there isn't a logical flaw.
void ConvertSecondsToDate()
{
int seconds;
Stream *pInStream = new InputStream();
pInStream->ReadInt( &seconds );
DateTime dateTime( seconds );
Stream *pOutStream = new OutputStream();
pOutStream->WriteString( "The date is " );
pOutStream->WriteInt( dateTime.GetMonth() );
pOutStream->WriteString("/" );
pOutStream->WriteInt( dateTime.GetDayInMonth() );
pOutStream->WriteString( "/" );
pOutStream->WriteInt( dateTime.GetYear() );
delete pInStream;
delete pOutStream;
}
Did you spot it? If you've never published software for a large company before, then you probably didn't. There's a few things wrong here, let's take a look at each.
Not Exception Safe
I allocated pInStream and pOutStream but I don't delete them until I've done a bunch of other operations. If any of those operations in between throw an exception, that memory will never be freed. Even in C# or Java this is still a problem even though they have garbage collection. The stream may not get flushed in a timely manner or it may stay open until your program closes. This uses up system resources and your program's footprint will grow in size. Even managed applications can have resource leaks of this sort.
For non-managed languages, use some kind of smart pointer which keeps track of the memory for you and deallocates it in its deconstructor. The STL is one place to use for such tools.
Not Localizable
The string "The date is" is hardcoded into the executable. There's no way to change it without recompiling the program again. This is bad because if you ever wanted to localize it, you would need to change strings directly in the source files. Now you have separate source trees per language, which will lead to inconsistencies in the source code. Finally, if you ever need to patch your application, you're in for a real challenge as you will have to test and issue separate patches for each localization. Remember that because you compile the strings into the source, the binary executable will be different on each machine.
For those that need to write applications that are used in different languages, load your strings from a resource that is outside of the EXE.
Not Internationalizable
The above output will be readable by those in the U.S., Canada, England, Australia and New Zealand, but it's still won't give the correct answer for all of them. The reason is that dates have different arangements in different regions. Some are MM/DD/YY or DD/MM/YY and others require four digits for year. Many things need internationalization attention including dates, weights and distance, time and even text orientation (is it read left to right or right to left? Displayed horizontally or vertically?).
Not Secure
The above code doesn't have any exploits that I can see, but it has potential for insecurity. Everything is dependant on our DateTime class since it validates and translates our input. Care must be taken to ensure that the DateTime constructor takes in an 'int' and not an 'unsigned int'. The compiler will warn you about this, but warnings don't mean it won't build an executable for you and so it's commonly ignored for class projects. I would suggest checking the "Turn warnings into errors" flag and get used to fixing them now so they don't bite you later.
Another problem could come from ReadInt. Will it always initialize seconds? What if the read fails, what value is in seconds? Because we don't initialze seconds, we may process the value that the compiler puts there. For some languages this is 0, for others its whatever memory was there before hand. In this instance the worse that could happen is we print the wrong date. In other more critical parts of code we may write the wrong data to a file, corrupting user date or even allow for code exeuction. Always initialize your variables.
Not Robust
This plays into the above, what if ReadInt fails? We have no code to check for it. We don't catch exceptions or check error codes. I didn't provide the interface for Stream, but any stream class will have some way of indicating it's failed to do what you asked. If that's returning HRESULTS or some enumeration, then you need to check it and handle it, otherwise you have no idea what your code may do. Exceptions are guarenteed to get handled somewhere even if that means exiting your program because of an unhandled exception. Which is exactly why you should catch all exceptions that your functions say they will throw or have verified that the code calling yours catches them.
Not Maintainable
There's no comments! The only way I know what the code is going to do is by interpreting the code itself. Trying to figure out what the code is doing by looking at the code is fine, trying to figure out the authors intent for what the code is for or what he intended it to do is not. Comment your code so others know what you were thinking.
