HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

A-Eye: Analysing Eye tracking Data

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
trackinganalysingdataeye

Problem

I'd appreciate your advice.

data file: https://drive.google.com/open?id=0B_UEM0TEmA_yeHJBZ0dzbV9Ca2c

```
/*
CNG213 Data Structures / Programming Assignment 1 - Sinan ULUSOY

"A-Eye: Analysing Eye tracking Data"

l1 is used for main list record
l2 is used for sub list record
*/

#include
#include
#include

struct webNamesNode
{
char StimuliName[50];
struct webNamesNode *next;
struct detailsRecord *dummyNodeOFdetails;
};

struct detailsNode
{
struct detailsNode *next;
int FixationIndex;
int Timestamp;
int FixationDuration;
int FixationPointX;
int FixationPointY;
};

struct webNamesRecord
{
struct webNamesNode *head;
struct webNamesNode *tail;
int size;
};

struct detailsRecord
{
struct detailsNode *head;
struct detailsNode *tail;
int size;
};

//create functions////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
struct webNamesRecord *makeMainList()
{
struct webNamesRecord *l1;

l1 = (struct webNamesRecord *) malloc(sizeof(struct webNamesRecord));
if (l1 == NULL)
printf("Out of memory!\n");

l1->head = (struct webNamesNode*) malloc(sizeof(struct webNamesNode));
if (l1->head == NULL)
printf("Out of memory!\n");

l1->head->next = NULL;
l1->head->dummyNodeOFdetails = NULL;
l1->tail = l1->head;
l1->size = 0;

return l1;
}

struct detailsRecord *makeSubList()
{
struct detailsRecord *l2;

l2 = (struct detailsRecord *) malloc(sizeof(struct detailsRecord));
if(l2 == NULL)
printf("Out of memory!\n");

l2->head = (struct detailsNode *) malloc(sizeof(struct detai

Solution

Nice question, it deserved more attention then it got when it was posted. It might have gotten more attention if you had only posted the contents of page 2 and page 3 (the program requirements).

It's not clear to that the modular programming requirement was met, since that might have indicated that the program should have been broken up into multiple C and header files and been built as a project in visual studio, eclipse or Xcode. The struct declarations at least could have been in header files. There could have been one C file for each of the structs which exclusively manipulated that particular struct.

It might have been easier to implement and debug the program if each of the list types (webNamesNode, webNamesRecord and detailsRecord) had common list functions such as insert(node, list), add(node, list) and delete(node, list).

Possible Bugs

There is a possible bug in load_data_points(), l2 may be used before it is defined in the following code:

//the most important part!!!//////////////////////////////////////////////////////////////////////////
        //compares web names to make node for main list and list record for sublist
        if( strcmp(StimuliName, previousStimuliName) != 0)
        {
            makeNodeForMainList(l1, StimuliName);
            l2 = makeSubList();
        }

        //starts to put values into sublist
        makeNodeForSubList(l2, FixationIndex, Timestamp, FixationDuration, FixationPointX, FixationPointY);


As a side note, the comment //the most important part! doesn't explain why it is the most important part.

A second possible future bug is that in the following code pointers are not initialized to NULL.

struct webNamesRecord *l1;
    struct detailsRecord *l2;


If either of these pointers is tested for != NULL they may fail the test.

Portability

Use Common Library Routines Whenever Possible

The code is not portable because it uses the function char strlwr(char str). strlwr() is only available some operating systems and the code may not link in some cases. See the first answer to this Stack Overflow question.

Prefer powers of Two When Creating String Sizes

There are many places in the code where the a character array is defined as

char StimuliName[50];
char previousStimuliName[50] = "web";


It might be better to size character arrays and strings on based word size, not doing so can sometimes result in memory alignment errors. At minimum use some power of 2 as a character array / string size. One possible way to do this might be:

#define CHARS_IN_WORD sizeof(int) // gives the number of characters in an int which is the basic C size.
#define COMMON_STRING_LENGTH        80 * CHARS_IN_WORD


or alternately:

#define COMMON_STRING_LENGTH        80 * sizeof(int)

char characterArray[COMMON_STRING_LENGTH];


Please note that using a symbolic constant such as COMMON_STRING_LENGTH makes it much easier in the future if the size of an array needs to change. This allows the code to be changed in only one place to change all of the arrays and change any for loops that may depend on the size. The words used might indicate what the number means as well which can help in debugging or if someone else needs to modify the code.

Numeric constants in code are sometimes referred to as Magic Numbers.

Provide Enough Memory for a Full File Specification in a File Name or a Full Web Address

In the function main() there is the following code:

char fileName[20], pageName[50];


There are 2 problems with this code, the first being that numeric constants rather than symbolic constants are used which may make it difficult to to resize these variables, the second and possibly more major problem is that these simply aren't large enough to hold the possible values of a full file specification or a full web page address.

A full file specification includes the full path from the root directory to the file, including the local file name. A windows file specification might be C:\user\documents\FILENAME.EXT. A Unix/Linux file specification might be /Users/USERNAME/Documents/a-eye-analysing-eye-tracking-data/P1.txt. The file to be opened may not be in the local directory and therefore enough space for the full path should be allowed. The size of fileName could quite easily cause the program to fail to load the file.

The same can also be true of a web page address, the page address is not guaranteed to be at the top level of the domain and generally the http:// portion should be attached as well. As an example, the URL of this
question which is a web page is:

https://codereview.stackexchange.com/questions/146870/a-eye-analysing-eye-tracking-data


File systems differ on different operating systems. A symbolic constant such as PATH_MAX, MAXPATHLEN, or FILENAME_MAX can generally be found in one of the common header files (stdio.h for example) that defines
the maximum file name size allowed. This migh

Code Snippets

//the most important part!!!//////////////////////////////////////////////////////////////////////////
        //compares web names to make node for main list and list record for sublist
        if( strcmp(StimuliName, previousStimuliName) != 0)
        {
            makeNodeForMainList(l1, StimuliName);
            l2 = makeSubList();
        }

        //starts to put values into sublist
        makeNodeForSubList(l2, FixationIndex, Timestamp, FixationDuration, FixationPointX, FixationPointY);
struct webNamesRecord *l1;
    struct detailsRecord *l2;
char StimuliName[50];
char previousStimuliName[50] = "web";
#define CHARS_IN_WORD sizeof(int) // gives the number of characters in an int which is the basic C size.
#define COMMON_STRING_LENGTH        80 * CHARS_IN_WORD
#define COMMON_STRING_LENGTH        80 * sizeof(int)

char characterArray[COMMON_STRING_LENGTH];

Context

StackExchange Code Review Q#146870, answer score: 6

Revisions (0)

No revisions yet.