8
\$\begingroup\$

Is there any way to reduce these lines of code?

var url = window.location.href.split("?");
if(url[1].toLowerCase().includes("chk=webdes-check")){
    $('#website-design').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=webdev-check")){
    $('#website-development').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=webmain-check")){
    $('#website-maintenance').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=seo-check")){
    $('#search-engine-optimisation').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=softdev-check")){
    $('#software-development').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=digmark-check")){
    $('#digital-marketing').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=digstrats-check")){
    $('#digital-strategy').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=digbrand-check")){
    $('#digital-branding').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=anivid-check")){
    $('#animated-videos').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=virtreal-check")){
    $('#3d-virtual-reality').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=augreal-check")){
    $('#augmented-reality').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=hostdom-check")){
    $('#hosting-and-domain').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=appdevuiuxexp-check")){
    $('#app-development').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=reprev-check")){
    $('#reputation-review').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=incgen-check")){
    $('#income-generating').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=datacol-check")){
    $('#database-collection').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=telcom-check")){
    $('#tel-communication').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=humres-check")){
    $('#human-resources').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=vidprod-check")){
    $('#video-production').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=aiweb-check")){
    $('#ai-for-website').attr('checked', true);
}
else if(url[1].toLowerCase().includes("chk=telestrats-check")){
    $('#telemarketing-and-stategy').attr('checked', true);
}
else{
    //false
}

I just wanted to shorten this code because i know it's not proper. Should I use a for loop maybe? I'm not sure how.

and by the way this is how i call it just forgot it earlier

function linkPageContact(clicked_id){

if(clicked_id === 'website-design-check'){
    window.location.href = "/contact?chk=webdes-check";
}
else if(clicked_id === 'website-development-check'){
    window.location.href = "/contact?chk=webdev-check";
}
else if(clicked_id === 'website-maintenance-check'){
    window.location.href = "/contact?chk=webmain-check";
}
else if(clicked_id === 'search-engine-optimization-check'){
    window.location.href = "/contact?chk=seo-check";
}
else if(clicked_id === 'software-development-check'){
    window.location.href = "/contact?chk=softdev-check";
}
else if(clicked_id === 'digital-marketing-check'){
    window.location.href = "/contact?chk=digmark-check";
}
else if(clicked_id === 'digital-strategy-check'){
    window.location.href = "/contact?chk=digstrats-check";
}
else if(clicked_id === 'digital-branding-check'){
    window.location.href = "/contact?chk=digbrand-check";
}
else if(clicked_id === 'animated-videos-check'){
    window.location.href = "/contact?chk=anivid-check";
}
else if(clicked_id === 'virtual-reality-3d-check'){
    window.location.href = "/contact?chk=virtreal-check";
}
else if(clicked_id ==="augmented-reality-check"){
    window.location.href = "/contact?chk=augreal-check";
}
else if(clicked_id ==="hosting-and-domain-check"){
    window.location.href = "/contact?chk=hostdom-check";
}
else if(clicked_id ==="app-dev-uiux-experience-check"){
    window.location.href = "/contact?chk=appdevuiuxexp-check";
}
else if(clicked_id ==="reputation-review-check"){
    window.location.href = "/contact?chk=reprev-check";
}
else if(clicked_id ==="income-generating-check"){
    window.location.href = "/contact?chk=incgen-check";
}
else if(clicked_id ==="database-collection-check"){
    window.location.href = "/contact?chk=datacol-check";
}
else if(clicked_id ==="tel-communication-check"){
    window.location.href = "/contact?chk=telcom-check";
}
else if(clicked_id ==="human-resources-check"){
    window.location.href = "/contact?chk=humres-check";
}
else if(clicked_id ==="video-production-check"){
    window.location.href = "/contact?chk=vidprod-check";
}
else if(clicked_id ==="ai-for-websites-check"){
    window.location.href = "/contact?chk=aiweb-check";
}
else if(clicked_id ==="telemarketing-strategy-check"){
    window.location.href = "/contact?chk=telestrats-check";
}
// window.location.href = "/contact";
// alert(clicked_id);   
}
\$\endgroup\$
  • \$\begingroup\$ if these are check boxes, why are they being treated as mutually exclusive? \$\endgroup\$ – vijrox Aug 8 '17 at 20:22
