• Status: Solved
  • Priority: Medium
  • Security: Public
  • Views: 317
  • Last Modified:

What's Happening Here? (Easy)

Hi guys,
I'm writing this code to learn the basics of C, can somebody please explain what's wrong with this, and how to fix it.

/*
 * inventory.c
 *
 * This program keeps track of an inventory of CD's for a record store.
 * It storest the Title, Artist, Number of tracks, if the CD is an album or a single
 * and the price.
 *
 * by SP, 2004.
 */
#include <stdio.h>

main()
{  
    //Declaring Variables
    char title[51];
    char artist[51];
    short tracks;
    short is_album;
    float price;
    //Get the CD's Details
    printf("CD Inventory Program\n");
    printf("Please enter CD's:\n");
    printf("Title:");
    fflush(stdin);
    scanf("%[^\n]",title);
    printf("Artist:");
    fflush(stdin);
    scanf("%[^\n]",artist);
    printf("No, of Tracks:");
    fflush(stdin);
    scanf("%d",&tracks);
    printf("(A)lbum/(S)ingle:");
    fflush(stdin);
    if(getchar() == 'a')
        is_album = 1;
    else
        is_album = 0;
    //scanf("%d",&is_album);
    printf("Price:");
    fflush(stdin);
    scanf("%f",&price);

    //Print the CD's Details
    printf("Title:%s\n",title);
    printf("Aritst:%s\n",artist);
    printf("No of tracks:%d\n",tracks);
    printf("Type of CD:");
    if (is_album)
        printf("Album\n");
    else
        printf("Single\n");
    printf("Price %d",price);
    fflush(stdin);
    getchar();
}

Input:
CD Inventory Program
Please enter CD's:
Title:Country Grammar
Artist:Nelly
No, of Tracks:16
(A)lbum/(S)ingle:a
Price:14.99

Output:
Title:Country Grammar
Aritst:
No of tracks:16
Type of CD:Album
Price 1073741824
   
0
SaMuEl
Asked:
SaMuEl
  • 4
  • 3
  • 3
  • +1
2 Solutions
 
Kent OlsenData Warehouse Architect / DBACommented:

Try using "int" as the datatype for "tracks" and "is_album".


Kent
0
 
PaulCaswellCommented:
>>    printf("Price %d",price);
You are printing 'price' as an int. Its actually a float.
Use:
    printf("Price %f",price);

Paul
0
 
PaulCaswellCommented:
I dont see why the artist is not printing! Does it still go wrong?

Paul
0
Protect Your Employees from Wi-Fi Threats

As Wi-Fi growth and popularity continues to climb, not everyone understands the risks that come with connecting to public Wi-Fi or even offering Wi-Fi to employees, visitors and guests. Download the resource kit to make sure your safe wherever business takes you!

 
Kent OlsenData Warehouse Architect / DBACommented:
Hi Paul,

That's what I was chasing with having him change the datatypes to int.  scanf() was using a %d as the data type.

Most compilers will define stack items "top to bottom" so that the first item declared is at a lower address than the second, etc.  If his compiler places them on the stack "upside down" then the wrong datatype for the integers would overwrite the first byte(s) of artist.


Kent
0
 
PaulCaswellCommented:
Ahh!!! I see!! So the printf library is assuming %d means 32 bit but they've defined 'tracks' as a short (16bits). This may overrwrite 'artist' if the stack is that way inclined. Good catch Kent! My head is still working in 16bit! I work with legacy code all day.

Samuel,

Kent and I are both right.

Use 'int' for 'tracks' to fix the artist not working.
Use "%f" for the format of the price.

Paul
0
 
Kent OlsenData Warehouse Architect / DBACommented:

Hi Paul,

My head still works in 16-bit, too.  Just too many years of coding that model.....   :)

I don't "know" that what I described is his problem, but it would make sense.  "%d" may not interpret a "short" correctly, just as it certainly won't print out a "float" correctly.

Kent
0
 
SaMuElAuthor Commented:
Aha!
Very good Guys. I changed it scanf to read in a short %hd

P.S. Has a short always been 16bit? I vaguely remember doing some c++ in dos, and short was only 8 bit i.e. shorter than an integer, I could be mistaken though.
0
 
Kent OlsenData Warehouse Architect / DBACommented:

Hi SaMuEl,

A "short" can be 8, 16, 32, or 64 bits.  So can an "int" or a "long".  According to the spec, the only restriction is that "a char can not be longer than a short, nor can a short be longer than an int, nor can an int be longer than a long".

Also, char, short, int, and long must be atomic operations.  (Fetch and Store are single instructions.)

So it gets back to the compiler and the system that you are using.  In DOS, you seldom see integers larger than 16 bit because of the limited memory models.

Odd that you chose to modify the scanf() instead of the datatype.  For the record, I NEVER use char or short for scalar variables unless they are embedded in a struct (or union) and data alignment requires a variable of the corresponding size.  My integers are always, int, long, or __int64.


Kent
0
 
SaMuElAuthor Commented:
Thanks a lot kent, you've really cleared that up for me :)
0
 
