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

Feature(CtPackage): Implement null object for CtPackage #3705

Open
MartinWitt opened this issue Nov 19, 2020 · 9 comments
Open

Feature(CtPackage): Implement null object for CtPackage #3705

MartinWitt opened this issue Nov 19, 2020 · 9 comments

Comments

@MartinWitt
Copy link
Collaborator

Problem

Sometimes code elements have no package and getPackage return nulls. This introduces null checks and surprises for new developers. 84c84e1 as seen here, package can be null.

Solution

A null object[0],[1] could improve code quality. A comparable implementation is NoSourcePosition. NoSourcePosition removed all null checks for positions and improves the code quality.

Your task

Design a null object and refactor old code. Find a solution for getSimpleName() as the empty String is reserved for default package.
The PR should include:

  • Design a null object for CtPackage
  • Improve already existing test code and remove unnecessary null checks

It's a good starter issue, feel free to give it a try and ping if you need help for some design decisions.

Literatur/Links

[0] https://en.wikipedia.org/wiki/Null_object_pattern
[1] https://www.baeldung.com/java-null-object-pattern

@monperrus
Copy link
Collaborator

Excellent idea.

@dhruvil-shah
Copy link

Hello sir,
I'm new to community and wanted to contribute as a part of Gsoc'21.
For the above issue we can use if(pkg != null && !pkg.isUnnamedPackage()) for checking null as mentioned in one of the code.
For the getSimpleName() issue with defult one we can use if(pkg!=null && pkg.isUnnamedPackage()) then just set it's corresponding name with setSimpleName().

I'm not sure about the above approach but if you could help me out with this.
Thanks and Regards
Dhruvil Shah

@tenax66
Copy link
Contributor

tenax66 commented Feb 18, 2023

I am interested in working on this issue. Can someone please tell me a little more about the scope of this task?
I suppose that it would require a large refactoring to apply this null object to the entire spoon, but for now, is it sufficient for the pull request to contain: a null object and associated test cases?

@MartinWitt
Copy link
Collaborator Author

is it sufficient for the pull request to contain: a null object and associated test cases?

This would be a perfect starting point.

Can someone please tell me a little more about the scope of this task?

  1. Create a Nullobject + testcases
  2. Create a check for locations we don't use the nullobject.
  3. Fix these locations.

All three tasks can be done in multiple pull requests .

@tenax66
Copy link
Contributor

tenax66 commented Feb 19, 2023

Thank you @MartinWitt !
I'll try to work on this issue.

@tenax66
Copy link
Contributor

tenax66 commented Mar 2, 2023

I am currently working on implementing this null object. I need some help with the following topics:

  1. Is this object a shadow element?
    To create this object, I have implemented CtPackage and now need to implement isShadow() method.
    Here's the problem: can this object be classified as a shadow element?
    For now, I'm considering that the method should always return true or throw an exception. Any opinions?

  2. What is the most suitable way to test this object?
    In testing this object, I think it is inefficient to test methods one by one since each method only returns a "trivial" value.
    So, what is the appropriate testing way here?
    This website mentions testing for null object and suggests testing whether it has the same methods as those of the original class, though I'm not sure if this is a good approach or not.
    https://thoughtbot.com/blog/testing-null-objects

@MartinWitt
Copy link
Collaborator Author

To create this object, I have implemented CtPackage and now need to implement isShadow() method.

Just return false because technical it is not a shadow object.

What is the most suitable way to test this object?

Create, for example, a class(Factory#createCtClass) and set the package to the null object.
Then test if the class is behaving the same as before.

@tenax66
Copy link
Contributor

tenax66 commented Mar 8, 2023

Thank you for your advice!

Then test if the class is behaving the same as before

I'd like to know what is meant by 'behaving the same' here.
I think we can't expect the same results as before for package-related behaviors (since null objects shouldn't have specific behaviors).
If so, what should I focus on for example?

@tenax66
Copy link
Contributor

tenax66 commented Mar 18, 2023

I made a WIP pull request for this issue.
Test cases are not yet well written, and I would like to have suggestions on what to test, as I previously mentioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants