algorithm class: rectangle #1340
Conversation
|
|
||
| const rectangle = new Rectangle(4, 2) | ||
|
|
||
| test('The area of a rectangle with length equal to 4 and width equal to 2', () => { |
There was a problem hiding this comment.
These tests are trivial; they don't need redundant names.
| return Math.sqrt(Math.pow(this.length, 2) + Math.pow(this.width, 2)) | ||
| } | ||
|
|
||
| isSquare = () => { |
There was a problem hiding this comment.
Won't isSquare = () => this.length === this.width work?
| } | ||
|
|
||
| diagonalLength = () => { | ||
| return Math.sqrt(Math.pow(this.length, 2) + Math.pow(this.width, 2)) |
There was a problem hiding this comment.
Why use Math.pow when JS has ** for exponentiation?
|
|
||
| export default class Rectangle { | ||
| constructor (length, width) { | ||
| this.length = length |
There was a problem hiding this comment.
I think this is usually called "height"
There was a problem hiding this comment.
Pretty trivial. @raklaptudirm thoughts on accepting trivial algorithms / data structures?
|
I would say only to allow trivial algorithm implementations if they are able to showcase good usage of a large range of language features. This implementation doesn't seem to do that, and imo, has little educational value. I am in favor of closing this pr. |
|
I saw the class "circle" and I wanted to implement for rectangle too. I can fixe the width to the height. Could you please consider to be merged? |
|
I agree with rak that this is pretty trivial and has little educational value. You could consider:
The latter is needed very often in game dev. It isn't terribly difficult to implement, but it is very useful and not as trivial as perimeter or area of a rectangle. |
Describe your change:
Checklist:
Example:
UserProfile.jsis allowed butuserprofile.js,Userprofile.js,user-Profile.js,userProfile.jsare notFixes: #{$ISSUE_NO}.