Solved

What's Happening Here? (Easy)

Posted on 2004-09-20
12
299 Views
Last Modified: 2010-04-15
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
Comment
Question by:SaMuEl
  • 4
  • 3
  • 3
  • +1
12 Comments
 
LVL 45

Expert Comment

by:Kdo
Comment Utility

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


Kent
0
 
LVL 16

Expert Comment

by:PaulCaswell
Comment Utility
>>    printf("Price %d",price);
You are printing 'price' as an int. Its actually a float.
Use:
    printf("Price %f",price);

Paul
0
 
LVL 16

Expert Comment

by:PaulCaswell
Comment Utility
I dont see why the artist is not printing! Does it still go wrong?

Paul
0
 
LVL 45

Accepted Solution

by:
Kdo earned 25 total points
Comment Utility
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
 
LVL 16

Assisted Solution

by:PaulCaswell
PaulCaswell earned 25 total points
Comment Utility
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
 
LVL 45

Expert Comment

by:Kdo
Comment Utility

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
Threat Intelligence Starter Resources

Integrating threat intelligence can be challenging, and not all companies are ready. These resources can help you build awareness and prepare for defense.

 
LVL 2

Author Comment

by:SaMuEl
Comment Utility
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
 
LVL 45

Expert Comment

by:Kdo
Comment Utility

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
 
LVL 2

Author Comment

by:SaMuEl
Comment Utility
Thanks a lot kent, you've really cleared that up for me :)
0
 

Expert Comment

by:danielvallas
Comment Utility
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
 
LVL 2

Author Comment

by:SaMuEl
Comment Utility
yep working on it...
0
 

Expert Comment

by:danielvallas
Comment Utility
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

Featured Post

Free Trending Threat Insights Every Day

Enhance your security with threat intelligence from the web. Get trending threat insights on hackers, exploits, and suspicious IP addresses delivered to your inbox with our free Cyber Daily.

Join & Write a Comment

Suggested Solutions

An Outlet in Cocoa is a persistent reference to a GUI control; it connects a property (a variable) to a control.  For example, it is common to create an Outlet for the text field GUI control and change the text that appears in this field via that Ou…
Windows programmers of the C/C++ variety, how many of you realise that since Window 9x Microsoft has been lying to you about what constitutes Unicode (http://en.wikipedia.org/wiki/Unicode)? They will have you believe that Unicode requires you to use…
The goal of this video is to provide viewers with basic examples to understand and use pointers in the C programming language.
The goal of this video is to provide viewers with basic examples to understand opening and writing to files in the C programming language.

762 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

10 Experts available now in Live!

Get 1:1 Help Now