4
\$\begingroup\$

This is a buy and sell game. I've revised it, though it's still unfinished. I would appreciate it if someone could review it again for me.

#include<stdio.h>
#include<conio.h>
#include<time.h>
#include<stdlib.h>
void end()
{
 system("cls");
 int P, L, choice;
 printf("          ***********          \n");
 printf("          END OF GAME          \n");
 printf("          ***********          \n");
 if(choice == P)
 main();
 else if(choice == L)
 exit(0);
 }

int PhoenixDown(int *phoenixdown)
{
 srand(time(NULL));
 *phoenixdown=rand()%(1200-500+1)%650;
 return *phoenixdown;
}
int ElixirEssence(int *elixiressence)
{
  srand(time(NULL));
 *elixiressence=rand()%(2100-1500+1)%910;\
 return *elixiressence;
}
int PlatinumIngot(int *platinumingot)
{
 srand(time(NULL));
 *platinumingot=rand()%(7000-5000+1)%1950;
 return *platinumingot;
}
int GoldenMateria(int *goldenmateria)
{
 srand(time(NULL));
 *goldenmateria=rand()%(5500-3500+1)%2600;
 return *goldenmateria;
}
int Scarletite(int *scarletite)
{
 srand(time(NULL));
 *scarletite=rand()%(12000-8000+1)%6500;
 return *scarletite;
}
int Adamantite(int *adamantite)
{
 srand(time(NULL));
 *adamantite=rand()%(30000-15000+1)%13500;
 return *adamantite;
}
int DarkMatter(int *darkmatter)
{
 srand(time(NULL));
 *darkmatter=rand()%(70000-40000+1)%36400;
 return *darkmatter;
}
int Trapezohedron(int *trapezohedron)
{
 srand(time(NULL));
 *trapezohedron=rand()%(90000-60000+1)%39000;
 return *trapezohedron;
}
int buyitem(int x, int y)//* function used to buy an item at any store*//
{                        
int bought;
bought=x-y;
return bought;       //*returns the value of the difference between x and y*//
}


