Test improvement: removed Magic Number test smell#364
Conversation
ryber
left a comment
There was a problem hiding this comment.
Hi @gvma
Thanks for the contribution. I have some thoughts.
The benefit of the magic number pattern is to give labels to things that need them, but it's not 100%, in fact sometimes it can be harmful and make our code harder to read and understand.
In this case "200" is a well known constant in the standard for HTTP, labeling it as such would be fairly redundant for any developer working on Unirest especially outside of the context of all of the other codes (many of which are NOT obvious. quick which one is "gone"?!)
We can also assume that if we want to give a readable label to 200 in a library like Unirest that we are going to want it in LOTS of places, not just this one test.
Most Http libraries have a enum or constant class of Http Status codes. Unirest does not, but it should. What would make this PR better is if you made a production HttpStatus constant class that listed ALL of the status codes. the jetty class org.eclipse.jetty.http.HttpStatus is a good example of this.
|
Hi @ryber. Thanks for the reply. As you recommended, I have implemented a constant class that lists all status code. |
| public static final int UNSUPPORTED_MEDIA_TYPE = 415; | ||
| public static final int RANGE_NOT_SATISFIABLE = 416; | ||
| public static final int EXPECTATION_FAILED = 417; | ||
| public static final int IM_A_TEAPOT = 418; |
There was a problem hiding this comment.
Bonus points for including 418 🤣
This is a test refactoring
Problem:
The Magic Number Test occurs when assert() statements in a test method contain numeric literals (i.e., magic numbers) as parameters.
Solution:
As magic numbers do not indicate the meaning/purpose of the number, they should be replaced with constants or variables, thereby providing a descriptive name for the input.
Before:
assertEquals(200, i.getStatus());
After:
private final int HTTP_STATUS_OK = 200;
assertEquals(HTTP_STATUS_OK, i.getStatus());