Update LasagnaTest.java #2085

laurahannah · 2022-01-19T01:24:33Z

pull request

Corrected the last two tests

Reviewer Resources:

ericbalawejder · 2022-01-19T19:20:19Z

Based on the instructions, the original values seem correct. Can you elaborate on how you arrived at these new test values?

laurahannah · 2022-01-20T00:52:40Z

Based on the instructions, the original values seem correct. Can you elaborate on how you arrived at these new test values?

Sure.

The example in the README.
Time in oven is a fixed 40 minutes.
3 layers leads to 2 times 3 = 6 minutes preparation time.
Preparation is 40 minutes plus 6.
Since the item has already been in the oven 20 minutes,
The total time left is 40 + (2 times 3) - 20 = 26

@test
public void total_time_in_minutes_for_three_layers() {
assertThat(new Lasagna().totalTimeInMinutes(3, 20)).isEqualTo(26);
}
First test is 1 layer and already in the oven for 30 minutes
Total time left is 40 + (1 times 2) - 30 = 12
@test
public void total_time_in_minutes_for_one_layer() {
assertThat(new Lasagna().totalTimeInMinutes(1, 30)).isEqualTo(12);
}

Second test is
4 layers having been in the oven 8 minutes already
Total time left is 40 + (2 times 4) - 8 = 40
@test
public void total_time_in_minutes_for_multiple_layers() {
assertThat(new Lasagna().totalTimeInMinutes(4, 8)).isEqualTo(40);
}

AlbusPortucalis · 2022-01-20T10:08:35Z

I believe you got the idea of the totalTimeInMinutes() method wrong.

The objective of this test is to return the total time (in minutes) spent preparing the lasagna until now (The function should return how many minutes in total you've worked on cooking the lasagna).

The example in the README.
Time in oven is a fixed 40 minutes.
3 layers leads to 2 times 3 = 6 minutes preparation time.
Preparation is 40 minutes plus 6.
Since the item has already been in the oven 20 minutes,
The total time left is 40 + (2 times 3) - 20 = 26

Should be:
3 layers leads to 2 times 3 = 6 minutes preparation time.
Since the item has already been in the oven for 20 minutes,
The total time spent cooking is (2 times 3) + 20 = 26

The tests should be:

    @Test
    public void total_time_in_minutes_for_one_layer() {
        assertThat(new Lasagna().totalTimeInMinutes(1, 30)).isEqualTo(32);
    }

    @Test
    public void total_time_in_minutes_for_three_layers() {
        assertThat(new Lasagna().totalTimeInMinutes(3, 20)).isEqualTo(26);
    }

    @Test
    public void total_time_in_minutes_for_multiple_layers() {
        assertThat(new Lasagna().totalTimeInMinutes(4, 8)).isEqualTo(16);
    }
}

AlbusPortucalis · 2022-01-20T11:10:08Z

Closes exercism/exercism#6199

laurahannah · 2022-01-20T13:32:35Z

Closes exercism/exercism#6199

Have updated the repo to remove the confusing superfluous parameter and updating the description. This is a base exercise the blocks the track. I would think we would want it to be very concise. Unless the point is to challenge the learner to understand that there is a superfluous parameter on the method. Which I would think that would be a different type of exercise completely.

ErikSchierboom · 2022-01-20T13:53:23Z

exercises/concept/lasagna/src/main/java/Lasagna.java

+    public static int expectedMinutesInOven() {
+        return 40;
+    }

-    // TODO: define the 'remainingMinutesInOven()' method
+    public static int remainingMinutesInOven(int timeInOven) {
+        return expectedMinutesInOven() - timeInOven;
+    }

-    // TODO: define the 'preparationTimeInMinutes()' method
+    public static int preparationTimeInMinutes(int layers) {
+        return layers * 2;
+    }

-    // TODO: define the 'totalTimeInMinutes()' method
+    public static int totalTimeInMinutes(int amountLayers) {
+        return preparationTimeInMinutes(amountLayers) + expectedMinutesInOven();
+    }


The goal of this exercise is to have students learn how to define methods. Putting the actual implementation in them would defeat that purpose.

ErikSchierboom

@laurahannah I appreciate your desire to help improve our exercises, but this PR seems to go against what we try to achieve with this exercise (having students learn how to define methods and call them).

I'm sorry that the time you spent on this PR will be for naught. Might I suggest opening an issue first for potential improvements/changes? That way, we can first agree upon what changes we want to see. Thanks!

