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

Simple treasure hunter

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

Problem

I have made a program that reads 3 numbers:

  • number of treasure chests



  • treasure hunter's position



  • number of moving instructions



It then reads a string (which symbolizes the treasure chests), being each character either T (chest has a treasure) or V (chest is empty). After that, it reads the moving instructions in the format %c %d, being the first character (direction) either E(left) or D(right) and then the number of steps.

When the treasure hunter moves, he loots the treasures which he stands in, and in the end the program tells how many he got.

The requirements are the following:


1 ≤ number of treasure chests ≤ 100 000

1 ≤ treasure hunter's position ≤ number of treasure chests

1 ≤ number of moving instructions ≤ 100 000

the number of steps is always higher than 0

the treasure hunter never goes beyond the treasure chests

My working code which I wish to improve in every way possible is the following:

#include 
#include 

int main() {
  char e;
  int a, b, c, f, g = 0, i, j;
  scanf("%d %d %d", &a, &b, &c);
  char *d = malloc(a * sizeof(char));
  scanf("%s", d);
  for(i = 0; i < c; i++) {
    scanf(" %c %d", &e, &f);
    for(j = 0; j <= f; j++) {
      if(d[b - 1] != 'V') {
        d[b - 1] = 'X';
      }
      if(e == 'E') {
        b++;
      } else {
        b--;
      }
    }
  }
  for(i = 0; i < a; i++) {
    if(d[i] == 'X') {
      g++;
    }
  }
  printf("%d\n", g);
  return 0;
}


Example of input and output:

I:

5 3 3
TVTTT
D 1
E 2
D 3


O:

3

Solution

Variable names

Variables such as a, b, c, d are meaningless. Use names with meaning such as position, numChests, chestValues, etc. Sure, this is just a small program with 8 total variables, but you should get in the habit of producing readable code and not just correct code.
Bug

You don't allocate enough space for your string here:

char *d = malloc(a * sizeof(char));
scanf("%s", d);


Suppose a had value 5, as in the example. The string "TVTTT" has 5 characters plus a terminating null character, for a total of 6 characters. So you need to allocate an extra byte for the null character when you call malloc().

Also, it is unsafe to just call scanf() because if the input string happened to be wrong and contain more than 5 characters, you would overflow your buffer. To be safe, you can use fgets(), which limits the number of characters it writes to your buffer:

char *chestValues = malloc(numChests+1);
fgets(chestValues, numChests+1, stdin);


Note also that sizeof(char) is always equal to 1, so you can omit that.
No need for second loop

You have a second loop to count the number of X in your chest array. But you could have counted the number of chests found that were T in your first loop and then you wouldn't need the second loop at all.
Sample rewrite

Here is a rewrite of your program which fixes all of the above:

#include 
#include 

int main(void)
{
    int   numChests   = 0;
    int   position    = 0;
    int   numMoves    = 0;
    int   numFound    = 0;
    char *chestValues = NULL;

    scanf("%d %d %d\n", &numChests, &position, &numMoves);
    chestValues = malloc(numChests+1);
    fgets(chestValues, numChests+1, stdin);

    for (int i = 0; i < numMoves; i++) {
        char direction = 0;
        int  distance  = 0;
        scanf(" %c %d", &direction, &distance);
        for (int j = 0; j <= distance; j++) {
            if (chestValues[position - 1] == 'T') {
                chestValues[position - 1] = 'X';
                numFound++;
            }
            position += (direction == 'E' ? 1 : -1);
        }
    }
    printf("%d\n", numFound);
    return 0;
}

Code Snippets

char *d = malloc(a * sizeof(char));
scanf("%s", d);
char *chestValues = malloc(numChests+1);
fgets(chestValues, numChests+1, stdin);
#include <stdio.h>
#include <stdlib.h>

int main(void)
{
    int   numChests   = 0;
    int   position    = 0;
    int   numMoves    = 0;
    int   numFound    = 0;
    char *chestValues = NULL;

    scanf("%d %d %d\n", &numChests, &position, &numMoves);
    chestValues = malloc(numChests+1);
    fgets(chestValues, numChests+1, stdin);

    for (int i = 0; i < numMoves; i++) {
        char direction = 0;
        int  distance  = 0;
        scanf(" %c %d", &direction, &distance);
        for (int j = 0; j <= distance; j++) {
            if (chestValues[position - 1] == 'T') {
                chestValues[position - 1] = 'X';
                numFound++;
            }
            position += (direction == 'E' ? 1 : -1);
        }
    }
    printf("%d\n", numFound);
    return 0;
}

Context

StackExchange Code Review Q#159372, answer score: 3

Revisions (0)

No revisions yet.