Space age Exercice added #122

Core-entin · 2017-02-17T14:15:53Z

I think nothing is missing.
Sorry about the multiple commits :(

ryanplusplus · 2017-02-18T00:47:47Z

You're failing CI but it looks like an issue either with Travis using a PPA that is either no longer supported or that is currently down. We can re-trigger the CI build later to see if it is resolved.

ryanplusplus · 2017-02-18T00:52:27Z

exercises/space-age/src/example.c

@@ -0,0 +1,41 @@
+#include "example.h"
+
+float convert_Earth_Age(const long input)


How about convert_earth_age? This will be consistent with the rest of the exercises.

ryanplusplus · 2017-02-18T00:53:23Z

exercises/space-age/src/example.h

+#define JUPITER_YEAR 11.862615
+#define SATURN_YEAR 29.447498
+#define URANUS_YEAR 84.016846
+#define NEPTUNE_YEAR 164.79132


Any reason why these are in the header file? It seems like these should be private to the implementation and thus in the .c file.

ryanplusplus · 2017-02-18T00:56:43Z

exercises/space-age/src/example.h

@@ -0,0 +1,25 @@
+#ifndef EXAMPLE_H
+#define EXAMPLE_H


Examples for our other exercises use the actual module name for the include guard. Can you change this to SPACE_AGE_H?

ryanplusplus · 2017-02-18T00:59:56Z

exercises/space-age/test/test_space-age.c

+
+void test_convert_Mercury_Year(void)
+{
+   TEST_ASSERT_EQUAL(0.2408467, convert_Mercury_Age(SECONDS_ON_EARTH_YEAR));


Unity includes an assertion meant for testing float equality: TEST_ASSERT_EQUAL_FLOAT that you should use to avoid the many complexities of comparing floating point numbers. You can read more about why this is important here: https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/.

ryanplusplus · 2017-02-18T01:02:33Z

Looks good overall, just a few nitpicks

ryanplusplus · 2017-02-18T19:12:58Z

The CI build is now running correctly but the example fails the tests.

Core-entin · 2017-02-18T23:28:36Z

thank you for your time, this is the kind of feedback that makes me want to persevere 👍 .
I think everything is ok now

ryanplusplus · 2017-02-19T00:30:50Z

exercises/space-age/makefile

+
+tests.out: test/test_space-age.c src/space-age.c src/space-age.h
+	@echo Compiling $@
+	@cc $(CFLAGS) src/space-age.c test/vendor/unity.c test/test_space-age.c -o tests.out


Looks like this expects the tests to be called test_space-age.c but the file was renamed to test_space_age.c. This is causing the CI failure.

Woops, my bad..
It should be ok now !

ryanplusplus · 2017-02-19T00:32:18Z

No problem, thanks for the quick follow up 😄

ryanplusplus · 2017-02-19T13:12:11Z

exercises/space-age/test/test_space_age.c

+
+void test_convert_neptune_year(void)
+{
+   TEST_ASSERT_EQUAL_FLOAT(164.79132, convert_neptune_age(SECONDS_ON_EARTH_YEAR));


Now your tests are failing because you're using SECONDS_ON_EARTH_YEAR in here but it is private to the implementation. Instead of using this, make sure you're using the input values from the standard test data: https://github.com/exercism/x-common/blob/master/exercises/space-age/canonical-data.json.

Core-entin · 2017-02-19T18:10:33Z

I tried another method, but I don't understand why the tests are failing in the first case..

ryanplusplus · 2017-02-19T18:54:22Z

Have you run it locally to see what values you're getting? It could be that there's simply an issue with your sample implementation, or it could be that your answer is close but not within the delta that the float comparison uses. If you're just slightly off, there are other assertions that allow you to specify the delta that you could use to open up the range of accepted values.

wolf99 · 2017-02-20T09:21:31Z

@Guntau If the issue is as ryanplusplus descrivbes then the assertion you need to read up on is TEST_ASSERT_FLOAT_WITHIN(delta, exp, actual) from the unity assertions reference guides, here: https://github.com/ThrowTheSwitch/Unity/tree/master/docs

Core-entin · 2017-02-20T11:01:49Z

I took a bigger delta for some tests, it seems to work. I also verified the values by hand, and they correspond with the canonical data

ryanplusplus · 2017-02-20T13:15:25Z

This looks good to me

ghost · 2017-02-20T14:16:50Z

`
#define EARTH 0
#define MERCURY 0.2408467
...

static float earth_year = 0.00000003168808781;

float space_age(float seconds, float planet)
{
if (planet == 0) {
return seconds * earth_year;
}
return (seconds * earth_year) / planet;
}
`
then you can just use ,
space_age(1000000000 , EARTH);
space_age(245463534 , MERCURY);
....

just a comment , for making the lines of code small and elegant looking , but your code Looks good to me

Core-entin · 2017-02-20T16:07:37Z

Yeah i thought about doing it that way, but the description of the exercise says that the only value given is the time in seconds. However I agree that your version is more elegant !

wolf99 · 2017-02-21T09:08:38Z

For simpler calculation at run-time you could instead do:

#define FOOPLANET_YEAR_IN_SECONDS (FOOPLANET_YEAR * SECONDS_ON_EARTH_YEAR)

float convert_fooplanet_age(const long input)
{
    return input / FOOPLANET_YEAR_IN_SECONDS; // No need to call convert_earth_age()
}

But this is only a minor detail.

arcuru · 2017-02-25T21:30:52Z

This looks good 👍 There is one future improvement I want to float out there though...

This problem has always annoyed me that you have to write a bunch of identical functions (maybe that's the point?). But I've always thought this could very easily be improved by just using an enum in the function argument for the planet conversion. So this would reduce down to a single function with the sig:

float convert_planet_age(Planet_t planet, uint64_t input)

That would make it much less annoying to write, and barely changes how it's used.

(I should've weighed in earlier, but real life got in the way :\ )

Core-entin · 2017-02-26T10:38:08Z

DeathSec proposed something like this too. I'll change it then, i think i just misunderstand the problem's description. thanks for the feedback

Bernard Corentin added 4 commits February 17, 2017 14:56

Added space age Exercice

0772317

Added space age to .json

4170b9e

removing useless String.h

1c882a1

added missing tests

62e1e9f

Removed tabs from config.json

6a839a2

ryanplusplus reviewed Feb 18, 2017

View reviewed changes

tests correction + changed header

b226907

ryanplusplus reviewed Feb 19, 2017

View reviewed changes

Makefile correction

2d1c1f9

ryanplusplus reviewed Feb 19, 2017

View reviewed changes

Bernard Corentin added 3 commits February 19, 2017 18:23

tests correction (no more dependencies)

2c01ba8

changed Test method

3c10145

undo on the tests method

f2bf432

tests correction

54d8d61

correcting indentation

6a5e285

correction for simpler calculation at run-time

4177d88

ryanplusplus merged commit d668820 into exercism:master Feb 25, 2017

Core-entin deleted the working branch February 26, 2017 10:35

Core-entin mentioned this pull request Mar 3, 2017

Improved code for space age #125

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Space age Exercice added #122

Space age Exercice added #122

Core-entin commented Feb 17, 2017

ryanplusplus commented Feb 18, 2017

ryanplusplus Feb 18, 2017

ryanplusplus Feb 18, 2017

ryanplusplus Feb 18, 2017

ryanplusplus Feb 18, 2017 •

edited

Loading

ryanplusplus commented Feb 18, 2017

ryanplusplus commented Feb 18, 2017

Core-entin commented Feb 18, 2017

ryanplusplus Feb 19, 2017

Core-entin Feb 19, 2017

ryanplusplus commented Feb 19, 2017

ryanplusplus Feb 19, 2017

Core-entin commented Feb 19, 2017

ryanplusplus commented Feb 19, 2017

wolf99 commented Feb 20, 2017

Core-entin commented Feb 20, 2017

ryanplusplus commented Feb 20, 2017

ghost commented Feb 20, 2017 •

edited by ghost

Loading

Core-entin commented Feb 20, 2017 •

edited

Loading

wolf99 commented Feb 21, 2017

arcuru commented Feb 25, 2017

Core-entin commented Feb 26, 2017

		@@ -0,0 +1,41 @@
		#include "example.h"

		float convert_Earth_Age(const long input)

Space age Exercice added #122

Space age Exercice added #122

Conversation

Core-entin commented Feb 17, 2017

ryanplusplus commented Feb 18, 2017

ryanplusplus Feb 18, 2017

Choose a reason for hiding this comment

ryanplusplus Feb 18, 2017

Choose a reason for hiding this comment

ryanplusplus Feb 18, 2017

Choose a reason for hiding this comment

ryanplusplus Feb 18, 2017 • edited Loading

Choose a reason for hiding this comment

ryanplusplus commented Feb 18, 2017

ryanplusplus commented Feb 18, 2017

Core-entin commented Feb 18, 2017

ryanplusplus Feb 19, 2017

Choose a reason for hiding this comment

Core-entin Feb 19, 2017

Choose a reason for hiding this comment

ryanplusplus commented Feb 19, 2017

ryanplusplus Feb 19, 2017

Choose a reason for hiding this comment

Core-entin commented Feb 19, 2017

ryanplusplus commented Feb 19, 2017

wolf99 commented Feb 20, 2017

Core-entin commented Feb 20, 2017

ryanplusplus commented Feb 20, 2017

ghost commented Feb 20, 2017 • edited by ghost Loading

Core-entin commented Feb 20, 2017 • edited Loading

wolf99 commented Feb 21, 2017

arcuru commented Feb 25, 2017

Core-entin commented Feb 26, 2017

ryanplusplus Feb 18, 2017 •

edited

Loading

ghost commented Feb 20, 2017 •

edited by ghost

Loading

Core-entin commented Feb 20, 2017 •

edited

Loading