10
\$\begingroup\$

Extract the chk parameter value from location.search and use a "lookup table" object:

var CHK_ID_LOOKUP = {
  'aiweb-check':         '#ai-for-website',
  'anivid-check':        '#animated-videos',
  'appdevuiuxexp-check': '#app-development',
  'augreal-check':       '#augmented-reality',
  'datacol-check':       '#database-collection',
  'digbrand-check':      '#digital-branding',
  'digmark-check':       '#digital-marketing',
  'digstrats-check':     '#digital-strategy',
  'hostdom-check':       '#hosting-and-domain',
  'humres-check':        '#human-resources',
  'incgen-check':        '#income-generating',
  'reprev-check':        '#reputation-review',
  'seo-check':           '#search-engine-optimisation',
  'softdev-check':       '#software-development',
  'telcom-check':        '#tel-communication',
  'telestrats-check':    '#telemarketing-and-stategy',
  'vidprod-check':       '#video-production',
  'virtreal-check':      '#3d-virtual-reality',
  'webdes-check':        '#website-design',
  'webdev-check':        '#website-development',
  'webmain-check':       '#website-maintenance',
};

var chk = (location.search.match(/[?&]chk=([^&#]+)|$/i)[1] || '').toLowerCase();
if (CHK_ID_LOOKUP.hasOwnProperty(chk)) {
  $(CHK_ID_LOOKUP[chk]).attr('checked', true);
} else {
  // unknown chk
}

If this code is frequently executed, declare the lookup object just once in the outside closure.

\$\endgroup\$
  • \$\begingroup\$ @IsmaelMiguel The jQuery was in the original code. If you believe it's inappropriate, write your own answer. Please don't harass wOxxOm for something that was carried over from the original code. (I'm leaving your initial comment, but deleting the subsequent debate, as this is not the place for it.) \$\endgroup\$ – 200_success Aug 9 '17 at 0:02
7
\$\begingroup\$

First of all, it would help tremendously if you named the URL parameter values and the form element IDs consistently. Why do you have chk=webdes-check corresponding to #website-design? How did you even arrive at the weird abbreviation "webdes"? And why do you need the redundant "-check" suffix? The element IDs make much more sense, I think. If you use the same parameters and element IDs, then you would avoid the need for a translation table.

Furthermore, why are the URL parameters case-insensitive? If you write your web application using lowercase values everywhere, then you wouldn't need to call .toLowerCase().

window.location gives you a Location object. From it, you can access the .search attribute, which is more direct than .split("?").

So, how to process window.location.search? I would scan for any chk parameters — that is, a ? or &, followed by chk=, followed by the value (one or more characters, but stopping before any & character). To look for a pattern like that, I would use a regular expression.

var chkParamRegexp = /[?&]chk=([^&]+)/g;
var chkParam;
while (chkParam = chkParamRegexp.exec(window.location.search)) {
    var chkValue = chkParam[1];
    $('#' + chkValue).attr('checked', true);
}
\$\endgroup\$
  • \$\begingroup\$ This makes the most sense. Consistent naming of variables and identifiers is step #1 to clean code. +1 \$\endgroup\$ – Chris Cirefice Aug 8 '17 at 18:02
  • \$\begingroup\$ Agreed - and it's not like there's any hidden information to protect. \$\endgroup\$ – corsiKa Aug 8 '17 at 19:23
  • \$\begingroup\$ I don't agree that a regex is a good solution for this at all. It's much better to use a hash table, as suggested in @wOxxOm's answer. This answer is much, much harder to read and understand, and a regex is totally unnecessary. \$\endgroup\$ – ell Aug 8 '17 at 23:04
  • \$\begingroup\$ @sgroves What do you mean, and why wouldn't regex be a good solution? wOxxOm's answer also uses a regex. \$\endgroup\$ – 200_success Aug 8 '17 at 23:58
6
\$\begingroup\$

I don't do javascript but what I noticed right away is that you are doing url[1].toLowerCase() for each if and else if.
You should do this once and store the result in a variable which you then use to call .includes().

\$\endgroup\$
3
\$\begingroup\$

As pointed out by Heslacher, you're repeating the code. The value of url can be cached and used in following if statements.

To get the value of chk param value, use RegEx. You're using split which will fail if there are other params followed by chk for example, /something?chk=some-val&rdo=other-val.

window.location.href.match(/chk=(.*?)([&#]|$)/)[1]

This will search for any string followed by chk= until # or & is found or the string ends. Since, match returns an array, use [1] subscript notation to extract the value of chk.

To check a checkbox, use prop() instead of attr(). See .prop() vs .attr().


Here are two suggestions:

  1. Use the same value in the query param as the ID of element in the HTML so it can be directly used as selector
  2. If you don't want to change the query param value and ID of checkbox, create a mapping of param value and the selector

For the first, the code can be reduced down to just two lines.

const val = (window.location.href.match(/chk=(.*?)([&#]|$)/)[1] || ['', '']).toLowerCase() || 'defaultValue';
$(`#${val}`).prop('checked', true);

For second, create an object with value-selector and use it

var const = (window.location.href.match(/chk=(.*?)([&#]|$)/)[1] || ['', '']).toLowerCase() || 'defaultValue';

const checkboxMappings = {
    'webdes-check': '#website-design',
    'webdev-check': '#website-development',
    'webmain-check': '#website-maintenance'
    ...
};
const selector = checkboxMappings[val] || 'defaultSelector';
$(selector).prop('checked', true);

The advantage of this method is that you can use any selector. For example, if you want to use class selector for one of the checkboxes, just use .some-class as the selector in mappings and it will work.


UPDATE: After editing by OP.

Both the approach will work for the other functionality too. So, the answer will cover those cases too without updating it. OP just needs to write similar code for other functionality.

\$\endgroup\$
2
\$\begingroup\$

I saw there still be some good answers, but I want to post my own solution to this refactoring problem.

The purpose is to have as many different solutions as possible to see which parts could be really improved.

When you have many strings that should be converted in different strings, a map is a good choice to made the code readable and easy to extend or change in the future.

var pageCheckerMap = {
    "aiweb-check":         "#ai-for-website",
    "anivid-check":        "#animated-videos",
    "appdevuiuxexp-check": "#app-development",
    "augreal-check":       "#augmented-reality",
    "datacol-check":       "#database-collection",
    "digbrand-check":      "#digital-branding",
    ....
}

In another answer someone said that you should avoid to have different strings that point to the same, let's say, value.

This is a good point, but I think there are cases when you need to use different text descriptors, even for the same value, and you cannot reconciliate those differences for good reasons.

To handle shuch a map it's better to have a proper translation function that decide what to do in case you could not map a key to a value.

function translate(paramCheckName) {
    var lowerParamCheckName = paramCheckName.toLowerCase();
    if (pageCheckerMap.hasOwnProperty(lowerParamCheckName)) {
        return pageCheckerMap[lowerParamCheckName];
    }
    console.error("param check name not found", lowerParamCheckName);
    return null;
}

In this trivial implementation, I just log an error and return null, that will be skipped further.

So here is my solution.

Firstly I use the javascript array functions to change the url query and get the only parameters I'm using. Secondly I formatted those by mapping to the checkbox DOM ids with the translate function and obtain a comma separated string.

Such string is a valid jQuery selector that I use to retrieve from the DOM just the elements that I'm lookging for.

As the task we have to do is set to true the checked attribute of the checkbox element, I just skip those that are already set to true.

var url = window.location.href.split("?");
if (url.length > 0) {
    var allChecksToTrueSelector = url[1]
        .split("&")
        .filter(function (itm) {
             return (itm.match(/^chk=/i));
         })
        .map(translate)
        .filter(function (itm) {
             return itm != null;
        })
        .join(",")

    $(allChecksToTrueSelector)
        .filter(function (idx, el) {
            return !$(el).attr('checked');
        })
        .each(function (idx, el) {
            $(el).attr('checked', true)
        })
}

I think the benefit of this solution is that is much easy to made readable (by creating functions with proper names instead of the lambdas), much easy to extend without hidden side effects.

And finally it's much based on the power of javascript and jQuery.

\$\endgroup\$

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.