0

I'm refactoring a small project of mine into an ordered class like structure using object and all methods as prototypes. it's quite a messy code but the principal is quite simple , i'm creating an object constructor that sets all necessary props on init , and outside i'm defining all the methods via ObjectName.prototype.funcname

now inside each method if i would like to call another method i'm using "this.methodname()" and if i would like to access the object properties again i'm using "this.someprop" thats works perfectly but when specific cases occur i'm actually creating another object (not via the constructor because i don't need so i just need to store some properties ) and than i would like to call another method with the new object as "this" so i'm calling this.someMethod.call(newobj) but i'm messing everything because now if the method tries to call a different method it will fail but if it will access a property it will succeed. My goal here is to store the new Obj inside the main obj (created via the constructor ) as a "nested" object and if some conditions are met than call some methods with the new "nested" object as the this and still be able to call my main obj methods. Any best practise for that? My first guess was to add a param to the constructor so if i need another "nested" object i can call the constructor with this param and just get a small object instead of the "big" one that also call some methods.

Edit: I have got my constructor that looks something like that:

function IframeMain (IframeNode,Html){
    this.IframeDomReady(IframeNode);
   Object.assign(this,this.CreateContext(window.location.href,IframeNode));
    this.ParsedDoc = this.HtmlParser(Html);
    this.DocReady = false; 
    this.Appender(this.ContentDocument.head,this.ParsedDoc.Head).then(data => {
        this.Appender(this.ContentDocument.body,this.ParsedDoc.Body)
    }).catch(err => {
        console.error(err);
    });     
}

i'm calling "Object.assign" to merge my new object (this) with an object returned from "CreateContext" method. after that i'm taking some html and appending it to an iframe supplied to the constructor, now in some cases there is another iframe inside the html so i'm appending it and than i need to create a new object with this iframe contentWindow/Document so i can append html to the nested iframe, so what do you think is best practise here? if i would create inside the main object a nested one and call the constructor with some param to only return part of the main object and populate it?

CreateContext method:

IframeMain.prototype.CreateContext = function (src,IframeNode){
    if(!IframeNode.contentWindow || !IframeNode.contentDocument || !IframeNode.contentWindow.document)console.error("Context Problem!");

    return {
        NodesArray : [],
        Natives: {},
        DocReady: null,
        NestedContexts : {},
        IframeNode: IframeNode,
        ContentWindow : IframeNode.contentWindow,
        ContentDocument: IframeNode.contentDocument || IframeNode.contentWindow.document,
        Src : src,
        Location: this.ParseUrl(src),
    };

};

HtmlParser Method:

