Hi! I wrote a little program in C++. It's the first I've ever done. All it does is split a binary SLIC (from FreeStyler's SLIC & OEMCERT collection, for example) file into marker.bin and pubkey.bin. While the compiled program *does* run, I'm sure that since it's my first program, I've made some mistakes, used bad practice, etc. I was hoping that maybe someone here in the MDL community might be able to help me learn the errors of ways. I'd really appreciate someone looking at this for me Here's the code: Code: #include <stdlib.h> #include <stdio.h> int main(int argc, char *argv[]) { if (argc == 1) { char UsageText [] = "\nUsage: slicsplit [FILE]\n\nThis program extracts \"marker.bin\" and \"pubkey.bin\"\nfrom [FILE] and saves them to the working directory"; puts (UsageText); } else { char * FileName = argv[1]; FILE * SLIC; FILE * PubKey; FILE * Marker; char Buffer1[156]; char Buffer2[182]; if ((SLIC = fopen(FileName, "rb")) == NULL) { printf("Unable to open file: %s\n", FileName); return EXIT_FAILURE; } if ((PubKey = fopen("pubkey.bin", "wb")) == NULL) { char PubKeyFailureText [] = "Unable to create \"pubkey.bin\""; puts (PubKeyFailureText); } if ((Marker = fopen("marker.bin", "wb")) == NULL) { char MarkerFailureText [] = "Unable to create \"marker.bin\""; puts (MarkerFailureText); } fseek(SLIC, 36, SEEK_SET); fread(Buffer1, 1, 156, SLIC); if (fwrite(Buffer1, 1, 156, PubKey) == 156) { char PubKeySuccessText [] = "\"pubkey.bin\" created successfully!"; puts (PubKeySuccessText); } fread(Buffer2, 1, 182, SLIC); if (fwrite(Buffer2, 1, 182, Marker) == 182) { char MarkerSuccessText [] = "\"marker.bin\" created successfully!"; puts (MarkerSuccessText); } fclose(PubKey); fclose(Marker); fclose(SLIC); } return 0; }
hi, try to declare first all variables and then write instructions. Don't declare variables in instructions. BTW is that code working ? (i think it shouldn't, at least with VS ...)
Thanks so much for replying! So, like this? Code: #include <stdlib.h> #include <stdio.h> int main(int argc, char *argv[]) { /* Declaring Variables */ char UsageText [] = "\nUsage: slicsplit [FILE]\n\nThis program extracts \"marker.bin\" and \"pubkey.bin\"\nfrom [FILE] and saves them to the working directory"; char * FileName = argv[1]; FILE * SLIC; FILE * PubKey; FILE * Marker; char Buffer1[156]; char Buffer2[182]; /* Instructions */ if (argc == 1) { puts (UsageText); } else { if ((SLIC = fopen(FileName, "rb")) == NULL) { printf("Unable to open file: %s\n", FileName); return EXIT_FAILURE; } if ((PubKey = fopen("pubkey.bin", "wb")) == NULL) { char PubKeyFailureText [] = "Unable to create \"pubkey.bin\""; puts (PubKeyFailureText); } if ((Marker = fopen("marker.bin", "wb")) == NULL) { char MarkerFailureText [] = "Unable to create \"marker.bin\""; puts (MarkerFailureText); } fseek(SLIC, 36, SEEK_SET); fread(Buffer1, 1, 156, SLIC); if (fwrite(Buffer1, 1, 156, PubKey) == 156) { char PubKeySuccessText [] = "\"pubkey.bin\" created successfully!"; puts (PubKeySuccessText); } fread(Buffer2, 1, 182, SLIC); if (fwrite(Buffer2, 1, 182, Marker) == 182) { char MarkerSuccessText [] = "\"marker.bin\" created successfully!"; puts (MarkerSuccessText); } fclose(PubKey); fclose(Marker); fclose(SLIC); } return 0; } The code is working as I intended it to. I'm compiling it in Microsoft Visual Studio Express 2012 for Windows Desktop. I'm really curious, why do you think it shouldn't work? Thanks again for replying!
Well i'm not sure about it's behaviour as it's a console app, but i know such a app will fail compiling in a Win32 GUI program, i would declare char PubKeyFailureText [] = "Unable to create \"pubkey.bin\""; and other variables at the begining, but maybe it works on console, idk...
I have some things to say. Not criticisms just tips. Declaring all variables up front is an old style of prgramming, based on c when all variables needed to be declared that way. Fewer bugs are created through the use of "Proximity of use". Declare the variable as close to the point it needs to be used. Reading the code makes you aware that it's a new variable and you have less chance of thinking it's yours to use earlier than than when it should be used. Putting multiple statements on one line can cause issues, reading code is more difficult and errors can slip though. Put every statement on a new line Code: if (argc == 1) { puts (UsageText); } Becomes Code: if (argc == 1) { puts (UsageText); } int main is the main code, you should call another function to do the processing and name it with meaning. It doesn't matter how long the name is, it should be self documenting. Use Pascal Casing for function names. SplitSlicIntoMarkerAndPubKey Pass in a variable (SLIC) to the function, try to limit the use of global variables as much as possible, you can never be sure which part of your program screwed your data with global variables. 32 years developing experience behind these tips btw.
Thank you so much for your extremely informative answer, it was very helpful! I have a few questions, though, and it would be great if you would be kind enough to help me So, was the way I had been declaring variables in my first post better than the way I redid it in my second post? This part makes perfect sense to me, thank you! I'll be sure to strictly use PascalCase from now on! How would I add in this function?
if you mean how to add this function, then i think you can simply call it in your main name_function_to_call(parameter,parameter1); e.g: SplitSlicIntoMarkerAndPubKey(SLIC); then do the needed work in that function using its parameter(s).