Thread: Can someone please clean up my code

  1. #1
    Registered User
    Join Date
    Aug 2007
    Posts
    59

    Question Can someone please clean up my code

    Hi everyone, new member here. For my assignement I have to write a program that takes a couple of things like ISO and shutter speed and the works out the correct exposure time for digital camera. Here is what I have so far ( it is not finished ) . I have no previous C experience so to get this far I got a LOT of help from other people and Internet so this looks rather messy . I have learned a lot already and while its imprortant to get a good mark in this assignement more imprtantly I need to leran implementation of various commands. Can someone please go through what I have so far and maybe clean it up a little and if possible explain what was wrong with it. Much appreciated

    Code:
    #include <stdio.h>
    
    typedef enum {FALSE, TRUE} BOOL;//This I do not know what it does. It looks unnecesary to me ?
    
    void main(void)
    {
    // Mainline Variable Declarations
    // FILE * output = stdout;
    // FILE * input = stdin;
    
    int option=0;
    
    char name[20]; //length limited upto 20 chars only
    printf("Enter name:");
    scanf("%s",name);
    
    //I have no idea how to separate these 3 options so they are not on one line 
    printf("Choose from one of the options");
    printf("1 - Enter ISO value");
    
    printf("2 - Enter Shutter speed");
    
    printf("3 - Help/n");
    
    scanf("%d", &option);
    
    if (option==1) goto label_1;
    else;
    if (option==2) goto label_2;
    // Help bit I still haven't added
    label_1:
    // type int needed for 400,800,1600,3200
    int ISO=0;
    // type float needed for 2 2.8 4 5.6 8 11 16 22
    float aperture=0;
    
    BOOL ReadSpeed = FALSE;
    BOOL ReadAperture = FALSE;
    
    while ((ReadSpeed == FALSE) || (ReadAperture == FALSE))
    {
    if (ReadSpeed == FALSE)
    {
    // prompt user for ISO setting.
    // The ONLY settings allowed are 50,100,200,
    // 400,800,1600,3200.
    
    printf("Hello %s Enter ISO settings: ", name);
    scanf("%d",&ISO);
    // flush the input stream in case of bad input
    fflush(stdin);
    printf("\n");
    
    switch(ISO)
    {
    case 50:
    printf("50 is selected ISO=%d\n", ISO);
    ReadSpeed = TRUE;
    break;
    
    case 6:
    case 8:
    case 10:
    case 12:
    case 16:
    case 20:
    case 25:
    case 32:
    case 40:
    case 64:
    case 80:
    case 100:
    case 125:
    case 160:
    case 200:
    case 250:
    case 320:
    case 400:
    case 500:
    case 640:
    case 800:
    case 1000:
    case 1250:
    case 1600:
    case 2000:
    case 2500:
    case 4000:
    case 5000:
    case 6400:
    // Shows how you can use multiple
    // case statements together
    printf("The selected ISO=%d\n", ISO);
    ReadSpeed = TRUE;
    break;
    
    case 3200:
    printf("3200 is selected ISO=%d\n", ISO);
    ReadSpeed = TRUE;
    break;
    
    default:
    printf("Invalid ISO=%d\nPlease Try Again\n", ISO);
    ReadSpeed = FALSE;
    break;
    }
    }
    
    if ((ReadAperture == FALSE) &&
    (ReadSpeed == TRUE))
    {
    label_2:
    // input the Shutter Speed .
    // The allowed values are : 2 2.8 4 5.6 8 11 16 22
    
    printf("%s please Enter shutter speed: ",name);
    scanf("%f", &aperture);
    // flush the input stream in case of bad input
    fflush(stdin);
    printf("\n");
    
    if (aperture == 1.2f)
    {
    printf("Selected apecture is=%2.1f\n", aperture);
    ReadAperture = TRUE;
    }
    else if (aperture == 1.4f)
    {
    printf("Selected apecture=%2.1f\n", aperture);
    ReadAperture = TRUE;
    }
    else if (aperture == 1.8f)
    {
    printf("Selected apecture=%2.1f\n", aperture);
    ReadAperture = TRUE;
    }
    else if ((aperture == 2.0f) || (aperture == 2.8f) ||
    (aperture == 4.0f) || (aperture == 5.6f) ||
    (aperture == 8.0f) || (aperture == 11.0f) || (aperture == 16.0f) || (aperture == 22.0f) || (aperture == 32.0f) )
    {
    printf("The selected apecture=%2.1f\n", aperture);
    ReadAperture = TRUE;
    }
    else if (aperture == 22.0f)
    {
    printf("Selected apecture=%2.1f\n", aperture);
    ReadAperture = TRUE;
    }
    else
    {
    printf("Invalid Aperture Selected =%f\nPlease Try Again\n",
    aperture);
    ReadAperture = FALSE;
    }
    }
    }
    
    printf("\nThe Entered Vales are ISO=%d and Aperture=%2.1f\n", ISO, aperture);
    {
    printf("This is the end of program --- still in development");
    }
    }
    Oh btw this needs to run on Unix system . If you have Unix do you mind checking if it runs properly please. Again much appreciated.

    Ben

  2. #2
    Cogito Ergo Sum
    Join Date
    Mar 2007
    Location
    Sydney, Australia
    Posts
    463
    Download the context editor and retype everything into that and it will automatically start setting the right indentations, or go and clean it up yourself.

  3. #3
    The larch
    Join Date
    May 2006
    Posts
    3,573
    Start by properly indenting the code (your IDE might have a code beautifier).

    Code:
    typedef enum {FALSE, TRUE} BOOL;//This I do not know what it does. It looks unnecesary to me ?
    Because your code uses TRUE and FALSE as "keywords" or constants and BOOL as a "datatype".

    Code:
    void main(void)
    That should be int main(void).

    Code:
    char name[20]; //length limited upto 20 chars only
    Use a constant, such as maxLenght.

    Code:
    if (option==1) goto label_1;
    else;
    if (option==2) goto label_2;
    Put the code you want to happen if option is 1 or 2 right there and get rid of goto-s. Better yet, put that code in separate functions and call them here instead of goto-s.

    The red semicolon is a mistake: because of that the second if is not part of an "else if" but starts a new if block.
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  4. #4
    pwns nooblars
    Join Date
    Oct 2005
    Location
    Portland, Or
    Posts
    1,094
    Code:
    char name[20]; //length limited upto 20 chars only
    Also, it can only hold 19 characters, and the \n to terminate the string. Just so you can understand future bugs if you rely on this 20 char max.

  5. #5
    and the hat of sweating
    Join Date
    Aug 2007
    Location
    Toronto, ON
    Posts
    3,545
    I'm not sure if the latest C standard allows this, but as far as I know, the //... single line comments are C++ only. Use /* ... */ comments in C.

  6. #6
    Registered User
    Join Date
    Sep 2006
    Posts
    835
    Quote Originally Posted by cpjust View Post
    I'm not sure if the latest C standard allows this, but as far as I know, the //... single line comments are C++ only. Use /* ... */ comments in C.
    They're allowed in C99 but not C90.

  7. #7
    Registered User
    Join Date
    Oct 2001
    Posts
    2,129
    Quote Originally Posted by Wraithan View Post
    Also, it can only hold 19 characters, and the \n to terminate the string. Just so you can understand future bugs if you rely on this 20 char max.
    you mean \0, not \n.

  8. #8
    Officially An Architect brewbuck's Avatar
    Join Date
    Mar 2007
    Location
    Portland, OR
    Posts
    7,396
    Quote Originally Posted by robatino View Post
    They're allowed in C99 but not C90.
    Annoyingly, GCC and MSVC both allow them, so often you don't even realize you've done it.

  9. #9
    Registered User
    Join Date
    Sep 2006
    Posts
    835
    Quote Originally Posted by brewbuck View Post
    Annoyingly, GCC and MSVC both allow them, so often you don't even realize you've done it.
    GCC will complain when given -ansi, so it's easy enough to just compile once with "-W -Wall -ansi -pedantic" when the code is done to purge these things.

  10. #10
    Registered User
    Join Date
    Sep 2007
    Posts
    54
    Hi killer.
    Im quite new to C aswell so the help that i give you might not be correct but ill do my best.

    you need a #include <stdbool.h> aswell at the top.
    Also as much as i hate fprintf u will lose marks if u dont use fprintf and fscanf.
    in your 10 line or so you forgot to include & in the fscanf command just before name.
    you can not use semicolons after else

    It is also a bit difficult to read it when its not indented its best if u do that first, you will lose a lot of marks if u dont do it anyway.

    plus i was on the impression that the psuedocode is due this week shouldnt you be working on that???

  11. #11
    Technical Lead QuantumPete's Avatar
    Join Date
    Aug 2007
    Location
    London, UK
    Posts
    894
    Quote Originally Posted by juststartedC View Post
    you can not use semicolons after else
    No true. It just doesn't do anything. If you have a semicolon after an else, you might as well delete the else statement

    QuantumPete
    "No-one else has reported this problem, you're either crazy or a liar" - Dogbert Technical Support
    "Have you tried turning it off and on again?" - The IT Crowd

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Enforcing Machine Code Restrictions?
    By SMurf in forum Tech Board
    Replies: 21
    Last Post: 03-30-2009, 07:34 AM
  2. Values changing without reason?
    By subtled in forum C Programming
    Replies: 2
    Last Post: 04-19-2007, 10:20 AM
  3. Obfuscated Code Contest
    By Stack Overflow in forum Contests Board
    Replies: 51
    Last Post: 01-21-2005, 04:17 PM
  4. Interface Question
    By smog890 in forum C Programming
    Replies: 11
    Last Post: 06-03-2002, 05:06 PM
  5. Replies: 0
    Last Post: 02-21-2002, 06:05 PM