void Tycoons(int *gil,int *onhand,int *day,int *debt, int *phoenixdown, int *elixiressence, int *platinumingot,int *goldenmateria, int *scarletite,int *adamantite, int *darkmatter, int *trapezohedron )//*function used as a shop, it does not return anything*//
{
 int i, j, g, on, d, de, count, N, option,p, e,pl,gm,s,a,dm,t;
 d=*day;
 g=*gil;
 de=*debt;
 on=*onhand;
 system("cls");
 printf("************************\n");
 printf("Tycoon Meteor's Minerals\n");
 printf("************************\n");
 printf("\n");
 printf("Shopkeeper: Welcome! How may I be of service?\n");
 printf("\n");
 printf("Item                   On hand                 Price\n");
 printf("\n");
 printf("Phoenix Down              %d                    %dG\n", on, PhoenixDown(&p));
 printf("Elixir Essence            %d                    %dG                 Day #%d\n",on,ElixirEssence(&e), d);
 printf("Platinum Ingot            %d                    %dG                Gil:%d\n", on,PlatinumIngot(&p), g);
 printf("Golden Materia            %d                    %dG                Debt:%d\n", on,GoldenMateria(&gm), de);
 printf("Scarlette                 %d                    %dG\n",on,Scarletite(&s));
 printf("Adamintite                %d                    %dG\n",on,Adamantite(&a));
 printf("Dark Matter               %d                    %dG\n",on,DarkMatter(&dm));
 printf("Trapezohedron             %d                    %dG\n",on,Trapezohedron(&t));
 printf("\n");
 printf("[1]Buy  [2]Sell  [3]leave\n");
 printf("\n");
 printf("Option: ", option);
 scanf("%d", &option);
 if(option == 1)
 {
 system("cls");
 printf("************************\n");
 printf("Tycoon Meteor's Minerals\n");
 printf("************************\n");
 printf("\n");
 printf("Shopkeeper: Buying an item? Which one?\n");
 printf("\n");
 printf("Item                      On hand                 Price\n");
 printf("\n");
 printf("[1]Phoenix Down              %d                    %dG\n", on, PhoenixDown(&p));
 printf("[2]Elixir Essence            %d                    %dG               Day #%d\n",on,ElixirEssence(&e), d);
 printf("[3]Platinum Ingot            %d                    %dG              Gil:%d\n", on,PlatinumIngot(&p), g);
 printf("[4]Golden Materia            %d                    %dG              Debt:%d\n", on,GoldenMateria(&gm), de);
 printf("[5]Scarlette                 %d                    %dG\n",on,Scarletite(&s));
 printf("[6]Adamintite                %d                    %dG\n",on,Adamantite(&a));
 printf("[7]Dark Matter               %d                    %dG\n",on,DarkMatter(&dm));
 printf("[8]Trapezohedron             %d                    %dG\n",on,Trapezohedron(&t));
 printf("\n");
 printf("Option: ", option);
 scanf("%d", &option);
 if(option == 1 || option == 2 || option == 3 || option == 4|| option == 5 || option == 6 || option == 7 || option ==8)
 {
                       if(option == 1 && g>PhoenixDown(&p))
                       {buyitem(g,PhoenixDown(&p));
                       on++;
                       Tycoons(gil,onhand,day,debt,phoenixdown,elixiressence,platinumingot,goldenmateria,scarletite,adamantite,darkmatter,trapezohedron);}
                       else if(option == 2 && g>ElixirEssence(&e))
                       {buyitem(g,ElixirEssence(&e));
                       on++;
                       Tycoons(gil,onhand,day,debt,phoenixdown,elixiressence,platinumingot,goldenmateria,scarletite,adamantite,darkmatter,trapezohedron);}
                       else if(option == 3 && g>PlatinumIngot(&p))
                       {buyitem(g,PlatinumIngot(&p));
                       on++;
                       Tycoons(gil,onhand,day,debt,phoenixdown,elixiressence,platinumingot,goldenmateria,scarletite,adamantite,darkmatter,trapezohedron);}
                       else if(option == 4 && g>GoldenMateria(&gm))
                       {buyitem(g,GoldenMateria(&gm));
                       on++;
                       Tycoons(gil,onhand,day,debt,phoenixdown,elixiressence,platinumingot,goldenmateria,scarletite,adamantite,darkmatter,trapezohedron);}
                       else if(option == 5 && g>Scarletite(&s))
                       {buyitem(g,Scarletite(&s));
                       on++;
                       Tycoons(gil,onhand,day,debt,phoenixdown,elixiressence,platinumingot,goldenmateria,scarletite,adamantite,darkmatter,trapezohedron);}
                       else if(option == 6 && g>Adamantite(&a))
                       {buyitem(g,Adamantite(&a));
                       on++;
                       Tycoons(gil,onhand,day,debt,phoenixdown,elixiressence,platinumingot,goldenmateria,scarletite,adamantite,darkmatter,trapezohedron);}
                       else if(option == 7 && g>DarkMatter(&dm))
                       {buyitem(g,DarkMatter(&dm));
                       on++;
                       Tycoons(gil,onhand,day,debt,phoenixdown,elixiressence,platinumingot,goldenmateria,scarletite,adamantite,darkmatter,trapezohedron);}
                       else if(option == 8 && g>Trapezohedron(&t))
                       {buyitem(g,Trapezohedron(&t));
                       on++;
                       Tycoons(gil,onhand,day,debt,phoenixdown,elixiressence,platinumingot,goldenmateria,scarletite,adamantite,darkmatter,trapezohedron);}
 }
 }
 else if(option == 3)
 {
        main(gil,onhand,day,debt,phoenixdown,elixiressence,platinumingot,goldenmateria,scarletite,adamantite,darkmatter,trapezohedron);
        d++;
        de *= (0.15);
     }
 else 
 {
      printf("wrong option");
      Tycoons(gil,onhand,day,debt,phoenixdown,elixiressence,platinumingot,goldenmateria,scarletite,adamantite,darkmatter,trapezohedron);
 }
 }

 void mainrift(int *gil,int *onhand,int *day,int *debt, int *phoenixdown, int *elixiressence, int *platinumingot,int *goldenmateria, int *scarletite,int *adamantite, int *darkmatter, int *trapezohedron)
{
 int d, g, de, Choice, on;
 char quit;
 d = 1;
 g=20000;
 de=50000;
 char option;
 on= 0;
 system("cls");
 printf("\n");
 printf("Gilgamesh: Where should my travels take me to?\n");
 printf("\n");
 printf("[1]Tycoon Meteor's Minerals                                        Day #%d\n", d);
 printf("[2]Pulsian Restoratives                                            Gil: %d\n", g);
 printf("[3]Archadian Luxuries                                              Debt: %d\n", de);
 printf("[4]Cid's Magical Escapades\n");
 printf("[5]Gaian Gratitudes\n");
 printf("[6]Riches and Minerals of Spira                                    [Q]uit\n", quit);
 printf("[7]Go see the Merchant of The Rift\n");
 printf("\n");
 printf("Your Choice: ", Choice);
 scanf("%d", &Choice);
 switch(Choice)
 {
               case 1:
                    Tycoons(&g,&on,&d,&de,phoenixdown,elixiressence,platinumingot,goldenmateria,scarletite,adamantite,darkmatter,trapezohedron);
                    break;
               case 'Q':
                            end();
                            exit(0);
                            break;
 }
 }
