Writing a user's profile image to disk using nested Node.JS closures I am writing non-blocking code as it should be in Node.JS. If I want run function A after B I do it using callback.

But I find this style guide:

  Use closures, but don't nest them. Otherwise your code will become a
  mess.

  Right:

setTimeout(function() {
  client.connect(afterConnect);
}, 1000);

function afterConnect() {
  console.log('winning');
}

  Wrong:

setTimeout(function() {
  client.connect(function() {
    console.log('losing');
  });
}, 1000);

How deep should my closures be? Specifically, I'd like to consider this sample:

exports.profileImage = function(req, res) {
    var id = req.params.user_id;
    userProvider.get(id, function(err, user){
        if (err) throw err;
        userProvider.getImageById(user['image_id'], function(err, image) {
            if (err) throw err;
            userProvider.writeImageToDisk(image, function(err, path){
                if (err) throw err;
                res.sendfile(path);
            });
        });
    });
};

First of all, you are correct. One way to prevent deep nesting is to pull out the callback into it's own named function. This allows you to only be as deep as 2-3 levels in blocks of code.

function task1(){
  async1(task2);
}

function task2(){
  async1(task3);
}

function task3(){
  async1(task4);
}

Another way to do it is by using flow-control libraries which take advantage of the concept of Futures/Deferreds/Promises. There are a lot of ways to call them, but they are the same thing most of the time.

The basic concept is that they usually stack up callbacks via utility functions and then run them, depending on the result of the previous or depending on the used utility function. The code will look linear and synchronous, but it actually is running async and the library controls the flow of calls and results. In jQuery, you can do something like

//Run an addition in the server
$.get('add.php',{
    op1 : 1,
    op2 : 2
  })
  .then(function(result){
    //get the addition result (3) and multiply it on the server (another async)
    return $.get('multiply.php'{
      op1 : result,
      op2 : 3
    });
  })
  .then(function(result){
    //get the multiplication result (9) and divide it on the server (another async)
    return $.get('divide.php'{
      op1 : result,
      op2 : 9
    });
  })
  .done(function(result){
    console.log(result); //1
  });


Given two vectors of ints, determine whether one vector is a prefix of the other Given two vectors of ints, write a program to determine whether one vector is a prefix of the other.

For vectors of unequal length, compare the number of elements of the smaller vector.
For example, given the vectors containing 0, 1, 1, 2, and 0, 1, 1, 2, 3, 5, 8, respectively, your program should return true.

#include <vector>
#include <iostream>

int main()
{
    const std::vector<int> ivec1 = { 0, 1, 1, 2 };
    const std::vector<int> ivec2 = { 0, 1, 1, 2, 3, 5, 8 };

    const decltype(ivec1.size()) size = ivec1.size() < ivec2.size() ? ivec1.size() : ivec2.size();
    const std::vector<int>& smallVec = ivec1.size() < ivec2.size() ? ivec1 : ivec2;
    const std::vector<int>& bigVec = smallVec == ivec1 ? ivec2 : ivec1;

    bool bPrefix = true;
    for(decltype(ivec1.size()) i = 0; i < size; ++i)
    {
        if(smallVec[i] != bigVec[i])
        {
            bPrefix = false;
            break;
        }
    }

    std::cout << "small vector:\n";
    for (const auto i : smallVec)
        std::cout << i << ", ";
    std::cout << "\nIs prefix of big vector:\n";
    for (const auto i : bigVec)
        std::cout << i << ", ";
    std::cout << "\n" << (bPrefix ? "true" : "false") << std::endl;
}

Embrace auto. Not only is it shorter, but avoids embarrassing bugs in choosing the wrong type.
decltype() is generally much more cumbersome.

Embrace abstraction. Solving the task itself deserves its own re-usable function. Good naming even reduces the need for comments, which can get out-of-sync.

Embrace the standard library. It is well-tested and well-known. Reinventing wheels for no reason is a poor use of time.

You refrained from using std::endl until your last line. No need to use it at all, ending the program flushed the output too.

std::boolalpha is the tool of choice for streaming a bool as "true" or "false".

Combine std::copy() and std::ostream_iterator for streaming an iterator-range.

#include <vector>
#include <iostream>
#include <algorithm>

template <class T, class U>
constexpr bool is_shorter_prefix(const T& a, const U& b) noexcept {
    const auto n = std::min(std::size(a), std::size(b));
    return std::equal(std::begin(a), std::begin(a) + n, std::begin(b));
}

template <class T>
void output_range(std::ostream& os, const T& t) {
    using U = decltype(*std::begin(t));
    std::copy(std::begin(t), std::end(t), std::ostream_iterator<U>(os, ", "));
}

int main() {
    const std::vector<int> a = { 0, 1, 1, 2 };
    const std::vector<int> b = { 0, 1, 1, 2, 3, 5, 8 };

    const auto is_prefix = is_shorter_prefix(a, b);

    std::cout << "small vector:\n";
    output_range(std::cout, a.size() <= b.size() ? a : b);
    std::cout << "\nIs prefix of big vector:\n";
    output_range(std::cout, a.size() > b.size() ? a : b);
    std::cout << "\n" << std::boolalpha << is_prefix << "\n";
}


Browser freeze - How to handle or optimize 50,000 rows in one HTML select element more than several times? I have an increasing table which already has records of 50,000+.
So in a combo box I have to load it, so that it shows which one is selected and its a main record which need to be selected and based on that it spread other relational records. But when I load it the whole web browser freeze.

How do you handle it?

My controllers:

$sql = "select name,name from countryCity";
$secondResult = $this->db->fetchPairs($sql);
$this->view->selectitems = array(''=>'') + $secondResult;

My view file:

<?for($i=0; $i<10; $i++):?>
<?=$this->formSelect("a",
                     "select one",
                     array('class'=>'largetosmall'),
                     $this->selectitems)?>

<?endfor;?>

Output:

<select class=largetosmall id=a>
 <option>...</option>
 <option>50,000+++... records</option>
</select>

More than 10 times I have a huge select element in one page. Never working.

50k is simply too many for a drop-down list. Use AJAX-based autocomplete input boxes or split the list to smaller parts, for example by state. I think the users also don't like it, it's really hard to scroll or search in a so huge drop-down list.


Asynchronously assembling a complex object Sanity check needed, as I'm still very unfamiliar with the nuances behind async/await and how they relate to more traditional Task / TPL code.
I have a high-level repository layer which is assembling a business object from various lower-level repositories.  As a business decision, we are doing keyed lookups at the data layer and assembling the composite object in code.  In the following code, "Account" is the business object, and the various _tAccountXXXRepo objects are all patterned similarly to this (we are using NPoco):
public class T_AccountMapperRepository : BaseNPocoRepository<T_AccountMapper>
    , IRetrieveMany<T_AccountMapper>
    , IRetrieve<AccountId, T_AccountMapper>
{
    public T_AccountMapperRepository(IDatabase database) : base(database)
    {
    }

    public async Task<T_AccountMapper> Retrieve(AccountId input)
    {
        return await Database.QueryAsync<T_AccountMapper>()
            .First(x => x.AccountId == input)
            .ConfigureAwait(false);
    }
}

The code to fetch the different values can be logically executed in parallel, and I am wondering if this implementation is the correct pattern to do so:  (the various methods being called on the Account object are thread-safe as well)
public async Task<Account> Retrieve(AccountId input1, DateTime input2)
{
    var account = new Account();

    await Task.WhenAll(
        _tAccountMapperRepo.Retrieve(input1)
            .ContinueWith(async result => account.SetAccountMap(await TranslateAccountMap(result))),

        _tPaymentRedirectionRepo.Retrieve(input1, input2)
            .ContinueWith(async result => account.ChangePayerToAccount(await TranslatePaymentRedirection(result))),

        _tAccountAncestorRepo.Retrieve(input1, input2)
            .ContinueWith(async result => await _tAccountMapperRepo.Retrieve((await result).AncestorId))
            .ContinueWith(async result => account.MoveAccountToNewParentAccountMap(await TranslateAccountMap(await result))),

        _tAccountRepo.Retrieve(input1)
            .ContinueWith(async result => await _tAccountTypeRepo.Retrieve((await result).TypeId))
            .ContinueWith(async result => account.SetAccountType(await TranslateAccountType(await result)))
    );

    return account;
}

Any method that is labelled with TranslateXXX all look similar to this:
private static async Task<AccountMap> TranslateAccountMap(Task<T_AccountMapper> mapTask)
{
    if (!mapTask.IsCompletedSuccessfully)
        throw new InvalidOperationException(nameof(mapTask), mapTask.Exception);

    var map = await mapTask;

    return new AccountMap(map.AccountId, map.Login, map.Namespace);
}

My main concerns are mixing Task and async/await, and whether or not my async & awaiting is re-introducing an element of synchronicity in what I'm hoping to make a very asynchronous process.  My end goal is that as many various properties as possible are fetched in parallel and assembled

As for me it looks fine except one thing. You have already completed Task in .ContinueWith block, thus you may use .Result then instead of await and avoid redundant async State Machine.
By the way, if you're using async/await, probably there's no continuations needed.
Consider this continuations-free implementation of 1st Task.WhenAll() job.
public async Task<Account> Retrieve(AccountId input1, DateTime input2)
{
    var account = new Account();

    // probably try-catch wrapper maybe useful here as replacement of !mapTask.IsCompletedSuccessfully
    await Task.WhenAll((Func<Task>)(async () =>
    {
        T_AccountMapper map = await _tAccountMapperRepo.Retrieve(input1);
        // continuation:
        AccountMap accountMap = new AccountMap(map.AccountId, map.Login, map.Namespace);
        account.SetAccountMap(accountMap);
    })
    //, (Func<Task>)(async() => {...})
    );

    return account;
}

As you can see, TranslateAccountMap method isn't needed anymore.
Finally, it's up to you.
P.S. Here's a useful link.


Shell script: is it doing what I think it's doing I've not written many shell scripts and cobbled this together from examples. The 'if' condition should check if this is already running and quite if so. The 'wget' call should trigger the php page and write any output to a txt file. The script will be launched by cron. Is this a reasonable approach?

#!/bin/sh
if ps -ef | grep -v grep | grep "dev.mydomain.biz/system_processdata.php" ; then
        exit 0
else
        wget -q -O /tmp/system_processimport.txt http://dev.mydomain.biz/system_processdata.php?a=37&b=61
        exit 0
fi

You should protect the URL with double quotes to prevent the shell from parsing the =, ?, and & as being special instead of just plain old characters.

Also, as a minor nitpick, you should use fgrep because grep will treat the . characters as being 'match any character' and not as a literal .. fgrep treats the whole string it's searching for (except for \n) as literal. But this is unlikely to cause you problems in practice.

The programmer in me shudders at this approach because so many things could potentially go wrong. In practice, they almost certainly won't. But I've learned to never, ever write programs as if the things I think should never happen never will. Because, in practice, they almost all do eventually.

