Implement exercise 'say' #275

vlzware · 2018-02-24T08:04:10Z

No description provided.

wolf99 · 2018-02-25T23:00:57Z

exercises/say/src/example.c

+      "", "", "twenty", "thirty", "forty",
+      "fifty", "sixty", "seventy", "eighty", "ninety"
+   };
+   const int below_100_len[] = {


These arrays may be a good candidate for introducing a typedef'd struct that holds both the number and the word. What do you think?

It may even be a good opportunity for you to learn what X-macros are and what they are sometimes used for, if you haven't come across them yet. Not suggesting that X-macros should be introduced to the C track here though as they would likely be an exercise (or several) in themselves 😃

Also, the arrays could have file-scope, rather than being part of the function.

X macros info if interested:

The late, great, Dr. Dobbs

embedded.com part 1

embedded.con part 2

embedded.com part 3

It is indeed a lot of clutter like this :/
I never thought about struct on this one, which is obviously a better, more integrated choice.
Also never heard of the 'X-macros'. It looks interesting - thanks for the links! Always glad to learn new stuff.

For this exercise I decided to go the 'struct'-route.
As you said maybe would be nice to have exercises with X-macros. I personally will gladly experiment more with them.

I use X-macros a lot at work. They add some cognitive overhead where they are used, but if you use them at the right time they are incredibly powerful.

wolf99 · 2018-02-25T23:03:17Z

exercises/say/src/example.c

+   /* digits count */
+   int digits = floor(log10(input)) + 1;
+
+   /* take care of spaces/hyphens, thousends and termination */


typo: thousends -> thousands, note the a

wolf99 · 2018-02-25T23:07:10Z

exercises/say/makefile

+
+tests.out: test/test_say.c src/say.c src/say.h
+	@echo Compiling $@
+	@cc $(CFLAGS) src/say.c test/vendor/unity.c test/test_say.c -o tests.out -lm


-lm should probably be added by the variable LDFLAGS (doesn't currently exist). See https://stackoverflow.com/questions/13249610/how-to-use-ldflags-in-makefile for an example of LDFLAGS

- update config.json where 'unlocked_by' was missing; - use 'LDFLAGS' in the 'Makefile'; - switch to structs for the names/sizes; - fix typo in example.c; - remove redundant 'TEST_ASSERT_NULL' in the tests.

vlzware · 2018-02-26T10:40:50Z

Thanks @wolf99 for the review and the suggestions.

wolf99 · 2018-02-26T12:57:55Z

exercises/say/src/example.h

@@ -1,6 +1,12 @@
 #ifndef SAY_H
 #define SAY_H
 #include <stdint.h>
+#include <stddef.h>
+
+typedef struct {


I think the typedef can go inside example.c, right? No need for it to be exposed to the caller if map is only used internally.

Also, I think we've been following the convention of giving type names a suffix of _t, e.g. map_t.
This is common in non-POSIX environments to make it clear what is a type name and what is a variable name (because a lot of editors don't do syntax highlighting for custom types).

Completely agree.

FYI, the reason I mention non-POSIX environment is that names ending with _t are reserved by POISX so code that uses POSIX will generally not follow this. Everywhere else it is a common idiom though.

wolf99 · 2018-02-26T12:58:49Z

exercises/say/test/test_say.c

@@ -15,7 +15,6 @@ void test_zero(void)
   char *ans = NULL;
   int res = say(0, &ans);
   TEST_ASSERT_EQUAL_INT(0, res);
-   TEST_ASSERT_NOT_NULL(ans);


Why remove the tests for NULL?

TEST_ASSERT_EQUAL_STRING checks already for NULL (see unity.c:1014).

This was first brought to my attention by @patricksjackson in this PR

- rename the typedef to 'map_t'; - move the typedef to internal declaration.

ryanplusplus · 2018-02-27T00:16:31Z

exercises/say/src/example.c

+         if (tmp / 100) {
+            int i = tmp / 100;
+            strcpy(head, below_10[i].name);
+            head += below_10[i].size;


Instead of .size, how about using strlen? This will be less fragile.

You mean renaming 'size' to 'strlen' or really using 'strlen' to calculate the size?
My point of using the string-length pairs was to spare the size calculation iterations over predefined const char*'s. I know there are small number of strings, but it hurts somehow to calculate their length over and over again. Also they are not supposed to be changed, as long as the names in English for numbers remain the same :)

I was suggesting that you use strlen to calculate on-demand instead of counting characters by hand. But I agree with your reasoning for using a length known at compile time.

ryanplusplus · 2018-02-27T00:17:07Z

exercises/say/src/example.c

+
+   char *head = *ans;
+   while (input) {
+


Was this line left intentionally blank? I have the same question about line 72.

Well that was me experimenting with formatting in my editor.

ryanplusplus · 2018-02-27T00:18:26Z

exercises/say/src/example.c

+
+   /* 'zero' is a corner case */
+   if (input == 0) {
+      *ans = malloc(5);


How about *ans = "zero"; instead?

I think this will require you to change to const char **ans

How can I then build the result if it is const?

It is a pointer to const data and the compiler will replace a string literal with a const void * that points to the constant string data.

I think I understand now what you're saying: the client needs to be able to free this memory so we can't declare it as const. I agree with you.

Good point.
My question was how to use strcpy when the destination string is a const? Build the result in a modifiable char and then cast the return to const char?

Well, in this case I was suggesting that you not use strcpy but instead just set *ans to a string literal. This would work except that it would not be possible to free the string.

Got it - thanks for the clarification!

ryanplusplus · 2018-02-27T00:22:01Z

exercises/say/src/example.c

+};
+
+/* below_20's are like two times below_10's so this will work */
+static const int widest = 8;


"seventeen" has length 9, so should this be 9 instead?

17 is in the order of 10^1, meaning it occupies two digits. But 17 also gets represented by a single word. So it is shorter or equal than everything that is not '-teen' on the same order of magnitude. One of the longest 10^1 order digit is for example 77 which makes together length of 12 ('seventy' + 'seven'), occupying 2*widest = 16 bytes, leaving 4 free.
The value of 8 is actually the length of 'thousand' which would be the longest, non-complex word.

ryanplusplus · 2018-02-27T00:23:13Z

exercises/say/src/example.c

+   int digits = floor(log10(input)) + 1;
+
+   /* take care of spaces/hyphens, thousands and termination */
+   *ans = malloc((widest + 1) * (digits + (digits + 1) / 3) + 1);


I'm a bit lost on what some of these terms mean. Could you extract some enumerations, #defines, or constants to make it clear what these mean?

I'll try to improve this.

ryanplusplus · 2018-02-27T00:26:46Z

exercises/say/src/example.c

+         } else if (tmp < 20) {
+            tmp -= 10;
+            strcpy(head, below_20[tmp].name);
+            head += below_20[tmp].size;


These two lines (or something very similar) are used quite often. How about extracting a helper function to reduce the duplication and make the code easier to read?

Of course. A very good suggestion.

ryanplusplus · 2018-02-27T00:30:12Z

Looks pretty good. This is a tough exercise. I think the most important improvements that can be made are ones that will make this a bit easier to read and understand since it is a dense algorithm.

vlzware · 2018-02-27T05:08:38Z

Thanks @ryanplusplus for the code review.

ryanplusplus · 2018-02-28T00:44:22Z

👍

Implement exercise 'say'

73c9d60

wolf99 reviewed Feb 25, 2018

View reviewed changes

code improvements:

4dd9617

- update config.json where 'unlocked_by' was missing; - use 'LDFLAGS' in the 'Makefile'; - switch to structs for the names/sizes; - fix typo in example.c; - remove redundant 'TEST_ASSERT_NULL' in the tests.

wolf99 reviewed Feb 26, 2018

View reviewed changes

vlzware and others added 2 commits February 26, 2018 15:47

code improvements:

ccbfd8d

- rename the typedef to 'map_t'; - move the typedef to internal declaration.

Merge branch 'master' into say

7348a60

ryanplusplus reviewed Feb 27, 2018

View reviewed changes

improve readability

bcec9d3

wolf99 merged commit 14e0cba into exercism:master Mar 1, 2018

vlzware deleted the say branch March 2, 2018 16:25

Implement exercise 'say' #275

Implement exercise 'say' #275

Conversation

vlzware commented Feb 24, 2018

wolf99 Feb 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

X macros info if interested:

vlzware Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlzware commented Feb 26, 2018

wolf99 Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wolf99 Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanplusplus commented Feb 27, 2018

vlzware commented Feb 27, 2018

ryanplusplus commented Feb 28, 2018

wolf99 Feb 25, 2018 •

edited

Loading

vlzware Feb 26, 2018 •

edited

Loading

wolf99 Feb 26, 2018 •

edited

Loading

wolf99 Feb 26, 2018 •

edited

Loading