13
\$\begingroup\$

I have the following functions in JavaScript:

function SelectFirstVisiblePublicationAndHideIt ( PageObj )
{
    var hideButtonXpath = "//img[@title='To hide this item in your profile, mark it as invisible']";
    var hideButtonArray;
    var numberOfHideButton;
    var publicationTitle;

    aqUtils.Delay(500);
    hideButtonArray = PageObj.EvaluateXpath(hideButtonXpath);
    numberOfHideButton = hideButtonArray.length;
    hideButtonArray[0].Click();
    publicationTitle = hideButtonArray[0].parent.parent.FindChildByXPath("//a").innerText;

    return publicationTitle;
}

function SelectFirstVisiblePublicationAndReclaimIt ( PageObj )
{
    var reclaimButtonXpath = "//button[@title='Claim this publication and stay on this page']";
    var reclaimButtonArray;
    var numberOfReclaimButton;
    var publicationTitle;

    aqUtils.Delay(500);
    reclaimButtonArray = PageObj.EvaluateXpath(reclaimButtonXpath);
    numberOfReclaimButton = reclaimButtonArray.length;
    reclaimButtonArray[0].Click();
    publicationTitle = reclaimButtonArray[0].parent.parent.FindChildByXPath("//a").innerText;

    return publicationTitle;
}

SelectFirstVisiblePublicationAndHideIt and SelectFirstVisiblePublicationAndReclaimIt are very similar except for the Xpath expression.

I am wondering if I should merge them into one function by:

  • re-naming variables and functions names in a more generic way
  • introduce an additional input argument that serves as a flag to indicate which Xpath expression is to be used

Doing so has pros and cons:

  • Pros: reduce the number of function by one
  • Cons: purpose of a function can no longer be read from its name; having one more input argument; become further away from the principle of "one function should do one thing and one thing only"

Any suggestions?

\$\endgroup\$
  • 3
    \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – BCdotWEB Oct 17 '16 at 11:40
  • 2
    \$\begingroup\$ Even if you want to keep separate functions for other code to call, you can create an internal helper function to get rid of this enormous redundancy. \$\endgroup\$ – usr Oct 17 '16 at 16:11
15
\$\begingroup\$

As the code is similar, they can be merged together.

I'll suggest you to break down the functions into smaller reusable chunks which will follow the Single Responsibility Principle (SRP). These functions will be testable and reusable.

Both functions are

  1. Adding a delay of 500 milliseconds
  2. Selecting elements from provided page
  3. Clicking the first element from the collection
  4. Finding an element from its ancestor and returning the innerText of it.

As these are the steps, we can similarly divide those functions into smaller functions each doing the task of one step above.

For first step (to add delay) I've not created a new function as I guess that is already created in utils module.


The first function (Step #2) will select the elements from the page using the provided xpath and return the first of them.

function getFirstVisiblePublication(page, xpath) {
    aqUtils.Delay(500);

    var elements = page.EvaluateXpath(xpath);
    return elements[0];
}

The second function combines Steps #3 & #4; as #3 only requires to click the element I haven't created new function.

The below function will accept the page in which the target element is to be searched and xpath of the element. This will call the above function to get the first element and click it.

This function will return the text/label of the anchor element which is obtained by using the first element's ancestors.

function getFirstVisiblePublicationAnchorLabel(page, xpath) {
    // Get first element
    var firstElement = getFirstVisiblePublication(page, xpath);
    firstElement.Click();

    return firstElement.parent.parent.FindChildByXPath('//a').textContent;
}

Usage:

Function can be called as follows:

var hideButtonXpath = "//img[@title='To hide this item in your profile, mark it as invisible']";
var publicationTitle = getFirstVisiblePublicationAnchorLabel(PageObj, hideButtonXpath);
\$\endgroup\$
  • 2
    \$\begingroup\$ I like this answer but I would add that if the OP is using page objects, and he is, the page object should contain the desired XPath rather than passing in a string. It could be a property on the page object, etc. That keeps the caller from having to look up the XPath for the desired element and keeps the storage of that string in a central location, the page object. \$\endgroup\$ – JeffC Oct 17 '16 at 15:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.