[C++] What's wrong with my first application?

Discussion in 'Mixed Languages' started by drewbug, Nov 6, 2012.

  1. drewbug

    drewbug MDL Member

    Aug 15, 2010
    232
    42
    10
    #1 drewbug, Nov 6, 2012
    Last edited by a moderator: Apr 20, 2017
    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;
    }
    
     
  2. Pr3acher

    Pr3acher MDL Member

    Aug 24, 2012
    142
    48
    10
    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 ...)
     
  3. drewbug

    drewbug MDL Member

    Aug 15, 2010
    232
    42
    10
    #3 drewbug, Nov 6, 2012
    Last edited by a moderator: Apr 20, 2017
    (OP)
    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!
     
  4. Pr3acher

    Pr3acher MDL Member

    Aug 24, 2012
    142
    48
    10
    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...
     
  5. BobSheep

    BobSheep MDL Guru

    Apr 19, 2010
    2,326
    1,362
    90
    #5 BobSheep, Nov 6, 2012
    Last edited by a moderator: Apr 20, 2017
    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.
     
  6. Pr3acher

    Pr3acher MDL Member

    Aug 24, 2012
    142
    48
    10
    :eek: wow very nice! professional dev ? (cuz i aint)
     
  7. BobSheep

    BobSheep MDL Guru

    Apr 19, 2010
    2,326
    1,362
    90
    Yes a Professional.
     
  8. drewbug

    drewbug MDL Member

    Aug 15, 2010
    232
    42
    10
    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?
     
  9. Pr3acher

    Pr3acher MDL Member

    Aug 24, 2012
    142
    48
    10
    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).
     
  10. drewbug

    drewbug MDL Member

    Aug 15, 2010
    232
    42
    10
    I remade this application with Python:
    [POST]677130[/POST]