DUMP written in SimpleIDE
tomcrawford
Posts: 1,126
Attached is a DOS-style DUMP program I wrote as an exercise. I did it in SimpleIDE, and, I hope, in keeping with the propellor style C. I would be grateful if any "C" guru(s) were to critique it.
Comments
I'm going to give opinions on lots of things, in the order I read them, not necessarily importance. Know that, overall, this is great and I like it a lot
lines 5-11: move inside the main function
Global variables are generally bad. Don't use them unless you have to, and even then, you probably don't have to and other forum members can help you find a way to avoid it.
line 20: change to "if (NULL == sourceFp)"
I love the concept of "yoda conditions." This is where your constant goes on the left and your variable on the right, so when read out loud it is "if null equals sourceFp" rather than "if sourceFp equals null". Getting in the habbit of always doing yoda conditions ensures that you never accidentally forget an equals sign and write "if (sourceFp = NULL)" because that has a slightly different effect!
line 22: Great job!
I love that you're returning -1 even though it doesn't have any functional effect. This makes your code more portable, and one could simply rename "main" to "dump" and then use it elsewhere.
line 25: change to yoda conditions
No chance of accidentally assigning something here, but it's just so that you get in the habit
line 26: 16 is a magic number
Magic numbers are mysterious and I have to ask myself? why 16? What's so special about 16? I know it is 2^4. I know it's 4^2. It's twice as many cores as are on the Propeller. Uhhh... what is it??? Rather than providing a comment at the end of line, create a constant ("const int" or preprocessor macro via "#define") with a descriptive name such as "CHARACTERS_PER_LINE". Notice that this name is in all caps, because that is a globally accepted standard for constants, no matter what programming language.
Use of CRSRX: Not a fan! It's taken me the last 5 minutes just to figure out what lines 33-35 do.
Line 38: Yoda conditions again
For further reading, I wrote a similar function here, but keep in mind that this function has access to the complete 512 byte buffer from the SD card, where as you only have access to a single byte at a time.
Many would say I went too far and created too many functions, each of which is too small. And in fact, if you take a gander at my own version of this function in PropWare, it is nearly the size of yours. But I wanted to show you an extreme version of "comments are bad" because your code is an extreme example of "comments are good" and I think the best code lies somewhere in the middle.
Notice how almost every single variable has been renamed - no more abbreviations. The functions that were created are all very wordy. The function parameters are also descriptive, but only within their own function (for instance, "dumpCharacter" has a parameter "character" not "fileCharacter" because the function could be used to dump a character from an in-memory buffer or keypad or who knows what else). Keeping in mind a large scope makes your code even more re-usable.
Also notice how lines 71-79 are part of an if-else block. To me, it is now much more obvious that main() has the potential for an early return - a failure. Without the else block, it is slightly harder to see that there are two return statements in the function.
instead of
In other words, associating the * with the type rather than with the variable. It is semantically incorrect and can get you trouble with statements like
That looks correct but isn't since the second variable "fp2" isn't being declared as a pointer.
Of course, I realize that I am in the minority on both of these issues. Oh well...
I am 100% in agreement with your second suggestions, about the pointer declaration. I always used "int* x" until one day I hit a weird compiler error and couldn't figure it out! Then... then I found out the difference between "int* x, y" and "int *x, *y" and I haven't gone back since!
I can see the reasoning for it but would never do it.
"if (RUN == state)" just seems to be all wrong to me. It seems to be implying that the value of "RUN" is the thing that might change to become equal to "state".
I agree about global variables. Don't know why I did that.
Yoda conditionals. I certainly cannot claim to have never used = when I meant == but still. It *does* just seem "all wrong". I have to think carefully about that.
Magic Numbers: I thought about making CHARACTERSPERLINE a constant but then thought "Shouldn't I make SIZEOFHEXVALUE and SPACEBETWEENHEXPARTANDASCIIPART constants as well?" I guess moderation is a Good Thing and think that you actually got it just right. When I re-write the program today, I will certain make some of these constants.
CRSRX: I just didn't know how else to position the cursor where I wanted it.
FILE* fp1: My bad. But, you know, that was a typo, not deliberate. And it seems to me that "C" has *so* many ways to get into trouble, compared to any other language I have used.
Again, Thank you, David and David and Heater.
I would read 16 characters into a temporary buffer and then iterate over that buffer twice.
At least, I say that having not tried to write it for real. I'm realizing now as I imagine some psuedo code that it is much harder in your case, where you don't know that the final line has exactly 16 characters. It's easier when you have access to the full buffer before you start.