danielvallasCommented:
Now try putting on those variables into a structure, creating add, delete, and query functions - you could just loop through a list of structures for the query until you learn about linked lists and binary trees or hash tables.
0
 
SaMuElAuthor Commented:
yep working on it...
0
 
danielvallasCommented:
Do you know how to use structures?

struct CD_INFO
{
    char title[51];
    char artist[51];
    short tracks;
    short is_album;
    float price;
};

// you might want to do a type define so that you don't have to type "struct CD_INFO" all the time, you just type
// CdInfo (that is what I called it on the next line) - oh, and don't use the // for comments - use /* */ ... the // is C++, not C
typedef struct CD_INFO CdInfo;

// you should probably declare a variable for your maximum value - that you can test on - it just replaces it with the value 100
#define MAX_CD_RECORDS   100

// then I would define a global array of structures to simplify things - instead of passing in the pointer to the sturcture to all
// your functions - but if you did, you would declare your functions like void AddRecord( CdInfo *pInfo ) - then you would
// reference the record the same way - to call AddRecord and pass in the pointer, you would just pass in, cd_records, as
// defined next.
CdInfo cd_records[MAX_CD_RECORDS];

//then write your main function to print a menu and prompt whether to add,delete,or print a record or exit the program.
//and do a switch statement on the input character and call AddRecord or DeleteRecord or if GetRecord/PrintReord.
//to keep things simple, just keep a total stored that is the total number of active records in the array - and make sure that in
//the AddRecord function it does not go beyond CD_MAX_RECORDS - 1 ...
// like if (iTotalRecords < (CD_MAX_RECORDS - 1))
//          AddRecord();
int   iTotalRecords;  //the i in the name stands for integer - just a naming convention - you should do that stuff, i = integer,
                             //dw = double word (C++), f = float, str for string... etc.  You can keep track of what type the variables
                             //are that way instead of looking them up all the time - it helps other programs to read your code.

//You should also have a sort function in your program - just do a bubble sort to sort alphabetically - actually it is the
//least effiecient method - but a good place to start - you do it like this....

int i, j;
CdInfo TempCDRecord;

for ( i = 0; i < MAX_CD_RECORDS; i++ )
{
    for ( j = 0; j < MAX_CD_RECORDS; j++ )
    {
         if (cd_records[i].price < cd_records[j].price)   // I forget if it is greater than > or less than <, I always forget
         {                                                                 //if it sorts backwards as is (Z to A), then just switch it.
             //switch the two records - have to use memcpy to copy all the fields in the structures at once.
             memcpy( &(TempCDRecrod), &(cd_records[i]),    sizeof(CdInfo) );
             memcpy( &cd_records[i]),     &(cd_records[J]),    sizeof(CdInfo) );
             memcpy( &cd_records[j]),     &(TempCDRecrod), sizeof(CdInfo) );
         }
     }
}

Have fun with it - I remember when I learned C... years ago - I took a course at some college - finished the book half way though the course and began trying to write my own CAD program with Borlan C++ - I did not know enough about memory holes that operating systems have if you keep allocating and deallocating memory allot - and it kept crashing...

A very important beginner leason...
Do not allocate and deallocate memory all the time in a program with malloc() and free() - what you should do instead is keep a free list... after allocating something, if you want to free it, don't actually free the memory to the Operating System, but put it in a free list - which you create as a linked list - which is a list of ponters that point to each other - you need  a structure like...

  struct FREE_LIST
  {
      void *p_data;
      struct FREE_LIST *p_next;
  };

You keep a global pointer to the begining of the list, then each entry in the list points to the next entry and the last entry has a NULL or 0 pointer value (use the word NULL though in your code).  And the p_data value of each entry is the pointer to the data that you are saving for later when you need it that you allocated with malloc in your program - just make sure that you keep separate lists for different size data elements.  

You have to allocate the FREE_LIST structures though  - as you put something on the free list if all your p_data elements are set (not NULL), you have to allocate another FREE_LIST structure and insert it into your linked list and use it  - set the p_data value to the value your trying to add - If after looping through - or testing and end of free list data global pointer - and you find a p_data that IS NULL, then just set the p_data element on that entry - make sure though if you are keeping an end of data pointer, that you increment it - and if it goes beyond the last structure, where p_next == NULL, then set it to NULL as well.

In a free list, the FREE_LIST structures that make up the linked list don't decrease in size (you don't free them either) - but if you keep putting things on the free list, it grows.  I hope you understand this - FREE LISTS ARE VERY IMPORTANT!!!

Just make sure that when you are exiting your program that you free everything in the free list - all p_data elements and all FREE_LIST structures that p_next point to.

That got long - I hope it helped.
0
Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.

Join & Write a Comment

Featured Post

Making Bulk Changes to Active Directory

Watch this video to see how easy it is to make mass changes to Active Directory from an external text file without using complicated scripts.

  • 4
  • 3
  • 3
  • +1
Tackle projects and never again get stuck behind a technical roadblock.
Join Now