For example, if you ever try to do two different fetches with different parameters at the same time, one will mysteriously not happen.

Also, if some random person happens to have put that URL on the command line for something, that will cause the script to not do anything until whatever command it is finishes.

As I said, these may or may not be serious problems. But I would be aware of them.


Priority Queue Linked List Implementation I am new to Python and wanted to make sure my code answers the question for my assignment due.

My Task:

  A priority queue is a queue in which each item is assigned a priority
  and items with a higher priority are removed before those with a lower
  priority, irrespective of when they were added. Integer values are
  used for the priorities with a smaller integer value having a higher
  priority. In an unbounded priority queue, there are no limits on the
  range of integer values that can be used for the priorities.

• PriorityQueue(): Creates a new empty unbounded priority queue.

• isEmpty(): Returns a boolean value indicating whether the queue is empty.

• length(): Returns the number of items currently in the queue.

• enqueue(item, priority): Adds the given item to the queue by inserting it in the proper position based on the given
priority.

• dequeue(): Removes and returns the next item from the queue, which is the item with the highest priority.
If two or more items have the same priority, those items are removed in FIFO order. An item cannot be dequeued from an empty queue.

• peek(): Returns a copy of the next item in the queue, without removing the item. The next item is the same value that would be returned by the dequeue operation. An item cannot be dequeued from an empty queue.

My Code:

class Node( object ) :
    def __init__( self, cargo = None, next = None ) :
        self.cargo = cargo
        self.next = next

 # Creates a new empty unbounded priority queue
class PriorityQueue :

     #
    def __init__( self ) :
        self.length = 0
        self.head = None
        self.last = None

     # Returns a boolean value indicating whether the queue is empty
    def isEmpty( self ) :
        return (self.length == 0)

     # Returns the number of items currently in the queue
    def __len__( self ) :
        return len(self.length)

     # Adds the given item to the queue by inserting it in the proper position
     # based on the given priority. The new node is appeneded to the end of the
     # linked list
    def enqueue( self, item, priority) :
        newNode = Node(cargo)
        newNode.next = None
        if self.length == 0:
            self.head self.last = newNode
        newNode.next = self.head
        self.head = newNode
        self.last.next = newNode
        self.last = newNode

        temp = self.head
        p = self.head.next
        while p != None :
            if p.cargo > newNode.cargo:
                temp = temp.next
                p = p.next
            break
        newNode.next = temp.next
        temp.next = newNode

     # Removes and returns the next item from the queue, which is the item with
     # the highest priority. If two or more items have the same priority, those
     # items are removed in FIFO order. An item cannot be dequeued from an
     # empty queue. The linked list is searched to find the entry with the
     # highest priority.
    def dequeue( self ) :
        cargo = self.head.cargo
        self.head = self.head.next
        self.length = self.length - 1
        if self.length == 0:
            self.last = None
        return cargo

A Priority Queue is synonymous with a heap so I'm assuming that the implication in this assignment was to implement a maximally efficient data structure.

The optimal implementation is spelled out in Wikipedia: http://en.m.wikipedia.org/wiki/Heap_(data_structure)

If it were me, I would use a tree instead of a list. There is no Node class. A Heap object has two nullable Heap children, and every operation is recursive in nature. Length, for example, would just return each child's length or 1 if no children existed. The other operations would take advantage of the tree's structure in order to return results in logarithmic time.


"Spread the Word" CodeChef challenge I am solving the "Spread the Word" problem on CodeChef:

  Snackdown 2019 is coming! People have started to spread the word and tell other people about the contest. There are \$N\$ people numbered 1 through \$N\$. Initially, only person 1 knows about Snackdown. On each day, everyone who already knows about Snackdown tells other people about it. For each valid \$i\$, person \$i\$ can tell up to \$A_i\$ people per day. People spread the information among the people who don't know about Snackdown in the ascending order of their indices; you may assume that no two people try to tell someone about Snackdown at the same moment. Each person is only allowed to start telling other people about Snackdown since the day after he/she gets to know about it (person 1 can start telling other people already on day 1). How many days does it take for all people to know about Snackdown?

But my code exceeds the given time limit. It solves the problem in 1.01 seconds, but the time limit in there, is 1.00 seconds. I am attaching the code snippet written in C++ below:

#include<iostream>
using namespace std;
int main()
{
    int t,n;
    cin>>t;
    for(int i = 0; i<t;i++)
        {
        int c = 0,f=0,p=1;
        cin>>n;
        int a[n];
        for(int i = 0; i<n; i++) cin>>a[i];
        f = a[0];
        while(p<n)
        {

            for(int i = 0; i<f; i++)
            {
                p += a[i];
            }
            c++;
            f = p;
        }
    cout<<c;
    }
}

It would be a great help if someone modifies the above code into an efficient one!

Do not do using namespace std; This is a bad habbit that pollutes your namespace needlessly. You are only using std in exaclty 4 places. Its really easy to be better here.
Please use sensible names. Your t is the number of test runs, so name it numTests or something similar. In a month you will never know the meaning of a given variable.
Stay consistent in your code style. You have different styles of for loop. This is really confusing and quite frankly annoying to read.
The question is marked as c++ but you use plain old C here. Rather than a C-style array you should use a std::vector<int>
You are getting timeouts because you use a brute force approach. You should realize, that you can directly calulate the number of infomred people directly. At the first day it is a[0]. At the second day it is a[0] + a[0] + ... a[a[0]]. Consequently you can determine the number of newly informed people for every day just once and reuse that value.


Move last node of the linked list to the front Description:

Given a linked list move the last node to the front.

For example:

  10 20 30 40 -> 40 10 20 30

Code:

class Main {
  static class Node {
    private int data;
    private Node next;

    Node(int data, Node next) {
      this.data = data;
      this.next = next;
    }

    Node append(int data) {
      Node newNode = new Node(data, null);
      Node current = this;

      while (current.next != null) {
        current = current.next;
      }
      current.next = newNode;
      return this;
    }

    public String toString() {
      Node current = this;
      StringBuilder sb = new StringBuilder();

      while (current != null) {
        // do something here
        sb.append(current.data);
        sb.append(" ");
        current = current.next;
      }
      return sb.toString();
    }
  }

  public static Node moveTailToHead(Node head) {
    if (head == null || head.next == null) {
      throw new IllegalStateException(
          "List should have atleast two elements");
    }

    Node current = head;
    Node prev = null;

    while (current.next != null) {
      prev = current;
      current = current.next;
    }
    prev.next = null;
    current.next = head;
    head = current;

    return head;
  }

  public static void main(String[] args) {
    Node head = new Node(10, null);
    head.append(20)
        .append(30)
        .append(40);
    System.out.println(head); // 10 20 30 40

    Node newHead = moveTailToHead(head);
    System.out.println(newHead); // 40 10 20 30

    try {
      System.out.println(
        "Should not happen: " + moveTailToHead(null));
    } catch (IllegalStateException e) {
      System.out.println(
        "Got expected exception for null node");
    }

    try {
      System.out.println(
        "Should not happen: " + moveTailToHead(new Node(10, null)));
    } catch (IllegalStateException e) {
      System.out.println(
        "Got expected exception for single node");
    }
  }
}

Question:

I am writing code in Java after a long time and would like to know if I am making any mistake or abusing any feature of Java.

You are only able to access Node's fields data and next from the method moveTailToHead(Node) because this method is declared in the same top level class as Node. I don't know why you chose this design for your code sample, but the fact that you made the method moveTailToHead a static method outside the class Node rather than an instance method of Node makes me think that moveTailToHead is also supposed to work "from the outside" without having access to Node's internal implementation details, so I'm wondering whether your code design in this regard is intentional. If it is, then I won't argue against it, but if it isn't and if moveTailToHead should also work if it were declared in a different top level class, then you need to implement a way for Node to provide some way of accessing the contents of the list it represents. A way to start could be for Node to implement the interface Iterable<Integer>, which would require Node to have an iterator() method. This method could, for instance, look like this:

@Override
public Iterator<Integer> iterator() {
    return new Iterator<Integer>() {
        private Node nextNode = Node.this; //the next node to be returned by next()
        private Node currentNode = null;   //the last node returned by next()
        private Node previousNode = null;  //the node that precedes currentNode

        @Override
        public boolean hasNext() {
            return nextNode != null;
        }

        @Override
        public Integer next() {
            if (!hasNext()) {
                throw new NoSuchElementException();
            } else {
                if (currentNode != null) {
                    previousNode = currentNode;
                }
                currentNode = nextNode;
                int nextData = nextNode.data;
                nextNode = nextNode.next;
                return nextData;
            }
        }

        @Override
        public void remove() {
            if (currentNode == null) {
                throw new IllegalStateException();
            } else if (previousNode != null) {
                currentNode.next = null;
                previousNode.next = nextNode;
                currentNode = null;
            } else {
                assert currentNode == Node.this;
                throw new UnsupportedOperationException();
            }
        }
    };
}

That way, you can also get rid of the code duplication whenever you iterate through the list using constructs like while (current.next != null) or similar.

If you look at the method remove(), it becomes apparent that the fact that you are using a Node object to refer to the actual list itself, which Xtreme Biker already criticized in his answer, is problematic. Since the head node also defines the list, it is not possible to remove the head node from the list, which is why I made the method throw an UnsupportedOperationException for the sake of completeness in this demonstration code.

But apart from this issue, with Iterable implemented, adding other methods to add or remove elements should not be very difficult, and then you can move the tail node to the head without having direct access to the private fields Node.data and Node.next.


Pattern printing with Name Found this problem somewhere online and wanted to try this out with my beginner skills. I was able to solve this problem but how can I make my code better?

We need to print a pattern like this (each line has one more character in length):

E
my
iyt
hTri
bbiann
i

Here is the solution I have written, and I want help in the following areas:

What am I doing wrong and how can I make it better?
How can I learn to write better code and logic?

public class nameIncrementing {

    public static void main(String[] args) {

                char[] text={'E','m','y','i','y','t','h','T','r','i','b','b','i','a','n','n','i'};
                int count =0;
                int length = text.length;
                int icnt=0;
                for(int i=0;i<length;i++){
                    if(i==0)
                        System.out.println(text[0]);
                    else{
                        if(count+1>=length)
                        {break;}
                        else{
                            for(int j=count+1;j<i+2+count;j++){
                                icnt++;
                                if(j>=length)
                                    break;
                                else
                                    System.out.print(text[j]);
                            }
                    count = count+icnt;
                    icnt=0;
                    }

                    System.out.println();}
                }
    }
}

First step - make your current code better

Instead of a char array you might prefer to use a String.
This would make the declaration easier:

String text = "EmyiythTribbianni";

I would say that most of us generally recommend that you shouldn't omit brackets.
You should also follow a consistent bracket pattern.

I recommend for every statment to be in a different line from your brackets.
This means that things like {break;} are not really that good.

Closing brackets should also be matching the same identation of the statment they were open.
By this I mean

ìf(condition){
    //code goes here
    for(declaration; evaluation; increment){
        //code goes here
    }//closing bracket with the same indentation as the previous for statmeent
    //code goes here
}//closing bracket with the same indentation as the previous if statement

You should also follow a consistent spacing and let prefer more space than less.

This int count =0; becomes int count = 0;
This for(int i=0;i<length;i++) becomes for(int i = 0; i < length; i++)
This if(j>=length) becomes if(j >= length)

I think you get the ideia. Every time you find a comparison, assignment, +, -, or ; (on for) you space it.

Your code would then become something like this

String text = "EmyiythTribbianni";
int count = 0;
int length = text.length;
int icnt = 0;
for(int i = 0; i < length; i++){
    if(i == 0){
        System.out.println(text[0]);
    }else{
        if(count + 1 >= length){
            break;
        }else{
            for(int j = count + 1; j < i + 2 + count; j++){
                icnt++;
                if(j >= length){
                    break;
                }else{
                    System.out.print(text[j]);
                }
            }
            count = count + icnt;
            icnt = 0;
        }
        System.out.println();
    }
}

Second step - taking a different approach

It seems that you want to put according to the number of character that you had printed so far.
I reckon that the arithmetic progression sum formula n(a1 + an) / 2 would be useful here.

The first time you print 1 character making the first term a1 = 1. Then you print a space.
The second time you print 2 characters a2 = 2, the total of characters print to this time is 2 * (1 + 2) / 2 = 3.
And so on.

By applying this idea you would get something along this lines:

String text = "EmyiythTribbianni";
int n = 1;

for(int i = 0; i < length; i++){
    int sum = n  * (1 + n) / 2;
    if(i != sum){
        System.out.print(text[i]);
    }else{
        System.out.print(" ");
        System.out.print(text[i]);
        ++n;
    }
}


Guessing a random number between a range of numbers What is the general design pattern for declaring variables in a class?

I have a homework assignment to create a simple Java program which asks the user for a range of two variables. The program then creates a random variable from within this range and prompts the user to guess the number.

I'm having trouble understanding how to declare my variables. I'm not sure if I should declare variables as public or private, and static or non-static. When is it correct to use a public/private variable?

Also, how much code should I include in my main() method? What is generally placed into the main() method?

import java.util.Scanner;
import java.util.Random;

public class HiLo {

  static int guess, guessCount, randomNumber, startRange, endRange;
  static Scanner scan = new Scanner(System.in);
  static Random random = new Random();

  public static void main(String[] args) {
      getRange();
      playGame();
  }

  public static void getRange() {
      System.out.println("Welcome! Enter integer for start of range (must be > 0)");
      startRange = scan.nextInt();

      System.out.println("Enter integer for end of range (must be >0)");
      endRange = scan.nextInt();
      randomNumber = random.nextInt(endRange - startRange + 1) + startRange;
      System.out.println(randomNumber);
  }

  public static void playGame() {
      guessCount = 0;
      System.out.println("Enter guess or 0 to quit: ");
      guess = scan.nextInt();

      while (randomNumber != guess) {
          if (guess == 0) {
              break;
          }
          else if (randomNumber > guess) {
              guessCount++;
              System.out.println("Too Low");
              System.out.println("Enter guess or 0 to quit: ");
              guess = scan.nextInt();
          }
          else if (randomNumber < guess) {
              guessCount++;
              System.out.println("Too High");
              System.out.println("Enter guess or 0 to quit: ");
              guess = scan.nextInt();
          }
      }
      if (randomNumber == guess) {
          guessCount++;
          System.out.println("Correct! That took you " + guessCount + " guesses.");
      }
  }
}

You can read this to understand scope of variables or method.

In short:

public - Any member of a class declared as public can be accessed outside of the class. So declare public when you want to give client code access to methods/variables of a class.
private - A private method or field is invisible and inaccessible to other classes, and can be used only within the class in which the field or method is declared.
protected - Protected variables and methods allow the class itself to access them, classes inside of the same package to access them, and subclasses of that class to access them.
static - In general when a number of objects created from the same class, they all have their distinct copies of instance variables stored in different location. Sometimes we want to have variables that are common to all objects. To accomplish this use static. Read this.

Think of the main method as a place where you can write code to test your class. No design / logic should be placed in main.

For example:

public class Hilo {
 public Hilo() {}
 //other methods...

public static void main(String[] args) {
 Hilo hilo = new Hilo();
 hilo.getRange();
 hilo.playGame();
 }
}

I would prefer to remove all the static variables used in your class and make those private. The two methods getRange and playGame can be public.


Translate a day in year to month and day Take an integer representing a day of the year and translate it to a string consisting of the month followed by day of the month. For example, Day 32 would be February 1.

This is what I came up with. I'm sure that there's something more streamlined than else if statements.

vector<string>DayOfYear::months = { "January","February","March","April",
                                "May","June","July","August",
                                "September","October","November","December"};

string DayOfYear::convertDayOfYear(int num){
if (num <= 31)
    return months[0] + " " + to_string(num);
else if (num > 31 && num <= 59)
    return months[1] + " " + to_string(num - 31);
else if (num > 59 && num <= 90)
    return months[2] + " " + to_string(num - 59);
else if (num > 90 && num <= 120)
    return months[3] + " " + to_string(num - 90);
else if (num > 120 && num <= 151)
    return months[4] + " " + to_string(num - 120);
else if (num > 151 && num <= 181)
    return months[5] + " " + to_string(num - 151);
else if (num > 181 && num <= 212)
    return months[6] + " " + to_string(num - 181);
else if (num > 212 && num <= 243)
    return months[7] + " " + to_string(num - 212);
else if (num > 243 && num <= 273)
    return months[8] + " " + to_string(num - 243);
else if (num > 273 && num <= 304)
    return months[9] + " " + to_string(num - 273);
else if (num > 304 && num <= 334)
    return months[10] + " " + to_string(num - 304);
else if (num > 334 && num <= 365)
    return months[11] + " " + to_string(num - 334);
else
    return "Invalid number.\n";
}

This code is incomplete, so we'll have to guess there's a definition of namespace DayOfYear, or struct DayOfYear, somewhere.  In future, please provide more context for reviews - ideally, we'd be able to take your code and compile it ourselves.

There's a problem with the interface: to convert from Julian day number to month, we need to know whether the year is a leap year or not.  So convertDayOfYear() needs two arguments - as well as the day number, we need to provide either the year number or an indication (as a bool) whether it's a leap year.  From C++20, it's straightforward to convert, using std::chrono::year::is_leap().  We can then use that information like this:

if (!is_leap_year && num >= 59) { // day 59 is 1st of March
    num += 1;
}

We want to reduce that big chain of repeated if/else.  One thing we could do to simplify is to not repeat the previous test: if we're in the else block from if (num <= 31), we already know that num > 31:

if (num <= 31)
    return months[0] + " " + to_string(num);
else if (num <= 59)
    return months[1] + " " + to_string(num - 31);
else if (num <= 90)

What we'd really like is to be able to just look it up in a table.  We can do this using a list of months with their day counts:

const std::vector<std::pair<int, std::string>> day_to_month = {
    { 31, "January" },
    { 29, "February" },         // we'll correct non-leap years to fit
    { 31, "March" },
    { 30, "April" },
    { 31, "May" },
    { 30, "June" },
    { 31, "July" },
    { 31, "August" },
    { 30, "September" },
    { 31, "October" },
    { 30, "November" },
    { 31, "December" },
};

We can walk this list, and at each element look to see if that month contains the day - if it does, create the return value; if not, then subtract that month's amount of days and move on to the next entry:

for (auto const& entry: day_to_month) {
    auto& days_in_month = entry.first;
    auto& month_name = entry.second;
    if (num <= days_in_month) {
        return month_name + " " + std::to_string(num);
    } else {
        num -= days_in_month;
    }
}

I've named the elements of each entry to make it clearer; we could just use entry.first and entry.second instead, but I believe the clarity helps here.

Let's put it all together, with some reasonable range checking and a main() to demonstrate that it works.  Let's also change the vector to be an array, since we know its length in advance.

Then we get:

#include <array>
#include <stdexcept>
#include <string>
#include <utility>

std::string convertDayOfYear(int num, bool is_leap_year)
{
    if (num <= 0)
        throw std::range_error("before beginning of year");

    // correct non-leap years
    if (!is_leap_year && num > 59) { // day 59 is 28th of February
        num += 1;
    }

    static const std::array<std::pair<int, std::string>, 12> days_in_month = { {
        { 31, "January" },
        { 29, "February" },         //  we've adjusted non-leap years
        { 31, "March" },
        { 30, "April" },
        { 31, "May" },
        { 30, "June" },
        { 31, "July" },
        { 31, "August" },
        { 30, "September" },
        { 31, "October" },
        { 30, "November" },
        { 31, "December" },
    } };

    for (auto const& entry: days_in_month) {
        auto& days_in_month = entry.first;
        auto& month_name = entry.second;
        if (num <= days_in_month) {
            return month_name + " " + std::to_string(num);
        } else {
            num -= days_in_month;
        }
    }

    throw std::range_error("after end of year");
}

#include <iostream>
int main()
{
    std::cout << convertDayOfYear(1, false) << '\n'
              << convertDayOfYear(1, true) << '\n'
              << convertDayOfYear(59, false) << '\n'
              << convertDayOfYear(59, true) << '\n'
              << convertDayOfYear(60, false) << '\n'
              << convertDayOfYear(60, true) << '\n'
              << convertDayOfYear(365, false) << '\n'
              << convertDayOfYear(366, true) << '\n';
}

If performance is important, then consider using std::lower_bound to find the month from the list - you'd sacrifice some clarity there, as you'd need to return to using the cumulative day count for each month rather than days in that month (or write code to build the former from the latter).

It's a little bit tricky to get right (both the conversion to cumulative day count and the lookup) but with the help of the test cases, I get this implementation (C++14, because of the auto parameters in the lambda expression, but easily adapted to C++11):

#include <algorithm>
#include <array>
#include <cassert>
#include <stdexcept>
#include <string>
#include <utility>

std::string convertDayOfYear(int num, bool is_leap_year)
{
    if (num <= 0)
        throw std::range_error("before beginning of year");

    // correct non-leap years
    if (!is_leap_year && num > 59) { // day 59 is 28th of February
        num += 1;
    }

    if (num > 366)
        throw std::range_error("after end of year");

    static const auto start_day = []{
        std::array<std::pair<int, std::string>, 12> days_in_month = { {
                { 31, "January" },
                { 29, "February" },         // we've adjusted non-leap years
                { 31, "March" },
                { 30, "April" },
                { 31, "May" },
                { 30, "June" },
                { 31, "July" },
                { 31, "August" },
                { 30, "September" },
                { 31, "October" },
                { 30, "November" },
                { 31, "December" },
            } };
        int offset = 0;
        for (auto& month: days_in_month) {
            std::swap(offset, month.first);
            offset += month.first;
        }
        return days_in_month;
    }();
    // start_day now looks like { {0, "January"}, {31, "February"},
    // {60, "March"}, ... } - i.e. the number of days to subtract for
    // that month.

    // Now search backwards for the first month whose offset is less
    // than num
    auto it = std::lower_bound(start_day.rbegin(), start_day.rend(), num,
                               [](auto a, auto b){ return a.first >= b; });

    assert(it != start_day.rend());
    return it->second + " " + std::to_string(num - it->first);
}


Passing lots of options to a function I am learning actionscript 3 coding by making a crappy platformer game :)

The problem I am facing is passing lots of options to a construct function that will setup the  display object where all of them have default values.

I could send the options the normal way, like this:

function Box(param1:Number=1, param2:Number=2, param3:Number=3, param4:Number=4){

}

but this seemed to be parameter pollution, so I made this class:

package
{
    import flash.display.Sprite;
    public class Box extends Sprite {
        public var color:Number = 0x000000;
        public var tileW:Number = 50;
        public var tileH:Number = 50;
        public var walkable:Boolean = true;
        public var speedY:Number = 0;
        public var speedX:Number = 0;
        function Box(options:Object) {
            for(var i:String in options) {
                this[i] = options[i];
            }
            graphics.beginFill(this.color);
            graphics.drawRect(0, 0, this.tileW, this.tileH);
            graphics.endFill();
        }
    }
}

This works, but I have a feeling that I am missing something very basic, that everything I try to write is wrong...

People do this sort of thing all the time in the JavaScript (this.arguments) and Python world (*args); however, by removing type constraints from the parameter list, you have to be wary that you're always providing the types of parameters your function is expecting. If one of your parameters is supposed to be a string, and someone passes in an object, bad things may happen.

Conversely, some very respectable projects use multi-line parameter lists. It's fine to have a lot of parameters, as long as they're all necessary, but it's always good to be skeptical. Long parameter lists is a good code smell – if you see them, the code may need refactoring, but not always.

As far as the attributes of Box's, forget about programming for a second. Conceptually, what are the properties of a box? Can a box still be a box without a color, tileW, tileH, walkable, speedY, and speedX? Can a box still be a box if it doesn't know how to draw itself? If your answer is no to all of these, then your code is fine, and requiring parameters as a single collection/object vs. individual parameters is (arguably) a matter of preference.

That being said, I would move the three graphics.* lines to a separate method Box.draw(). It's best to only do construction in the constructor, and leave the rest – like rendering – to other methods.


Merge sort implementation using divide-and-conquer I was told in an interview to write a program for implementing merge sort on the concept of divide-and-conquer.

var myGlobalArray = undefined;

myGlobalArray = [8,4,17,2,1,32];
example01(myGlobalArray);

myGlobalArray = [48,14,17,2,11,132];
example01(myGlobalArray);

myGlobalArray = [45,14,5,2,1,12];
example01(myGlobalArray);

myGlobalArray = [45,-14,-5,2,1,-12];
example01(myGlobalArray);

myGlobalArray = [38,27,43,3,9,82,10];
example01(myGlobalArray);

myGlobalArray = [34,45,1,23,19,12,10];
example01(myGlobalArray);

function example01(myArray){
   var mainArray = [];

   // Divide
   createSubArray(myArray,0);

   // Conquer
   mainArray = mergeArrays(mainArray);

   console.log(myArray+" => "+mainArray[0]);

   // creates an array which contains n arrays for n numbers present in myarray
   // i.e. if array = [ 34, 1, 27, 3 ] that the below method will return
   // [ [34], [1], [27], [3] ]
   function createSubArray(subArray,index){
        var localArray = [];

        if(subArray[index] !== undefined){
            localArray.push(subArray[index]);
            mainArray.push(localArray);
            createSubArray(subArray,++index); // dividing recursively
        }
   }//createSubArray

   // merge the arrays present i.e.
   // if gblArray = [ [2,5], [1,7] ]
   // then the below method will return
   // an merged array [ [1, 2, 5, 7] ]
   function mergeArrays(gblArray){
        var mergedArrays = [],
            main_array = gblArray,
            arr = [],
            left_array = undefined,
            right_array = undefined,
            counter = 0,
            nextCounter = 0;

        do{

            while(counter < main_array.length){
                nextCounter = counter + 1;

                if(main_array[nextCounter] !== undefined){

                    left_array = main_array[counter];
                    right_array = main_array[nextCounter];

                    // merge left and right arrays and sort it
                    arr = mergeAndSort(left_array,right_array);

                    mergedArrays.push(arr);
                }else{
                    mergedArrays.push(main_array[counter]);
                }
                counter = nextCounter + 1;
            }

            main_array = mergedArrays;
            mergedArrays = [];
            counter = 0;
            nextCounter = 0;

        }while(main_array.length > 1);

        return main_array;
   }//mergeArrays

   // merges two array and sorts i.e.
   // if array1 = [1,23] and array2 = [4,12] than
   // the below method returns [1,4,12,23]
   function mergeAndSort(array1,array2){
        var array2Counter = 0,
            array1Counter = 0,
            mergedArray = [];

        while(array2Counter < array2.length && array1Counter < array1.length){

            if(array2[array2Counter] < array1[array1Counter]){
                mergedArray.push(array2[array2Counter]);
                array2Counter++;
            }else{
                mergedArray.push(array1[array1Counter]);
                array1Counter++;
            }
        }

        while(array1Counter < array1.length){
            mergedArray.push(array1[array1Counter]);
            array1Counter++;
        }

        while(array2Counter < array2.length){
            mergedArray.push(array2[array2Counter]);
            array2Counter++;
        }

        return mergedArray;
   } //mergeAndSort

}//example01

The output is:

8,4,17,2,1,32 => 1,2,4,8,17,32
48,14,17,2,11,132 => 2,11,14,17,48,132
45,14,5,2,1,12 => 1,2,5,12,14,45
45,-14,-5,2,1,-12 => -14,-12,-5,1,2,45
38,27,43,3,9,82,10 => 3,9,10,27,38,43,82
34,45,1,23,19,12,10 => 1,10,12,19,23,34,45

But by looking at my implemented merge-sort program, the inteviewer said that it doesn't follow the divide-and-conquer concept.

I tried to convince him that method mergeArrays and mergeAndSort do the divide-and-conquer, but he didn't agreed. Where am I going wrong?

To expand on CiaPan's answer. Your solution might look like:

result = mergeAndSort([8,4,17,2,1,32]);

You would write code that would do:

let a = mergeAndSort([8,4,17]);
let b = mergeAndSort([2,1,32]);
return merge(a, b);

This would be recursive, so mergeAndSort([8,4,17]) would do:

let a = mergeAndSort([8,4]);
let b = mergeAndSort([17]);
return merge(a, b);

Now mergeAndSort would be called with [8,4] which would be:

let a = mergeAndSort([8]);
let b = mergeAndSort([4]);
return merge(a, b);

Now lastly you have mergeAndSort([8]) which is trivially implemented as

return [8]

Or alternatively, in pseudocode:

function mergeAndSort(array) {
  if (array.length <= 1)
    return array;
  let a = mergeAndSort( leftHalfOfArray(array)  );
  let a = mergeAndSort( rightHalfOfArray(array) );
  return merge(a,b);
}


Sending a message based on selected row I have a gridview with CheckBox inside it.  When the user selects a row and clicks
on a button, a message is sent for the specific user.

How can this be optimized?

protected void btnSendSMSForSpeceficUser_Click(object sender, EventArgs e)
    {
        using (NoavaranModel.NoavaranEntities1 dbContext=new NoavaranModel.NoavaranEntities1())
        {
            int userId = Int32.Parse(Session["UserId"].ToString());
            var query = (from p in dbContext.Users
                         where p.UserId == userId
                         select p.CountOfSMS).FirstOrDefault();
            var query2 = (from p in dbContext.Users
                          where p.UserId == userId
                          select p).FirstOrDefault();
            int j = 0,sendCount=0;
            for (int i = 0; i <= grdStudents.Rows.Count - 1; i++)
            {
                string MobileNumbers = grdStudents.Rows[i].Cells[2].Text;
                int stID = Int32.Parse(grdStudents.Rows[i].Cells[0].Text);
                var query3 = (from p in dbContext.Students
                              where p.Id == stID
                              where p.IsRecivedSMS==false
                              select p).FirstOrDefault();
                GridViewRow row = grdStudents.Rows[i];
                CheckBox Ckbox = (CheckBox)row.FindControl("chkSelectStudents");
                if (Ckbox.Checked)
                {
                    j++;
                    if (j <= query) {
                        sendCount++;
                        Utility.SendMessageForStudents(MobileNumbers, txtMessage.Text);
                        query3.IsRecivedSMS = true;
                    }
                }
            }
            query2.CountOfSMS = query.Value - sendCount;
            dbContext.SaveChanges();
            ListBox1.Items.Add(sendCount.ToString());
        }
    }

I'm not sure of everything (worked with notepad) but:

protected void btnSendSMSForSpeceficUser_Click(object sender, EventArgs e)
{
    using (NoavaranModel.NoavaranEntities1 dbContext = new NoavaranModel.NoavaranEntities1())
    {
        var userId = int.Parse(Session["UserId"].ToString());

        var user = dbContext.Users.FirstOrDefault(p => p.UserId == userId);
        var countOfSms = user.CountOfSMS.Value;
        var j = 0;
        foreach (var row in grdStudents.Rows)
        {
            var cells = row.Cells;
            var MobileNumbers = cells[2].Text;
            var stID = int.Parse(cells[0].Text);

            var student = dbContext.Students.FirstOrDefault(p => p.Id == stID && !p.IsRecivedSMS);

            var Ckbox = (CheckBox)row.FindControl("chkSelectStudents");
            if (Ckbox.Checked)
            {
                j++;
                if (j <= countOfSms) {
                    Utility.SendMessageForStudents(MobileNumbers, txtMessage.Text);
                    student.IsRecivedSMS = true;

                    user.CountOfSMS = countOfSms - 1;
                }
            }
        }

        dbContext.SaveChanges();
        ListBox1.Items.Add(sendCount.ToString());
    }
}

This is a cleaner version of your code with less query (the first was unneccesary) and inside the loop you are making a lot of small query which can be really bad but first you have to modify your code to not work directly with the context in you page code but through an interface (first iterate through the rows and get all data then pass them to the worker (which can work with the context) and return the result & update the UI).


Decoupling tests from implementation and increase their resilience I have read on numerous occasions that tests should not be coupled to implementation details - this makes perfect sense.

Now I am having some trouble decoupling my tests from implementation details and am kindly asking for some help.

The relevant portion of the repository that I am testing looks like this:

public class PostsRepository : IPostsRepository
{
    private readonly DatabaseContext context;

    public PostsRepository(DatabaseContext context)
    {
        this.context = context;
    }

    public Post Find(string slug)
    {
        return context
            .Posts
            .SingleOrDefault(post => post.Slug == slug);
    }

    public bool Delete(string slug)
    {
        var post = Find(slug);

        if (post == null) return false;

        context.Posts.Remove(post);
        return true;
    }
}

The context is mocked using the technique described here and then manually injected into the repository.

The corresponding tests that I am concerned about look like this:

[Test]
public void Delete_ExistentPost_DeletesPost()
{
    databaseSet.SetupSeedData(new List<Post>
    {
        PostsMother.CreatePost(withSlug: "abc")
    });

    repository.Delete("abc");

    databaseSet.Verify(d => d.Remove(It.IsAny<Post>()));
}

[Test]
public void Delete_NonExistentPost_DoesNotThrow()
{
    repository.Delete("abc");
}

[Test]
public void Delete_ExistentPost_ReturnsTrue()
{
    databaseSet.SetupSeedData(new List<Post>
    {
        PostsMother.CreatePost(withSlug: "abc")
    });

    var actual = repository.Delete("abc");
    Assert.True(actual);
}

[Test]
public void Delete_NonExistentPost_ReturnsFalse()
{
    var actual = repository.Delete("abc");
    Assert.False(actual);
}

These tests seem to work well enough but I am concerned about their brittleness. If you look at the implementation of the Delete method you will see that it uses the Remove method but what if that changed? The test would break.

Furthermore (and more importantly), the test is highly dependent on the Delete method using the Find method as the Find method uses the Posts property of the database set which is stubbed according to the seed data configured in the arrange phase of the test (using SetUpSeedData).

It is perfectly conceivable that I will adjust the implementation of the Delete method to catch an exception and return the boolean according to the exception instead of using the Find method.

How can I improve the resilience of my tests in this case?

Without trying to sound tautological, the tests in your original question are coupled to your implementation because that's exactly what they are testing.

You're testing that -

Remove is called on a database abstraction
The method doesn't throw an exception for an invalid slug
The method returns true when it finds something to delete
The method returns false when it does not find something to delete

These are all quite low level concepts, are any of them important?

As a developer that's new to a code base, I look to the tests to tell me what's important and needs to be preserved. Most of the time, this boils down to behaviour that the user can observe and interact with.

If you re-focus your tests so that they ensure -

When there are Posts in the repository, a user can view them
When a Post is deleted, it is no longer listed on an index
When a Post is deleted, it can no longer be viewed

Then you will not be coupling them to such details as "my repositories return false when they can't find something to delete".


Filling name accounts I think the content is too large and I am deeply think about how to refactor it. Does anyone have good advice on refactoring it?

List<AccountEntity> fillNameAccounts = agentDao.getAgentListByIds(accountIdList);
    for (T bean : results)
    {
        for (AccountEntity entity : fillNameAccounts)
        {
            String chineseName = entity.getChineseName();
            String firstName = entity.getFirstName();
            String fillName = StringUtils.isBlank(chineseName) ? firstName : chineseName;
            String entityId = entity.getId().toString();
            if (bean instanceof AgentPostResponseBean)
            {
                if (StringUtils.equals(((AgentPostResponseBean) bean).getParentId(), entityId))
                {
                    ((AgentPostResponseBean) bean).setParentName(fillName);
                }
                if (StringUtils.equals(((AgentPostResponseBean) bean).getPosterId(), entityId))
                {
                    ((AgentPostResponseBean) bean).setPosterName(fillName);
                }
            }
            else if (bean instanceof ImportAgentPostResponseBean)
            {

                if (StringUtils.equals(((ImportAgentPostResponseBean) bean).getParentId(), entityId))
                {
                    ((ImportAgentPostResponseBean) bean).setParentName(fillName);
                }
                if (StringUtils.equals(((ImportAgentPostResponseBean) bean).getPosterId(), entityId))
                {
                    ((ImportAgentPostResponseBean) bean).setPosterName(fillName);
                }
            }
        }
    }

In this case, you take an action depending on class type of bean, so I would recommend you to implement a Visitor Pattern, because it is not god practice to use instanceof and because Visitor Pattern lets you growth your code without coupling.

My answer is more complex than previous, but it will give you more flexibility. What if you want to do more things than setNameForEntity? or what if you wnat to implement more concrete classes? Visitor Pattern will suite this needs.

Now, here comes how I would do the refactor. First, create an abstract class (or an interface) as base class for beans, like AgentPostResponseBean and ImportAgentPostResponseBean. This base class have a method accept(Visitor visitor).

public abstract class AbstractResponseBean{
    public abstract void accept(Visitor visitor, AccountEntity entity);
}

If your ResponseBeans already inherit from an existing abstract class or they implement an existing interface, simply add the accept method to that class/interface.

Then, implement visit method on each concrete class, as follows

public void accept(Visitor visitor, AccountEntity entity){
    visitor.visit(this, entity);
}

Finally, create a Visitor class, that accept the visit of each concrete ResponseBeans class you are interested in. For example:

public class Visitor{

    private String getFillName(AccountEntity entity){
        String chineseName = entity.getChineseName();
        String firstName = entity.getFirstName();
        return StringUtils.isBlank(chineseName) ? firstName : chineseName;
    }

    public void visit(AgentPostResponseBean respBean, AccountEntity entity){

        if (StringUtils.equals(respBean.getParentId(), entityId)) {
            respBean.setParentName(getFillName);
        }
        if (StringUtils.equals(respBean.getPosterId(), entityId)) {
            respBean.setPosterName(getFillName);
        }
    }

    public void visit(ImportAgentPostResponseBean respBean, AccountEntity entity){

        if (StringUtils.equals(respBean.getParentId(), entityId)) {
            respBean.setParentName(getFillName);
        }
        if (StringUtils.equals(respBean.getPosterId(), entityId)) {
            respBean.setPosterName(getFillName);
        }
    }
}

Of course, visitor's private method getFillName would be better implemented in AccountEntity. This way, your code will be refactored as follows:

List<AccountEntity> fillNameAccounts = agentDao.getAgentListByIds(accountIdList);
Visitor visitor = //get instance from some factory or singleton or app context ...

for (T bean : results)
{
    for (AccountEntity entity : fillNameAccounts)
    {
        bean.visit(visitor, entity);
    }
}


Code for testing user inputs in python3 #loop to get input that is not blank, strips input of whitespace
def ckin(a):
    while a=='':
        a=input().strip()
    return  a

#creates list with includes user admin
user_names = ['admin']

#Ask for 5 names to store as usernames
print('Please enter 5 user names: ')

#if username is valid, add to list
for i in range(0,5)
    lib_name=input().strip()
    lib_name=ckin(lib_name)
    user_names.append(lib_name)

#asks for login name, must be valid
print('Enter login name:')
log_name=input().strip()
log_name=ckin(log_name)

#Create a test list that is lower case to compare login
#against without altering the cases used by user to creat username list

test=[name.lower() for name in user_names]

#if login name not in list continue to prompt
while log_name.lower() not in test:
    print('Enter login name:')
    log_name=input().strip()
    log_name=ckin(log_name)

#find login name in test, then return either response for admin, or login    name using
#case formatting that original used for original entry
for i in range(len(test)):
    if log_name.lower() == 'admin':
        print('Hello admin, would you like to see a status report?')
        break
    elif log_name.lower() == test[i]:
        print('Hello '+user_names[i]+', thank you for logging in again')
        break

Looking for general review self-teaching, Day 4

I haven't learned much but I keep attempting to do what I'm learning with user inputs instead of set entries

My goal here was to be able to create a list that keeps the original entry cases to be used for responding. I created a second list in lower cases to compare to and then responded using the cases the entries were stored as.

Any comments are greatly appreciated!

You can reduce your code duplication a bit. First, the way you are currently using the ckin function is like this:

name = input("Enter a name").strip()
name = ckin(name)

Wouldn't it be nicer to be able to do:

name = ask_user("Enter a name: ")

For this, just modify your function like this:

def ask_user(message=""):
    user_input = ""
    while not user_input:
        user_input = input(message).strip()
    return user_input

Note that I renamed it (to make it clearer what it does), used the fact that empty strings are falsy and added whitespace around the = operator, as recommended by Python's official style-guide, PEP8.

Going further, you should get into the habit of defining clearly named functions for separate things.

One such concern is getting a pre-populated username list. Currently this list is populated with user input, but this might change in the future. So it would be nice to bundle that part all together in one function:

def get_users():
    # creates a users set, which always includes admin
    users = {'admin'}

    #Ask for 5 names to store as usernames
    print('Please enter 5 unique user names: ')
    while len(users) < 6:
        users.add(ask_user("Enter a valid user name: ").lower())
    return users

Note that I made the datastructure a set here, which gives you \$\mathcal{O}(1)\$ time complexity for in tests. I also made sure that there are actually 5 + 1 users (in other words, user names need to be unique). I also put the str.lower transformation here so we don't need to do it later and only store the canonical user name.

Next is a login function, that asks for a username until a valid one is entered:

def login(users):
    user_name = None
    while user_name not in users:
        user_name = ask_user("Enter login name: ").lower()
    return user_name

And then, the greeting. Instead of iterating over all users until you reached the user that is currently logging in, use the fact that we already know that the user is in users (since we explicitly checked for that in login). So this becomes a simple if..else check:

def print_greeting(user_name):
    if user_name == 'admin':
        print('Hello admin, would you like to see a status report?')
    else:
        print(f'Hello {user_name}, thank you for logging in again')

Here I used an f-string, as introduced in Python 3.6, to make the string formatting a bit easier.

And finally, tying it all together in a main function:

def main():
    users = get_users()
    user_name = login(users)
    print_greeting(user_name)

if __name__ == "__main__":
    main()

Note that I call this main function only under a if __name__ == "__main__": guard. This ensures that the main function does not run if you import from this script in another script.


Todo List Application Code in Pure JavaScript I'd highly appreciate it if you can:

Spot flaws, especially bad practices and suggest a possible alternative method.
Spot inefficiencies and suggest how to perform that task efficiently.
You may review my HTML/CSS as well.

var addRecordFormVisibility = false;
var addRecordForm = document.getElementById('addRecord');
var addRecordFormButton = document.getElementById('submit');
var errors = document.getElementById('errors');
var addRecordFormTrigger = document.getElementById('addRecordFormTrigger');

function prepare() {
    addRecordForm.style.display="none";
}

function triggerAddRecordForm() {
    if(addRecordFormVisibility === false) {
        addRecordForm.style.display="block";
        addRecordFormVisibility = true;
    } else {
        addRecordForm.style.display="none";
        addRecordFormVisibility = false;
    }
}

