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.

Sign up for GitHub

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

Jump to bottom

replaced classes with objects #327

Merged
merged 8 commits into from
Oct 10, 2019
Merged

replaced classes with objects #327

merged 8 commits into from
Oct 10, 2019

Conversation

shyamks
Copy link
Contributor

@shyamks shyamks commented Oct 2, 2019 •
edited

Description

Refactor for performance improvements.

Issue

Replacing classes with objects. #317

Type

  • Enhancement

kobawan
kobawan suggested changes Oct 3, 2019
View reviewed changes
src/ad_extension.js Outdated Show resolved Hide resolved
src/ad_extension.js Outdated Show resolved Hide resolved
src/vast_response.js Outdated Show resolved Hide resolved
@kobawan kobawan added 3.0 3.0 roadmap hacktoberfest Participate in 2019's hacktoberfest! refactor labels Oct 3, 2019
@kobawan
Copy link
Contributor

kobawan commented Oct 3, 2019

Also please make sure all the documentation .md files are updated

@kobawan kobawan mentioned this pull request Oct 3, 2019
Merged
5 tasks
@kobawan kobawan added this to the 3.0 milestone Oct 3, 2019
@rumesh
Copy link
Contributor

rumesh commented Oct 3, 2019 •
edited

Please note that function myObject() {....} is still a 'class' you can just do a new myObject().
Idea of #317 was to replace Class by plain Object which will have functions.

@shyamks shyamks requested a review from kobawan October 3, 2019 10:01
kobawan
kobawan suggested changes Oct 3, 2019
View reviewed changes
src/companion_ad.js Outdated Show resolved Hide resolved
src/ad_extension.js Outdated Show resolved Hide resolved
rumesh
rumesh requested changes Oct 4, 2019
View reviewed changes
Copy link
Contributor

@rumesh rumesh left a comment

Choose a reason for hiding this comment

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

This 3 line will break vast-client.
if (this.creative instanceof createCreativeLinear) {
if (this.variation instanceof createNonLinearAd) {
if (this.variation instanceof createCompanionAd) {

will need to implement isCreativeLinear, isCreativeNonLinear and isCreativeCompanionAd,

src/ad_extension.js Outdated
this.children.length === 0
);
}
export function isEmpty(adExtension) {
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT ?

Suggested change
export function isEmpty(adExtension) {
export function isEmptyExtension(adExtension) {

Copy link
Contributor

Choose a reason for hiding this comment

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

I also agree it needs renaming

src/parser/ad_parser.js Outdated
@@ -329,20 +329,20 @@ function _parseExtension(extNode) {
}

// Only return not empty objects to not pollute extentions
return ext.isEmpty() ? null : ext;
return isEmpty(ext) ? null : ext;
Copy link
Contributor

Choose a reason for hiding this comment

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

with the renaming, it will be much more explicit here while reading the code

src/vast_tracker.js Outdated
@@ -65,7 +65,7 @@ export class VASTTracker extends EventEmitter {
// Nonlinear and companion creatives provide some tracking information at a variation level
// While linear creatives provided that at a creative level. That's why we need to
// differentiate how we retrieve some tracking information.
if (this.creative instanceof CreativeLinear) {
if (this.creative instanceof createCreativeLinear) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will not work, probably need a function next to createCreativeLinear which is isCreativeLinear and this will check that this plain object contain all linear creative property

kobawan
kobawan reviewed Oct 4, 2019
View reviewed changes
src/ad_extension.js Outdated
this.children.length === 0
);
}
export function isEmpty(adExtension) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I also agree it needs renaming

src/parser/ad_parser.js Outdated Show resolved Hide resolved
src/parser/vast_parser.js Outdated Show resolved Hide resolved
src/vast_tracker.js Outdated Show resolved Hide resolved
@shyamks shyamks requested review from rumesh and kobawan October 4, 2019 14:22
rumesh
rumesh reviewed Oct 4, 2019
View reviewed changes
Copy link
Contributor

@rumesh rumesh left a comment

Choose a reason for hiding this comment

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

One last comment have a commented import to remove.
Thanks a lot for your contribution 👍 ⭐️

test/vast_parser.js Outdated
@@ -2,10 +2,11 @@ import path from 'path';
import should from 'should';
import sinon from 'sinon';
import { VASTParser } from '../src/parser/vast_parser';
import { VASTResponse } from '../src/vast_response';
// import { createVASTResponse } from '../src/vast_response';
Copy link
Contributor

Choose a reason for hiding this comment

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

this line should be removed right ?

kobawan
kobawan reviewed Oct 7, 2019
View reviewed changes
src/companion_ad.js Outdated
};
}

export function isCompanionAd(companionAd) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you named the param companionAd but it might not be one. I would rename it to ad or adVariation

Copy link
Contributor

Choose a reason for hiding this comment

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

same for the other functions

test/vast_parser.js Outdated
@@ -137,7 +137,7 @@ describe('VASTParser', function() {
});

it('should have returned a VAST response object', () => {
this.response.should.be.an.instanceOf(VASTResponse);
isVASTResponse(this.response).should.eql(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is a good idea to introduce a function in the production code just for unit tests. You should instead make sure that this.response has the correct properties with the correct values explicitly in the unit test.

@shyamks shyamks requested review from rumesh and kobawan October 9, 2019 21:46
kobawan
kobawan reviewed Oct 10, 2019
View reviewed changes
Copy link
Contributor

@kobawan kobawan left a comment

Choose a reason for hiding this comment

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

LGTM! Only thing left is to fix the conflicts :)

rumesh
rumesh approved these changes Oct 10, 2019
View reviewed changes
Copy link
Contributor

@rumesh rumesh left a comment

Choose a reason for hiding this comment

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

Thanks 💯
🚢 !

@rumesh rumesh merged commit bfdc2be into dailymotion:3.0-version Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kobawan kobawan kobawan requested changes

@rumesh rumesh rumesh approved these changes

Assignees
No one assigned
Labels
3.0 3.0 roadmap hacktoberfest Participate in 2019's hacktoberfest! refactor
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@shyamks @kobawan @rumesh