Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New concept: Optional #2913

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

josealonso
Copy link
Contributor

Write explanation for the Optional concept, as well as the test file, the stub implementation, the reference solution and other required files. Some files must be modified. This is a draft.

This solves #2555


Reviewer Resources:

Track Policies

… the stub implementation, the reference solution and other required files. Some files must be modified. This is a draft.
@josealonso
Copy link
Contributor Author

josealonso commented Feb 10, 2025

I just committed a first draft. Don't worry about the linting errors, I will sort them out during this week.
I'd like to receive feedback about the reference solution and the test file, the bulk of the work is done.

  • Is there an exercise for the generics concept ? That is a prerequisite for this concept.
  • If that concept is not explained on the Java track, I can adapt it from other track.

@kahgoh @jagdish-15

@kahgoh
Copy link
Member

kahgoh commented Feb 11, 2025

Thanks @josealonso! I plan to have a look at the draft soon.

In regards to the generics concept, it is covered as generics type.

@josealonso
Copy link
Contributor Author

Thanks @josealonso! I plan to have a look at the draft soon.

In regards to the generics concept, it is covered as generics type.

Thanks for the link to the generic-types exercise!! Shame on me

@josealonso
Copy link
Contributor Author

Which icon should I choose?

@kahgoh
Copy link
Member

kahgoh commented Feb 12, 2025

Did you mean the icon for the exercise? They get added to exercism/icons repository, though that can be done after the exercise is added. We'll have to make one, but I think it will be the same icon as the original Tim From Marketing, but with a "2" in the top right corner. Compare Wizards and Warriors and Wizards and Warriors 2 for an example.



class EmployeeService {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I didn't quite get the purpose of these two methods (getAllTheEmployeesById and getEmployeeById) especially as we're not expecting the students to implement these and I don't think its clear when we expect them to call them.

Since the Employee have methods that return Optional objects, I'd suggest having the test pass Employee objects or a List of them instead.

Is it also worth having a task for where the student has to return an Optional as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback.
I agree, I did it like that because I had all the code in one single file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this has been resolved.

Copy link
Member

@kahgoh kahgoh Feb 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had another thought - the getEmployeeById could also be left up to the student to implement. We already have the foreach concept, so they could use that to find the employee and that'll give them the chance to practice making an Optional object (assuming they are looking through a List<Employee>) and there would be no need for streams in the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kahgoh, despite I spent many hours creating this exercise, I have to admit it's probably a bit complex for a learning exercise. This is what I propose:

  • Leaving this one for a future practice exercise.
  • Create a concept exercise for Streams first. And maybe another one for Suppliers or functional interfaces, based on my article or a similar one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave a longer answer tomorrow, but I just wanted to thank you for your support and clarify that my struggle is due to being my first exercise designed from scratch and also because I wasn't not very familiar with the Optional API.
I would cover more simple streams operations in a first Streams learning exercise, like map, filter and other simple operations.
@kahgoh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had another thought - the getEmployeeById could also be left up to the student to implement. We already have the foreach concept, so they could use that to find the employee and that'll give them the chance to practice making an Optional object (assuming they are looking through a List<Employee>) and there would be no need for streams in the class.

Are you saying I can implement the same methods without using streams ? I'm not sure that's doable. 🤔

Copy link
Member

@kahgoh kahgoh Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking the EmployService could take in a List<Employee> in the constructor, so then the students could be tasked with finding the Employee by id. I imagine the stub would like this:

class EmployeeService {
  public EmployeeService(List<Employee> employees) {
    throw new UnsupportedOperationException("Please implement the EmployeeService constructor");
  }

