Bank transaction system I was hoping someone could provide some feedback with regards to this:

import java.util.InputMismatchException;
import java.util.Scanner;

public class Bank {

    protected double myBalance = 10.40;
    protected double bankBalance = 1000000;
    protected double creditRating = 0.1;
    private static final int PIN = 1234;
    boolean access = false;
    protected Scanner s = new Scanner(System.in);

    public Bank() {

        int pin = 0000;
        int option;

        System.out.print("Please enter your 4 digit pin code: ");
        try {
            pin = s.nextInt();
        } catch (InputMismatchException e) {
            System.out.print("Invalid character entered, bye bye...");
        }

        if (pin != PIN) {
            System.err.println("Incorrect PIN entered, please try again later.");
            System.exit(0);
        }
        access = true;
        System.out.println("Welcome to JCBank please choose from the following options");
        System.out.println();
        System.out.println("Withdraw (1)\tDeposit (2)\tCheck Balance (3)\tApply for Loan(4) \tQuit (5)");
        System.out.println();

        do {
            option = s.nextInt();
        } while(!selectOption(option));
    }

    public boolean selectOption(int option) {
        switch (option) {
            case 1:
                System.out.println("You would like to withdraw");
                System.out.println();
                break;
            case 2:
                System.out.println("You would like to Deposit");
                System.out.println();
                break;
            case 3:
                System.out.println("Your account balance is £" + myBalance);
                System.out.println();
                break;
            case 4:
                System.out.println("Please wait whilst we perform a credit check.");
                try {
                    checkCreditRating();
                } catch (InterruptedException e) {
                    System.out.println("How embarrassing, the system is currently unavailable. Try again later.");
                }
                System.out.println();
                break;
            case 5:
                System.out.println("Successfully logged out, see you soon!");
                System.out.println();
                return true;
            default:
                System.out.println("Invalid option, please choose another option.");
                System.out.println();
                return false;
        }
            System.out.println("Please choose another option.");
            return false;
    }

    public void deposit(double amount) {
        myBalance += amount;
    }

    public void withdraw(double amount) {
        if (myBalance > amount) {
            System.out.println("Your funds have been successfully withdrawn.");
        } else {
            System.out.println("You don't have the funds to complete this transaction");
        }
    }

    public void checkCreditRating() throws InterruptedException {
        Thread.sleep(1000);
        if (creditRating <= 0.0) {
            System.err.println("Sorry, we can't lend you any money at this time.");
            return;
        }

        System.out.print("Please enter your loan amount: ");
        double amount = s.nextDouble();
        if (amount > bankBalance) {
            System.err.println("We are unable to loan you such an amount.");
        }
        double amountToPayBack = (20/100)*amount;
        System.out.println("The amount to payback is " + amountToPayBack);
    }

    public double checkAccountBalance() {
        return myBalance;
    }
}

A number of points:

Do not use floating-point data types (float or double) for amounts of money. Floating-point data types have limited precision, they cannot represent all decimal values exactly, and you're going to get roundoff errors sooner or later. Use integers instead (int or long) and store cents instead (for example, 1040 cents) or use BigDecimal.
Make member variables private by default; only make them protected (or another access level) if there is a good reason to do so.
Prefer local variables above member variables. Is there a good reason why s is a member variable instead of a local variable?
Don't write the main part of a program in the constructor of a class. You have the main part of your program in the constructor of the Bank class. The constructor is meant to initialize the state of a new object, so the only thing that the constructor should do is initialize member variables.
In your constructor, you catch an InputMismatchException and print an error message ("bye bye...") but you are not actually returning from the constructor. So execution will continue and print "Incorrect PIN entered ..." which is not what you meant.
The program doesn't actually do what you'd expect it to do; withdrawal, deposit etc. functionality never gets called anywhere.
You need a public static void main(String[] args) method to be able to run this as a stand-alone program.


Design LRU cache interview questions Code reviewers, I request you review the code and suggest all tips for improvement.

public class LRU<K, V> {

    private final Map<K, Entry<K, V>> map;
    private Entry<K, V> eldest;
    private final int lruSize;

    public LRU (int lruSize) {
        if (lruSize <= 0) {
            throw new IllegalArgumentException("size should atleast be one");
        }
        map = new HashMap<K, Entry<K, V>>();
        this.lruSize = lruSize;
    }

    private static class Entry<K, V> {
        Entry<K, V> left;
        K key;
        V value;
        Entry<K, V> right;

        Entry (Entry<K, V> left, K key, V value, Entry<K, V> right) {
            this.left = left;
            this.key = key;
            this.value = value;
            this.right = right;
        }

    };

    /**
     * Head is first node of the linked list.
     * Similar to - convert tree to doubly linkedlist.
     */
    private void addFirst(Entry<K, V> entry) {
        remove(entry);
        if (eldest == null) {
            entry.left = entry.right = entry;
            eldest = entry;
        } else {
            // last node of the list. extra storage just for purpose of simplification
            Entry<K, V> tail = eldest.left;

            tail.right = entry;
            entry.left = tail;

            // now deal with circularing
            eldest.left = entry;
            entry.right = eldest;
        }
    }

    private void remove (Entry<K, V> entry) {
        assert entry != null;

        if (entry.left != null) entry.left.right = entry.right;
        if (entry.right != null) entry.right.left = entry.left;
        if (entry == eldest) eldest = entry.right;
    }

    /**
     * Allowing null. Since, this is behavior of linkedhashmap.
     * Still adding synchronization as interviewer is bound to ask this question
     */
    public synchronized void put(K key, V value) {
        Entry<K, V> entry = new Entry<K, V>(null, key, value, null);
        map.put(key, entry);
        addFirst(entry);

        if (removeEldestEntry()) {
            remove(eldest.key);
        }
    }

    public synchronized V get (K key) {
        Entry<K, V> entry = map.get(key);
        if (entry != null) {
            addFirst(entry);
            return entry.value;
        }
        return null;
    }

    public synchronized void remove (K key) {
       Entry<K, V> entry = map.remove(key);
       if (entry != null) {
           remove(entry);
       }
    }

    private boolean removeEldestEntry() {
        return map.size() > lruSize;
    }
}

The first thing I had to do when seeing your code was looking up “LRU”. Apparently it stands for “Least Recently Used”, something to do with caching.

In your constructor, you initialize the map. This could also be done at the point of declaration:

private final Map<K, Entry<K, V>> map = new HashMap<K, Entry<K, V>>();

You also keep the eldest entry around – it would be more idiomatic to call it the oldest entry.

In your Entry class, I would swap the arguments to (key, value, left, right). Just because one variable points to the left sublist, this does not mean that it has to be the leftmost parameter in the constructor. For each node, the key and value are the more important information, and should come first. Actually, you never use left and right in the Entry constructor.

(…time goes by, now I understand you are implementing a circular buffer as a doubly linked list…)

It would be beneficial to add some helper methods in the Entry class, as this would be better encapsulation.

/**
 * An Entry in a (circular) doubly linked list.
 */
private static class Entry<K, V> {
  K key;
  V value;
  Entry<K, V> left, right;

  Entry(K key, V value) {
    this.key   = key;
    this.value = value;
  }

  /**
   * Connect "this" and "left" so that "left" is on the left side.
   *
   * @param left The new left side, may be null
   * @return This entry
   */
  Entry<K, V> addLeft(Entry<K, V> left) {
    this.left = left;
    if (left != null) left.right = this;
    return this;
  }

  /**
   * Connect "this" and "right" so that "right" is on the right side.
   *
   * @param right The new right side, may be null
   * @return This entry
   */
  Entry<K, V> addRight(Entry<K, V> right) {
    this.right = right;
    if (right != null) right.left = this;
    return this;
  }

  /**
   * Insert this entry between the specified nodes, keeping track of backlinks.
   *
   * @param left  The new left side, may be null
   * @param right The new right side, may be null
   * @return This entry
   */
  Entry<K, V> insertBetween(Entry<K, V> left, Entry<K, V> right) {
    return this.addLeft(left).addRight(right);
  }

  /**
   * Remove this entry out of the linked list, so that the left
   * and right entries now connect to each other directly.
   */
  void remove() {
    if (this.left  != null) {
      this.left .addRight(this.right);
    }
    else if (this.right != null) {
      this.right.addLeft (this.left );
    }
    this.left = this.right = null;
  }
}

Doing so would also increase testability of your design, and it makes it easier to reason about your code.

This now simplifies your other methods, e.g. addFirst becomes

private void addFirst(Entry<K, V> entry) {
    remove(entry);
    if (oldest == null) {
        oldest = entry.addLeft(entry); // implies addRight(entry)
    } else {
        entry.insertBetween(oldest.left, oldest);
    }
}

and remove simplifies to:

private void remove(Entry<K, V> entry) {
    assert entry != null;

    if (entry == oldest) oldest = entry.right;
    entry.remove();
}

The remaining methods are Ok, because they build upon these operations.

In general, the lack of comments made it very hard for me to understand your code.

For example, I was wondering why you weren't using a LinkedList as a Queue. Then I realized that removing an element would be O(n) instead of O(1). It makes sense to keep track of design decisions in comments, and to specify complexity guarantees in the documentation.
Then, such comments like “Allowing null. Since, this is behavior of linkedhashmap.” do not convey useful information. What variable may be null? What is linkedhashmap? What does all of that have to do with insertion?

The algorithmic side was excellent, but your code was rather hard to understand.


Prime Sieve and brute force Got caught up with this morning's C++ question.
So I thought I would have a go.

SO here is my attempt at printing the first 1001 primes.

#include <iostream>
#include <vector>
#include <cmath>

bool isPrime(std::size_t num, std::vector<std::size_t> const& primes)
{
    std::size_t max = std::sqrt(num);
    for(auto const& prime: primes)
    {
        if (prime > max) {
            return true;
        }
        if (num % prime == 0)
        {
            return false;
        }
    }
    return true;
}

std::size_t sieve(std::size_t size, std::vector<std::size_t>& results)
{
    // primeCandidates holds 0/1 for each potential prime candidate.
    // The index represents the potential prime (index * 2 + 1)
    // This allows us to ignore all factors of 2
    // max is one past the highest prime candidate we can test for and store in primeCandidates
    std::size_t const           max = size * 2 + 1;
    std::vector<std::size_t>    primeCandidates(size, 1);

    // Add some default well know primes.
    results.push_back(2);
    results.push_back(3);

    // We will use the technique of incrementing by 2 then 4 then 2 then 4
    // This means skip all factors of 2 and 3 automatically.
    std::size_t       inc = 2;
    std::size_t loop = 5;
    for(; loop < max; loop += inc, inc = 6 - inc) {
        std::size_t index = loop/2;

        // If we find a candidate that is valid then add it to results.
        if (primeCandidates[index]) {
            results.push_back(loop);

            // Now strain out all factors of the prime we just found.
            for(; index < primeCandidates.size(); index += loop) {
                primeCandidates[index] = 0;
            }
        }
    }
    return loop;
}

int main()
{
    std::size_t constexpr   primeRequired   = 1001;
    std::size_t constexpr   siveStartSize   = 1000;

    std::vector<std::size_t>    result;
    result.reserve(primeRequired);

    // Use the Sieve of Eratosthenes to get a basic set of primes.
    std::size_t next = sieve(siveStartSize, result);

    // Want to re-use the 2/4/2/4 increment pattern
    // So work out the correct start point and increment value.
    std::size_t inc  = next % 6 == 5 ? 2 : 4;

    // We now use brute force checking each number against all the primes
    // that we have already found.
    for(; result.size() <= primeRequired; next += inc, inc  = 6 - inc) {
        if (isPrime(next, result)) {
            result.emplace_back(next);
        }
    }

    // Print out the primes we have found
    for(auto val: result) {
        std::cout << val << " ";
    }
    std::cout << "\n";
}

When you find a prime number, say 7, you begin crossing out all odd multiples of 7 from the primeCandidates vector.  You do this by incrementing index += loop, where loop is the prime number, and but primeCandidates only holds odd candidates, so the step size in natural numbers would be 2*loop.

The issue is you start by removing 7 from primeCandidates, then 21, then 35.  At this point in the sieve, you've already eliminated multiples of 3 and 5, so marking off 3*7=21 and 5*7=35 is just busy work.  And marking off 1*7=7 is just pointless.  What you should be doing is starting to eliminate at 7*7=49.  It saves only three eliminations during the 7 loop, but the saving get more substantial as the prime gets larger.  For instance, when you get to 199, you wouldn't eliminate 1*199, 3*199, 5*199, 7*199, ... 191*199, 193*199, 195*199, 197*199.  You would just start at 199*199 and go up from there.

Code change:

// Now strain out all factors of the prime we just found, starting with prime^2
index = loop * loop / 2

std::vector<std::size_t> is overkill for primeCandidates.  You only ever store 1 or 0, so allocating 4 or 8 bytes for each candidate is wasting memory.

std::vector<char> would reduce the memory load to 1 byte per candidate.

std::bitset<N> will reduce this to 1 bit per candidate.  Alternately, the vector specialization std::vector<bool> will also give 1 bit per candidate, with a non-compile-time fixed number of bits.


Avoid calling nested method two times For a given file name path, I need to get the folder one level before it. This can be accomplished in ruby with the following code:

File.dirname(File.dirname("/a/b/c.txt"))
 => "/a"

Independently from this exact case, how can I DRY two method calls in sequence?

You should consider any DRYing efforts in terms of its effects on code clarity. I don't think that trying to reduce foo(foo(bar)) is worth the reduced clarity. Now, if we're trying to reduce foo(foo(foo(foo(foo(bar))))), that's something else. One way you can do that is using reduce:

5.times.reduce(1 << 64) { |x,| Math.sqrt(x) }
=> 4.0

5.times.reduce('/a/b/c/d/e/f.txt') { |x,| File.dirname(x) }
=> "/a"


SQL - How's my formatting? I'm somewhat new to SQL (using it in the past but only being exposed to it heavily in my current role). Unfortunately nobody at my current company has really given me any advice on formatting. How can I format this better / what should I avoid doing that I may have done below? Any feedback on re-factoring would be greatly appreciated also.

/* Uncomment when testing this SQL */
declare @period_id as int = 252 -- ### Potentially redundant ###
declare @building_id as varchar(10) = '20500'
declare @section_name as varchar(20) = 'ALL'
declare @lease_expiry_period as int = 1;

/* SQL for Section_Name parameter on report */
select   'ALL',
         1 as order_by
union all
select distinct upper(isnull(section_name, '')) as section_name,
                2 as order_by
from     property.lease_period
where    section_name <> ''
order by 2;

declare @truncateddate as date = dateadd(d,0,datediff(d,0,getdate()));

/* SQL for Report (dsDetail) */
select
    lp.period_id ,
    lp.building_id ,
    lp.suite_id ,
    lp.lease_id ,
    lp.tenant_trading_name,
    lp.suite_status ,
    lp.suite_name ,
    lp.suite_area,
    lp.lease_status ,
    lp.lease_current_start_date ,
    lp.lease_current_stop_date ,
    lp.lease_current_term ,
    lp.lease_occupancy_date ,
    lp.lease_vacated_date
from
    property.lease_period lp
where
    lp.lease_current_stop_date < @truncateddate
    and (upper(lp.lease_status) = 'ACTIVE' or upper(lp.lease_status) = 'OVERHOLDING')
    --and lp.period_id = @period_id -- ##Potentially redundant (query now running into the future)
    and lp.building_id = @building_id
    and not exists
    (
        select  1
        from    lease_deal.lease l
        where   lp.building_id = l.building_id
                and lp.lease_id = l.lease_id
    )
    and (@section_name = 'ALL'
        or (@section_name <> 'ALL' and upper(section_name) = upper(@section_name)))
    and lp.lease_current_stop_date between @truncateddate
        and dateadd(MONTH, @lease_expiry_period, @truncateddate)
order by period_id desc

What you've got is pretty readable, but there are a few points to make:

Your sub-query is written with a different indentation style from the main query.
I prefer to see the keywords in upper-case.
Personally, I dislike spaces before commas intensely (and you aren't 100% consistent about adding them).  However, I loathe even more the style where the comma is put at the start of the next line:

     item1
   , item2
   , item3

I'd use explicit AS to introduce table aliases.  At least on the DBMS I use mainly, using AS gives a degree of future-proofing against aliases that become keywords in a later version of the product.  That is, writing FROM SomeTable st runs into a problem later if st becomes a keyword, but writing FROM SomeTable AS st avoids that problem.  Whether this helps in other DBMS is open to debate.

For your query, I'd probably write:

SELECT lp.period_id,
       lp.building_id,
       lp.suite_id,
       lp.lease_id,
       lp.tenant_trading_name,
       lp.suite_status,
       lp.suite_name,
       lp.suite_area,
       lp.lease_status,
       lp.lease_current_start_date,
       lp.lease_current_stop_date,
       lp.lease_current_term,
       lp.lease_occupancy_date,
       lp.lease_vacated_date
  FROM property.lease_period AS lp
 WHERE lp.lease_current_stop_date < @truncateddate
   AND (UPPER(lp.lease_status) = 'ACTIVE' OR UPPER(lp.lease_status) = 'OVERHOLDING')
   AND lp.building_id = @building_id
   AND NOT EXISTS
       (SELECT *
          FROM lease_deal.lease AS l
         WHERE lp.building_id = l.building_id
           AND lp.lease_id = l.lease_id
       )
   AND (@section_name = 'ALL'
        OR (@section_name <> 'ALL' AND UPPER(section_name) = UPPER(@section_name)))
   AND lp.lease_current_stop_date BETWEEN @truncateddate AND
                        DATEADD(MONTH, @lease_expiry_period, @truncateddate)
 ORDER BY period_id DESC;

Sometimes, I'd indent the sub-query more than I did here, occasionally even placing it after the EXISTS:

   AND NOT EXISTS (SELECT *
                     FROM lease_deal.lease AS l
                    WHERE lp.building_id = l.building_id
                      AND lp.lease_id = l.lease_id
                  )

That has a tendency to run off the right-hand edge of the screen, though.


jQuery AJAX form using bad practice I have not worked with jQuery for years so my knowledge of it is extremely limited. I wrote the following form and received a comment elsewhere saying:

  This is some of the worst JavaScript I've seen. Your selectors are
  terrible ($('div#foo') instead of $('#foo')), you copy-paste code all
  over the place, you return false instead of preventing the default
  action alone, you manually concatenate your data string instead of
  using the JSON library, you compare things with "==" instead of
  "===".... This is just awful style and being a novice.

Most of the notes included in that comment doesn't make much sense to me, I haven't had my mind inside the insanity of Javascript for years now.

My question is, how do I upgrade my code to actually use good practice and up-to-date technique?

HTML:

<form name="contact" action="">
  <fieldset>
    Name
    <input type="text" name="name" id="name" size="30" value="" class="text-input" />
    This field is required.

    Return Email
    <input type="text" name="email" id="email" size="30" value="" class="text-input" />
    This field is required.

    Return Phone
    <input type="text" name="phone" id="phone" size="30" value="" class="text-input" />
    This field is required.

    <input type="submit" name="submit" class="button" id="submit_btn" value="Send" />
  </fieldset>
</form>

Form validation:

  $(function() {
    $('.error').hide();
    $(".button").click(function() {
      // validate and process form here

      $('.error').hide();
      var name = $("input#name").val();
        if (name == "") {
        $("label#name_error").show();
        $("input#name").focus();
        return false;
      }
        var email = $("input#email").val();
        if (email == "") {
        $("label#email_error").show();
        $("input#email").focus();
        return false;
      }
        var phone = $("input#phone").val();
        if (phone == "") {
        $("label#phone_error").show();
        $("input#phone").focus();
        return false;
      }

    });
  });

Process form:

  var dataString = 'name='+ name + '&email=' + email + '&phone=' + phone;
  //alert (dataString);return false;
  $.ajax({
    type: "POST",
    url: "bin/process.php",
    data: dataString,
    success: function() {
      $('#contact_form').html("

");
      $('#message').html("
Contact Form Submitted!
")
      .append("
We will be in touch soon.
")
      .hide()
      .fadeIn(1500, function() {
        $('#message').append("");
      });
    }
  });
  return false;

In a sense, your code has already been reviewed, though I disagree with the excessively negative assessment.  It is far from being awful and some of the worst JavaScript code ever.

you manually concatenate your data string instead of using the JSON library

As the documentation for $.ajax() says:

  The data option can contain either a query string of the form key1=value1&key2=value2, or an object of the form {key1: 'value1', key2: 'value2'}. If the latter form is used, the data is converted into a query string using jQuery.param() before it is sent.

To create the query string from the form, you could take advantage of the .serialize() function.

So, that's what you should use instead.  Concatenating your own data is wrong because you are failing to escape special characters.  If any of the fields contains a =, &, or % character, the application will break — possibly in a way that introduces a security problem.
Your selectors are terrible ($('div#foo') instead of $('#foo'))

From the jQuery Learning Center:

  Beginning your selector with an ID is always best. … ID-only selections are handled using document.getElementById(), which is extremely fast because it is native to the browser.

So do that.
you compare things with == instead of ===

The == operator does type conversion.  In this case, all of your comparisons are between strings anyway.  Using === would be better practice, but use of == in this code is not that harmful.
you return false instead of preventing the default action alone

Your version works, but event.preventDefault() is clearer.
copy-paste code all over the place

"All over the place" is an exaggeration.  However, it would be better to extract repeated code into a function.

$('.button').click(function(event) {
    function checkRequiredFieldFilled(fieldSelector, errorSelector) {
        if ('' === $(fieldSelector).val()) {
            $(errorSelector).show();
            $(fieldSelector).focus();
            return false;
        }
        return true;
    }

    $('.error').hide();
    var ok = checkRequiredFieldFilled('#phone', '#phone_error') &
             checkRequiredFieldFilled('#email', '#email_error') &
             checkRequiredFieldFilled('#name',  '#name_error');
    if (!ok) event.preventDefault();
}

Note that this version validates all of the fields at once and focuses on the first field with a problem, to provide a better user experience.


Program to find megaPrime numbers between 2 given integers A number is said to be megaPrime if it is prime, and all its digits are also prime.

Example:
53 is megaPrime, but 35 is not as it is divisible by 5 and 7.
13 is not a megaPrime number as it contains 1, which is non-prime.
INPUT
Two integer numbers, first and last are entered.
OUTPUT
Display the number of megaPrimes in the inteval between first and last, both inclusive.
CONSTRAINTS
1 <= first <= last <= 10^15
last - first <=  10^9
/* PROGRAM TO FIND MEGA PRIME NUMBERS BETWEEN TWO GIVEN INTEGERS */

#include<stdio.h>
int isPrime(int);
int isMegaPrime(int);

int main(void)
{
int i=0,st,sp,count=0;
    scanf("%d%d",&st,&sp);
    for(i=st;i<=sp;i++){
        if((i%2)!=0 && isMegaPrime(i)==1)
            count++;
    }

    printf("%d",count);
    return 0;
}

int isPrime(int n)
{
    int i=0,flag=0;
    if(n==1)
        return 0;
    else
    {
        int t=sqrt(n);
        for(i=2;i<=t;i++){
            if((n%i)==0){
                flag=1;
                break;
            }
        }
    }
    if(flag==1)
        return 0;
    else
        return 1;
}

int isMegaPrime( int n)
{
    int i=0,flag=0,temp=0;
    if(isPrime(n)==0)
        return 0;
    else{
        while (n!=0){
            temp=n%10;
            flag=isPrime(temp);
            if(flag==0)
                return 0;
            n/=10;

        }
    }
    if(flag==1)
        return 1;
    else
        return 0;
}

Please suggest the possible optimizations.

I think John Coleman did a good job explaining speeding up the testing for mega-primality, but you're still using trial division to test for primality. The Miller-Rabin test is much faster, and can be made deterministic given the size of your range.

The algorithm starts by checking if \$n\$ is 1 or even. These cases are trivial, and should return 0 unless \$n = 2\$.

Next we take n-1 and remove the largest power of 2. Put another way we want to find s and d such that \$2^sd = n - 1\$ and d is odd. For example, if \$n = 49\$, \$n - 1 = 2^43\$, so \$s = 4\$ and \$d = 3\$.

Next we pick a random number for \$a\$ such that \$1 \lt a \lt n - 1\$. Now we make a loop on \$r\$ where \$r\$ goes from 0 to \$s - 1\$ inclusively. If for all values of r:

\$a^{2^rd} \% n \ne n - 1\$ and \$a^d\%n\ne1 % n != 1\$, then \$n\$ is composite. Otherwise, \$n\$ has a large probability of being prime. If you test every \$a\$ where \$a = {2, 3, 5, 7, 11, 13, 17, 19, 23}\$ and \$n\$ comes up as a likely prime for every time, it is prime.

If you are unfamiliar with this you may want to do \$a^b \% c\$ one step at a time, but computing \$a^b\$ may take a long time, and will quite likely result in an overflow error.

Use this trick:

\$a^b \%c =\$
If \$b = 0\$ then 1
If \$b\$ is odd then \$(a^{b-1} \% c ) b \% c\$
If \$b\$ is even then \$(a^{\frac{b}{2}} \% c)^2 \% c\$

This way, the number is constantly being reduced, and as long as recursion is used, the time will scale logarithmically with the size of \$b\$.


LeetCode 01: Matrix challenge Recently, I've solved the "01 Matrix" LeetCode problem and the solution was accepted by the LeetCode OJ:

  Given a matrix consists of 0 and 1, find the distance of the nearest 0
  for each cell.

  The distance between two adjacent cells is 1.

  Example:

Input:
0 0 0
0 1 0
1 1 1

Output:
0 0 0
0 1 0
1 2 1

  Note:

  The number of elements of the given matrix will not exceed 10,000.
  There are at least one 0 in the given matrix.
  The cells are
  adjacent in only four directions: up, down, left and right.

The idea behind the solution above is to use Dynamic Programming - starting with 0 cells work outwards putting not processed cells on the queue:

from collections import deque

class Solution(object):
    def updateMatrix(self, matrix):
        """
        :type matrix: List[List[int]]
        :rtype: List[List[int]]
        """
        if not matrix:
            return matrix

        row_length = len(matrix)
        col_length = len(matrix[0])

        queue = deque()

        # put all 0 cells on queue, set all other cells to a big number
        for row_index in range(row_length):
            for col_index in range(col_length):
                if matrix[row_index][col_index] == 0:
                    queue.append((row_index, col_index))
                else:
                    matrix[row_index][col_index] = 10000

        # work from the 0 cells outwards while the queue is not empty
        while queue:
            row_index, col_index = queue.popleft()
            for i, j in [(row_index - 1, col_index),
                         (row_index + 1, col_index),
                         (row_index, col_index - 1),
                         (row_index, col_index + 1)]:
                if 0 <= i < row_length and \
                   0 <= j < col_length and \
                   matrix[i][j] > matrix[row_index][col_index] + 1:
                        matrix[i][j] = matrix[row_index][col_index] + 1
                        queue.append((i, j))

        return matrix

Even though the code works, I am not happy with the readability, in particular:

setting the non-zero cells to "magical" 10000 does not look good
getting the cell neighbors and checking if they are not out-of-bounds seems overly complicated

What would you improve code style and organization or time and space complexity wise?

By storing the return value in a different variable and placing the distance in the queue, the magic number can be avoided:

class Solution(object):
    def updateMatrix(self, matrix):
        """
        :type matrix: List[List[int]]
        :rtype: List[List[int]]
        """
        if not matrix:
            return matrix

        row_length = len(matrix)
        col_length = len(matrix[0])

        result = [[None for j in range(col_length)] for i in range(row_length)]

        queue = deque()

        # put all 0 cells in queue, set all other cells to a big number
        for row_index in range(row_length):
            for col_index in range(col_length):
                if 0 == matrix[row_index][col_index]:
                    queue.append((row_index, col_index, 0))

        # work from the 0 cells outwards while the queue is not empty
        while queue:
            i, j, dist = queue.popleft()
            if 0 <= i < row_length and 0 <= j < col_length and result[i][j] is None:
                result[i][j] = dist
                queue.append((i-1, j, dist+1))
                queue.append((i+1, j, dist+1))
                queue.append((i, j-1, dist+1))
                queue.append((i, j+1, dist+1))

        return result


“Real world” JSON transformation using Node.js Background

I transformed a non-trivial JSON response that uses arrays into

a nested object
eliminating some of the original properties
inserting a new summary property that is a unique list of values from the original object

I did this with temporary variables, foreach loops and some conditional logic.  My first working code, sample input / output JSON are below.

Question / Concern

While this works, I'm concerned about the "smell" of the code and the maintainability.  Is there a way to do this that would be more maintainable (perhaps with Array. ..., map, filter, reduce) ?  I'm not looking for a working solution but more than "one liners".

CODE

const raw = require('../tests/data/sample-input.json');

const types = new Set();

function parseLoc(loc) {
  // constant properties with loc_id the name of the object
  let o = { [loc.loc_id]: { last_update: loc.last_update, type_id: loc.type_id } };
  types.add(loc.type_id);
  // optional property
  if (loc.contents) {
    o = { [loc.loc_id]: { ...o[loc.loc_id], contents: loc.contents } };
    loc.contents.forEach((item) => {
      types.add(item.type_id); // can have nested types
    });
  }
  // optional property
  if (loc.plan_id) {
    o = { [loc.loc_id]: { ...o[loc.loc_id], plan_id: loc.plan_id } };
  }
  // summary properties
  o = {
    [loc.loc_id]: {
      ...o[loc.loc_id],
    },
  };
  return o;
}

const driver = () => {
  const { locs } = raw[Object.keys(raw)[0]];

  let out = {};
  locs.forEach((loc) => {
    const t = parseLoc(loc);
    out = { ...out, [Object.keys(t)[0]]: t[Object.keys(t)[0]] };
  });
  console.log('types', ...types);
  out = { ...out, Types: [...types] };
  console.log('out', JSON.stringify(out));
};

driver();

Input

   {
  "Input": {
    "locs": [
      {
        "contents": [{ "amount": 1, "type_id": 2393 }],
        "last_update": "2013-09-22T21:53:51Z",
        "obsolete1": 1.52384062551,
        "obsolete2": 1.56361060962,
        "loc_id": 1011160470678,
        "type_id": 2524
      },
      {
        "last_update": "2019-07-29T10:56:27Z",
        "obsolete1": 1.60921860432,
        "obsolete2": 1.60545414964,
        "loc_id": 1028580652821,
        "plan_id": 97,
        "type_id": 2474
      },
      {
        "contents": [
          { "amount": 560, "type_id": 2393 },
          { "amount": 560, "type_id": 9838 },
          { "amount": 560, "type_id": 2317 },
        ],
        "last_update": "2019-02-28T22:09:51Z",
        "obsolete1": 1.55924075537,
        "obsolete2": 1.58171860958,
        "loc_id": 1029669382563,
        "type_id": 2544
      }
    ]
  }
}

OUTPUT

{
  "1011160470678": {
    "last_update": "2013-09-22T21:53:51Z",
    "type_id": 2524,
    "contents": [{ "amount": 1, "type_id": 2393 }]
  },
  "1028580652821": {
    "last_update": "2019-07-29T10:56:27Z",
    "type_id": 2474,
    "plan_id": 97
  },
  "1029669382563": {
    "last_update": "2019-02-28T22:09:51Z",
    "type_id": 2544,
    "contents": [
      { "amount": 560, "type_id": 2393 },
      { "amount": 560, "type_id": 9838 },
      { "amount": 560, "type_id": 2317 }
    ]
  },
  "Types": [2524, 2393, 2474, 2544, 9838, 2317]
}

Maintenance by design
There are many times when I see this type of question regarding maintainability of the code. The line between code maintenance and user friendly becomes blurred. This is most evident when the coder is also the end user, the interface UI is misunderstood by familiarity of use, it is of course the IDE. This may sound somewhat pedantic, but the mind set it helps introduce can go a long way into how you design your code.
Encapsulate the interface
In this example if we look at the task of maintenance as an end user task, we design the code with all the possible changes encapsulated in a simple structure that isolates the code from the user. (Users can't code so don't let them near it, especially those users than think they can code (I am my most difficult client))
If done well the maintenance is trivial and the meat of the logic safe from the corruption inevitable when the logic must be reacquired in some maybe distant future.
So we can create am object called rules. It is the interface (abstract representation of codes functionality), and as with all good UI it will require some help information in the form of comments (remember the interface is the IDE)
As I have little to no clue what the possible alterations may be I have had to guess, this interface may not be adaptable to your needs. It is an example only
const rules = {
    name: "Outputs",     // name of transformed object
    typesName: "types",  // name of type ids array eg {Outputs: types: [1,4,23] }
    itemsName: "locs",   // name of items object in and out  eg {Outputs: locs: {} }
    keep: ["contents", "last_update", "plan_id", "type_id"],  // List of properties
                                                              // to keep per item
    itemName: "loc_id",  // name of property to use as named item
    ids: ["type_id"],    // list of property names containing ids for the array of types
};

The code can then use this encapsulated information as input, doing its thing and spitting out the result. Many changes can be made by just changing some text and never going near the logic.
function transform(data, rules) {
    const items = {}, types = new Set();
    const addTypes = from => rules.ids.forEach(key =>
         from[key] !== undefined && (types.add(from[key]));
    const addProps = from => rules.keep.reduce((obj, key) =>
         from[key] !== undefined ? (obj[key] = from[key], obj) : obj, {});
    const transformLoc = loc => {
        const lName = loc[rules.itemName];
        if (lName !== undefined) {
            items[lName] = addProps(loc);
            if (loc.contents) { loc.contents.forEach(addTypes) }
            addTypes(loc);
        }
    }
    data[rules.itemsName].forEach(transformLoc);
    return {
        [rules.name]: {
            [rules.typesName]: [...types],
            [rules.itemsName]: items
        }
    };
}

const transformed = transform(data.Input, rules);

Even if we ignore this UI/code abstraction, the resulting code is more maintainable as you have moved many of the magic constants out of the code, and have been forced to think of what you are doing in a higher level abstract, that helps any future coders wrap their thinking nog around the logic.


Notify owner of post and other commenters I have a rails4 app. When a user comments on a post it should send a notification to all the guys who commented on the post and to the post creator. I have a working method in the controller, but it's kinda ugly and I'd like to refactor it.

Could somebody tell me what the rails convention for refactoring this?

Comment belongs_to :post, post has_many :comments

create#comment action

    if @post_comment.save
      ((@post.users + [@post.user]).uniq - [current_user]).each do |post_commenter|
        Notification.create(recipient_id: post_commenter.id, sender_id: current_user.id, notifiable: @post_comment.post, action: "commented")
    end
    ....
end

First of all I would add a method to Post:

# app/models/post.rb
def notify_others_about_new_comment(comment)
  recipients = [user]               # the author of the original post
  recipients << users               # people that wrote a comment
  recipients.delete(comment.user)   # writer of the new comment

  recipients.uniq.each do |recipient|
    Notification.create(
      recipient_id: recipient.id,
      sender_id: comment.user.id,
      notifiable: self,
      action: 'commented'
    )
  end
end

And just call that method in your controller:

if @post_comment.save
  @post.notify_others_about_new_comment(@post_comment)
  #...
end


COMpatible+ Google+ Integrations+ Scoping+ So I have part of a library which deals with scopes in Google+ integration, and I'm curious on the overall view of it.

Yes, things are stringly typed in one of the methods, that is because this has to be COMpatible, which does not support enum's.

I have (purposefully) omitted XML documentation, as most people think I go overboard and I'd rather not bloat.

public bool COM_GetLoginStatus(ref GoogleIdentity identity, WebBrowserSettings settings, bool force, string scopes = "")
{
    GooglePermissionScopes googlePermissionScopes = GooglePermissionScopes.None;

    scopes = scopes.ToLower();

    if (scopes == "")
    {
        scopes = "all";
    }

    foreach (string scope in scopes.Split(','))
    {
        switch (scope)
        {
            case "openid":
                googlePermissionScopes |= GooglePermissionScopes.OpenID;
                break;
            case "email":
                googlePermissionScopes |= GooglePermissionScopes.Email;
                break;
            case "plus.login":
            case "pluslogin":
                googlePermissionScopes |= GooglePermissionScopes.PlusLogin;
                break;
            case "all":
                googlePermissionScopes |= GooglePermissionScopes.All;
                break;
        }
    }

    return GetLoginStatus(ref identity, settings, force, googlePermissionScopes) == LoginStatus.Success;
}

The next method is responsible for building an endpoint to contact the API at.

public string GetLoginUrl(GooglePermissionScopes googlePermissionScopes = GooglePermissionScopes.None, bool force = false, string state = "")
{
    string endpoint = "https://accounts.google.com/o/oauth2/auth?";

    if (googlePermissionScopes != GooglePermissionScopes.None)
    {
        endpoint += "scope=";

        if ((googlePermissionScopes & GooglePermissionScopes.OpenID) == GooglePermissionScopes.OpenID)
        {
            endpoint += "openid";
        }

        if ((googlePermissionScopes & GooglePermissionScopes.Email) == GooglePermissionScopes.Email)
        {
            if (endpoint.Length > 6)
            {
                endpoint += "%20";
            }

            endpoint += "email";
        }

        if ((googlePermissionScopes & GooglePermissionScopes.PlusLogin) == GooglePermissionScopes.PlusLogin)
        {
            if (endpoint.Length > 6)
            {
                endpoint += "%20";
            }

            endpoint += "https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fplus.login";
        }
    }

    if (state != "")
    {
        endpoint = endpoint + "&state=" + state;
    }

    endpoint = endpoint + "&redirect_uri=" + CallbackUrl + "&response_type=code" + "&client_id=" + ClientId + "&access_type=offline";

    if (force)
    {
        endpoint += "&approval_prompt=force";
    }

    return endpoint;
}

I'm not sure if I should be using a StringBuilder here or not, since it's not a looped string concatenation.

In COM_GetLoginStatus,
you can get rid of the if (scopes == "") by incorporating it in case "all" in the switch:

        case "":
        case "all":
            googlePermissionScopes |= GooglePermissionScopes.All;
            break;

This is probably good to move to a constant declared at the top of the file where it's easy to see:

string endpoint = "https://accounts.google.com/o/oauth2/auth?";

If the values in GooglePermissionScopes have distinct bits,
then instead of this:

    if ((googlePermissionScopes & GooglePermissionScopes.OpenID) == GooglePermissionScopes.OpenID)

You could write simpler like this:

    if ((googlePermissionScopes & GooglePermissionScopes.OpenID) > 0)

But then again, I'm not familiar with GooglePermissionScopes so I don't know if this is correct, or acceptable.

It's not clear what magic number 6 is about here:

        if (endpoint.Length > 6)
        {
            endpoint += "%20";
        }

It would be good to make that clear somehow, at the minimum with a comment.

As @Heslacher pointed out, endpoint is initialized to a string longer (much longer) than 6 characters, so this condition is completely pointless.

This can be written simpler using +=:

    endpoint = endpoint + "&state=" + state;

This can be similarly simplified,
and "&response_type=code" + "&client_id=" can be joined:

endpoint = endpoint + "&redirect_uri=" + CallbackUrl + "&response_type=code" + "&client_id=" + ClientId + "&access_type=offline";

As for your question:

  I'm not sure if I should be using a StringBuilder here or not, since it's not a looped string concatenation.

Since it's not in a loop, I doubt the performance difference would be measurable. I think it doesn't matter. It's fine to keep it simple.

You wrote URL-encoded strings at multiple places, for example %20 and https%3A%2F%2F.
Writing this by hand is error prone, and a PITA.
It would be better to write in human-readable format,
and call a url-encoder method at the end.
Granted, the current approach is better in terms of "performance",
but again, it's probably not measurable and not worth it.


Reversing an array in Python Reversing an array in Python without using a built-in function:

def reverse(alist):
    end = len(alist)-1
    limit = int(end/2) + 1
    for i in range(limit):
        alist[i],alist[end] = alist[end],alist[i]
        end = end - 1
    return alist

print reverse([1,2,3,4,5,6])

The first solution that comes to mind is to take advantage of the powerful list comprehension and abuse the xrange function:

def reverse(some_list):
    return [some_list[n] for n in xrange(len(some_list) - 1, -1, -1)]

The above is nothing more than the usual reversed for loop that you may find in other languages (like C / C++) as:

for (int i = array.length - 1; i >= 0; i--)
{
    // Do something ...
}

Details about the range function can be found here but basically, all you have to know about it is its prototype:

  range(start, stop[, step])

which is very descriptive.

I'd go with this instead of your solution because it looks cleaner and I don't see any reasons of why it'd be slower or harder to understand.


Helper class for Null and Empty checks I have some static helper class:

public static class Helper
{
    public static bool IsNull<T>(this T value) where T : class
    {
        return (value == null);
    }

    public static bool NotNull<T>(this T value) where T : class
    {
        return (!value.IsNull<T>());
    }

    public static bool IsNullOrEmpty(this string value)
    {
        return string.IsNullOrEmpty(value);
    }

    public static bool NotNullAndNotEmpty(this string value)
    {
        return (!value.IsNullOrEmpty());
    }

    public static bool NotNullAndNotEmpty(this ICollection value)
    {
        return (value.NotNull<ICollection>() && (value.Count > 0));
    }

    public static bool IsNullOrEmpty(this ICollection value)
    {
        return (!value.NotNullAndNotEmpty());
    }
}

It is comfortable (habit) for me to use this helper, but I am not sure that it is correctly (worth it). How about performance? Are there any other reasons to not use it?

This gets to be a sticky point.  There are those who will argue fervently that an extension method should never work on a null reference, and there are those who are fine with that.

Personally, I don't mind extension methods on null if the name is a clear indicator that null is a possibility (like IsNullOrEmpty) but as I said, that's my opinion and may not match others.

Don't worry about the performance, method calls are not a serious impact and check methods like this eliminate some of the readability concerns over compound expressions when maintaining the code.  After all, that's what string.IsNullOrEmpty() does, but it does it through a static method instead of an extension method.

So the only concern, really, you may get push back that these are extension methods as opposed to ordinary static methods, but you'll get differing opinions on whether calling an extension method on null is bad/okay.

One could argue Microsoft disallows LINQ extension methods on null sequences, but then again Microsoft also doesn't disallow extension method calls on a null source in general.

So, the choice is yours, I don't have an issue with them since the names clearly indicate null is an accepted possibility, but I can see both sides of the argument.

On a side note, I'd keep your naming consistent:

NotNull -> IsNotNull
NotNullAndNotEmpty -> IsNotNullAndNotEmpty


Read created files in a folder I have been working with where I use watchdogs to be able read whenever I have created a file by the name -> kill_.flag and it contains:
*kill_*.flag contains:

hello;https://www.google.se/;123

and my script is then going to read the created file, split the ";" between each other and give each a key where I later on print it out which I for now has the text "Deleting" where I will continue my script after I have completed this part.
# !/usr/bin/python3
# -*- coding: utf-8 -*-

import os
import os.path
import time

import watchdog.events
import watchdog.observers

class AutomaticCreate(watchdog.events.PatternMatchingEventHandler):

    def __init__(self):
        self.delete_flag = "*kill_*.flag"
        watchdog.events.PatternMatchingEventHandler.__init__(self, patterns=[os.path.split(self.delete_flag)[1]],
                                                             case_sensitive=False)

    def on_created(self, event):
        while True:
            try:
                with open(event.src_path) as f:
                    content = f.read().split(";")
                    payload = {
                        "name": content[0].strip(),
                        "link": content[1].strip(),
                        "id": content[2].strip()
                    }
                break
            except OSError:
                print(f"Waiting for file to transfer ->  {event.src_path}")
                time.sleep(1)
                continue

        while True:
            try:
                os.remove(event.src_path)
                break
            except Exception as err:
                print(f"Trying to remove file -> {err}")
                time.sleep(1)
                continue

        print(f'Deleting: {payload["name"]} - {payload["link"]} - {payload["id"]}')

    def main(self):
        observer = watchdog.observers.Observer()
        observer.schedule(AutomaticCreate(), path=os.path.split(self.delete_flag)[0], recursive=True)
        observer.start()
        try:
            while True:
                time.sleep(1)
        except KeyboardInterrupt:
            observer.stop()
        observer.join()

if __name__ == "__main__":
    AutomaticCreate().main()

I wonder if there is a smarter or maybe even cleaner way to improve this code both performance and whatever it can be?

I genuinely hope you don't take offence by this, but this implementation is so insane that it's frankly impressive.
Since you have delete_flag = "*kill_*.flag", calling os.path.split on it is pointless. Perhaps you're substituting that with something else that has a path, but if you are, why isn't it parametrized?
Do a tuple-unpack here:
                    content = f.read().split(";")
                    payload = {
                        "name": content[0].strip(),
                        "link": content[1].strip(),
                        "id": content[2].strip()
                    }

should just be
name, link, id = (part.strip() for part in f.read().split(';'))

Putting these in a payload dict is actively harmful. You only ever use these as variables later. About the most generous I can be here is to guess that there's actually more code in your application that you haven't shown which relies on the payload for some other network or serialized file operation, but if that's the case the whole question is invalid.
AutomaticCreate implements PatternMatchingEventHandler - so then why are you reinstantiating it here?
        observer.schedule(AutomaticCreate()

You already instantiated it here:
AutomaticCreate().main()

Keep the second instantiation, and replace the first instantiation with a reference to self.
The following is a strange and non-standard way of calling a base constructor:
    watchdog.events.PatternMatchingEventHandler.__init__(self, patterns=[os.path.split(self.delete_flag)[1]],  # ...

Just use super().
Rework your class to be a context manager, where __enter__ calls self.observer.start(), and __exit__ calls self.observer.stop(). Then, use your class instance in a with at the top level and delete your main() method.


How to improve this jQuery code for tabs I'm quite new to javaScript and jQuery and wrote the following function to creat some dynamic tabs. I'd be very interested to know how professional programmers would improve this code.

jsFiddle

JavaScript

$(document).ready(function(){

function createVerticalTabs(options){

    var container = options["container"];
    var parentElement = options["parentElement"];
    var childElement = options["childElement"];
    var firstElement = options["firstElement"];
    var tabClass = options["tabClass"];
    var tabClassActive = options["tabClassActive"];
    var closeOtherElements = options["closeOtherElements"]
    var preventEvent = options["preventEvent"];

    if(firstElement){
        $(container+" "+childElement).not(":first").hide();
        classToggle($(container+" "+childElement+":first").prev(parentElement), tabClass, tabClassActive);
    }else{
        $(container+" "+childElement).hide();
    }

    /* Click event */
    $(container+" > "+parentElement).on("click", function(event){
        if(preventEvent == true){
            event.preventDefault();
        }

        /* Check if there're other active elements and close them if requested*/

        if(closeOtherElements){
            var otherActiveElement = $(this).parent().children(childElement);

            if($(otherActiveElement).is(":visible")){
                $(otherActiveElement).slideUp("slow");
                classToggle($(otherActiveElement).prev(parentElement), tabClassActive, tabClass);
            }
        }

        /* Show requested element */
        var details = $(this).next(childElement);

        if($(details).is(":visible")){
            classToggle($(this), tabClassActive, tabClass);
            $(details).slideUp("slow");
        }else{
            classToggle($(this), tabClass, tabClassActive);
            $(details).slideDown("slow");
        }

    });

    function classToggle(obj, remove, add){
        return obj.removeClass(remove).addClass(add);
    }

}

var options = {
    container: ".testBox",
    parentElement: "a",
    childElement: "p",
    tabClass: "title",
    tabClassActive: "active",
    firstElement: true,
    closeOtherElements: true,
    preventEvent: true
};

var options2 = {
    container: ".testBox2",
    parentElement: "h3",
    childElement: "p",
    tabClass: "title",
    tabClassActive: "active",
    firstElement: false,
    closeOtherElements: false,
    preventEvent: true
};

createVerticalTabs(options);
createVerticalTabs(options2);
});

HTML

    Anchor 1
Details of Anchor 1

    Anchor 2
Details of Anchor 2

    Anchor 3
Details of Anchor 3

    Anchor 4
Details of Anchor 4

Anchor 1

Details of Anchor 1

Anchor 2

Details of Anchor 2

Anchor 3

Details of Anchor 3

Anchor 4

Details of Anchor 4

CSS

.testBox, .testBox2{ float:left; height:500px; margin:10px; }
.testBox a{ display:block; }

.testBox2 h3{ margin:0; padding:0; }

.title{ font-weight:bold; }
.active{ background:#333; border:#000 1px solid; color:#fff; }

I'm going to go through a few main points here about your code but don't be overwhelmed by the size of this book, I prefer to be on the side of too much information than too little.

- Cache your selectors: Probably the most important thing you can do for your code now. As a rule of thumb, if you use a selection more than once, you should cache it. What happens when you use $("someElem") is jQuery queries the DOM to try and find elements that match. So imagine that every time you do that it runs a search. Would it be better if you could save the search results? This way you can look and play with them whenever you want without having to look for them again.  Ex.:

//Something like this:

var childFound = $(container).find(childElement);
/* Here I saved my search to a variable, which I use later.
This will return all "p" elements in "container".
You can filter more and get first element or all the others
The .find() method is just an example, there are countless others you can use
to help you select the elements you want
Methods like .map(), .each(), and .nextAll() will be increadibly useful for you */

//Then use like this
childFound.hide();

You're on the right track by setting up the options variable. This concept will be vital when you start to develop plugins, if that's what you want to do.

- IIFE's: If you have your code in the footer (which you should be doing) what you'll want to do is wrap it in a IIFE. "What the hell is that?" you may ask. A basic syntax looks like this:

(function () {
    //Code goes in here.
})();

You'll see several variations of this all over the internet as you read to find out more. The one I recommend you use in this case is like this:

;(function ($, window, document) {
    //Code goes here and the $ belongs to jQuery
})(jQuery, window, document);

So lets break this down. The ; in the front is a safety net against unclosed scripts, which can be common if you use plugins in Wordpress, or if you concat and minify your files it will protect you from your function becoming an argument.

Then we pass in $, window, and document. We pass in $ and at the bottom assign it to jQuery. So now in this function, no matter what value the $ carried outside, in here it's jQuery. Then we pass in window and document. These are optional in your case, but a good idea since you do use references to window in your code. It saves window as a local variable, and also will be good when you minify your code as the window reference can be changed to something like a or foo automatically. If you don't use a window or document reference you can just remove them all together.

Now keep in mind there might be times where you have to put your script in the header. Modernizr is an example of this. Then you'll probably end up using the document.ready. Don't sweat because all you have to do is this:

jQuery(document).ready(function( $ ) {
    //Code goes here and $ will belong to jQuery.
});

I wouldn't rely on $.noConflict to protect your code from other libraries that use the $ as well.

- Click Event Handler: As was mentioned before, there are several ways to set up your events. The ones that were mentioned were .click() and .on("click"). The .click() method simply calls the .on() method and passes in the click. The .on() method is incredibly useful since you can use it to set up almost any kind of event - not only clicks.  So yes, the one you are using now is "the best" because it saves you a function call. Now saving a single function call in your app won't be a significant increase in performance and you probably won't even notice it. Although arguing over such a small and possibly insignificant changes is really what we developers do best.

Using the .on() method directly is hands down going to be faster, but there are many other things you could be spending your time on that will generate more significant performance results.

- Don't re-invent the wheel: I strongly believe that if there's a working solution out there by all means use it. There are tons of tabs and accordion plugins available I'm sure you can find one that fits your project. If you can't find an exact fit, you can still always tweak it and make it fit. jQuery it self has an entire UI package with tabs, accordions, sliders, and all kinds of cool stuff just ready and waiting to be used. Also the jQuery UI will let you use your own CSS files and customizations really easily. Making a whole new app from scratch should be done if absolutely necessary, or if you're trying to learn (which is your case). Back to the subject of learning, like you said, you are just starting out with jQuery, and with that I highly recommend this screencast by Jeffrey Way called 30 Days to Learn jQuery. He does a really good job of explain some basic principals as well as some more complex concepts.

One thing in specific I'm going to point out is the following:

var details = $(this).next(childElement); //Details here is a jQuery object
//You don't need to wrap it again, just use details.someMethod();
if(details.is(":visible")){
    classToggle($(this), tabClassActive, tabClass);
    details.slideUp("slow");
}else{
    classToggle($(this), tabClass, tabClassActive);
    details.slideDown("slow");
}

When you cache a selection like you did there, the .next() gives you a new jQuery object with the results from the search which then are stored in details. So doing this: $(details) is the same as doing $($("p")). You're wrapping it twice. This is a common mistake with beginners and Jeffery, in that screencast, explains it much better than I ever could.


How to properly detect multiple devices failure? My software has multiple devices connected before starting. I must be sure these devices are working and are well connected. every device has different error ID code so we can see every component that is not working.

When the software will have completly test connection for every devices, we would like to show a message that will show every failing devices:

  Unable to connect on one or multiple devices [Error Code: XXXXXXX].
  Please look at the user manual at www.xxxx.com/en/UserManual and if
  you need further information, contact customer support or mail at
  xxxxx@example.com.

For the moment, i am using bitmask but i feel like it's not very "generic" and it kinda hard-coded.

We have made an ID for every class and put a base 2 number like this:

enum eErrorCode
{
    MOTORA    = 0x0000001,
    MOTORB    = 0x0000010,
    SERIAL    = 0x0000100,
    LAN       = 0x0001000,
    MAINBOARD = 0x0010000,
    GENERATOR = 0x0100000,
    MAX = 6
}

So when the Activating function fire, we can add every error:

Int32 ConnectToEveryDevice()
{
    Int32 Error = 0;
    foreach(IDevices device in this.Devices)
    {
         Error = Error | device.Connect();
    }
    return Error;
}

For example: If MotorA, Lan and Serial fail, The screen will popup the Hex [Error Code: 0x00001101] so the client can easily find the error in the user manual (online) and if we need to add devices we can do it without needing to change everything.  Does anyone have a better or cleaner alternative than this one?

I think your enum is taking naming conventions, making a little paper ball out of it, and throwing it out the window.

An enum is a type. As such, its name should be PascalCase. Enum members are, well, public members of a type. As such, their names should be PascalCase as well.

Consider:

enum ErrorCode
{
    MotorA    = 0x0000001,
    MotorB    = 0x0000010,
    Serial    = 0x0000100,
    Lan       = 0x0001000,
    MainBoard = 0x0010000,
    Generator = 0x0100000,
    Max = 6
}

A number of things are not clear:

Why use a hexadecimal representation of the values?
Why is 6 not represented as hex, too?
Is Max supposed to represent all 6 bit flags turned on?

  We have made an ID for every class and put a base 2 number...

No, you haven't. Your numbers are hexadecimal numbers, they're base-16.

Consider:

[Flags]
enum ErrorCode
{
    None = 0,
    MotorA = 1,     // 2^0, or 1 << 0
    MotorB = 2,     // 2^1, or 1 << 1
    Serial = 4,     // 2^2, or 1 << 2
    Lan = 8,        // 2^3, or 1 << 3
    MainBoard = 16, // 2^4, or 1 << 4
    Generator = 32, // 2^5, or 1 << 5
    Max = MotorA | MotorB | Serial | Lan | MainBoard | Generator
}

Take a look at this comment on SO:

  Flags itself does nothing. Also, C# does not require Flags per se. But the ToString implementation of your enum uses Flags, and so does Enum.IsDefined, Enum.Parse, etc. Try to remove Flags and look at the result of MyColor.Yellow | MyColor.Red; without it you get "5", with Flags you get "Yellow, Red". Some other parts of the framework also use [Flags] (e.g., XML Serialization). –  Ruben Aug 17 '09 at 17:30

This means the [Flags] attribute changes how ToString() represents an ErrorCode value. Your code breaks the type safety and treats the enum as an int, but you could do this as well:

ErrorCode ConnectToEveryDevice()
{
    var result = ErrorCode.None;
    foreach(IDevices device in this.Devices)
    {
         result = result | device.Connect();
    }
    return result;
}

Then the return value of this method could be made meaningful to a user, simply with a ToString() call:

var result = ConnectToEveryDevice();
var resultString = result.ToString();

Where resultString holds "MotorA, MainBoard, Generator" if MotorA, MainBoard and Generator are in an error state.


Checking for circular primes - Project Euler Problem 35 I have been trying to solve Project Euler problem number 35. The problem is to find:

  How many circular primes are there below one million?

A circular prime is a prime where all the rotations of its digits are primes too. E.g. 197 is a circular prime as 197, 971, and 719 are all primes too.

Initially I was "brute-forcing" it by checking for divisors of the number up to its square root. However, I've realised how inefficient this was when the browser crashed above 10,000. Then I've tried using the Sieve of Eratosthenes, and now it can calculate the primes up to 1 million in ~4 seconds.

However, the part that is still to inefficient is the function to check if the primes are circular. I have made a few changes, but the browser still crashes above 100,000. What extra changes can I make?

//Erastosthenes sieve
function findPrimes(range) {
    var numbers = [1];
    var prime = 2;
    var primesArr = [2]

    //Stop searching once prime is as big as range
    while(prime < range) {
        //Fill var numbers all mulitipes of prime
        for(var i = 1; i <= range && prime * i <= range ; ++i) {
            numbers[prime * i - 1] = prime * i;
        }
        //Move onto the next prime
        while(numbers[prime-1] !== undefined) {
            ++prime;
        }
        //Add current prime to the array
        primesArr.push(prime);
    }
    //Delete the last prime which is bigger than range
    primesArr.pop();
    return primesArr;

}

//Find primes within range which are circular primes
function findCircularPrimes(range) {
    //Generate array of primes from findPrimes array
    var primes = findPrimes(range);
    var circularPrimes = [];
    //Loop through each of the primes
    for(var i = 0; i < primes.length; ++i) {
        //Split current prime into an array
        var numArr = primes[i].toString().split('');
        var truefalse = true;
        //Cycle through each of the digits and check if its a prime (and therefore a member of primes)
        for (var j = 0; j < numArr.length; ++j) {
            numArr.push(numArr.shift());
            //If current rotation is not a prime break the loop and 'return' false
            if (primes.indexOf(Number(numArr.join(''))) === -1) {
                truefalse = false;
                break;
            }
        }
        //If truefalse variable is true add circular prime to array
        if(truefalse) {
            circularPrimes.push(primes[i]);
        }

    }

    return circularPrimes;
}
var then = Date.now();
var primes = findCircularPrimes(100000).length
alert(primes);

Updated since reading the wikipedia definition of circular prime

There are a few things I would recommend you change in your findCircularPrimes algorithm. First, you should note the algorithm is O(d*n^2) where d is the length of the max digit. If we use a hash instead of an array to store all the primes we can stop using .indexOf() to check if a number is prime, making our algorithm O(d*n).

Another note is that javascript will represent large numbers in scientific notation. This is probably not important as your algorithm will crash the browser before it becomes an issue :)

I've re-implemented your algorithm using es5 methods reduce to create the primeHash and filter to produce the circularPrime list, but your old approach will work as well.

Edit stole @200_success much faster rotate function to make the comparison a bit fairer and updated the jsperf :)

var LN10 = Math.log(10);
// Rotates the least-significant base-10 digit to the front
function rotate(number) {
    return (number / 10 >> 0) +
           (number % 10) * Math.pow(10, Math.floor(Math.log(number) / LN10));
}
//Find primes within range which are circular primes
function findCircularPrimes(range) {

    var primes = findPrimes(range);

    //use a hash of primes for faster lookup
    var primeHash = primes.reduce(function(memo, prime) {
        memo[prime] = true;
        return memo;
    }, {});

    //use a hash of primes for faster lookup
    var circularPrimes = primes.filter(function(prime) {
        //check ciruclar primes
        var nextDigit = prime;
        while( (nextDigit = rotate(nextDigit)) !== prime) {
            if(! (nextDigit in primeHash)) {
                return false;
            }
        }

        return true;
    });

    return circularPrimes;
}

Performance test of your implementation vs mine (original set of tests)


Check mp4 integrity in c++ I have a big collection of MP4 files and an unstable internet connection so I use this batch file to check for the files integrity.

Having posted this primitive code and received valuable reviews, I decided to take into consideration as much as I can, especially not mixing C code unless when using Windows API  and implement the said batch file using that code as the basis.

This program works in CMD it takes a path to MP4 location as argument.

#include <iostream>
#include <string>
#include <windows.h>

#ifndef INVALID_FILE_ATTRIBUTES
#define INVALID_FILE_ATTRIBUTES ((DWORD)-1)
#endif

bool DirectoryExists(const std::string& path)
{
   DWORD present = GetFileAttributesA(path.c_str());

   if (present == INVALID_FILE_ATTRIBUTES)
          return false;

   if (present & FILE_ATTRIBUTE_DIRECTORY)
          return true;

   return false;
}

std::string replace (const std::string& path)
{
   std::string in = path;
   std::replace(in.begin(), in.end(), '\\', '/');
   return in;
}

void findVideos (std::string& fspath)
{
   while(*(fspath.rbegin()) =='/')  //to get rid of trailing '/' like a user inputs path/to/file//////\\\
       fspath.pop_back();

   size_t i=1;
   WIN32_FIND_DATA FindFileData;
   std::string destpath = fspath + std::string("/")+ std::string("*.mp4");
   std::string ffmpegPath = "ffmpeg.exe -v error -f null - -i ";

   HANDLE hFind = FindFirstFile(destpath.c_str(), &FindFileData);

   if (hFind != INVALID_HANDLE_VALUE)
   {
        do
          {
            std::string fullpath = std::string(fspath)+std::string("/")+std::string(FindFileData.cFileName);
            std::cout<<i<<"-"<<"Checking "<<fullpath<<" for errors"<<std::endl;
            std::string command = ffmpegPath +std::string("\"")+fullpath+std::string("\"");
            system(command.c_str());
            i++;
          }
        while(FindNextFile(hFind, &FindFileData));
   }
   FindClose(hFind);
}

int main(int argc, char**argv)
{
    const char* path = argv[1];

    if (path == nullptr)
    {
        std::cout<<"No path provided"<<std::endl;
        return 0;
    }
    else
        if ( DirectoryExists(path) )
           std::cout<<"Provided path is: "<<path<<std::endl;
        else
             {
             std::cout<<"Path doesn't exist"<<std::endl;
             return 0;
             }

   std::string fspath;
   fspath = replace(path);
   findVideos (fspath);
   return 0;
}

The code is working, I am open to criticism and any way to improve it especially if I work in a professional company and asked to implement such program.

EDIT: Please forgive my ignorance about indentation and other programming paradigms, I graduated a year ago and failed to get an internship or a job in the domain. I tried to go with the comments as much as I can after reading some articles about indentations.

Missing Header File

You need to include algorithm so that std::replace() can be accessed.

Error Checking

Rather than checking if argv[1] is null, it might be better to use argc in the error check. If argc is not greater than 1 than there is an error. This is what argc is meant to be used for, as well as for a loop control when there are more arguments. It might be good to give an example of the proper program call as part of the error message.

The else that follows the error check is not necessary because of the return statement.

The first return statement in main() should not return zero, it should return 1 to indicate a failure. What might be even better is to include cstdlib and use the system defined symbols EXIT_SUCCESS and EXIT_FAILURE.

It would be better to report errors to std::cerr.

int main(int argc, char** argv)
{
    if (argc <= 1)
    {
        std::cerr << "No path provided" << std::endl;
        return EXIT_FAILURE;
    }

    const char* path = argv[1];
    if (!DirectoryExists(path))
    {
        std::cerr << "Path doesn't exist" << std::endl;
        return EXIT_FAILURE;
    }

    std::cout << "Provided path is: " << path << std::endl;

    std::string fspath;
    fspath = replace(path);
    findVideos(fspath);

    return EXIT_SUCCESS;
}

Horizontal Spacing

A best practice in many programming languages is to put spaces between operators and operands, this makes the code much more readable and maintainable:

void findVideos (std::string& fspath) {
    while (*(fspath.rbegin()) == '/')
    {
        fspath.pop_back();
    }

    size_t i = 1;
    WIN32_FIND_DATA FindFileData;
    std::string destpath = fspath + std::string("/") + std::string("*.mp4");
    std::string ffmpegPath = "ffmpeg.exe -v error -f null - -i ";

    HANDLE hFind = FindFirstFile(destpath.c_str(), &FindFileData);

    if (hFind != INVALID_HANDLE_VALUE)
    {
        do
        {
            std::string fullpath = std::string(fspath)+std::string("/") + std::string(FindFileData.cFileName);
            std::cout << i << "-" << "Checking " << fullpath << " for errors" << std::endl;
            std::string command = ffmpegPath +std::string("\"") + fullpath+std::string("\"");
            system(command.c_str());
            i++;
        }
        while(FindNextFile(hFind, &FindFileData));
    }
    FindClose(hFind);
}

Prefer Modern C++ constexpr Over #define

In C++ creating a constant using constexpr is preferred over a macro because it it type safe and a macro is not type safe.

#ifndef INVALID_FILE_ATTRIBUTES
constexpr DWORD INVALID_FILE_ATTRIBUTES = ((DWORD)-1);
#endif

Indentation

It would be better not to mix spaces with tabs when indenting. Many programmers use 4 spaces for indenting, by default a tab is 8 spaces. This can lead to inconsistent indentation as demonstrated in the function bool DirectoryExists(const std::string& path) in the question. An IDE such as Visual Studio allows you to set the number of spaces used in a tab to prevent this problem, where notepad does not do this.

Replace

It's not clear that the function std::string replace (const std::string& path) since windows understand both the forward and backward slash in file specifications.


Finding binary combinations based on substitute wildcards I am given a string containing the possible characters of : 0, 1, and ?, iterating over possibilities, replacing the wildcard ? with either 0 or 1.

For example; given "1?00?101", the outcome would be: ["10000101", "10001101", "11000101", "11001101"].

My code seems to work, I tested with two wildcards and three wildcards, but is there a better way to solve this.

class MainClass
    {
        static void BinaryString(char [] word, int a, int b, int count){
            if (count == 0) {
                Console.WriteLine (word);
            } else {
                for (int i = a; i < b; i++) {
                    if (word [i].Equals ('?')) {
                        word [i] = '0';
                        count--;
                        BinaryString (word, i, word.Length - 1, count);
                        word [i] = '1';
                        BinaryString (word, i, word.Length - 1, count);
                        word [i] = '?';
                        count++;
                    }
                }
            }
        }
        public static void Main (string[] args)
        {
            string binary = "1?00?1?1";
            char[] bi = binary.ToCharArray ();
            int count = 0;
            for (int i = 0; i < bi.Length; i++) {
                if (bi [i].Equals ('?')) {
                    count++;
                }
            }
            BinaryString (bi, 0, bi.Length - 1, count);
        }
    }

Let's get the obvious out of the way first.
Style.
The first thing that hits me is the indentation being offset here:

class MainClass
    {

Should be:
class MainClass
{

The next thing that hits me (in the same millisecond, really), is the inconsistent braces. You have them next-line-style at class level, and anything under class scope has them same-line-style. It's.... annoying to read. C# typically uses next-line braces:
if (count == 0)
{
    Console.WriteLine (word);
}
else
{
    ...
}

Also, it seems like you're fighting your IDE here:

char[] bi = binary.ToCharArray ();

And here:

if (bi [i].Equals ('?'))

These spaces feel very awkward. C# reads better like this:
char[] bi = binary.ToCharArray();

if (bi[i].Equals('?'))

The last thing, and perhaps the most important one, is about method ordering: code should read like a book, and unveil like a story. Just like you wouldn't start reading a story at the end, a class' code shouldn't start with its implementation details - I expect void Main to be the first thing: that's where everything starts, after all.

Abstractions & Responsibilities.
Programming is about making abstractions - the abstraction level of Main is too low, it's not clear what that loop is there for and what's being counted: count isn't exactly the most meaningful name to use here. You're counting question marks, and question marks are a thing in the context of this app. Why isn't there a concept of wildcard anywhere?
private static readonly char Wildcard = '?';

You have a loop in Main, that's counting the wildcards. You could extract a method to better express that intent, but it's probably simpler to inline a bit of LINQ there - note that ToCharArray() is completely useless, since a string can already be iterated as an IEnumerable<char> in C#:
var wildcardCount = binary.Count(c => c.Equals(Wildcard));

Notice how much easier it is to figure out exactly what's going on - and why.
Now, looking at the BinaryString method (bad name, methods should start with a verb, they're actions), I'm questioning why you would need to pass in the wildcardCount as a parameter, when all it's used for is really to exit the looping/recursion logic - you already have that information in the character array itself:
if (!word.Any(c => c.Equals(Wildcard)))
{
    Console.WriteLine(word);
}

Ah.. and now I see why ToCharArray was used - the method returning void, your entire logic depends on the ability to mutate array elements! That smells funky, because parameters are passed by value. A much more idiomatic way, in my opinion, would be to take a string input, and return a new string after processing.
That would make it easier to leave the console output out of the function's logic, too - I don't like that the function is responsible for producing/computing the string (/mutating the character array) and producing the output. It just feels wrong, clearly it breaks the Single Responsibility Principle.


Change architecture of Ruby app to be more object oriented, readable I'm working on a Slack bot for service desk which sends direct message to user on a Slack when their ticket will be on user_action_needed status. I'm using AWS Lambda to handle Jira incoming webhooks. Everything works well but I think I've got an issue with whole architecture of the app - I quess the code is not so readable, name of the class probably doesn't match what they do.

First of all I've got the handler on AWS lambda:

  module JiraHandler
    extend self
    def handle(event:, _context:)
      Parsers::JiraParser.new(event).call
      { statusCode: 200 }
    end
  end

Parsers::JiraParser is responsible not only for parsing events but it calls another class which grabs userId from Slack, and then inside of GetUserId I've got another class which sends message to user. So at the end if you call Parsers::JiraParser class you will receive slack message instead of parsed data.

Details of what I wrote about each class below:

Parsers::JiraParser

module Parsers
  class JiraParser
    def initialize(event)
      payload = event['body']
      @event = JSON.parse(payload)
    end

    def call
      ::Slack::GetUserId.new(reporter_email, reporter_name, ticket_number).call
    end
    # reporter_email, reporter_name, ticket_number are methods to pull data by .dig from event hash

GetUserId

  class GetUserId
    SLACK_LOOKUP_BY_EMAIL = 'https://slack.com/api/users.lookupByEmail'

    def initialize(email, name, ticket_number)
      @email = email
      @name = name
      @ticket_number = ticket_number
    end

    def call
      user_id = set_slack_user.dig('user', 'id')
      ::Slack::SlackMessenger.new(user_id, name, ticket_number).call
    end

    def set_slack_user
      HTTParty.get(SLACK_LOOKUP_BY_EMAIL, options)
    end

SlackMessanger

module Slack
  class SlackMessenger
    SLACK_API_ENDPOINT = 'https://slack.com/api/chat.postMessage'

    def initialize(user_id, name, ticket_number)
      @user_id = user_id
      @name = name
      @ticket_number = ticket_number
    end

    def call
      HTTParty.post(SLACK_API_ENDPOINT, body: params, headers: headers)
    end

I don't think this is a good approach, should I create an extra class where all those classes will be called? or maybe I should use monads?

Your code is not very clear indeed, as you said the name of the classes don't reflect what they do.

GetUserId sends the message to the user, I can't infer that just by the name of the class.

You know the steps of your program, which is good, but you haven't separated them in your program, so, the first step would be to create each of these steps in an independent way and then create a class that coordinates what has to be called. I think the handle method can do this coordination, it doesn't need to be as clean as you did.

Also, try to create classes that have states and operations, this way it is more object-oriented IMO. In terms of DDD, you are creating Service classes, you can try to create Entity classes.

For example (just a draft):

slack_api = SlackAPI.new

user = ::Slack::User.new(reporter_email, reporter_name, ticket_number, slack_api.user_id)
message = ::Slack::Message.new(user)

slack_api.send_message!(message)

(I've introduced here a class that will handle the communication with the Slack API.)

User:

class Slack::User
  def initialize(email, name, ticket_number, id)
    @id = id
    @email = email
    @name = name
    @ticket_number = ticket_number
  end
end

Message:

class Slack::Message
  def initialize(user_id, name, ticket_number)
    @user_id = user_id
    @name = name
    @ticket_number = ticket_number
  end
end


Dependency Injection With Unity in MVC 5 without repository and unit of work I want to use dependency injection with Unity in my application. I am not following repository pattern and unit-of-work (and don't want to). I also have ViewModel in my controller action method. Please review my code and add your comments.

public class ProductCatagoryController : Controller
{
    private dataBaseContext _context;

    public ProductCatagoryController(IdataBaseContext context)
    {
        this._context = context as dataBaseContext;
    }

    [HttpPost]
    [ValidateAntiForgeryToken]
    public ActionResult addNewCategory([Bind(Include = "catName,desc")]RegisterNewCatagoryViewModel RegisterNewCatagoryViewModel)
    {
        if (ModelState.IsValid)
        {
            try
            {
                Mapper.CreateMap<RegisterNewCatagoryViewModel, Category>();
                Category category = Mapper.Map<Category>(RegisterNewCatagoryViewModel);
                category.catID = Guid.NewGuid().ToString();
                category.date_from = DateTime.Now;
                category.active = true;
                _context.Categories.Add(category);
                _context.SaveChanges();
            }
            catch (Exception e)
            {
                loggerElmah.logElmah(e, "Adding New Category Failed");
            }
        }
        return View();
    }

    [HttpGet]
    public ActionResult addNewCategory()
    {
        return View();
    }

    // GET: ProductCatagory
    public ActionResult Index()
    {
        return View();
    }
}

In addNewCategory I am passing an object of a concrete class object, but I want to pass an interface of the viewmodel and bind it with actual implementation in the run-time through unity. How can I do this?

Also, can someone guide me on writing unit-tests for the above code?

Naming

Based on the naming guidelines methods should be named using PascalCasing casing.
addNewCategory -> AddNewCategory
Based on the same guideline classes should be named using PascalCasing casing.
dataBaseContext -> DataBaseContext and therefor IdataBaseContext -> IDataBaseContext

General

You are injecting an interface in the constructor, but then you cast it to the concrete implementation. If you don't need any methods/properties of the concrete implementation, you should stick to the interface.

private IDataBaseContext _context;

public ProductCatagoryController(IDataBaseContext context)
{
    this._context = context;
}

This will make the ProductCatagoryController class independent of the concrete DatabaseContext object.

For the case that ModelState.IsValid is false, how will the consumer of this method see, that no category is added ?
If an exception occurs, the consumer of this method will only see it, if the log file is parsed. Returning an Exception view would do better.


Saving web search results for EANs to CSV This is my first program in Python. Following code is taken from different posts (mostly this site) and combined together so that I can automate my routine task.

It's working properly but I need opinion from experts to enhance it further.

import mechanize
from bs4 import BeautifulSoup
import numpy as np
import csv
import os

# reading file
np.genfromtxt('EAN.txt', delimiter ='\n')
dtype=str
with open('EAN.txt') as f:
    lines = f.read().splitlines()

new_dictionary = {}
count = 0

for line in lines:
    count += 1
    new_dictionary['sequence_{}'.format(count)] = line

# searching items

print "Running..."

for i in new_dictionary:
    myean = new_dictionary[i]
    url = "https://mysite"
    br = mechanize.Browser()
    br.set_handle_robots(False)
    br.open(url)
    br.select_form(id="searchForm")
    br["q"] = myean
    res = br.submit()
    content = res.read()
    with open("mechanize_results.html", "a") as f:
        f.write(content)
    soup = BeautifulSoup(open("mechanize_results.html"),'html.parser')

    for div in soup.findAll('div', {'class': 'small-12 columns product-title'}):
        a = div.findAll('a')[1]

    #writing file

    if a is None:

        with open('Results.csv', 'ab') as csvfile:
            spamwriter = csv.writer(csvfile, delimiter='|',
                                    quotechar='|', quoting=csv.QUOTE_MINIMAL)
            spamwriter.writerow([myean, "Rejected"])
            #, title.string])

    else:
       #print myean, '|', a.text.strip()
       with open('Results.csv', 'ab') as csvfile:
            spamwriter = csv.writer(csvfile, delimiter='|',
                                    quotechar='|', quoting=csv.QUOTE_MINIMAL)
            spamwriter.writerow([myean, "Approved", a.text.strip()])
            #, title.string])

        #deleting file

    os.remove("mechanize_results.html")
    a=None
    i =+ 1

print "%d items Searched" %count
new_dictionary.clear()
raw_input("Press any key to continue...")

Although this code is running perfectly but I was wondering that when I used pyinstaller to create exe file, size became around 400+MB. Is it because, I am importing complete package of 'mechanize', 'numpy', 'csv' and 'os'?

There are multiple things to tackle and improve, let's summarize.

Code Style

In general, try to follow the PEP8 style guide, you have multiple violations, like:

use of spaces around operators and blank lines
organizing imports

There are tools like flake8 or pylint which can analyze your code statically and report existing violations, make sure to give them a try.

Variable naming is also a huge issue in the presented code - variables like i or my_dictionary are meaningless - make sure to give your variables meaningful names to improve readability. Remember: code is much more often read than written.

Code Organization

Currently, you are putting everything into a single program without any organization - think about logically splitting it into functions. You should also put your main execution logic into the if __name__ == '__main__': to avoid the code being executed on import.

Making the code Pythonic

you can use a dictionary comprehension with enumerate() to define your new_dictionary dictionary:

with open('EAN.txt') as f:
    new_dictionary = {
        'sequence_{}'.format(line_number): line
        for line_number, line in enumerate(f)
    }

On the second though, you don't actually even need a dictionary here - the keys are useless in the main execution logic of the program. A list of search queries should be sufficient enough.
you don't need read().splitlines() and can directly iterate over the file object lines - this will avoid having an extra list of lines created in memory and allow to get every next line in a "lazy" fashion
use print() as a function for Python-3.x compatibility

Performance and Web-Scraping related Improvements

First of all, you don't need to save the page source into a file and then reading that file to parse with BeautifulSoup - you can avoid that step altogether by feeding mechanize Browser's page source into BeautifulSoup directly.
You should also be able to initialize the Browser once and re-use for all the subsequent requests.
Also, you may improve on HTML parsing speed by using lxml as an underlying parser for BeautifulSoup - this requires lxml to be installed though.
I would also use a .product-title CSS selector to match product titles.
I would just collect the results and then write at the end instead of dealing with re-opening the resulting CSV file in the loop.
Performance-wise, this is also a good use case for a SoupStrainer which will make BeautifulSoup parse only the desired parts of a document - in your case the product links

Here is the code with some of the improvements applied:

import csv

from bs4 import BeautifulSoup, SoupStrainer
import mechanize

URL = "https://mysite"

if __name__ == '__main__':
    print("Running...")

    results = []
    with open('EAN.txt') as f, mechanize.Browser() as browser:
        browser.set_handle_robots(False)

        for search_query in f:
            browser.open(URL)

            browser.select_form(id="searchForm")
            browser["q"] = search_query
            response = browser.submit()

            soup = BeautifulSoup(response, 'lxml', parse_only=SoupStrainer(class_='product_title'))

            product_title = soup.select_one('.product-title')
            product_title_text = product_title.get_text(strip=True) if product_title else None
            results.append([search_query, product_title_text])

    # dump results
    with open('Results.csv', 'ab') as csvfile:
        writer = csv.writer(csvfile, delimiter='|', quotechar='|', quoting=csv.QUOTE_MINIMAL)

        for search_query, product_title in results:
            result_label = "Approved" if product_title else "Rejected"
            writer.writerow([search_query, result_label, product_title])

Might not work as is, since I don't have a way to test it. There are also some assumptions there - like the presence of a only one "product title" on a search result page per form query. But, at least, I hope that will give you something to start with.

And, I am not sure you need that np.genfromtxt() since currently you don't even read the results of this function call - hence removed that part in the posted code above.


Fields are required, except if all fields are empty I have a group of four inputs, the first three are required, the fourth is optional.

$fieldA
$fieldB
$fieldC
$fieldD #Optional

So to validate the inputs, I just use this:

if($fieldA === '' || $fieldB === '' || $fieldC === '') {
    #error
}

That works pretty well, except there is a special case: If all four inputs are empty, the input is still valid.

That special case complicates the if condition quite a bit:

if(($fieldA !== '' || $fieldB !== '' || $fieldC !== '' || $fieldD !== '')
    && ($fieldA === '' || $fieldB === '' || $fieldC === '')) {
    #error
}

The condition in the first pair of parenthesis makes sure that at least one field is not empty, while the rest of the condition makes sure that none of the required fields is empty.

Is there some sneaky way to simplify that if statement that I did not think of?

You have two situations where input is invalid:

fieldD is empty and one or more are empty
fieldD is not empty and one ore more are empty

This makes that you have to check for different situations and so your if-statement cannot really be shortened to one statement.

What you also can do is loop over the fields and do a count of the empty fields and validate this:

$emptyFields = 0;

foreach(array($fieldA, $fieldB, $fieldC, $fieldD)as $field) {
    if ($field === '') {
        $emptyFields++;
    }
}

if ($emptyFields === 4 || $emptyFields === 0 || ($emptyFields === 1 && $fieldD === '')) {
    //input is VALID
}


Writing .ppm images to a file I am struggling with commenting and variable naming. My teacher says my comments need to be more explanatory and explicit. He says my variable names are also sometimes confusing. I was just wondering whether you could go through my code and see whether you are able to understand the comments and add in comments/edit where you feel I should add comments or improve them. Lastly, are there any general rules to follow with commenting?

class PPM(object):
    def __init__(self, infile, outfile):
        self.infile=infile
        self.outfile=outfile

        #Read in data of image
        data= open(self.infile,"r")
        datain=data.read()
        splits=datain.split(None, 4)

        #Header info and pixel info
        self.type=splits[0]
        self.columns=int(splits[1])
        self.rows=int(splits[2])
        self.colour=int(splits[3])

        #(Return a new array of bytes)
        self.pixels=bytearray(splits[4])

    def grey_scale(self):
            for row in range(self.rows):
                for column in range(self.columns):
                    start = row * self.columns * 3 + column * 3
                    end = start + 3
                    r, g, b = self.pixels[start:end]
                    brightness = int(round( (r + g + b) / 3.0 ))
                    self.pixels[start:end] = brightness, brightness, brightness

    def writetofile(self):
        dataout= open(self.outfile, "wb")
        #Use format to convert back to strings to concatenate them and Those {} in the write function get's replaced by the arguments of the format function.
        dataout.write('{}\n{} {}\n{}\n{}'.format(self.type,
                                                 self.columns, self.rows,
                                                 self.colour,
                                                 self.pixels))

sample = PPM("cake.ppm", "Replica.ppm")
sample.grey_scale()
sample.writetofile()

To steal an old quote: "There are 2 hard things in computer science. Naming, cache invalidation, and off-by-one errors".

That being said, there is room for improvement here. Firstly, I'm assuming the class name, PPM, is short for Portable Pixmap Format. However, this isn't immediately obvious, and if you aren't familiar with that format (I'm not), it required a search. Hence, the first thing I'd do is change the name to something a bit more descriptive, and add a docstring explaining something about the format:

class PortablePixmap(object):
    '''A class encapsulating basic operations on images that use the
       portable pixmap format (PPM).

    '''

Python itself has a style guide known as PEP8 that you should try to follow as much as possible. Generally the convention in python is to name ClassesLikeThis, methods_like_this, and variables_like_this. Hence, another change I'd make is to rename infile and outfile to in_file and out_file respectively.

Continuing on, the first comment under __init__ is fairly obvious:

#Read in data of image
data= open(self.infile,"r")
datain=data.read()

As a minor aside, try and keep the whitespace around operators like = consistent. Again, as per PEP8, these should be:

data = open(self.infile, "r")
data_in = data.read()

I'd also consider renaming data_in to something like raw_image_data.

Back to the comments. The next line has no comment, but needs it far more than the previous 2 lines:

# Break up the image data into 4 segments because ...
splits = datain.split(None, 4)

The comment #(Return a new array of bytes) is both obvious and misleading: this is __init__; you're constructing the object - assigning to self.pixels isn't returning anything!

For grey_scale, your indentation moves to 8 spaces instead of 4. Be careful with this - especially in Python, where whitespace can modify the semantics (meaning) of your program. This function should again have a docstring:

def grey_scale(self):
    '''Converts the supplied image to greyscale.'''

The final function, def writetofile(self): should again use _ as separators
to make it easier to read. I'd also probably move the out_file parameter to this function, rather than passing it in __init__:

def write_to_file(self, output_location):
    '''Writes the image file to the location specified output location.
       The file is written in <some description of the file format>.

    '''

Watch the length of your comments (they should stick to the same line lengths as everything else in your program).

#Use format to convert back to strings to concatenate them and Those {} in the write function get's replaced by the arguments of the format function.

The comment itself is also difficult to understand:

  "convert back to string to concatenate them and Those {} ..."

That made me do a double take. Try and write comments as complete sentences.