ErikSchierboom · 2022-01-20T13:55:15Z

exercises/concept/lasagna/src/test/java/utils/Lasagna.java

    }

-    public int totalTimeInMinutes(int amountLayers, int actualMinutes) {
+    public int totalTimeInMinutes(int amountLayers) {


This changes the spec from two parameters to one, whereas we want students to use two parameters.

I guess I am still not understanding. The goal of this exercise is to have the student use two parameters even though one is never used. I did open a defect, but it was closed as working as expected. But I don't believe that is actually the case since we were working on this one as a group and all of us had the same thought. And we are communicating and this is a good thing. This exercise is blocking our progress in the Java track. And we don't want to submit a solution that is bad practice, because then that looks bad on us if anybody reviews our solution. Please advise

@laurahannah Take a look at some other solutions: https://exercism.org/tracks/java/exercises/lasagna/solutions

For example:

public class Lasagna { private final int COOKING_TIME_IN_MINS = 40; // cooking time is 40 minutes private final int MINS_PREP_PER_LAYER = 2; // each layer takes 2 minutes to cook // Returns expected length of 40 minutes public int expectedMinutesInOven(){ return COOKING_TIME_IN_MINS; } // Returns minutes remaining in oven, based on expected minutes public int remainingMinutesInOven(int minutesPassed){ return this.expectedMinutesInOven() - minutesPassed; } // Returns number of minutes you have spent preparing the lasagna public int preparationTimeInMinutes(int numLayers){ return MINS_PREP_PER_LAYER * numLayers; } // Returns the total time you have been making the lasagna public int totalTimeInMinutes(int numLayers, int minutesPassed){ return this.preparationTimeInMinutes(numLayers) + minutesPassed; } }

You'll see both params are used when the exercise is solved correctly.

Can the instructions be made clearer somehow?

maybe explain what the second parameter for the totalTimeInMinutes method is. Also, I have a virtually identical solution to one that was accepted 5 months ago that does not pass

The instructions say:

Define the totalTimeInMinutes() method that takes two parameters: the first parameter is the number of layers you added to the lasagna, and the second parameter is the number of minutes the lasagna has been in the oven. The function should return how many minutes in total you've worked on cooking the lasagna, which is the sum of the preparation time in minutes, and the time in minutes the lasagna has spent in the oven at the moment.

How could we improve that to make it more clear?

I have to agree with the OP.
In my opinion, the method/function name should be totalElapsedTimeInMinutes.
When doing this exercise the first couple of times (in different languages) I was confused by this.
In my mind, the method would ask "how many minutes it should be in the oven" as apposed to "how many minutes it has been in the oven."
I was able to infer the correct behaviour from the tests, but the naming is still confusing

I agree 100%. Plus the example in the readme, when added to the unit tests, won't pass. Like, even if you are not talking about method names and such. Add the unit test from the readme, then you have this to solve:

totalTimeIntMintues(3, 20) = 26 totalTimeInMintues(4, 8) = 48``` When in doubt, I went with the README. So, please update the README, which was the first request that I had.

totalTimeIntMintues(3, 20) = 26 totalTimeInMintues(4, 8) = 48

As far as I can tell, the second example isn't in the readme or tests.
The tests have the following test

totalTimeInMinutes(4, 8) == 16

which makes total sense given the intended purpose of the method.

Also, the remake's example is

totalTimeIntMintues(3, 20) == 26

which also makes sense given the intended purpose of the method.

laurahannah · 2022-01-20T19:12:39Z

I have requested mentoring since it is completely unclear to me how to past this exercise and continue my learning track.

ErikSchierboom · 2022-01-25T07:43:56Z

I have requested mentoring since it is completely unclear to me how to past this exercise and continue my learning track.

I think this is the way to go forward. Good luck.

I'm closing this PR.

Update LasagnaTest.java

f6b0fc5

ErikSchierboom requested changes Jan 20, 2022

View reviewed changes

updating the description and the input parameters

d4d72d4

laurahannah force-pushed the main branch from 62a1ec3 to d4d72d4 Compare January 20, 2022 13:58

ErikSchierboom closed this Jan 25, 2022

ErikSchierboom mentioned this pull request Jan 27, 2022

Java track: lasagna exercise has two tests that need to be updated exercism/exercism#6199

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update LasagnaTest.java #2085

Update LasagnaTest.java #2085

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Update LasagnaTest.java #2085

Update LasagnaTest.java #2085

Uh oh!

Conversation

pull request

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants