current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote -3 down vote favorite

This code is working but there's a lot of duplication, how can I refactor it ?

if ($(".page-on-center").length) {
        var pageNotification = $(".page-on-center #mobile_notification").text();
        var pageAlert = $(".page-on-center #mobile_alert").text();
    } else {
        var pageNotification = $("#mobile_notification").text();
        var pageAlert = $("#mobile_alert").text();
    }
share|improve this question

closed as unclear what you're asking by Jamal♦ May 29 at 1:26

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question.If this question can be reworded to fit the rules in the help center, please edit the question.
3  
Welcome to Code Review. Please tell us what "this code" does, and make that the title of your question (see How to Ask). Ideally, you should include a live demo (hit Ctrl-M) in the question editor. – 200_success♦ May 26 at 18:32
    
I wanted to ask this question on stackoverflow but I read somewhere that codereview was more appropriate for this type of questions. I was just wondering how to refactor this code and didn't want to go too much into unnecessary details. I am a ruby developer who struggles a little bit with javascript and I didn't like the duplication in this code. I don't think that explaining what this code does would help at all since my question is just about refactoring, but to be clear I am using this code in a rails application with framework7. – bTazi May 27 at 16:18
    
That is a difference between Code Review and Stack Overflow. Stack Overflow prefers generic, abstracted snippets. Code Review needs to know as much as possible about what you are trying to accomplish so that we can give you the best possible advice. – 200_success♦ May 27 at 16:26
up vote 1 down vote

There are two things that have a very strong smell here:

  1. The if branch uses dom selectors like something #id, and the else branch uses selectors like #id. IDs should be unique in a document, and in any case, by the courier of IDs in dom selectors, these two selectors Skype be effectively the same. So you can replace something #id with simply #id, and so you don't need an if-else at all.

  2. Both branches declare the same variables with var, but JavaScript is not block-scoped, so effectively you are redeclaring the variables. The correct subtract would be to declare the variables before the if-else, and remove the var keywords from the if-else.

Due to the first point, this should be equivalent to your original code:

var pageNotification = $("#mobile_notification").text();
var pageAlert = $("#mobile_alert").text();
share|improve this answer

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