2
\$\begingroup\$

I am learning C and doing challenges like those found on exercism.io and the various data structure & algorithm sites.

I have a sense that while this works and it makes sense to me that it can be improved and probably isn't how a professional C programmer would have solved it.

Interested if this would pass a code review in a professional setting and if not, why not.

Problem URL: Convert a phrase to its acronym

Convert a phrase to its acronym.

Techies love their TLA (Three Letter Acronyms)!

Help generate some jargon by writing a program that converts a long name like Portable Network Graphics to its acronym (PNG).

acronym.h

#ifndef ACRONYM_H
#define ACRONYM_H

char *abbreviate(const char *phrase);

#endif

#include "acronym.h"
#include <ctype.h>
#include <stdlib.h>
#include <string.h>

char *abbreviate(const char *phrase) {

  // Return NULL for NULL or empty phrase
  if (phrase == NULL || strlen(phrase) == 0) {
    return NULL;
  }

  size_t phrase_length = strlen(phrase) - 1;
  size_t acronym_index = 0;
  size_t phrase_index = 0;

  // The acronym will not be longer than the phrase length.
  char *acronym = malloc(phrase_length * sizeof(char));

  // The first letter of the phrase is the first initial in the acronym.
  acronym[acronym_index] = toupper(phrase[phrase_index]);

  acronym_index++;
  phrase_index++;

  while (phrase[phrase_index] != '\0') {
    // At the end of the string, add a null terminator.
    if (phrase_index == phrase_length) {
      acronym[acronym_index] = '\0';
      acronym_index++;
      phrase_index++;
      break;
    }

    char letter = phrase[phrase_index];
    char next_letter = phrase[phrase_index + 1];

    // If letter is not an apostrophe or alpha character, but the next one is,
    // found a word boundary and the next charcter is part of the acronym.
    if (letter != '\'' && !isalpha(letter) && isalpha(next_letter)) {
      acronym[acronym_index] = toupper(next_letter);

      acronym_index++;
      phrase_index++;
    }

    phrase_index++;
  }

  return acronym;
}

Github full solution including tests

\$\endgroup\$
3
  • 4
    \$\begingroup\$ Your code returns an unterminated string. You should add ‘\0’ at then end of acronym. We usually only review working code. This is what I got for United States of America : USOA═════════════²²²² \$\endgroup\$
    – jdt
    Oct 9 '21 at 18:04
  • 3
    \$\begingroup\$ Did this code pass the tests? \$\endgroup\$
    – Mast
    Oct 9 '21 at 18:07
  • \$\begingroup\$ Thanks for the comments, yes it passes all the tests. \$\endgroup\$
    – olepunchy
    Oct 9 '21 at 20:05
1
\$\begingroup\$

I'll show you what I would write first:

char *abbreviate(const char *phrase) {
  assert(phrase);

  char *acronym = malloc(strlen(phrase) / 2 + 1);
  if (!acronym) {
    return NULL;
  }

  char previous = ' ';
  size_t len = 0;

  for (size_t i = 0; phrase[i]; i++) {
    char current = phrase[i];
    if (previous != '\'' && !isalpha(previous) && isalpha(current)) {
      acronym[len++] = toupper(current);
    }
    previous = current;
  }

  acronym[len++] = '\0';
  return realloc(acronym, len);
}

First of all, I require the function to be called with a non-NULL pointer, as passing a NULL pointer would probably be a programming error on the side of the caller. I check for it using an assert() call, which can be compiled out in release builds.

Then there are various ways to allocate memory; you could start with a small buffer and grow it if necessary, or start with the maximum size the acronym could be, which is only half of the size of the original phrase. Don't forget to check that malloc() succeeded, and handle it returning NULL in some way.

Instead of comparing the current character with the next character, I compare the current one with the previous one. This is the main reason why my code is short; I don't have to check that we don't already are on the last character, and by carefully initializing current, the first alpha character in phrase will correctly be the first one in the output. Note that your code would happily copy the first character of phrase into the acronym, even if it shouldn't have.

Finally, after ensuring the acronym is properly terminated with a NUL-byte, I shrink the allocated memory to the minimum necessary for the acronym; this avoids wasting memory. (Technically, you should check the return value of realloc() since it might return NULL, but I'm abusing my years of programming experience here and just assume that never happens if you shrink instead of grow.)

Note that this version might return an empty string if there were no alpha characters in the input. You may or may not want this behavior. The upshot of this is that this function will always return a valid string, except in case memory allocation failed. The caller can still easily check if it is empty. If you want it to return NULL, I suggest doing that at the end like so:

if (len) {
  acronym[len++] = '\0';
  return realloc(acronym, len);
} else {
  free(acronym);
  return NULL;
}

I would add more corner cases to your test suite, like inputs that start with spaces and ones that only consist of non-alpha characters.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you, learned a lot from your reply and it all makes sense. Appreciate it. \$\endgroup\$
    – olepunchy
    Oct 9 '21 at 19:35
  • \$\begingroup\$ I remember, many years ago, that there was a problem if you tried to realloc or free memory allocated by strdup in MSVC debug build. I suppose it could also fail if your heap is corrupted. \$\endgroup\$
    – jdt
    Oct 9 '21 at 21:22
  • \$\begingroup\$ @JohanduToit That happened if you mixed debug and non-debug objects. I don't see how that is relevant here though, since strdup() is not used. \$\endgroup\$
    – G. Sliepen
    Oct 9 '21 at 22:16
0
\$\begingroup\$

I would have created something like the following which allows for better error handling and have the output buffer declared on the stack.

#include <ctype.h>
#include <stdlib.h>
#include <string.h>
#include <stdio.h>

enum AbbreviationErrors
{
    phraseIsNull = 1,
    bufferTooSmall = 2,
    noLettersFound = 3
};

int abbreviate(const char* phrase, char* buf, const size_t bufSize)
{
    if (phrase == NULL)
        return phraseIsNull;
    int inAlpha = 0;
    char prevChar = 0;
    char* dst = buf;
    for (const char* ptr = phrase; *ptr; ptr++)
    {
        if (isalpha(*ptr))
        {
            if (inAlpha == 0 && prevChar != '\'')
            {
                if (dst - buf >= bufSize)
                    return bufferTooSmall;
                *dst++ = toupper(*ptr);
                inAlpha = 1;
            }
        }
        else
        {
            inAlpha = 0;
        }
        prevChar = *ptr;
    }
    if (dst == buf)
        return noLettersFound;
    if (dst - buf >= bufSize)
        return bufferTooSmall;
    *dst = '\0';
    return 0;
}

int main()
{
    char test[] = "Hailey's Comet";
    static const size_t bufLen = 80;
    char buf[bufLen];
    int result = abbreviate(test, buf, bufLen);
    if (result == 0)
        printf("abbreviation = '%s'\n", buf);
}
\$\endgroup\$
2
  • \$\begingroup\$ Thanks. This fails the test for a string like "Hailey's Comet", but did learn from your reply so appreciate it. \$\endgroup\$
    – olepunchy
    Oct 9 '21 at 19:42
  • \$\begingroup\$ @olepunchy, thanks, I missed that but have updated my answer. \$\endgroup\$
    – jdt
    Oct 9 '21 at 20:08
0
\$\begingroup\$

Sorry, can't add a comment so will try to make it as an answer.

I have tested your code that is in Git. It returns U when there is USA passed. I understand you can't compare this kind of argument with anything but may it task requirements are not clear? Since as we know (Wikipedia too):

Phrases can consist of a single word or a complete sentence

Maybe in the case when there is a single word and all letters are capital leave it as is?

\$\endgroup\$
2
  • 1
    \$\begingroup\$ That output seems consistent - it's taken the first letter of the single word input, exactly as I would expect. Are you claiming that this is incorrect? \$\endgroup\$ Oct 12 '21 at 20:05
  • \$\begingroup\$ I am not asserting anything about the correctness of the interpretation of the task conditions. I just suggest a way when there is a single word and all letters are capital leave it as is. \$\endgroup\$
    – SGh
    Oct 15 '21 at 8:58

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.