IframeMain.prototype.HtmlParser = function(HtmlString){
let doc = new this.ContentWindow.DOMParser().parseFromString(HtmlString, 'text/html');
//Some more code...... and return parsed html

Appender method:

IframeMain.prototype.Appender = async function(target,nodes){
// append each node recursively to the iframe document 
//More calls here to different methods and if we hit an iframe we create another context and i would like to call the exact same methods and not change a thing because on each method i'm using "this" so technically i can do this recursively for 1000 nested iframes.
so what do you think is best practise here?  

I'm creating a new instance like so:

Let Iframe = new IframeMain(iframeel,html);

and that's it the logic starts to execute immediately that way i tried to avoid creating another instance because it will execute it again and i don't need it.

when i encounter a new iframe i'm calling the :

IframeMain.prototype.nestedframes = async function(iframeNode,html,PrevSrc){

this.NestedContexts = this.NestedContexts || {};
this.NestedContexts[UID] = this.CreateContext(PrevSrc,IframeEl);
// more code ...
and then i would like to call my appender method like this:
 await this.NestedContexts[UID].Appender(this.NestedContexts[UID].ContentDocument.head,this.NestedContexts[UID].ParsedDoc.Head);

but obviously my created context doesn't contain those methods and if i will call my method like so:

await this.Appender.call(this.NestedContexts[UID],this.NestedContexts[UID].ContentDocument.head,this.NestedContexts[UID].ParsedDoc.Head);

// i will have problems in the appender method because my this is my new object but inside appender i can't call my methods because they doesn't exist for that obj.
avi dahan
  • 539
  • 3
  • 19
  • 7
    If you are doing something, and it seems way harder than it should be, and a quick google search does not turn up lots of articles about how the something seems way harder than it should be, take a minute and reevaluate whether or not you *should* do the something. Also, please post code. Your description of it usually will not help us help you. – Jared Smith Aug 20 '18 at 13:24
  • 1
    Yes, if it's supposed to have the class' methods then it's an instance and should be constructed by its constructor. A flag parameter may be one solution if you have to "kinds" of objects, inheritance might be another, but you really need to tell us more about your specific use case or show us the actual code so that we can suggest an appropriate solution. – Bergi Aug 20 '18 at 13:26
  • 1
    *"creating another object (not via the constructor because i don't need…"* – Well, apparently you *do* need… – deceze Aug 20 '18 at 13:28
  • @Bergi i've added my constructor , on the nested object i don't need it to start calling all of those methods because the logic is already executing – avi dahan Aug 20 '18 at 13:37
  • 1
    @avidahan You [shouldn't start the logic in the constructor anyway](https://stackoverflow.com/a/24686979/1048572). It seems like your constructor should take the context and the parsed document as arguments, and all that logic should go inside a static method that returns a new instance. – Bergi Aug 20 '18 at 13:44
  • @Bergi i need to create the context it doesn't passed as argument the arguments passed are the iframe element and the html , i can move all the method calls to a different method (lets call it Init() ) and than call "test = new IframeMain " and "test.Init()" but i still need the constructor to create the context so i need to move the context logic to the constructor and thats it? – avi dahan Aug 20 '18 at 13:51
  • @avidahan I mean that you should change the parameters and adapt every call to the current constructor to call a static method instead. If you can post your complete code, I can write up an answer. – Bergi Aug 20 '18 at 13:57
  • @Bergi i've added some more methods to help explain the logic but the entire code is huge and i can't paste it here , i hope that's fine – avi dahan Aug 20 '18 at 14:08
  • @avidahan The pseudo code is very much ok. But could you please add a) a typical invocation of the `new IframeMain` constructor b) the part where you create the "nested" or "small" object that doesn't need the init logic and c) any method of the instance that actually uses `this` (instead of just parameters) as well as a typical invocation of it? – Bergi Aug 20 '18 at 14:13
  • @Bergi i hope thats enough , thanks a lot! – avi dahan Aug 20 '18 at 14:32
  • @Bergi should i just create the context in the constructor and move all the method calls to somewhere else? and use a param in the constructor to control it? – avi dahan Aug 21 '18 at 08:47

1 Answers1

2

I'm still not quite sure, but I think what you are actually looking for is a separate Context class:

class Context {
    constructor(src, iframeNode) {
        if (!iframeNode.contentWindow || !iframeNode.contentDocument || !iframeNode.contentWindow.document) throw new Error("Context Problem!");
        this.nodesArray = [];
        this.natives = {};
        this.nestedContexts = {};
        this.iframeNode = iframeNode;
        this.contentWindow = iframeNode.contentWindow;
        this.ontentDocument = iframeNode.contentDocument || iframeNode.contentWindow.document,
        this.src = src;
        this.location = Context.parseUrl(src);
    }
    static parseUrl(src) { … } // if it needs properties of IframeMain, make it a method there
                               // and pass the parsed location to the Context constructor
    htmlParser(htmlString) {
        const doc = new this.ContentWindow.DOMParser().parseFromString(HtmlString, 'text/html');
        // Some more code...... and return parsed html
    }
    async append(target, nodes) {
        // append each node recursively to the iframe document 
        // More calls here to different methods and if we hit an iframe we create another context 
    }
    async nestedFrames(iframeNode, html, prevSrc) {
         const nested = this.NestedContexts[UID] = new Context(prevSrc, iframeEl);
         // more code ...
         await this.nestedContexts[UID].append(nested.contentDocument.head, nested.parsedDoc.Head);
    }
    // etc

i'm calling "Object.assign" to merge my new object (this) with an object returned from "CreateContext" method.

Don't do that. I think here is your main problem. Don't make contexts plain objects, make them class instances that have all the methods and properties that they need.

If your IframeMain does more than that minimal Context class, introduce a second kind of objects. Make a Context subclass, or probably better use composition.

class IframeMain {
    constructor(iframeNode, html) {
        this.IframeDomReady(IframeNode); // ???
        this.context = new Context(window.location.href, iframeNode);
        this.parsedDoc = this.context.htmlParser(Html);
        this.docReady = false;
    }
}

Not sure though what the exact difference between them would be. In your nestedFrames method you use the parsedDoc property of a context - if every context should have a parsed document, put that property there as well. Or if only the IframeMain is responsible for managing the collection of nestedContexts, put that there (instead of each context having own child contexts, in a tree-like structure).

Maybe I misjudged and you don't actually need two different data structures. If so, just keep the Context constructor minimal and don't call any method but only initialise properties. The work should be done in separate functions then:

 async function iframeMain(iframeNode, html) {
     const context = new Context(html, iframeNode);
     await context.append(context.contentDocument.head, context.parsedDoc.head);
     await context.append(context.contentDocument.body, context.parsedDoc.body);
 }
 try {
     const iframe = await iframeMain(iframeel, html);
} catch (err) {
    console.error(err);
}
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • Thanks a lot for your awesome answer! before i begin fixing my code i just need a bit more clarification , my purpose is to contain everything in a single object so the user will just do "var test = new IframeMain(iframeel,html); test.Init();". there are a lot more methods than i posted so creating a new object for the context will probably cause more problems , my thought is to clear the constructor from all the method calls and only initiate the properties , Is that ok? after that can i just call new iframeMain(...,...) create a new context and use it to call other methods? – avi dahan Aug 21 '18 at 09:53
  • @avidahan "*clear the constructor from all the method calls and only initiate the properties*" - yes, that's the best practice for constructors. Whether you make an `init` method or just write a wrapper function for constructing and initialising (like the `async function iframeMain` in my last snippet) is your choice. The wrapper however can also do stuff before constructing the instance, which might be an advantage especially with asynchrony. – Bergi Aug 21 '18 at 09:56
  • Thanks again ! i will just move all the methods calls to prototype.init , and for the wrapper function i don't think i need it because as i mentioned i need everything to be contained inside the object . – avi dahan Aug 21 '18 at 10:01
  • BTW just one more thing , when i create a new content and store it in the main context under NestedContexts and than call "this.NestedContexts[UID].Appender" the appender will execute with the "this" set to my new context right? and every call inside the appender with the "this" prefix will also be called with my new context? – avi dahan Aug 21 '18 at 10:03
  • Yes, `this` will refer to the `this.NestedContexts[UID]` object when that's what you're calling the `Appender` method on – Bergi Aug 21 '18 at 12:16