function submit(event) {
    var userInput = document.getElementById('description').value;

    errors.innerHTML = '';

    if(userInput.length === 0) {
        errors.innerHTML = "Please fill in the form!";
    } else {
        var listItem = document.createElement('li');
        listItem.innerHTML = userInput;

        document.getElementById('todoList').appendChild(listItem);
    }

    event.preventDefault();
}

document.addEventListener("DOMContentLoaded", prepare, false);
addRecordFormButton.addEventListener("click", submit, false);
addRecordFormTrigger.addEventListener("click", triggerAddRecordForm, false);
#todoApplication {
    width: 800px;
    height: 100%;
    margin: 0 auto;
    background: #EEEEEE;
    border: 1px solid #DDDDDD;
    border-radius: 5px;
    padding: 10px;
}
#errors {
    color: red;
}
<!DOCTYPE html>

        <meta charset="UTF-8">
        <title>JavaScript</title>
        <link rel="stylesheet" href="src/style.css">

            <button id="addRecordFormTrigger">Trigger Add Record Form</button>

            <form id="addRecord" method="POST">
                <input id="description" type="text" name="description" placeholder="Enter the task here">

                <button id="submit">Submit</button>
            </form>

Ok so, your code is overall looking nice, but a few changes could be made:

It's important to make sure you leave whitespace around your operators, like:

addRecordForm.style.display="none";
                           ^^

Could use whitespace around the operators.

I try not to suggest ternaries very often, but I believe that triggerAddRecordForm could be simplified with the use of one.

function triggerAddRecordForm() {
    if(addRecordFormVisibility === false) {
        addRecordForm.style.display="block";
        addRecordFormVisibility = true;
    } else {
        addRecordForm.style.display="none";
        addRecordFormVisibility = false;
    }
}

into:

function triggerAddRecordForm() {
    addRecordForm.style.display = addRecordFormVisiblity ? "none" : "block";
    addRecordFormVisibility = !addRecordFormVisibility;
}

On the subject of triggerAddRecordForm(), in general you shouldn't compare to false, then else it. You should leave the check empty (checks if the variable is true) like so:

if(addRecordFormVisibility) {
    doStuff();
} else {
    doOtherStuff();
}

In submit(), you could improve errors.innerHTML = ''.
Why don't you just set errors to document.getElementById('errors').innerHTML instead, seeing as you don't use errors other than for that?

In submit() you should also improve your line spacing as well, as it seems a little strange.


Extracting data in ES6 I have this array const idArray = ["12", "231", "73", "4"] and an object

const blueprints = {
  12: {color: red, views: [{name: "front}, {name: "back}, {name: "top}, {name: "bottom}]},
  231: {color: white, views: [{name: "front}, {name: "back}]},
  73: {color: black, views: [{name: "front}, {name: "back}, {name: "top}, {name: "bottom}]},
  4: {color: silver, views: [{name: "front}, {name: "back}, {name: "top}, {name: "bottom}]},
}

How can I return an array of the following objects that have all front, back, top, and bottom using ES6 map/filter/some and etc?:

result =[
    {colorId: "12", views: [{name: "front}, {name: "back}, {name: "top}, {name: "bottom}]}
    {colorId: "73", views: [{name: "front}, {name: "back}, {name: "top}, {name: "bottom}]}
    {colorId: "4", views: [{name: "front}, {name: "back}, {name: "top}, {name: "bottom}]}
]

I did it here but I feel like it is too messy and hard to read. Anybody could recommend on how to shorten it and make it easier to read using the ES6 functions (map, filter ...)?

const result = idArray.map(id => {
      const bluePrint = bluePrints[id];
      const exists = bluePrint.views.some(view => view.name === 'top' || view.name === 'bottom');

      if (exists) {
        return {
          colorId: id,
          views: bluePrint.views
        }
      }
    }).filter(bluePrint => bluePrint);

The initial data is not formatted as proper JSON. There appear to be mismatched quotes around values like front, back, etc... try running the snippet below to see the error that is caused by this.

const blueprints = {
  12: {color: red, views: [{name: "front}, {name: "back}, {name: "top}, {name: "bottom}]},
  231: {color: white, views: [{name: "front}, {name: "back}]},
  73: {color: black, views: [{name: "front}, {name: "back}, {name: "top}, {name: "bottom}]},
  4: {color: silver, views: [{name: "front}, {name: "back}, {name: "top}, {name: "bottom}]},
}

Also, unless red, white, black and silver are defined as variables, they need to be enclosed in quotes as string literals. See the rewrite below for a sample of this.

The indentation is inconsistent- the first level in the .map() callback is four spaces and then two spaces within the if block. Stick with one or the other.

You could use Array.prototype.reduce() to conditionally add the desired elements to an output array, reducing the need to call .filter():
const result = idArray.reduce((output,id) => {
  const bluePrint = bluePrints[id];
  //conditionally add to output and then return it
  return output;
}, []);

You could also use Object.keys() to iterate over the keys of bluePrints instead of idArray, though it appears to be slightly slower (see performance link below).
const result = Object.keys(bluePrints).reduce((output,id) => {
  const bluePrint = bluePrints[id];
  //conditionally add to output and then return it
  return output;
}, []);

See a comparison of the three snippets in this jsPerf.

One other change for readability would be to move that filter function outside the call to .map() or .reduce():
const viewIsTopOrBottom = view => view.name === 'top' || view.name === 'bottom'

Then that function can be called within the call to .some():
const exists = bluePrint.views.some(viewIsTopOrBottom);