int main(int *gil,int *onhand,int *day,int *debt, int *phoenixdown, int *elixiressence, int *platinumingot,int *goldenmateria, int *scarletite,int *adamantite, int *darkmatter, int *trapezohedron)
{
 int d, g, de, on;
 system("cls");
 int choice, S, L;
 printf("                           *****************************         \n");
 printf("                           GILGAMESH MEGA GIL ADVENTURES         \n");
 printf("                           *****************************         \n");
 printf("\n\n\n\n\n\n\n\n");
 printf("[S]tart\n\n");
 printf("[L]eave\n\n");
 printf("Choice: ", choice);
 scanf("%d", &choice);
 switch(choice)
 {
               case 'S':
                    mainrift(&g,&on,&d,&de,phoenixdown,elixiressence,platinumingot,goldenmateria,scarletite,adamantite,darkmatter,trapezohedron);
                    break;
               case 'L':
                    exit(0);
                    break;
                    }

 getch();
}
\$\endgroup\$
7
  • \$\begingroup\$ It is just a minor suggestion, but try not to put actual values in numbers in code. For example: *phoenixdown=rand()%(1200-500+1)%650 . Define what 1200, 500 or 650 mean. Use proper named constants and enums (for the option part for example). \$\endgroup\$ – Coral Doe Aug 14 '12 at 10:41
  • \$\begingroup\$ hmm, i see, but i actually need to make it random. \$\endgroup\$ – Dave Garcia Aug 14 '12 at 11:15
  • \$\begingroup\$ It would still be random, but it would be rand()%(some_meaningful_name1 - some_meaningful_name2 + 1) % some_meaningful_name3 . It would be easiser to understand, easier to modify and it respects the C language guidelines lrdev.com/lr/c/ccgl.html#numbers. \$\endgroup\$ – Coral Doe Aug 14 '12 at 11:43
  • \$\begingroup\$ okay, also, my functions didn't work after i revised it, any suggestions? \$\endgroup\$ – Dave Garcia Aug 14 '12 at 12:03
  • 1
    \$\begingroup\$ Welcome to Code Review. Please note that the preferred way of saying 'thanks' around here is by up-voting good questions and helpful answers (once you have enough reputation to do so), and by accepting the most helpful answer to any question you ask (which also gives you a small boost to your reputation). Please see the FAQ and especially How do I ask questions here? \$\endgroup\$ – Jonathan Leffler Aug 14 '12 at 14:48
3
\$\begingroup\$

Are you familiar with structures and arrays in C?

Detailed critique of a minor part of your system:

void end()
{
    system("cls");
    int P, L, choice;
    printf("          ***********          \n");
    printf("          END OF GAME          \n");
    printf("          ***********          \n");
    if (choice == P)
        main();
    else if (choice == L)
        exit(0);
}

