-2

Just started programming so any suggestion or constructive criticism would be much appreciated. I'm pretty sure there are ways to shorten this I just don't know how, especially the use of functions.

#include <stdio.h>
#include <stdlib.h>

int input, check = 1, ans;
int menu ();
int convert24(int hour, int min, int sec, int meridiem);
int convert12(int hour, int min, int sec, int meridiem);
int result(int hour, int min, int sec, int meridiem);
void processAnother();

int main()
{
do
{
    menu();
    processAnother();
    system("cls");
}while (ans = 'y');

}
int menu ()
{
    int hour, min, sec, meridiem;
    printf("[1] Convert to 24-hour notation\n");
    printf("[2] Convert to 12-hour notation\n");
    printf("[3] Quit\n");
    scanf("%d", &input);


    switch(input)
    {
        case 1 :
            {
                printf("Enter hours (1-12):minute(1-59):seconds(1-59): ");
                scanf("%d %d %d", &hour, &min, &sec);
                printf("[1] AM or [2] PM: ");
                scanf(" %d", &meridiem);
                return convert24(hour, min, sec, meridiem);
                break;
            }
        case 2 :
            {
                printf("Enter hours(1-24):minute(1-59):seconds(1-59): ");
                scanf("%d %d %d", &hour, &min, &sec);
                printf("[1]AM or [2]PM: ");
                scanf(" %d", &meridiem);
                return convert12(hour, min, sec, meridiem);
                break;
            }
        default :
            {
                printf("Goodbye :)");
                exit(0);
            }
    }
}
int convert24(int hour, int min, int sec, int meridiem)
{
    if ((hour > 12 || hour < 0))
    {
        printf("\nError! Invalid Hour\n");
        check = 0;
    }
    if ((min >= 60 || min <0))
    {
        printf("\nError! Invalid Minute\n");
        check = 0;
    }
    if ((sec >= 60 || sec <0))
    {
        printf("\nError! Invalid Second\n");
        check = 0;
    }
    if (meridiem > 2 || meridiem < 0)
    {
        printf("\nError! Invalid Meridiem\n");
        check = 0;
    }
    if (check == 1)
    {
        if (meridiem == 1)
        {
            if (hour == 12)
            {
                hour = 0;
                result(hour, min, sec, meridiem);
            }
            else
            {
                result(hour, min, sec, meridiem);
            }
        }
        else if (meridiem == 2)
        {
            if (hour == 12)
            {
                hour = 12;
                result(hour, min, sec, meridiem);
            }
            else
            {
                hour+=12;
                result(hour, min, sec, meridiem);
            }
        }
    }
    processAnother();
}
int convert12(int hour, int min, int sec, int meridiem)
{
    if ((hour > 24 || hour < 0))
    {
        printf("\nError! Invalid Hour\n");
        check = 0;
    }
    if ((min >= 60 || min <0))
    {
        printf("\nError! Invalid Minute\n");
        check = 0;
    }
    if ((sec >= 60 || sec <0))
    {
        printf("\nError! Invalid Second\n");
        check = 0;
    }
    if (meridiem > 2 || meridiem < 0)
    {
        printf("\nError! Invalid Meridiem\n");
        check = 0;
    }
    if (check == 1)
    {
        if (meridiem == 1)
        {
            if (hour <= 12)
            {
                result(hour, min, sec, meridiem);
            }
            else
            {
                hour -= 12;
                result(hour, min, sec, meridiem);
            }
        }
        else if (meridiem == 2)
        {
            if (hour <= 12)
            {
                result(hour, min, sec, meridiem);
            }
            else
            {
                hour -= 12;
                result(hour, min, sec, meridiem);
            }
        }
    }
    processAnother();
}
int result(int hour, int min, int sec, int meridiem)
{
    char *x;
    int y;
    if(meridiem == 1)
    {
       x = "AM";
    }
    else if (meridiem == 2)
    {
       x = "PM";
    }
    if (input == 1)
    {
        y = 24;
    }
    else if (input == 2)
    {
        y = 12;
    }
    printf("The %d-hr Notation is : %d:%d:%d %s\n",y, hour, min, sec, x);
    return;
}
void processAnother()
{
    printf("Process another [y/n]");
    scanf("%c", &ans);
    return;
}

Thank you very much.

Sean Ervinson
  • 123
  • 3
  • 12

1 Answers1

0

ans, is used as a char but declared as a int, it will work but it's ugly, also don't abuse global variable, input, check and ans can be declared somewhere else and some of them dont even need to be declared.

On convert12() and convert24() you're doing the same thing on the 20 first line, write a function that return 0 if they're an error and 1 otherwise.

I usualy put the main() function at the bottom of my file it allow me to not prototype every other function on the file

Instead of passing hour, min, sec and meridiem everytime you call most of your functions, create a struct with every element inside and pass a pointer of it.

main() should be declared like this main(void) see here for more information

for this one i'm gona use multiple steps

first:

int convert12(int hour, int min, int sec, int meridiem)
{
    if ((hour > 24 || hour < 0))
    {
        printf("\nError! Invalid Hour\n");
        check = 0;
    }
    if ((min >= 60 || min <0))
    {
        printf("\nError! Invalid Minute\n");
        check = 0;
    }
    if ((sec >= 60 || sec <0))
    {
        printf("\nError! Invalid Second\n");
        check = 0;
    }
    if (meridiem > 2 || meridiem < 0)
    {
        printf("\nError! Invalid Meridiem\n");
        check = 0;
    }
    if (check == 1)
    {
        if (meridiem == 1)
        {
            if (hour <= 12)
            {
                result(hour, min, sec, meridiem);
            }
            else
            {
                hour -= 12;
                result(hour, min, sec, meridiem);
            }
        }
        else if (meridiem == 2)
        {
            if (hour <= 12)
            {
                result(hour, min, sec, meridiem);
            }
            else
            {
                hour -= 12;
                result(hour, min, sec, meridiem);
            }
        }
    }
    processAnother();
}

can become:

int convert12(my_time_t *time, int input)
{
  if (are_time_correct(time, input))
    {
      if (time->meridiem == 1)
        {
          if (time->hour <= 12)
            {
            }
          else
            {
              time->hour -= 12;
            }
        }
      else if (time->meridiem == 2)
        {
          if (time->hour <= 12)
            {
            }
          else
            {
              time->hour -= 12;
            }
        }
    }
  result(time, input);
  processAnother();
}

I followed my previous advice and moved the call of result at the end

I know it won't work like this but you can now write it like that:

int convert12(my_time_t *time, int input)
{
  if (are_time_correct(time, input))
    {
      if (time->meridiem == 1 && time->hour > 12)
        {
          time->hour -= 12;
        }
      else if (time->meridiem == 2 && time->hour > 12)
        {
          time->hour -= 12;
        }
    }
  result(time, input);
  processAnother();
}

just before we tested if meridiem was between 1 and 2 so we can write this function like this

int convert12(my_time_t *time, int input)
{
  if (are_time_correct(time, input) && time->hour > 12)
    {
      time->hour -= 12;
    }
  result(time, input);
  processAnother();
}

do the same process for convert24()

If you followed all of my instruction you should have something like this:

#include <stdio.h>
#include <stdlib.h>

// time_t aleardy exist so I used my_time_t instead
typedef struct my_time_s
{
  int hour;
  int min;
  int sec;
  int meridiem;
} my_time_t;


void processAnother(void)
{
  char ans;

  printf("Process another [y/n]");
  // note the space at the start
  // that means to discard all whitespace
  // exit if ans different than 'y'
  if (scanf(" %c", &ans) <= 0 || ans != 'y')
    exit(0);
}

// don't prototype your fuctions with int if they return nothing
void result(my_time_t *time, int input)
{
  // use variable with explicit name
  char *meridiem;
  int notation;

  if(time->meridiem == 1)
    {
      meridiem = "AM";
    }
  else if (time->meridiem == 2)
    {
      meridiem = "PM";
    }

  // it's the same as old notation if you don't know the << operator search what bitshift is on the internet
  // y = 6 << input <==> y = 6 * 2 ^ input
  // so if input = 1 : y = 6 * 2 ^ 1 = 12
  //    if input = 2 : y = 6 * 2 ^ 2 = 24
  notation = 6 << input;
  printf("The %d-hr Notation is : %d:%d:%d %s\n", notation, time->hour, time->min, time->sec, meridiem);
  return;
}