And then that boolean exists could be eliminated by moving the call to .some() into the conditional statement of the if:
if (bluePrint.views.some(viewIsTopOrBottom)) {
  //add to array

Proposed Rewrite

const idArray = ["12", "231", "73", "4"];
  const bluePrints = {
    12: {color: "red", views: [{name: "front"}, {name: "back"}, {name: "top"}, {name: "bottom"}]},
    231: {color: "white", views: [{name: "front"}, {name: "back"}]},
    73: {color: "black", views: [{name: "front"}, {name: "back"}, {name: "top"}, {name: "bottom"}]},
    4: {color: "silver", views: [{name: "front"}, {name: "back"}, {name: "top"}, {name: "bottom"}]},
  };
const viewIsTopOrBottom = view => view.name === 'top' || view.name === 'bottom';
const result = idArray.reduce((output,id) => {
  const bluePrint = bluePrints[id];
  if (bluePrint.views.some(viewIsTopOrBottom)) {
    output.push({
      colorId: id,
      views: bluePrint.views
    });
  }
  return output;
}, []);
console.log('result,', result);


Extremely Simple Paint Program in Java I have written a very basic paint program in java using AWT and Swing. I have separated the program into two different class files; one of them holds the JFrame and adds a custom panel (this custom panel being the second class file, extending JPanel).

In my first try, I tried using Timer and an ActionListener with repaint() but that seemed very very laggy (I could not draw straight lines/curves, they would always be laggy when I dragged my mouse fast.)

Then I switched to using a different approach (stored previous x and y locations and drew a line between the current position and the previous location). Here are the two class files:

The custom JPanel class:

import java.awt.*;
import java.awt.event.*;
import javax.swing.*;

public class Panel extends JPanel
{
  // PROPERTIES
  private final int DEFAULT_WIDTH  = 800;
  private final int DEFAULT_HEIGHT = 800;
  private final Color BACK_COLOR   = Color.WHITE;

  private int x1, y1, x2, y2;

  private MyMouseHandler handler;
  private Graphics g;

  // CONSTRUCTOR
  public Panel()
  {
    setBackground( BACK_COLOR );
    setPreferredSize( new Dimension( DEFAULT_WIDTH, DEFAULT_HEIGHT ) );

    handler  = new MyMouseHandler();

    this.addMouseListener( handler );
    this.addMouseMotionListener( handler );
  }

  // METHOD
  public void paintComponent(Graphics g)
  {
    super.paintComponent(g);
  }

  private void setUpDrawingGraphics()
  {
    g = getGraphics();
  }

  // INNER CLASS
  private class MyMouseHandler extends MouseAdapter
  {
    public void mousePressed( MouseEvent e )
    {
      x1 = e.getX();
      y1 = e.getY();

      System.out.println("Mouse is being pressed at X: " + x1 + " Y: " + y1);

      setUpDrawingGraphics();

      x2=x1;
      y2=y1;
    }

    public void mouseDragged( MouseEvent e )
    {
      x1 = e.getX();
      y1 = e.getY();

      System.out.println("Mouse is being dragged at X: " + x1 + " Y: " + y1);

      g.drawLine(x1,y1,x2,y2);

      x2=x1;
      y2=y1;
    }
  }
}

and the JFrame class with the main method:

import java.awt.*;
import javax.swing.*;

public class RunPanel
{
  public static void main(String[] args)
  {
    JFrame frame = new JFrame( "Run Panel" );
    frame.setDefaultCloseOperation(3);

    Panel  panel = new Panel();
    frame.add( panel );

    frame.pack();
    frame.setVisible( true );
  }
}

The program works fine, I can draw normal curves at low mouse speed. But again, as I increase the mouse speed the lines/curves start the get jaggy and ugly.

Maybe this is because I am drawing lines in between the positions, but drawing individual pixels look MUCH worse.

NOTE: I have gathered the two classes into one class file and the program stopped the lag. What is causing this? You can see the code here.

Your code formatting deviates from the default Java code formatting, just saying.

import java.awt.*;
import java.awt.event.*;
import javax.swing.*;

Wildcard imports are kinda bad. If you can avoid them, please do so.

public class Panel extends JPanel

I'm sure you can find a more descriptive name for this class, like PaintablePanel or MousePaintablePanel.

// PROPERTIES

Personally, I don't like such "structuring" comments, they do nothing and only clutter up the code and need to be manually moved when refactoring automatically.

private final int DEFAULT_WIDTH  = 800;
private final int DEFAULT_HEIGHT = 800;
private final Color BACK_COLOR   = Color.WHITE;

Personally I also don't like this column based alignment, it puts my brain into some sort of "column reading mode" which makes it hard for me to associate the value with the variable.

private int x1, y1, x2, y2;

You should not declare multiple variables on the same line, it makes it easy to miss the declaration of variables.

private MyMouseHandler handler;

MyMouseHandler is not a very good name.

public void paintComponent(Graphics g)
{
  super.paintComponent(g);
}

This method seems to override a super method, yet misses the @Override annotation. Also, if it does not do anything, why override it?

public void mousePressed( MouseEvent e )
{
...
     SetUpDrawingGraphics();

Why are you doing this on every mouse press?

public class RunPanel

This is another bad name. What does it run? And it is not a panel.

frame.setDefaultCloseOperation(3);

Any reason why you're not using the constants defined in JFrame?

The program works fine, I can draw normal curves at low mouse speed. But again, as I increase the mouse speed the lines/curves start the get jaggy and ugly.

That is how it is.

I have gathered the two classes into one class file and the program stoped the lag. What is causing this?

I see this behavior in both programs. Combining them in the same file has for sure nothing to do with it.


jQuery plugin boilerplate jquib Inspired by jqueryboilerplate.com, I extended their boilerplate to fit my needs. Coming from PHP development, I wanted to have a good starting point for writing jQuery plugins with a defined API/interface (more at jquib.web-sav.de).

/**
 * JQUIB - JQUERY INTERFACEPLUGIN BOILERPLATE
 *
 * This is a solid starter template for your jQuery plugins. Its designed
 * to keep all logic and data hidden from public access except the set of
 * functions you defined inside its interface function namespace.
 *
 * Inspired by http://jqueryboilerplate.com
 *
 * @see        http://web-sav.de/jquib-boilerplate
 * @copyright  Andr&eacute; Kroll
 * @license    MIT
 */
;
(function ($, window, document, undefined) {
  const PLUGIN_NAME = 'your_plugin';

  const defaults = {
    option_one: 'value_1',
    option_two: 'value_2'
  };

  /* PLUGIN - CONSTRUCTOR AND INSTANCE METHODS ............................... (scope: private) */
  var Plugin = {

    options: {},
    /* ... more plugin properties here ... */

    // Gets called whenever a new plugin instance is created
    __construct: function (options) {
      this.options = this.initOptions(options);
      /* ... you can define more parameters for your constructor                     */
      /*     - the bootstrap process will pass them through                          */

      /* ... start writing your plugin logic ...                                     */
      /*     From here you have access to a plugin                                   */
      /*     .. member with           "this.propertyName;"                           */
      /*     .. method with           "this.methodName();"                           */
      /*     .. interface method with "this.public_interface.interfaceMethodName();" */
      /*     .. helper function with  "helperFunctionName();"                        */
      /*                                                                P e a c e    */
    },

    // Validate options and merge with defaults
    initOptions: function (options) {
      /* ... put your option validation logic here ... */
      return $.extend({}, defaults, options);
    }

  };

  /* PLUGIN - PUBLIC / INTERFACE METHODS ...................................... (scope: public) */
  Plugin.public_fn = {

    // Get the jQuery-wrapped plugin container
    get$: function () {
      return $(this.container);
    }

  };

  /* HELPER FUNCTIONS ------------------------------------------------- (scope: private static) */
  {
    // Simple and safe function to log/debug elements
    function log(elementToLog) {
      if ( window.console && window.console.log ) {
        window.console.log(elementToLog);
      }
    }
  }

  /* BOOTSTRAP AND PLUGIN REGISTRATION ======================================================== */
  {
    // Register the plugin at jQuerys function namespace
    $.fn[PLUGIN_NAME] = function (options) {
      var origArgs = arguments;
      this.each(function () {
        if ( undefined == $.data(this, "plugin_" + PLUGIN_NAME) ) {
          // First call by this element: create new instance of the plugin
          $.data(this, "plugin_" + PLUGIN_NAME, (function (container, options) {
            var plugin = Object.create(Plugin);
            plugin['container'] = container;
            plugin['public_interface'] = {};
            for ( prop in plugin.public_fn ) {
              if ( typeof plugin.public_fn[prop] === 'function' ) {
                // Register all interface functions to call within the plugin object context
                plugin['public_interface'][prop] = $.proxy(plugin.public_fn[prop], plugin);
              }
            }
            plugin.__construct.apply(plugin, origArgs);
            return plugin;
          })(this, options));
        }
      });
      // Return plugin interface of the first element
      return $.data(this.get(0), "plugin_" + PLUGIN_NAME).public_interface;
    };
  }
})(jQuery, window, document);

Very interesting and fun question. Boilerplate code is a reason to close questions down, but this code is a perfectly valid question.

Naming

JavaScript uses lowerCamelCase variable and function names, avoid names like public_interface and option_one.
You have anonymous functions, this makes stack-traces needlessly complicated, go for __construct: function pluginConstructor (options) {
Prefixing with _ should be avoided. If you do it, ensure _ items are 'private' unlike __construct

Idioms and Custom

Yoda conditionals like if ( undefined == $.data(this, "plugin_" + PLUGIN_NAME) ) are hard to read
Worse, your conditionals are not consistently Yoda: if ( typeof plugin.public_fn[prop] === 'function' )

Comments

I like your approach to commenting

JSHint.com

Your have problems in your code:

Unnecessary semicolon
Declaring functions in an extra set of {} is bad practice
Use plugin.container, not plugin['container']
prop leaks in to the global namespace here: for ( prop in plugin.public_fn ) {

Finally, having undefined as a parameter is silly. If you were to pass anything but undefined, then your yoda condition would fail.


SmartRandom bombing strategy for Battleship game Now that I have some infrastructure in place to test it (see my Battleship test framework and the updated GitHub project for the full context), I have finally written a non-trivial bombing strategy class for the May 2015 Community Challenge.

The SmartRandom class guesses randomly until it gets a hit.  It then checks for possible ship positions based on that hit, and caches those guesses.  It then uses the cached guesses until either a ship is sunk (at which point it dumps the cache) or until the cache of guesses is exhausted.

SmartRandom.h

#ifndef SMARTRANDOM_H
#define SMARTRANDOM_H
#include "Ocean.h"
#include "Bomber.h"

class SmartRandom : public Bomber
{
public:
    SmartRandom() : Bomber(), next() {}
    SmartRandom(Ocean &o) : Bomber(o), next() {}
    bool turn();
    const char *id() const { return "SmartRandom"; }
private:
    // vector holding the next guesses
    std::vector<unsigned> next;
};

#endif // SMARTRANDOM_H

SmartRandom.cpp

#include <iostream>
#include <string>
#include <random>
#include "SmartRandom.h"

static std::random_device rd;
static std::uniform_int_distribution<> r{0, Ocean::dim*Ocean::dim-1};

/*
 * The strategy here is to bomb randomly, but to follow
 * up promptly when a ship is found.
 */
bool SmartRandom::turn() {
    ++turns;
    unsigned location = r(rd);
    // try using our pre-stored guesses first
    if (!next.empty()) {
        for (location = next.back();
                tracking[location] && !next.empty();
                location = next.back())
        {
            next.pop_back();
        }
    }
    if (tracking[location])
        for (location = r(rd);
                tracking[location];
                location = r(rd))
        { }
    char result = ocean.bomb(location);
    if (result != ocean.empty) {
        if (result == ocean.hit) {  // generic hit
            unsigned shortship = ocean.shipcount-1;
            if (tracking.place(location, shortship, true, true))
                next.push_back(location+1);
            if (tracking.place(location, shortship, false, true))
                next.push_back(location+ocean.dim);
            if (tracking.place(location-1, shortship, true, true))
                next.push_back(location-1);
            if (tracking.place(location-ocean.dim, shortship, false, true))
                next.push_back(location-ocean.dim);
        } else {  // specific ship was just sunk
            next.clear();
        }
    }
    tracking.record(location, result);
    if (verbose)
        std::cout << "Turn " << turns << ", bombing " <<
            location << '\n' << ocean << std::endl;
    return ocean.remaining();
}

Results

I used two different placement strategies (as explained in the Battleship test framework question) and two different bombardment strategies.  The first was a relatively stupid random bombing strategy and the second is this one, which performs much better.  (The numbers are the number of turns it took to sink all ships.)

There are 2 placers, and 2 bombers
Running 10 iterations
UpperLeft/RandomBomber  UpperLeft/SmartRandom   RandomPlacer/RandomBomber   RandomPlacer/SmartRandom
95  45  94  84
94  57  95  61
98  97  97  90
76  81  89  79
96  52  95  82
96  36  96  44
92  51  87  66
92  62  83  40
96  84  81  74
91  70  96  62

Questions

I'm interested in comments on the code, of course, but also in suggestions for improvements to the algorithm.

It looks good to me. The random_device and its distribution were the only things to catch my attention.

It seems like overkill in this case. random_device produces cryptography quality number sequences, which is probably much more accuracy than you need. Since its implementation is platform-specific, it is hard to tell how much more expensive a random_device is, but I'd start with something more usual, like a Mersenne Twister seeded with one call from a random_device - That's a very common usage pattern by the way.
No apparent reason to make it a global. That makes your code unsuitable for multithreaded use. (I would expect random_device to hold some member state that is most likely not synchronized). A member instance would be cleaner and avoids threading issues.
Lastly, r is a bit of a weak name for a file-scoped global. Eventually you might want to declare loop counters and iterators named r and that could shadow the random generator... Perhaps consider a slightly longer name just to avoid this potential for shadowing.


Weighted phase Laplacian formula We have the following equation (weighted phase Laplacian):

\$c_{i,j} = U(i,j)\Delta^{^x}_{i,j} - U(i-l, j)\Delta^{x}_{i-1,j} + V(i,j)\Delta^{y}_{i,j} - V(i,j-1)\Delta^{y}_{i,j-1}\$

Where

\$U(i,j) = min(w^2_{i+1}, w^2_{i+1})V(i,j) = min(w^2_{j+1}, w^2_{j+1})\$

with \$U, V, \Delta \$ (N,N) matrices

I coded the following code:

def compute_Weights(w):
    U = np.zeros_like(w)
    V = np.zeros_like(w)
    U = np.minimum(w[1:, :]**2, w[:-1, :]**2)
    V = np.minimum(w[:, 1:]**2, w[:, :-1]**2)

    """add the last ones rows and cols
    """
    U = np.vstack((U, U[-1]))
    V = np.vstack((V.T, V.T[-1])).transpose()

    return U, V

def Gradient(p1, p2):
    r = p1 - p2
    if r > np.pi:
        return r - 2 * np.pi
    if r < -np.pi:
        return r + 2 * np.pi
    return r
"""
    phase is not flattened
    wts are flattened
"""
def ComputePhaseLaplacian(phase, U, V):
    rows, cols = phase.shape
    phi = phase.flatten()
    rho = np.zeros_like(phi)

    for j in range(rows):
        for i in range(cols):
            k = j * rows + i
            k1 = k + 1 if i < cols - 1 else k - 1
            k2 = k - 1 if i > 0 else k + 1
            k3 = k + cols if j < rows - 1 else k - cols
            k4 = k - cols if j > 0 else k + cols

            w1 = U[k]
            w2 = U[k-1] if i > 0 else U[k]
            w3 = V[k]
            w4 = V[k-cols] if j > 0 else V[k]

            rho[k] = w1 * Gradient(phi[k], phi[k1]) \
                + w2 * Gradient(phi[k], phi[k2]) \
                + w3 * Gradient(phi[k], phi[k3]) \
                + w4 * Gradient(phi[k], phi[k4])

    return rho

I want to know if there is a "pythonic way" to write this. I'm a C/C++ programmer so this was my perspective. Maybe something to avoid to work with flattened arrays, and to work with 2D arrays.

I.e:

phase = np.load('phase.npy') #phase 512x512 float array
U,V = ComputeWeigths(W) #W 512x512 float array with values in (0,1)
uphase = ComputePhaseLaplacian(phase, U, V) #uphase 512x512 float array

A few points.  In your code, some of these render others irrelevant, but they may be relevant in other code you write:

Try to follow PEP8
It should be possible to vectorize ComputePhaseLaplacian.
It is better to loop over rows and columns directly rather than looping over indexes.  You can use enumerate to keep track of the rows and columns.
You shouldn't flatten, you can index in two dimensions.
Instead of vstack, you should use pad in this case.
You create U and V, then immediately overwrite them.  This is redundant.
You should use ravel instead of flatten since it doesn't make a copy.


Counting SQL guids from a server log and printing stats I've made a python script to get some statistics from a server log. I would like some guidelines to improve my code: make it run faster, be more pythonic, etc. I don't write python professionally (embedded developer) so please go easy on me.

Here's the code:

'''
Find the number of 'exceptions' and 'added' event's in the exception log
with respect to the device ID.

author: clmno
date: Dec 23 2018
'''

from time import time
import re

def timer(fn):
    ''' Used to time a function's execution'''
    def f(*args, **kwargs):
        before = time()
        rv = fn(*args, **kwargs)
        after = time()
        print("elapsed", after - before)
        return rv
    return f

def find_sql_guid(txt):
    ''' From the passed in txt, find the SQL guid using re'''
    re1 = '.*?'
    # SQL GUID 1
    re2='([A-Z0-9]{8}-[A-Z0-9]{4}-[A-Z0-9]{4}-[A-Z0-9]{4}-[A-Z0-9]{12})'
    rg = re.compile(re1+re2, re.IGNORECASE|re.DOTALL)
    m = rg.search(txt)
    if m:
        guid1 = m.group(1)
        return guid1
    else:
        print("ERROR: No SQL guid in line. Check the code")
        exit(-1)

def find_device_IDs(path, element):
    """ Find the element (type: str) within the file (file path is
        provide as arg). Then find the SQL guid from the line at hand.
        (Each line has a SQL guid)
        Return a dict of {element: [<list of SQL guids>]}
    """
    lines = dict()
    lines[element] = []
    with open(path) as f:
        for line in f:
            if str(element) in line:
                #find the sql-guid from the line-str & append
                lines[element].append(find_sql_guid(line))
    return lines

# @timer
def find_num_occurences(path, key, search_val, unique_values):
    """ Find and append SQL guids that are in a line that contains a string
        that's in search_val into 'exception' and 'added'
        Return a dict of {'exception':set(<set of SQL guids>),
                        'added': set(<set of SQL guids>)}
    """
    lines = {'exception':set(), 'added': set()}

    with open(path) as f:
        for line in f:
            for value in unique_values:
                if value in line:
                    if search_val[0] in line:
                        lines['exception'].add(value)
                    elif search_val[1] in line:
                        lines['added'].add(value)
    return lines

def print_stats(num_exceptions_dict):
    for key in num_exceptions_dict.keys():
        print("{} added ".format(key) +
            str(len(list(num_exceptions_dict[key]["added"]))))
        print("{} exceptions ".format(key) +
            str(len(list(num_exceptions_dict[key]["exception"]))))

if __name__ == "__main__":
    path  = 'log/server.log'
    search_list = ['3BAA5C42', '3BAA5B84', '3BAA5C57', '3BAA5B67']

    #find every occurance of device ID and find their corresponding SQL
    # guids (unique ID)
    unique_ids_dict = dict()
    for element in search_list:
        unique_ids_dict[element] = find_device_IDs(path, element)

    #Now for each unique ID find if string ["Exception occurred",
    # "Packet record has been added"] is found in it's SQL guid list.
    search_with_in_deviceID = ["Exception occurred",
                                "Packet record has been added"]

    num_exceptions_dict = dict()
    for elem in search_list:
        num_exceptions_dict[elem] = find_num_occurences(path, elem,
            search_with_in_deviceID, list(unique_ids_dict[elem].values())[0])

    print_stats(num_exceptions_dict)

and here's a small server log for you to experiment on.
This code works but the time elapsed to run through a log file of 55000 lines is 42 seconds

$ time python findUniqueVal_log.py
 real   0m42.343s
 user   0m42.245s
 sys    0m0.100s

I would like guidelines on:

Optimizing in both coding and performance efficiency
Any better approach to this problem
I keep opening the file and closing it. Is that IO delay causing any noticeable delay?

Your timer function should probably go away, to use the built-in timeit instead; or at least timer should wrap timeit.

I'm unclear on why re1 and re2 are separate. Just have them in the same string. If you want to make it clear that they're separate parts of the regex, you could show the construction of the regex as a concatenation, i.e.

rg = re.compile('.*?'  # Prefix to do foo
                # This is a SQL GUID
                '([A-Z0-9]{8}-[A-Z0-9]{4}-[A-Z0-9]{4}-[A-Z0-9]{4}-[A-Z0-9]{12})',
                re.IGNORECASE | re.DOTALL)

Also, you should not have an outer group on that regex.

Your else after return guid1 is redundant, since the previous block has already returned.

lines = dict() should be lines = {}. There's a reason for this: generally, shorter code is considered more "Pythonic" (as long as it doesn't become arcane or difficult to understand). A dictionary literal is short, simple, and easy to understand.

Here:

if str(element) in line:

You've already stated in the function doc that element is assumed to be a string, so you don't need to convert it using str.

Here:

unique_ids_dict[element] = find_device_IDs(path, element)

Your data structure use is a little strange. You're constructing a dictionary by element of single-key dictionaries by element of GUIDs. It would probably make more sense to change find_device_IDs such that it doesn't return a dictionary, and instead just returns a set.

Speed tips:

Sort out your data structure strategy, mentioned above
Don't re-compile your regex every time an inner function is called. Compile your regex once, at the global level

This:

num_exceptions_dict = dict()
    for elem in search_list:
        num_exceptions_dict[elem] = find_num_occurences(path, elem,
            search_with_in_deviceID, list(unique_ids_dict[elem].values())[0])

should probably be reduced to something like

num_exceptions_dict = {
    elem: find_num_occurences(path, elem, search_with_in_deviceID,
                              unique_ids_dict[elem].values())
    for elem in search_list
}

The performance-affecting change is to avoid re-constructing an inner list. But there's something more sinister going on there. A dictionary is unordered, but you're taking element [0] of an arbitrarily ordered list of values(). If you actually do intend to pull a non-deterministic value from the dictionary, then fine; but this is probably not your intention.

This:

search_with_in_deviceID = ["Exception occurred", "Packet record has been added"]

should be a tuple instead of a list, since you aren't changing it; so:

search_within_device_ID = ('Exception occurred', 'Packet record has been added')

I'm going to suggest that you attempt to address the issues that I've raised, above, in particular with respect to the structure of your dictionary and the non-deterministic behaviour of your values call; and then submit a new question so that further performance issues may be addressed.


Showing a message when the column is true I'm making an app using Rails. I have 4 Boolean columns in a table like below, and I want to show a message if that column is true.

class CreatePosts < ActiveRecord::Migration[5.2]
  def change
    create_table :posts do |t|
      t.integer :user_id, null: false, default: 0
      t.string :title, null: false, default: ''
      t.text :description, null: true
      t.boolean :tag_1, null: false, default: false
      t.boolean :tag_2, null: false, default: false
      t.boolean :tag_3, null: false, default: false
      t.boolean :tag_4, null: false, default: false

      t.timestamps
    end
  end

posts_controller.rb

def index
  @posts = Post.all
end

index.html.erb

<% @posts.each do |post| %>

    <% if post.tag_1 %>

<%= 'tag_name_A' %>

    <% end %>
    <% if post.tag_2 %>

<%= 'tag_name_B' %>

    <% end %>
    <% if post.tag_3 %>

<%= 'tag_name_C' %>

    <% end %>
    <% if post.tag_4 %>

<%= 'tag_name_D' %>

    <% end %>
    <% if !post.tag_1 && !post.tag_2 && !post.tag_3 && !post.tag_4 %>

none

    <% end %>

<% end %>

tag_name_A will be shown if tag_1 is the only true column, and there'll be 'tag_name_B' & 'tag_name_D' shown if tag_2 and tag_4 are both true. (I hope I'm making myself clear.)

This code is working fine and I'm already getting what I want, but I just don't like how they are written. It looks messy and obviously not smart. How would you optimize it?

There's a lot of great work here, I have a few suggestions, I'll start with a few recommended changes, and finish with some suggestions for improvements.

Let's firstly take a look at your migration.

Do not set a default on a foreign key column. Your user_id defaults to 0, which could be an actual user record, so if a record was ever (somehow) created or modified to without a user_id, it would suddenly belong to that user.

You can take this opportunity to add an index on the user_id for the posts table, it will make certain requests significantly faster, such as looking up all of the posts belonging to one user.

Moving on to your view.

You can add a method to your Post model to indicate if the post has no tags. Rather than using !post.tag_1 && !post.tag_2 && !post.tag_3 && !post.tag_4 in the view, we can make a method to handle that logic for us.

Imagine something like this in your Post model:

def no_tags?
  !tag_1 && !tag_2 && !tag_3 && !tag_4
end

This would mean you could change your view from:

<% if !post.tag_1 && !post.tag_2 && !post.tag_3 && !post.tag_4 %>

none

<% end %>

to:

<% if post.no_tags? %>

none

<% end %>

which I think reads a lot more easily. This kind of method, where you're not modifying data but simply reading it, is sometimes refactored (tidied up) using design patterns like decorators or presenters. It's common to have methods that are really only used for the presentation of data, rather than manipulation, and those design patterns can help keep them away from the business logic of the model.

Those are the first minor changes I'd make to this code, without significantly changing the architecture. If you wanted to take it further, however, I would suggest the following.

It's clear from what you've said in your post that you can see there's something not quite right. Those conditionals in the view are probably the obvious 'smell'.

Think through a moment, and consider what you would do if you needed to add another tag.

You'd first need to add a column in the posts table, to store the boolean value. You'd need to modify our new no_tags? method to include tag_5, and you'd need to change the view to add another conditional, checking to see if tag_5 was present.

I think you'll be much better off if you define a many-to-many relationship between the Post model and a new Tag model. Each tag model can have a name, and your post model will has_and_belongs_to_many tags, allowing you to change your view code to something like:

<% post.tags.each do |tag| %>

<%= tag.name %>

<% end %>

I recommend you read the rails guide on relationships, specifically: https://guides.rubyonrails.org/association_basics.html#has-and-belongs-to-many-association-reference