I'm not keen on system("cls") as a way of clearing the screen, but on Windows, it works.

You don't need the trailing blanks at the end of each line of the 'end of game' message.

You don't initialize any of P, L or Choice (so they contain indeterminate garbage), but you compare Choice with P and if they're equal, you call main() once more; if Choice is equal to L, you exit with success (which is good), and otherwise you return. You should think hard about this. Presumably, you needed a prompt such as 'Play again (P) or Leave (L):' and then you need to get input for which 'P' and 'L' are possible inputs. You'd probably want to upshift what the user typed, too.

While it is possible in C (but not C++) to call main() again, it is aconventional to do so. On the whole, you'd be better off not doing so. Your main program should have a loop which invokes the game (a function call), and then calls the end() function to get the choice from the user. If the choice is 'P', then you loop and play the game again; otherwise, you exit the loop and then the whole program.

\$\endgroup\$
3
\$\begingroup\$

I have a few points, some of which duplicate what others have said but are included for completeness:

  • What are you trying to achieve? Give some more detail as a header to the program.

  • Turn on more warnings (lots more) on your compiler.

  • All functions except main() should be static

  • Be consistent with naming. Some functions are lowercase, some are camel-case etc.

  • end() is using uninitialised variables. Better to exit by returning from main() if possible. Also end() should be end(void). And why are you calling main ???

  • Functions PheonixDown etc:

    • why do you need the output parameters in all of these functions;

    • don't call srand in each function;

    • numbers produced by rand() % x will not be truely random;

    • use constants for numerics.

  • buyitem() is verbose and terminating comment is just noise:

    int buyitem(int x, int y) 
    {
        return x - y;
    }
    
  • Tycoons() has too many parameters - pass a structure, perhaps.

    • you have 8 lines of duplicated printing from printf("Phoenix Down ...

    • yikes! it's recursive here too! What are you doing??? In C, it is best to avoid recursion unless you really know what you are doing...

I'm bailing out there - you really need to start again. First write down what you want to achive. Then write it as a sequence or flow chart. Then code it starting with a main, which has the prototype int main(int argc, char ** argv); and use a loop in main, calling a function that does the guts of your program.

Sorry to be negative, but you are on the wrong tracks completely and are better off realising it.

\$\endgroup\$
3
\$\begingroup\$

Here is feedback on C programming and style only. I haven't looked at algorithms or performance:

Style

  • In general, your source code would benefit from adding empty lines here and there, to separate sections of the code that aren't related to each other. Especially between different functions. This is de facto standard coding style, everyone adds empty lines between different functions.
  • You really need to fix your indention . If you use tab key for intention, then setup your editor so that it inserts spaces and not tab characters. Never write code on the same line as the opening { nor at the same line as the closing }.
  • Avoid "magic numbers", ie strange numeric constants in the meaning of the code. Define these numbers as constants, at a central location of your program (either as #defines or as const variables).
  • Use meaningful variable names, not "i, j, k, l, a, b, c".
  • Parameters that are passed by pointer by a function, but not modified by that function, should be declared as const. Search the web for "const correctness".
  • Try to keep each row short. Out of tradition, programmers strive to keep lines less than 80 characters (for historical reasons: this was the screen width on ancient operative systems). Instead of func(param1, param2, param3, param4, param5); you can write each parameter on a separate source code row. Example below.

func (param1,       // comment explaining param1
      param2,       // comment explaining param2 ...and so on
      param3,
      param4,
      param5);

Bugs

  • srand() must only be called one single time in your program, before any call to rand(). Otherwise you will keep seeding the random generator, in this case with the same number, meaning you could keep getting the same sequence of numbers that are not random at all.

  • There is absolutely no reason for your program to use recursive functions, they will only slow down, and possibly crash your program. Especially don't do this if you don't know what a recursive function is :) (It is a function calling itself from inside its own body). The uses of recursive functions are few, they are limited to various searching and sorting algorithms that aren't really suitable beginner topics.

  • Please note that all your floating point arithmetic has no use on variables declared as int and your program therefore won't behave as you expect. If you need floating point accuracy, you need to use float variables.

  • Note that scanf() leaves an empty line feed character ('\n') in the input buffer each time the user presses enter. These line feed characters need to be discarded or you might get them as user input instead of what you expected. (Every single C beginner encounters this bug) Easiest way to dodge it is to always keep an empty space at the beginning of scanf: scanf(" %d, x);. This initial space in the scanf format string tells it to get rid of all whitespace characters, including the pesky line feed one.

Dangerous practice

  • In C, avoid empty parameter lists such as void end(). It means "accept any parameter" and can lead to bugs. (In C++ however, and empty () is considered proper style.)

  • Never call main() from your program. Not only does it suggest an obscure program design, it may also cause severe, stack-related bugs if the compiler assumes that main() won't be called from anywhere but the "invisible" startup routines executed before your program.

  • Don't invent your own parameters to main(). Either use int main (void) or int main(int argc, char *argv[]). Don't use any other form of main.

  • Never use the exit() function. Reasons why can be found here.

General advise

Never write the whole big program at once. Write a litte bit at a time, compile it, test it, if there is anything wrong with it, fix it. Develop the program a tiny bit at a time.

\$\endgroup\$
0
2
\$\begingroup\$

I don't know any C, so I can't comment on the actual code. But it seems to me like you could move the text to txt files or xml files, instead of literally placing it in your code. You could probably walk through the txt file and print the text in a loop, rather than each line on its own.

If you really want to keep the text in the code, you could move the print lines to separate functions to clean it up. As an example, let's clean this part:

void mainrift(int *gil,int *onhand,int *day,int *debt, int *phoenixdown, int *elixiressence, int *platinumingot,int *goldenmateria, int *scarletite,int *adamantite, int *darkmatter, int *trapezohedron)
{
 //your code
 }

I would create a new function printMainRift():

void printMainRift(int *gil,int *day,int *debt)
{
     printf("\n");
     printf("Gilgamesh: Where should my travels take me to?\n");
     printf("\n");
     printf("[1]Tycoon Meteor's Minerals                                        Day #%d\n", d);
     printf("[2]Pulsian Restoratives                                            Gil: %d\n", g);
     printf("[3]Archadian Luxuries                                              Debt: %d\n", de);
     printf("[4]Cid's Magical Escapades\n");
     printf("[5]Gaian Gratitudes\n");
     printf("[6]Riches and Minerals of Spira                                    [Q]uit\n", quit);
     printf("[7]Go see the Merchant of The Rift\n");
     printf("\n");
}

The original function would become:

void mainrift(int *gil,int *onhand,int *day,int *debt, int *phoenixdown, int *elixiressence, int *platinumingot,int *goldenmateria, int *scarletite,int *adamantite, int *darkmatter, int *trapezohedron)
{
    int d, g, de, Choice, on;
    char quit;
    d = 1;
    g=20000;
    de=50000;
    char option;
    on= 0;
    system("cls");
    printMainRift(g, on, d, de);
    printf("Your Choice: ", Choice);
    scanf("%d", &Choice);
    switch(Choice)
    {
                case 1:
                        Tycoons(&g,&on,&d,&de,phoenixdown,elixiressence,platinumingot,goldenmateria,scarletite,adamantite,darkmatter,trapezohedron);
                        break;
                case 'Q':
                                end();
                                exit(0);
                                break;
    }
 }

This seems a bit better already :) But I think it would still be better to move the text to .txt files

\$\endgroup\$
2
\$\begingroup\$

A quick glance:

srand(time(NULL));

Should ONLY be called once.

int main()
{
     #if !defined(TESTING)
     srand(time(NULL));
     #endif

     // Your code
}

Calling srand() resets the random number sequence and thus makes the next number not so random.

\$\endgroup\$
2
  • \$\begingroup\$ yes but it's reinitialised with the current timestamp \$\endgroup\$ – hroptatyr Aug 15 '12 at 8:04
  • \$\begingroup\$ @hroptatyr: Yes. That is wrong. Never do that. Initialize it once. Otherwise you will not get a random (to the human eye) sequence. \$\endgroup\$ – Martin York Aug 15 '12 at 8:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.