int are_time_correct(my_time_t *time, int input)
{
  int check = 1;

  // double parenthesis are not needed
  // (6 << input) can be equal at 12 or 24 in function of input see above for more informations
  if (time->hour > (6 << input) || time->hour < 0)
    {
      printf("\nError! Invalid Hour\n");
      check = 0;
    }
  if (time->min >= 60 || time->min < 0)
    {
      printf("\nError! Invalid Minute\n");
      check = 0;
    }
  if (time->sec >= 60 || time->sec < 0)
    {
      printf("\nError! Invalid Second\n");
      check = 0;
    }
  // i don't think the meridiem can be below 0 so I replace (time->meridiem < 0) by (time->meridiem < 1)
  if (time->meridiem > 2 || time->meridiem < 1)
    {
      printf("\nError! Invalid Meridiem\n");
      check = 0;
    }
  return (check);
}

int convert24(my_time_t *time, int input)
{
  if (are_time_correct(time, input) && time->hour == 12)
    {
      // if time->meridiem == 1 :
      //    time->hour = 12 * (1 - 1) = 0
      // if time->medriem == 2 :
      //    time->hour = 12 * (2 - 1) = 12
      time->hour = 12 * (time->meridiem - 1);
    }
  result(time, input);
  // processAnother(); is not needed you already do it on the main loop
  return (0);
}

int convert12(my_time_t *time, int input)
{
  if (are_time_correct(time, input) && time->hour > 12)
    {
      time->hour -= 12;
    }
  result(time, input);
  // processAnother(); is not needed you already do it on the main loop
  return (0);
}

int menu(void)
{
  int input;
  my_time_t time;

  // string concatenation
  printf("[1] Convert to 24-hour notation\n"
     "[2] Convert to 12-hour notation\n"
     "[3] Quit\n");

  if (scanf(" %d", &input) <= 0)
    {
      printf("\nWrong input\n");
      input = 3;
    }


  switch(input)
    {
    case 1 :
      {
    printf("Enter hours (1-12):minute(1-59):seconds(1-59): ");
    if (!scanf(" %d", &time.hour) <= 0 ||
        !scanf(" %d", &time.min) <= 0 ||
        !scanf(" %d", &time.sec) <= 0)
      {
        printf("\nWrong input\n");
        return (1);
      }
    printf("[1] AM or [2] PM: ");
    if (!scanf(" %d", &time.meridiem) <= 0)
      {
        printf("\nWrong input\n");
        return (1);
      }
    return convert24(&time, input);
    break;
      }
    case 2 :
      {
    printf("Enter hours(1-24):minute(1-59):seconds(1-59): ");
    if (scanf(" %d", &time.hour) <= 0 ||
        scanf(" %d", &time.min) <= 0 ||
        scanf(" %d", &time.sec) <= 0)
      {
        printf("\nWrong input\n");
        return (1);
      }
    printf("[1]AM or [2]PM: ");
    if (scanf(" %d", &time.meridiem) <= 0)
      {
        printf("\nWrong input\n");
        return (1);
      }
    return convert12(&time, input);
    break;
      }
    default :
      {
    printf("Goodbye :)");
    exit(0);
      }
    }
  return (0);
}

// function without parameters are declared with void
int main(void)
{
  // a while loop is simpler imo
  while (1)
    {
      // if menu() return 1 it means the user enter a wrong input
      // in that case no need to call processAnother()
      if (!menu())
    {
      processAnother();
    }
      // DON'T USE SYSTEM NEVER EVER SEE HERE FOR MORE INFO https://stackoverflow.com/questions/19913446/why-should-the-system-function-be-avoided-in-c-and-c
      // anyway some shell (such as mine) don't reconise `cls` and use clear instead
      system("cls");
    }
  return (0);
}

you also didn't test if scanf returned something, they're some case where you program will segfault

Community
  • 1
  • 1
plean
  • 124
  • 1
  • 9
  • Unclear why you comment "you also didn't test if scanf returned something, they're some case where you program will segfault" and then post recommended code with `scanf("%d", &input);`, `scanf(" %c", &ans);` and others. Seems inconsistent. – chux - Reinstate Monica Oct 17 '16 at 13:38
  • @chux you're right I'll update my post later with more clarification – plean Oct 17 '16 at 13:41
  • Detail: Concerning `if ((sec >= 60 || sec <0))` instead of `if ((sec > 60 || sec <0))`, you may enjoy [Leap Second](https://en.wikipedia.org/wiki/Leap_second). – chux - Reinstate Monica Oct 17 '16 at 13:43
  • You're right it looks much better. I still have a long way. Thanks for the feedback. – Sean Ervinson Oct 18 '16 at 13:25
  • @SeanCOng if my answer helped you you can validate it by clicking the checkmark on the top of my response. – plean Oct 18 '16 at 14:14