  public Optional<Employee> getEmployeeById(int id) {
     throw new UnsupportedOperationException("Please implement the EmployeeService.getEmployeeById() method");
  }
}

Students can still make a solution for this using a for / while loop that goes through the list until it finds the Employee. Here's one rough way:

for (Employee candidate: employeesList) {
  if (Objects.equals(candidate.getId(), Optional.of(id)) {
    return Optional.of(candidate);
  }
}

return Optional.empty();

Alternatively, I think passing the method a List<Employee> to the method would also work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking the EmployService could take in a List<Employee> in the constructor, so then the students could be tasked with finding the Employee by id. I imagine the stub would like this:

class EmployeeService {
  public EmployeeService(List<Employee> employees) {
    throw new UnsupportedOperationException("Please implement the EmployeeService constructor");
  }

  public Optional<Employee> getEmployeeById(int id) {
     throw new UnsupportedOperationException("Please implement the EmployeeService.getEmployeeById() method");
  }
}

Students can still make a solution for this using a for / while loop that goes through the list until it finds the Employee. Here's one rough way:

for (Employee candidate: employeesList) {
  if (Objects.equals(candidate.getId(), Optional.of(id)) {
    return Optional.of(candidate);
  }
}

return Optional.empty();

Alternatively, I think passing the method a List<Employee> to the method would also work.

Thanks @kahgoh. I'll try to change it this weekend. I was stuck, because I designed this exercise to try to mimic the access to a database. But you're right, I made it too complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your help, @kahgoh. Take a look when you have a chance. I think I did it the way you suggested. I'm sure it can be improved, but streams are not used in the reference solution.

@josealonso
Copy link
Contributor Author

Did you mean the icon for the exercise? They get added to exercism/icons repository, though that can be done after the exercise is added. We'll have to make one, but I think it will be the same icon as the original Tim From Marketing, but with a "2" in the top right corner. Compare Wizards and Warriors and Wizards and Warriors 2 for an example.

Who can make that icon? I'm sorry I don't know how to edit a simple icon :(

…ns have been added. The tests have been corrected and integrated in an IDE. What remains are the hints.md and the icon files, and the gradle files for this exercise.
@josealonso
Copy link
Contributor Author

I don't understand why the java-test-runner failed, @kahgoh. It probably has to do with some missing configuration files,

@kahgoh
Copy link
Member

kahgoh commented Feb 15, 2025

Who can make that icon? I'm sorry I don't know how to edit a simple icon :(

That's fine, we can do that later. For now, let's just work on the concept content.

@kahgoh
Copy link
Member

kahgoh commented Feb 15, 2025

I don't understand why the java-test-runner failed, @kahgoh. It probably has to do with some missing configuration files,

Its a bit hard to find, but this appears in the log:

tim-from-marketing-2: exemplar solution did not pass the tests

Are the tests passing locally for you?

@josealonso
Copy link
Contributor Author

Are the tests passing locally for you?

Yes.

@manumafe98 manumafe98 added the x:size/large Large amount of work label Feb 17, 2025
@manumafe98
Copy link
Contributor

Maybe this doc on configlet could be useful for you as well to check


WIP


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josealonso I suggest you to use this vscode extension for markdown linting, you will be able to solve them easily

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion!! I use Intellij, there might be a similar plugin for this IDE, I guess.

@josealonso
Copy link
Contributor Author

Maybe this doc on configlet could be useful for you as well to check

Thanks, I went through it already. I'll have to read it again.

@josealonso
Copy link
Contributor Author

@kahgoh, can you please go through these files and tell me if that's the right approach ? Thanks :)

Copy link
Member

@kahgoh kahgoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @josealonso, I have had a look through and I think the exercise is going in the right direction.

In a couple of the comments I mention an about.md. The about.md is shown after they complete the exercise and would be something that will need to eventually be added for the concept.

dependencies {
testImplementation platform("org.junit:junit-bom:5.10.0")
testImplementation "org.junit.jupiter:junit-jupiter"
testImplementation "org.assertj:assertj-core:3.25.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion is needed to fix a deprecated warning from Gradle (related to #2905).

Suggested change
testImplementation "org.assertj:assertj-core:3.25.1"
testImplementation "org.assertj:assertj-core:3.25.1"
testRuntimeOnly "org.junit.platform:junit-platform-launcher"

"name": "tim-from-marketing-2",
"uuid": "a6cfc286-8c62-4f5b-8e59-f6bfc4374092",
"concepts": [
"optional type"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add hyphen to be consistent with the existing concepts.

Suggested change
"optional type"
"optional-type"


## Introduction

The **Optional<T>** type was introduced in Java 8 as a way to indicate that a method will return an object of type T or an empty value. It is present in type signatures of many core Java methods.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of suggestions for this one:

  • I think it might be more easier to understand to describe in terms of trying to describe a method that needs to return "no value" instead of "empty" (I'm not sure if everyone would understand what empty means).
  • I'm not sure I would consider it to be "many" core Java methods (I can only think of the Stream API off the top of my head). I'd suggest removing it or toning it down.
Suggested change
The **Optional<T>** type was introduced in Java 8 as a way to indicate that a method will return an object of type T or an empty value. It is present in type signatures of many core Java methods.
The **Optional<T>** type was introduced in Java 8 as a way to indicate that a method _may_ return a value.
In other words, there is a chance the method returns "no value" at all.

}
}
```

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like something is missing here, as you've gone straight to how to simplify it. There are probably some basics that we should cover. For example, how do you make an optional? What does empty or present mean?

The goal of this exercise is to teach the student how to use the Optional API.
We will use the most common methods: `ifPresent`, `orElse`, `ifPresentOrElse`, `orElseThrow`.
The `isPresent` and `get` methods are not presented, since they do not provide any value over an ordinary null check.
Some methods of the Stream API are needed. This is a bit problematic, since they have not been explained in the current Java track.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this sentence is no longer needed.

Suggested change
Some methods of the Stream API are needed. This is a bit problematic, since they have not been explained in the current Java track.

@@ -0,0 +1,53 @@
import java.util.Objects;
import java.util.Optional;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're not expecting students to change this, I think it would be handy to add some comments here. For example:

/**
 * Holds information about an employee. There is  no need to change this file.
 */


@BeforeEach
void setup() {
initList();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this style of setting up the list (declare the list as field, call initList to populate the list and set up EmployeeService with it here) a little hard to follow because there is quite a bit of jumping around to figure out who the employees are. I'd suggest just setting up the list here:

Suggested change
initList();
listOfEmployees = List.of(
new Employee(1, "Tim", "Marketing"),
new Employee(2, "Joe", "Sales"),
new Employee(3, "Jane", "IT"),
new Employee(4, null, null)
);

## Introduction

The **Optional<T>** type was introduced in Java 8 as a way to indicate that a method will return an object of type T or an empty value. It is present in type signatures of many core Java methods.
Before Java 8, developers had to implement null checks:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep the introduction focused on Optional and how to use it, I think this may be moved to the about.md.

listOfEmployees.add(new Employee(1, "Tim", "Marketing"));
listOfEmployees.add(new Employee(2, "Joe", "Sales"));
listOfEmployees.add(new Employee(3, "Jane", "IT"));
listOfEmployees.add(new Employee(4, null, null));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are a number of cases missing, such as a null name and non-null department and non-null name and null department.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect not everyone knows what a "service" class is. I'd suggest using a term that is easier to understand by more people (e.g. EmployeeDatabase?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:size/large Large amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants