JavaScript constructor and namespace in object literal style Is this a good way to create a class with a constructor and namespace in object literal style?

// root namespace
var myApp = myApp || {};

// sub namespace
myApp.model = {
    // Constructor
    Person: function (name) {
        this.name = name;
    }
};

myApp.model.Person.prototype = {
    sayName: function () {
        alert(this.name);
    },
    sayHi: function () {
        alert("Hi, " + this.name);
    }
};

var p1 = new myApp.model.Person("CK");
p1.sayName();
p1.sayHi();

JSHint shows no error and the full source code is here.

I would suggest splitting the addition of the methods as follows:

myApp.model.Person.prototype.sayName = function () {
    alert(this.name);
};

myApp.model.Person.prototype.sayHi = function () {
    alert("Hi, " + this.name);
};

This way you can define additional methods in different places (if needed), as you're not replacing the entire prototype in one go.


Java Stream Exception Handling I find this piece of Java code really ugly and cumbersome. How can I refactor it to be more clean and easier to read?

ByteArrayOutputStream out = null;
try {
    out = new ByteArrayOutputStream();
    workbook.write(out);
    return out.toByteArray();
} catch (Exception e) {
    e.printStackTrace();
} finally {
    try {
        if (out != null) {
            out.close();
        }
    } catch (IOException e) {
        e.printStackTrace();
    }
}

return null;

From java 7 on you can use the try-with-resources syntax :

try (ByteArrayOutputStream out = new ByteArrayOutputStream()) {
    workbook.write(out);
    return out.toByteArray();
} catch (Exception e) {
    e.printStackTrace();
}

return null;

Which handles the closing of the Stream, and any excptions that may throw, for you.

In earlier versions of Java, you're pretty much stuck with what you have.

Edit:

It is worth noting that in case you get an exception on the close() method, it is still the original exception that is being thrown. The exception on the close() method is then added to that exception as a suppressed exception. You can get at the suppressed exceptions by calling java.lang.Throwable#getSuppressed


Verify a credit card with Luhn This is my solution for the Credit problem on CS50's (Introduction to CS course) Pset1. It involves using Luhn's Algorithm to test the validity of the Credit Card Number entered and based on a few conditions attempts to identify the Credit Card Company.

check_length attempts to find the length of the number entered. check_company attempts to ID the company. check_luhn validates the number based on Luhn's Algorithm.

Lunh's Algorithm:

The formula verifies a number against its included check digit, which is usually appended to a partial account number to generate the full account number. This number must pass the following test:

From the rightmost digit and moving left, double the value of every second digit.  If the result of this doubling operation is greater than 9 (e.g., 8 × 2 = 16), then add the digits of the result (e.g., 16: 1 + 6 = 7, 18: 1 + 8 = 9) or, alternatively, the same final result can be found by subtracting 9 from that result (e.g., 16: 16 − 9 = 7, 18: 18 − 9 = 9).
Take the sum of all the digits.
If the total modulo 10 is equal to 0 (if the total ends in zero) then the number is valid according to the Luhn formula; otherwise it is not valid.
I am not very comfortable with the so many if-else conditions. I'd like to know if they can be eliminated. Any other changes are also welcome.

#include <stdio.h>
#include <cs50.h>
#include <stdbool.h>

int check_length(long);
void check_company(int,long);
bool check_luhn(long,int);
int length;

int main(void)
{
    long c = get_long("Enter Credit Card Number: ");
    check_length(c);
    check_luhn(c,length);
    if(check_luhn(c,length)==true)
    {
            check_company(length,c);
    }
    else printf("INVALID\n");

}

int check_length(long w)
{
    for(int i=12;i<16;i++)
    {
            long power = 1;
            for (int k=1;k<i+1;k++)
            {
                    power = power * 10;
            }
            int scale = w/power;
            if (scale<10 && scale>0)
            {
                    length = i+1;
            }
   }
   return length;
 }

 void check_company(int x,long z)
 {
    if(x == 15)
    {
            int y = z/10000000000000;           //z/10^13
            if(y==34||y==37)
            {
                    printf("AMEX\n");
            }
            else
            {
            printf("INVALID\n");
            }
    }
    else if(x==13)
    {
            int y = z/100000000000;                 //z/10^11
            if(y==4)
            {
                   printf("VISA\n");
            }
     }
     else if(x==16)
     {
             int q = z/1000000000000000;
             int y = z/100000000000000;
             if(y==51||y==52||y==53||y==54||y==55)
             {
                     printf("MASTERCARD\n");
             }
             else
             if(q==4)
             {
                     printf("VISA\n");
             }
             else printf("INVALID\n");
     }
     else printf("INVALID\n");
  }

  bool check_luhn(long a,int b)
  {
     int f = 0;
     int j=0;
     for(int d=1;d<b+1;d++)
     {
             int e = a%10;
             a = a/10;
             if(d%2==1)
             {
                    f = f+e;
             }
             else
             {
                     int m = 2*e;
                     int g = m%10;
                     int h = m/10;
                     j = j+g+h;
              }

      }
      int l = j + f;
      if(l%10==0)
      {
             return true;
      }
      else return false;
    }

Instead of all the if-else statements, you could use a switch statement, like so:

switch (x) {
  case 15:
    int y = z/10000000000000;
    if (y==34||y==37)
    ...
    ...
    ...
    break;
  case 13:
...
...
...
  default:
    printf("INVALID\n");
}

Also to make your code look cleaner, eliminate any curly brackets around if  statements that execute only one statement, for example,

if (l%10==0)
{
        return true;
}

could be:

if (l%10==0) return true;

Additionally, instead of

if (y==51||y==52||y==53||y==54||y==55)

use

if (y>=51&&y<=55)


Improving Java LinkList implementation I am trying to implement linklist data structure in java. Following is my implementation:

// A singly Linked List
class Node{

int value;
Node next;

Node(int n){
   this.value = n;
   this.next = null;
}

public void printNode(){
   System.out.println("Value: " + value);
}

}

class LinkList{

 private Node first;
 private Node last;

 public LinkList(){
  first = null;
  last = null;
}

public boolean isEmpty(){
   return first == null;
}

public void insertFirst(int n){

 Node node = new Node(n);

 if(first == null){
    System.out.println("1. Vlaue of n: "+n);
    first = node;
    last = node;
 }else{
    System.out.println("2. Vlaue of n: "+ n);
    Node tmp = first;
    first = node;
    node.next = tmp;

  }

 }

 public void deleteFirst(){
   if (!isEmpty()){
      Node tmp = first;
      first = tmp.next;
  }
}

public void deleteLast(){

 if(!isEmpty()){

    Node tmp = first;
    Node oneBeforeLast = null;

    while (tmp.next != null){
        oneBeforeLast = tmp;
        tmp = tmp.next;
    }

    oneBeforeLast.next = null;

   }
 }

 public void deleteNode(int value){

  if (!isEmpty()) {

    Node tmp = first;
    Node oneBeforeLast = first;

    while (tmp.next != null) {

        if (tmp.value == value) {

            if (oneBeforeLast == first) {
                first = tmp.next;
            } else
                oneBeforeLast.next = tmp.next;
        }
        oneBeforeLast = tmp;
        tmp = tmp.next;
        System.out.println("Btmp: " + oneBeforeLast.value + " tmp:
        " + tmp.value);
    }

     if (tmp.next == null && tmp.value == value) {
        oneBeforeLast.next = null;
     }
    }
 }

public void printList(){

  Node tmp = first;

  System.out.println("\nPrinting List:");

  while(tmp != null){
     tmp.printNode();
     tmp = tmp.next;
   }
 }
}

public class LinkListTest {

 public static void main(String[] args) {

 // TODO Auto-generated method stub
 LinkList linklist = new LinkList();
   linklist.insertFirst(1);
   linklist.insertFirst(2);
   linklist.insertFirst(3);
   //linklist.insertFirst(3);
   linklist.insertFirst(4);

   linklist.printList();

   //linklist.deleteFirst();
   //linklist.printList();

  //linklist.deleteLast();
  //linklist.printList();

   linklist.deleteNode(1);
  linklist.printList();
 }

}

I would like to improve it further and check its efficiency for large inputs. Can anyone give some pointers on these two pointers, please?

One area you could improve performance is by making it a doubly linked list.

At the moment your deleteLast() method is an O(n) operation, meaning it needs to traverse the entire list to delete the last element. And since this is the case, there's no real reason for keeping a reference to it at all.

If each Node had a next and a prev Node, your delete last could look something like,

last.prev.next = null

and let the garbage collector worry about the rest.
You would of course still need to check that all of these values are okay to use like this before hand.

LinkedLists are supposed to be good at adding and deleting elements from the start. And if it's a doubly linked list, also the end.

As side note, I would prefer to see your Node class as a private class inside your linked list class. As this Node class probably shouldn't be used anywhere else.

Your public interface is a bit unusual, for a list, I would expect to have public methods like list.add(element), list.removeAt(0) etc.

list.insertFirst(data) is an implementation detail, this should be a private method that's called when the list is empty. if I wanted to insert an element at the head of the list, I would prefer list.insertAt(0)

Another thing that stands out is your deleteNode method. This is definitely an implementation detail leaking out. A user of the list shouldn't know at all about Nodes.

The method doesn't even take a node, it takes an int, I would prefer list.delete(element).

Also, your list won't properly support duplicate values, if I have 3 nodes with the value if 5, and I try to delete 5, it will delete the first node with a value of 5 it finds.

As another side, you should make as many variables private as possible.


Boggle board solver in Ruby The following is my attempt at creating a Boggle board solver in Ruby. It's not especially fast. I would love to have input on how the solution can either be sped up, or whether a different algorithm might be applied that is faster.

The solution corresponds to my answer on a related question on the Math Stack Exchange site: https://math.stackexchange.com/a/2908479/30919

require 'set'
# List of words in a file, imported into sorted set for quick lookup:
WORDS = SortedSet.new(open('wordlist.txt').readlines.map(&:chomp))
# Possible directions from a space and their x/y deltas:
DIRS = { n: [0, -1], ne: [1, -1], e: [1, 0], se: [1, 1], s: [0, 1], sw: [-1, 1], w: [-1, 0], nw: [-1, -1] }.freeze

# Note on "Qu" -- "Qu" should require special handling in the

# Sample board we want to solve; we could create a random board with
# the known configuration of the cubes.
# For 'Qu' on the Boggle board, only 'Q' is entered in @board, because the traversion routine
# has special handling for 'Q' by adding a 'U' each time
@board = [
  %w[ N P E R ],
  %w[ P L R O ],
  %w[ I U N T ],
  %w[ J E G V ]
]

# This method determines the next position based on the currend position
# and direction. If the position is out of bounds, it returns nil
def next_pos(position, direction)
  npos = [
    position[0] + DIRS[direction][0],
    position[1] + DIRS[direction][1]
  ]
  return nil if (npos[0] < 0) || (npos[1] < 0) ||
                (npos[0] > 3) || (npos[1] > 3)
  npos
end

@stack = [] # Stack containing the positions traversed
# NB: **The letters have their own stack due to the special handling of "Qu"**
@letters = [] # Corresponding letters from the positions in stack
@found_words = SortedSet.new # Words found
def traverse(cpos)

    # Nested method that handles popping of stack with special 'Qu' handling (because it's used more than once)
    def pop_stack
        @stack.pop
        @letters.pop
        # Special handling for "Qu":
        @letters.pop if @letters.last == "Q"
    end

  #The longest words in my list are 15 letters - omit this check if you have more
  return if @stack.length == 15

  # Push current position and letters collected so far on the respective stacks
  @stack << cpos
  @letters << @board[cpos[0]][cpos[1]]
  # Special handling for Qu:
  @letters << "U" if @letters.last == "Q"

  # From 2 letters onwards, check whether this path is worth pursuing
  if @stack.length >= 2
    unless WORDS.grep(/^#{@letters.join}/).any?
      pop_stack
      return
    end
  end

  # From 3 letters onwards, check if the word is in the dictionary
  if @stack.length >= 3
    word = @letters.join
    if WORDS.include?(word)
      @found_words << word
    end
  end

  # Try each direction in succession from the current position
  DIRS.keys.each do |dir|
    npos = next_pos(cpos, dir) # Calculate next pos from current pos
    next unless npos # Check it is not out of bounds
    # Check whether the new position had been visited in current stack
    visited = false
    @stack.reverse_each do |pos|
      if pos == npos
        visited = true
        break
      end
    end
    next if visited # Skip the new position if it had been visited
    traverse(npos) # Start looking from next position
  end

  # Take the last position/letter off the stack before returning
  pop_stack

end

# Traverse from every square on the board
4.times.map do |x|
  4.times.map do |y|
    traverse([x, y])
  end
end

puts @found_words.sort

I can't comment on Ruby style, but there are a couple of algorithmic points which don't need Ruby knowledge to identify.

  if @stack.length >= 2
    unless WORDS.grep(/^#{@letters.join}/).any?
      pop_stack
      return
    end
  end

Although you've preprocessed the wordlist into a sorted data structure with logarithmic time lookup, grep seems to be a method from Enumerable and I can't see any indication that there are special cases. In other words, this is (worst case) doing a linear scan of the entire wordlist. There should be a massive speed improvement either by exploiting the SortedSet interface or (more likely) by preprocessing the wordlist into a trie. The latter gives the advantage that you can pass the current node down the stack and not have to run the entire route from the root every time.

    # Check whether the new position had been visited in current stack
    visited = false
    @stack.reverse_each do |pos|
      if pos == npos
        visited = true
        break
      end
    end

Again, this is doing a linear scan for something which doesn't need to be so slow. The OO way would be to use a Set, which ought to have fast member? checks. The optimisation-over-all approach would be to use an integer as a bitmask. Either approach requires minimal maintenance on the stack.


Iterative Implementation of Negamax Alpha-Beta Please help me check the correctness of this c# iterative implementation of negamax! Thank you! Your help highly appreciated! Also, please help me out how to add alpha beta pruning to this(it was supposed to be alpha beta with negamax, but i forgot the alpha beta part... :<)! Thanks in advance!

EDIT: I DO think that the implementaqtion works. I only want it to be checked. Also, i simply want SUGGESTIONS and GUIDANCE on how to add alpha beta to the algorithm, not someone write the code for me. Please do not be mistaken. Thank you.

Random r = new Random();

Stack<int> maxs = new Stack<int>();

Stack<int> levels = new Stack<int>();
Stack<string> todo = new Stack<string>();

Stack<string> undo = new Stack<string>();

maxs.Push(int.MinValue);

var le = legals(bd);
while (le.Any())
{
    var mv = le[r.Next(le.Count)];
    le.Remove(mv);
    todo.Push(mv);
    levels.Push(1);
}

while (todo.Any())
{
    //init
    string mv = todo.Pop();
    int level = levels.Pop();
    undo.Push(mv);
    if (level != depthleft) maxs.Push(int.MinValue);
    move(mv, level % 2 == 0 ? "b" : "w");

    //expand children/do operations,cleanup
    if (level == depthleft)
    {
         int score = -eval(bd, level % 2 == 0);
         if (score > maxs.Last())
         {
              maxs.Pop();
              maxs.Push(score);
         }
         unmove(undo.Pop());

         int diff = level - levels.Last();
         if (diff != 0)
         {
              for (; diff > 0; diff--)
              {
                   score = -maxs.Pop();
                   if (score > maxs.Last())
                   {
                        maxs.Pop();
                        maxs.Push(score);
                   }
                   unmove(undo.Pop());
              }
         }
     }
     else
     {
          var ls = legals(bd);
          while (ls.Any())
          {
              var child = ls[r.Next(ls.Count)];
              ls.Remove(child);
              todo.Push(child);
              levels.Push(level + 1);
          }
     }
}

Some quick suggestions:

Split the code into smaller methods, with understandable names - it will become simpler!
Use clear variable names (even if they are longer) and try to avoid "shortenings";
Never use less than 3 characters for variable names... le? r? mv?!? (int i in for cycles is an exception because it is idiomatic);
Use PascalCase (not camelCase) for methods and properties;
Use camelCase (not lowercase) for local variables;
Don't repeat code (var le = legals(bd); while (le.Any()) { ... }), extract it to a method.


Parent/child relationships: adding/updating details to an answer Answer is the Parent table and AnswerDetail is the child.  Below works, but I'm wondering if there is a better way to do this using EF?

My Method Signature is this:

    [HttpPost]
    public ActionResult Evaluator(EvaluationVM evaluation, string command)

It is coming from a MVC 4 application.

Answer AnswerRecord;
AnswerRecord = (db.Answers).Where(x => x.TeacherID.Equals(evaluation.CurrentTeacher.ID)).Where(y => y.LeaderID != null).FirstOrDefault<Answer>();

if (AnswerRecord == null)
{
     //Add New Parent Record:
     AnswerRecord.CreatedBy = userName;
     AnswerRecord.CreateStamp = DateTime.Now;
     db.Entry(AnswerRecord).State = EntityState.Added;

     AnswerRecord.UpdatedBy = userName;
     AnswerRecord.UpdateStamp = DateTime.Now;
 }

 AnswerRecord.UpdatedBy = userName;
 AnswerRecord.UpdateStamp = DateTime.Now;

 //I use this block if I know the child record is new:
 adItem = db.AnswerDetails.Find(item.LeaderAnswerDetailKey);
  if (adItem != null)
  {
       adItem.Comment = item.LeaderComment;
       adItem.AnswerOptionKey = item.LeaderAnswerOptionKey.Value;
       adItem.UpdatedBy = userName;
       adItem.UpdateStamp = DateTime.Now;
       db.Entry(adItem).State = EntityState.Modified;
  }

 foreach (EvaluationObject item in evaluation.ResultSet)
 {
 //I use this block if I am updated the child record
 adItem = new AnswerDetail();
 adItem.QuestionID = item.QuestionID.Value;
 adItem.AnswerOptionKey = item.TeacherAnswerOptionKey.Value;
 adItem.Comment = item.TeacherComment;

 adItem.CreatedBy = userName;
 adItem.CreateStamp = DateTime.Now;
 adItem.UpdatedBy = userName;
 adItem.UpdateStamp = DateTime.Now;
 adItem.AnswerKey = AnswerRecord.AnswerKey;

 AnswerRecord.AnswerDetails.Add(adItem);
 db.Entry(adItem).State = EntityState.Added;

 //This is called at the end of my method:
 db.SaveChanges();

 }//End of ForEach Loop

EvaluationVM

public class EvaluationVM
{
    public bool IsAdmin { get; set; }
    public bool IsLeader { get; set; }
    public int TeacherStatus { get; set; }
    public int LeaderStatus {get;set;}

    public bool IsPublicTeacher { get; set; }
    public bool IsFinalTeacher { get; set; }
    public bool IsPublicLeader { get; set; }
    public bool IsFinalLeader { get; set; }

    public List<EvaluationObject> ResultSet {get; set;}
    public TeacherInfo CurrentTeacher {get; set;}
    public LeaderInfo CurrentLeader { get; set; }

    public List<EvaluationRating> RatingSet {get;set;}
}

EvaluationObject:

    public class EvaluationObject : IEvaluationObject
    {
        public int? QuestionID { get; set; }
        public string IndicatorID { get; set; }
        public string QuestionDescription { get; set; }

        public string TeacherID { get; set; }
        public int? TeacherStatusKey { get; set; }
        public int? TeacherAnswerKey { get; set; }
        public int? TeacherAnswerDetailKey { get; set; }
        public int? TeacherAnswerOptionKey { get; set; }
        public string TeacherComment { get; set; }

        public string LeaderID { get; set; }
        public int? LeaderStatusKey { get; set; }
        public int? LeaderAnswerKey { get; set; }
        public int? LeaderAnswerDetailKey { get; set; }
        public int? LeaderAnswerOptionKey { get; set; }
        public string LeaderComment { get; set; }

    }

Separation of Concerns
I like that you're using EF's built-in unit-of-work implementation. However db (your DbContext class) looks like it's declared at instance level in your controller - make sure it's disposed properly at the end of the request (you could inject the context in the controller's constructor and use an IoC container to ensure per-request instantiation & disposal).
That doesn't mean the controller's [HttpPost] methods should be doing all the work!
Looking at the method's code, I think you can extract an entire service class that exposes at least 3 methods!

Answer CreateNewAnswer(string userName)
AnswerDetail FindByLeaderId(int leaderId)
AnswerDetail CreateNewAnswerDetail(int answerId, AnswerDetailVM item)

Extracting these methods into their own class will make it much easier to follow your controller methods' code, by adding a level of abstraction - the controller shouldn't be dealing with minute details, rather should call into more specialized objects that do their specialized stuff. Shortly put, separate the concerns ;)

SaveChanges
You don't need to call db.SaveChanges() for every entity you create in the loop - EF's DbContext is a unit-of-work that encapsulates a transaction, so calling SaveChanges is like saying "I'm done, now commit all these pending changes!" - call it once (or, as sparingly as possible - e.g. the parent would typically needs to exist in the db before a child can be added), when you're done.

Naming & Other Nitpicks

Answer AnswerRecord;
AnswerRecord = (db.Answers).Where(x => x.TeacherID.Equals(evaluation.CurrentTeacher.ID)).Where(y => y.LeaderID != null).FirstOrDefault<Answer>();

For readability, I prefer to split these across multiple lines:
AnswerRecord = (db.Answers).Where(x => x.TeacherID.Equals(evaluation.CurrentTeacher.ID))
                           .Where(y => y.LeaderID != null)
                           .FirstOrDefault<Answer>();

This is confusing, because x and y refer to the same object. Also I'd prefer == over .Equals in most cases, so I'd write it like this instead:
var answer = db.Answers.Where(answer => answer.TeacherID == evaluation.CurrentTeacher.ID
                                     && answer.LeaderID != null)
                       .FirstOrDefault();

The type parameter for FirstOrDefault is inferred from usage, doesn't need to be specified ;)

Comments
If these comments are real, in-code comments...

//I use this block if I know the child record is new:

//I use this block if I am updated the child record

...then they're both lying - the code blocks under each comment seems to be doing what the other comment is saying! Remove these misleading comments, calling _service.CreateNewAnswerDetail should be clear enough that you're creating a new AnswerDetail entry ;)
As for this one:

//Add New Parent Record:

and this one:

//End of ForEach Loop

...they both say nothing that the code doesn't say already, remove them as well, thank yourself later ;)


SO golang example with infinite for / channel workers I whipped this up as an example of how infinite loops can often be used to great effect in golang, and also to discuss closing channels and worker communications.  It was strictly off the cuff, but I was curious if anybody would like to offer a critique.  In particular, I feel as though inflight is a crude implementation detail.  I've done it many times, and looking at wait group implementation, seems like it is a synchronized version of the same thing.
package main

import (
  "log"
)

func doStuff(datachan <-chan map[string]string, reschan chan<- int) {
  for {
    data, ok := <-datachan
    if !ok {
      log.Print("Channel closed.")
      break
    }
    log.Printf("Data had %d length: %+v", len(data), data)
    reschan<-len(data)
  }
  return
}

const workers = 3

func main() {
  var datachan = make(chan map[string]string)
  var reschan = make(chan int)
  var inflight = 0
  var inputs = []map[string]string {
    map[string]string{ "hi": "world" },
    map[string]string{ "bye": "space", "including": "moon" },
    map[string]string{ "bye": "space", "including": "moon" },
    map[string]string{ },
    map[string]string{ },
  }
  // an inline funciton definition can change inflight within main()'s scope
  processResults := func (res int) {
    log.Printf("Main function got result %d", res)
    inflight--
  }
  // start some workers
  for i := 0; i < workers; i++{
    go doStuff(datachan, reschan)
  }
  for _, data := range inputs {
      //Select allows reading from reschan if datachan is not available for
      // writing, thus freeing up a worker to read from datachan next loop
      written := false
      for written  != true {
        select {
          case res := <-reschan:
            processResults(res)
          case datachan <- data:
            inflight++
            written = true
        }
      }
  }
  close(datachan)
  for inflight > 0 {
    processResults(<-reschan)
  }
}

You wrote:
func doStuff(datachan <-chan map[string]string, reschan chan<- int) {
    for {
        data, ok := <-datachan
        if !ok {
            log.Print("Channel closed.")
            break
        }
        log.Printf("Data had %d length: %+v", len(data), data)
        reschan <- len(data)
    }
    return
}

I don't understand why you didn't write:
func doStuff(datachan <-chan map[string]string, reschan chan<- int) {
    for data := range datachan {
        log.Printf("Data had %d length: %+v", len(data), data)
        reschan <- len(data)
    }
    log.Print("Channel closed.")
}


Cookie Clicker Switches In Cookie Clicker there are Golden cookies, and Wrath Golden Cookies.

Golden cookies are Clickable cookies that randomly appear, and when clicked they give Bonus cookies, or temporary boosts to clicking/Cookies Per second (i.e. Cps).

The Wrath Golden cookies can still give boosts and bonuses, but they have the chance of taking cookies or giving a reverse boost to Cps.

Wrath Golden Cookies happen as an effect of pushing the Grandmas too far.

Some "power-ups" require Golden cookies to stay on the screen and not be clicked right away, so I wanted a fast way to turn off the clicker.

I came up with a way to turn on and off the Golden/Wrath switches using a single command in the console, and they give a little more information as well.

// allows me to turn on and off the clicking of the Wrath Cookies

var wrathCookieSwitchOn = false;
var cookieSwitchOn = false;

var wrathSwitch = function() {
    wrathCookieSwitchOn = !wrathCookieSwitchOn;
    console.log("WrathCookieSwitchOn is set to " + wrathCookieSwitchOn);
}

var flipCookieSwitch = function(){
    cookieSwitchOn = !cookieSwitchOn;
    if (cookieSwitchOn) {
        var SpecialCookieClicker =
            setInterval(function() {
                Game.shimmers.forEach(function(shimmer) {
                    (shimmer.type == "golden") && (shimmer.wrath == 0 || wrathCookieSwitchOn) ? shimmer.pop() : 0;
                })
            }, 500);
        console.log("Golden Cookie Clicker is on and wrathCookieSwitchOn is set to " + wrathCookieSwitchOn);
    } else {
        clearInterval(SpecialCookieClicker);
        console.log("Golden Cookie Clicker is Turned off");
    }
};
flipCookieSwitch();

I was thinking about overloading the function so that I could flip the Wrath Switch as well, but I haven't decided how I want to do that yet.

Does the code make sense to you? Is there anything I could make better or more efficient?

Link to the Game itself, it is addicting and YOU HAVE BEEN WARNED

none of the scripts that I have posted here are considered cheating by the game

Responses

  Does the code make sense to you?

Yes, for the most part. I am still becoming acquainted with the game play and hacks.

  anything I could make better or more efficient?

In general, using an integer 0/1 instead of boolean would lead to faster performance1. To change the value, the bitwise NOT operator (i.e. ~) can be used. Note that applying NOT on a signed integer will yield -` but that is still a truthy value. While it was asked more than 6 years ago, this post has some interesting discussion on that matter.

var wrathCookieSwitchOn = 0;
var cookieSwitchOn = 0;

var wrathSwitch = function() {
  wrathCookieSwitchOn = ~wrathCookieSwitchOn;
  console.log("WrathCookieSwitchOn is set to " + wrathCookieSwitchOn);
}

function checkWrathCookieSwitchOn() {
  if (wrathCookieSwitchOn) {
    console.log('wrathCookieSwitchOn',wrathCookieSwitchOn);
  }
}
wrathSwitch();
checkWrathCookieSwitchOn();
wrathSwitch();
checkWrathCookieSwitchOn();

Other thoughts

Ternary vs Short-circuiting logical operators

As I mentioned in an answer to your previous post, the ternary could be simplified from:

(shimmer.type == "golden") && (shimmer.wrath == 0 || wrathCookieSwitchOn) ? shimmer.pop() : 0;

To:

(shimmer.type == "golden") && (shimmer.wrath == 0 || wrathCookieSwitchOn) && shimmer.pop();

While it only saves about 2 keystrokes, it is essentially like removing an else clause from an if statement.

Variable declaration

The declaration of the timer variable SpecialCookieClicker is within the true block of the if statement. While hoisting allows the variable to exist throughout the function,
"it is recommended to always declare variables at the top of their scope"2 and then assign the value later (e.g..

var flipCookieSwitch = function(){
    var SpecialCookieClicker; //declare here
    cookieSwitchOn = !cookieSwitchOn;
    if (cookieSwitchOn) {
        SpecialCookieClicker = setInterval(/*function */, 500); //assign here
    }
}

While it should be the case that SpecialCookieClicker will always be assigned before the else block, it may be wise to guard against the scenario of an undefined value (e.g. due to cosmic rays) and ensure it is at least truthy before calling clearInterval().

SpecialCookieClicker && clearInterval(SpecialCookieClicker);

Variable names

The word On in variables wrathCookieSwitchOn and cookieSwitchOn convey the sense that the switch is on when the variable is true. Without the word On it could still be considered semantically the same - so one could argue that the names could be shortened to wrathCookieSwitch and cookieSwitch without any loss of semantics

ES-6

Presuming you aren't using IE, you should be able to utilize ES-6 features, like const for values that shouldn't be re-assigned and let for variables that can change. Then the one-liner for that function to pop shimmers can be simplified with an arrow function:

Game.shimmers.forEach((shimmer) => (shimmer.type == "golden") && (shimmer.wrath == 0 || wrathCookieSwitchOn) && shimmer.pop(), 500);

1https://jsperf.com/bool-vs-int/8

2https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/var#var_hoisting


Lambda .Where() expression to return selected/flagged objects In our software we enable certain features based on the existence of a flag.  The Flags class is a static class used to determine the existence of such a flag.

I came across one piece of logic that I wrote that I feel could benefit from refactoring.  The boolean logic works, but I think it's difficult to understand or read because it relies very heavily on short-circuiting.

The basic logic is:

Get all menu items.
Return a subset of those menu items by using the following rule:

Some menu items are tied to a specific flag that is expected to be hard-coded in some way.  The menu item name may not match the associated flag.
Any other menu items should be displayed.

Any help would be much appreciated.

private IEnumerable<MenuItemDefinition> GetMenuItems()
{
     return GetAllMenuItems().Where(mi =>
          (mi.MethodName != "NewWidget" || Flags.HasFlag("Feature1")) &&
          (mi.MethodName != "NewDongle" || Flags.HasFlag("Feature2")));
}

If you control the MenuItemDefinition class, why not just add a feature tag to it?

public class MenuItemDefinition
{
    public string MethodName { get; set; }
    public string Feature { get; set; }
}

// each instance
new MenuItemDefinition { MethodName = "NewWidget", Feature = "Widgets" }

// GetMenuItems():
return GetAllMenuItems().Where(m => FeatureIsEnabled(m.Feature));

If not, you could put them in a dictionary:

var items = new Dictionary<string, MenuItemDefinition[]> {
    { "Widgets", new[] {
        new MenuItemDefinition { MethodName = "NewWidget" },
        new MenuItemDefinition { MethodName = "DeleteWidget" }
    },
    { "Default", new[] {
        // ....
    }
}

return items.Where(pair => FeatureIsEnabled(pair.Key)).SelectMany(pair => pair.Value);

Or better yet, build the initial list based upon content in a dictionary. That way you don't run the risk of having typos in the switch or the initial setup:

var dict = new Dictionary<string, string[]>
{
    { "Widgets", new[] { "NewWidget", "DeleteWidget" },
    { "Default", new[] { "..." }
    ...
}

var menuItems = dict
    .Where(p => FeatureIsEnabled(p.Key))
    .SelectMany(p => p.Value)
    .Select(mn => new MenuItemDefinition { MethodName = mn });

// GetMenuItems()
return menuItems; // :)


Writing C#'s analog of setTimeout, setInterval and clearInterval I got tired of starting timer every time I need to do something simple as running some function 5 seconds after something in my code happens. So I tried to write a function (actually, 3 of them: setTimeout, setInterval and clearInterval) to write one line of code when I need it instead of, say, five.

It is working, at least for the cases I tested.

I understand that maybe something in that code is not right, maybe I forgot about something. Should I improve anything in my approach?

using System;
using System.Windows.Forms;

namespace timeouts_test
{
    public partial class Form1 : Form
    {
        private Timer[] timerList = new Timer[100];
        private int timerMaxIndex = -1;
        public Form1()
        {
            InitializeComponent();
            int aInterval = SetInterval(500, a);
            int bInterval = SetInterval(500, b);
            int cInterval = SetInterval(500, c);

            SetTimeout(3500, () =>
            {
                ClearInterval(aInterval);
            });

            SetTimeout(7000, () =>
            {
                ClearInterval(bInterval);
            });

            SetTimeout(10500, () =>
            {
                ClearInterval(cInterval);
            });
        }

        public void a()
        {
            label1.Text += "a";
        }

        public void b()
        {
            label1.Text += "b";
        }

        public void c()
        {
            label1.Text += "c";
        }

        public int SetInterval(int time, Action function)
        {
            Timer tmr = new Timer();
            tmr.Interval = time;
            tmr.Tick += new EventHandler(delegate (object s, EventArgs ev)
            {
                function();
            });
            tmr.Start();

            timerMaxIndex++;
            var index = timerMaxIndex;
            timerList[timerMaxIndex] = tmr;
            return index;
        }

        public int SetTimeout(int time, Action function)
        {
            Timer tmr = new Timer();
            tmr.Interval = time;
            tmr.Tick += new EventHandler(delegate (object s, EventArgs ev)
            {
                function();
                tmr.Stop();
            });
            tmr.Start();

            timerMaxIndex++;
            var index = timerMaxIndex;
            timerList[timerMaxIndex] = tmr;
            return index;
        }

        public void ClearInterval(int interval)
        {
            if (timerList[interval] == null)
                return;

            timerList[interval].Stop();
            timerList[interval] = null;
            if (interval == timerMaxIndex)
                timerMaxIndex--;
        }
    }
}

(Functions a, b and c were made just to test this.)

Right now your timer methods have dependencies on stuff that shouldn't be.

You're relying on System.Windows.Form. That's blocking you if you ever want to use these methods in a console application or well... stuff that isn't related to WinForms. Check for the System.Timers.Timer class!

Your helper methods are relying on variables that aren't in their scope (The Timer[]). That's a dependency you want to remove.

To do so, we need a way to stop the timer. We could return the timer, but that might be a little too much for what we want to do. So let's create an interface, IInterruptable.

public interface IInterruptable
{
    void Stop();
}

Now, we need something to wrap our Timer so we can implement the interface.

public class TimerInterrupter : IInterruptable
{
    private readonly Timer _timer;

    public TimerInterrupter(Timer timer)
    {
        if (timer == null) throw new ArgumentNullException(nameof(timer));
        _timer = timer;
    }

    public void Stop()
    {
        _timer.Stop();
    }
}

So far so good. That deals with the problem of the Timer[].

Next, let's see how we can change your implementation using System.Timers.Timer.

Point one, not totally related to the new implementation but very related to your old one. Using delegate is a little old-school. Since C#... 3? There are Functions you can use, that are backward compatible to delegate. That means :

//Syntax might not be good, but you get the point
fooBar.SomeEvent += new delegate(object,object){asd();};

Becomes :

fooBar.SomeEvent += (a,b) => asd();

So, let's look at that timer! This Timer class as an AutoReset property, which well... auto-resets the timer at the end of each interval is set to true. Which will be the main difference between your SetInterval and SetTimeout methods.

public static IInterruptable SetInterval(int interval, Action function)
{
    Action functionCopy = (Action)function.Clone();
    Timer timer = new Timer { Interval = interval, AutoReset = true };
    timer.Elapsed += (sender, e) => functionCopy();
    timer.Start();

    return new TimerInterrupter(timer);
}

Notice that the method is now static, since it's an helper method. You'll be able to use it in any context, which is great!

I changed the time parameter to interval, as it's clearer this way. You didn't pass a time, you passed a time interval as a parameter. It's important to be very clear.

I'm copying the Action. I'm not 100% sure that's useful, but it's to avoid a problem with references. If for example I did :

Action someFunction = CreateAFunctionThatIsntNull();
SetInterval(1,someFunction);
someFunction = null;

You're screwed. So copying the Action has a purpose here.

Now the SetTimeout :

public static IInterruptable SetTimeout(int interval, Action function)
{
    Action functionCopy = (Action)function.Clone();
    Timer timer = new Timer { Interval = interval, AutoReset = false };
    timer.Elapsed += (sender, e) => functionCopy();
    timer.Start();

    return new TimerInterrupter(timer);
}

Notice something? Well, that's the same code, with one different variable in AutoReset. So let's take care of that by extracting another method :

public static class TimerHelper
{
    public static IInterruptable SetInterval(int interval, Action function)
    {
        return StartTimer(interval, function, true);
    }

    public static IInterruptable SetTimeout(int interval, Action function)
    {
        return StartTimer(interval, function, false);
    }

    private static IInterruptable StartTimer(int interval, Action function, bool autoReset)
    {
        Action functionCopy = (Action)function.Clone();
        Timer timer = new Timer { Interval = interval, AutoReset = autoReset };
        timer.Elapsed += (sender, e) => functionCopy();
        timer.Start();

        return new TimerInterrupter(timer);
    }
}

That's the final version in my opinion. I think it looks better. Though there's a final problem. SetTimeout and SetInterval are terrible method names. I know the idea is to copy the Javascript functions, but let's make an exception. These methods names are not intuitive and overall pretty terrible. Maybe something like ExecuteEvery instead of SetInterval and ExecuteIn instead of SetTimeout would be better names.


Applying prepare statements I don't feel that my code is secure, and would like some help in using prepare statements for the following;

$track = "INSERT INTO resources_record (name,email,stage,format,topic,max_cost,mentor,total_cost,duration)
          VALUES ('".$fullName."', '".$email."', '".$stage."', '".$format."', '".$topic."', '".$cost."', '".$mentor."', '".$price."', '".$duration."')";

// Execute track query
 mysqli_query($con, $track);

You are right, your code is indeed very insecure. I'm not sure which part of using prepared statements you are having trouble with, and rewriting your code for you seems off-topic, but the guide for PDO prepared statements is very clear. It's first example is even an INSERT. The guide for Mysqli prepared statements also contains an example for an INSERT. Just choose either Mysqli or PDO and follow the example.

Your code is quite short, so I only have a couple of points about it:

your comment is not needed, as it doesn't add any information.
your code could use more spaces to increase readability. Eg after , and between ..
I would use the same names for the variables as the db columns have to avoid confusion. Your db column names also seem to be more accurate. I can easily understand the meaning of max_cost and total_cost, but it's not that clear what cost and price stand for.


jQuery toggle class and content on click of link The following code is working exactly as it is expected to work, the thing is that I'm sure that it can be written in a more efficient and less verbose way, any reviews?

JavaScript:

$(document).ready(function() {
    $('.hidden-content').hide();
    $(".hidden-content-reveal").click(function() {
        var $i = $(this).children();
        if ($i.is(".angle-down")){
            $i.removeClass("angle-down");
            $i.addClass("angle-up");
        } else {
            $i.removeClass("angle-up");
            $i.addClass("angle-down");
        }
        return false;
    });
});

HTML:

Lorem ipsum dolor sit amet, consectetur adipiscing elit.

Nulla facilisi. Cras non egestas lectus. Duis malesuada eleifend neque, eget tincidunt elit convallis sed.Nulla facilisi. Cras non egestas lectus.

$('.hidden-content').hide(); can be moved to CSS. This will be better UX and user will not see any flickr on the screen if page take long time to load and thus hide the element by JavaScript.

.hidden-content {
    display: none;
}

To check if an element have a class, hasClass() can be used instead of is().

The methods can be chained $i.removeClass("angle-down").addClass("angle-up"); but there is better way to do this.

To toggle the class on an element, toggleClass() can be used. This also accepts callback function.

$('selector').toggleClass(function() {
    return $(this).children().hasClass('angle-down') ? 'angle-up' : 'angle-down';
});

toggleClass also accepts list of classes separated by space. If the element have already applied one of the class from it, toggleClass will remove it and add other classes. Now, the code will become,

$('selector').toggleClass('fa-angle-down fa-angle-up');

Demo:

Note that in Demo, I've added Font-Awesome to demonstrate the functionality.

$(document).ready(function() {
  $(".hidden-content-reveal").click(function() {
    $(this).children().toggleClass('fa-angle-down fa-angle-up');

    $(this).next('.hidden-content').slideToggle();
    return false;
  });
});
.hidden-content {
  display: none;
}
<link href="https://maxcdn.bootstrapcdn.com/font-awesome/4.7.0/css/font-awesome.min.css" rel="stylesheet" />

Lorem ipsum dolor sit amet, consectetur adipiscing elit.

Nulla facilisi. Cras non egestas lectus. Duis malesuada eleifend neque, eget tincidunt elit convallis sed.Nulla facilisi. Cras non egestas lectus.


Regex to enforce password requirements I'm trying to write regex to validate the password for the given rule.

Passwords must be at least 8 characters in length and contain at least 3 of the following 4 types of characters:

lower case letters (i.e. a-z)
upper case letters (i.e. A-Z)
numbers (i.e. 0-9)
special characters (e.g. !@#$&*)

I was going through this discussion and found this really great answer over there.

Now I'm trying to write regex for the mentioned requirements and I came up with the solution like this

^(?=.*[A-Z])(?=.*[!@#$&*])(?=.*[0-9])(?=.*[a-z]).{8,}|
(?=.*[!@#$&*])(?=.*[0-9])(?=.*[a-z]).{8,}|
(?=.*[A-Z])(?=.*[0-9])(?=.*[a-z]).{8,}|
(?=.*[A-Z])(?=.*[!@#$&*])(?=.*[a-z]).{8,}|
(?=.*[A-Z])(?=.*[!@#$&*])(?=.*[0-9]).{8,}$

and it is working perfect see rubular but I want to optimize these regex and I'm not sure If there are any way to simplify this.
Any suggestion will be appreciated.

Split the regex into smaller parts to check each rule individually.

Here's the code for JavaScript. The same simple logic can be used for Ruby or any other language.

// Check the length
if (str.length >= 8 && validate()) {

}

// Check if all the characters are present in string
function validate(string) {
    // Initialize counter to zero
    var counter = 0;

    // On each test that is passed, increment the counter
    if (/[a-z]/.test(string)) {
        // If string contain at least one lowercase alphabet character
        counter++;
    }
    if (/[A-Z]/.test(string)) {
        counter++;
    }
    if (/[0-9]/.test(string)) {
        counter++;
    }
    if (/[!@#$&*]/.test(string)) {
        counter++;
    }

    // Check if at least three rules are satisfied
    return counter >= 3;
}


Using a single validation handler for all controllers in a form In input validation I'm using this validation method in my all Forms. Here I have a single handler for all TextBoxes in my Form and if I have other controllers like ComboBoxes I would use a separate handler for all ComboBoxes and so on.

My only doubt in this approach is how users would feel about this as you can't focus on other text boxes while keeping the current one empty if it's not an optional field!

Could you please review this and provide your feedback?

public partial class frmForm : Form
    {
    public frmForm()
            {
                InitializeComponent();

                foreach (TextBox tb in this.Controls.OfType<TextBox>().Where(x => x.CausesValidation == true))
                {
                    tb.Validating += textBox_Validating;
                }
            }

            private void textBox_Validating(object sender, CancelEventArgs e)
            {
                TextBox currenttb = (TextBox)sender;
                if (currenttb.Name != OptionalFields .txtPolicy.ToString())
                {
                    if (string.IsNullOrWhiteSpace(currenttb.Text))
                    {
                        MessageBox.Show(string.Format("Empty field {0 }", currenttb.Name.Substring(3)));
                        e.Cancel = true;
                    }

                    else
                    {
                        e.Cancel = false;
                    }
                }

            }

            private enum OptionalFields
            {
                txtPolicy, txtRemarks
            }
    }

Just a few random observations:

foreach (TextBox tb in this.Controls.OfType<TextBox>().Where(x => x.CausesValidation == true))

tb is a bad name; without the explicit TextBox type one would have to read the entire part after the in, to figure out that we're talking about a TextBox. And it's not uncommon for loops to use implicit types, like this:

foreach (var tb in this.Controls.OfType<TextBox>().Where(x => x.CausesValidation == true))

Now in this part:

.Where(x => x.CausesValidation == true)

You're comparing a bool to true - that's a no-no. Write it like this instead:

.Where(x => x.CausesValidation)

It's probably also more readable to iterate over an identifier instead of an expression; I'd split it like this:

var textBoxes = Controls.OfType<TextBox>()
                        .Where(textBox => textBox.CausesValidation);

foreach (var textBox in textBoxes)
{
    // ...
}

Kudos for IsNullOrWhiteSpace:

if (string.IsNullOrWhiteSpace(currenttb.Text))

However, currenttb should be camelCase, and would be more readable as currentTextBox.

Careful with extra spaces:

string.Format("Empty field {0 }"

I don't think this is legal, should be "Empty field {0}".


JavaScript-function for to convert a dash-separated string into camel case I have to write a function which turns a string with the words separated by dashes ( - ) into a sentence using camel case notation.

I've had the following idea for an implemenation using the reduce-method of Array:

// -------- THE ACTUAL FUNCTION -------------------------

/**
* Converts a string with dashes as word-separators
*  to a string using camel case notation.
* --------------------------------------------------
* @param {String} Sentence-string with dashes (-)
*  as word-separators.
*
* @returns {String} Sentence-string in Camel Case
*  notation.
*  Returns an empty string as an indicator of failure.
* ---------------------------------------------------
* Usage-Example:
* dashedToCamelCase('north-east-east') // Returns 'northEastEast'
*/
function dashedToCamelCase( dashed ) {
  var ret;
  var parts;

  if (typeof dashed !== 'string' || !dashed) {
    return '';
  }

  parts = dashed.split('-');
  ret = parts[0].toLowerCase();
  parts = parts.slice(1);

  ret = parts.reduce(function(previous, current) {
    return previous +
      current.slice(0, 1).toUpperCase() +
      current.slice(1).toLowerCase();
  }, ret);

  return ret;
}

// -------- THE ACTUAL FUNCTION - END -------------------

// --- TESTING ---------------------------
var test = [ 'north',
             'north-east',
             'north-east-east',
             'south-west-west-north',
             'south-wEst-EaSt',
             'North-west-EAST',
             'NORTH-EAST-SOUTH-WEST',
             248,
             '' ];
var expect = [ 'north',
               'northEast',
               'northEastEast',
               'southWestWestNorth',
               'southWestEast',
               'northWestEast',
               'northEastSouthWest',
               '',
               '' ];

test.forEach(function(item, i) {
  dashedToCamelCase(item) === expect[i]
  ? console.log('%s passed.', i)
  : console.error('%s FAILED!', i);
});

I have seen similar functions which use the replace-method of String together with a function as a parameter. Still not convinced if I should keep with my solution ...

So:
Should I use an implementation using String.replace? Or are just some improvements needed?

I would like to try it out with a large amount of automatically created data.

Does someone know a good way for measuring the performance?

Yes, I think you should use String.prototype.replace instead of array looping. I think so because .replace is probably going to be faster and because the code will be easier to understand.

I would also like to warn you about equality operator (===) in JavaScript. This returns false:

"ddd"===new String("ddd")

This returns true:

"ddd"==new String("ddd")

Also try to avoid if( !xxx ) with variables that are not supposed to be boolean, because it can get quite confusing for people reading your code.

Here's the callback replacement you talked about. You should use that and improve it so that it fits your exact definition if the conversion, which is missing in the question, so it wasn't addressed.

// -------- THE ACTUAL FUNCTION -------------------------

/* blah blah blah */
function dashedToCamelCase( dashed ) {
  if( typeof dashed != "string" || dashed.length==0 )
    return "";
  return dashed.toLowerCase().replace(/\-([a-z]?)/g, function(match, letter) {
      return letter.toUpperCase();
  });
}

// -------- THE ACTUAL FUNCTION - END -------------------

// --- TESTING ---------------------------
var test = [ 'north',
             'north-east',
             'north-east-east',
             'south-west-west-north',
             'south-wEst-EaSt',
             'North-west-EAST',
             'NORTH-EAST-SOUTH-WEST',
             248,
             '' ];
var expect = [ 'north',
               'northEast',
               'northEastEast',
               'southWestWestNorth',
               'southWestEast',
               'northWestEast',
               'northEastSouthWest',
               '',
               '' ];

var output = document.getElementById("output");
test.forEach(function(item, i) {
  try {
    var result = dashedToCamelCase(item);
    if(result==expect[i]) {
      writeOutput(item+" => "+result, "correct");
    }
    else {
      writeOutput(item+" => "+"got '"+result+"' instead of '"+expect[i]+"'", "wrong");
    }
  }
  catch(e) {
    writeOutput(e.message, "error");
  }
});
function writeOutput(text, className) {
  var p = document.createElement("p");
  p.appendChild(new Text(text));
  p.className = className;
  output.appendChild(p);
}
.correct {
  color: green;
}
.wrong {
  color: red;
}
.error {
  font-weight: bold;
  color: red;
}


xml is XML is xml is XML I had to write some code for an interview, to print out the name attribute of each node. I started off going in the wrong direction, so I didn't get to finish.  I wanted to finish writing the code, so I created my console app similar to what they had a button event doing.  Please let me know what you think of the code and what I can do better.

class XMLRecursionReader
{
    private StringBuilder _outputString = new StringBuilder();
    private XmlNode _root;

    public XMLRecursionReader(XmlDocument xDoc)
    {
        _root = xDoc.ChildNodes[1];
    }
    public string ReturnNameAsString (XmlNode node)
    {
        return node.Attributes["name"].InnerXml.ToString();
    }
    public void buildString (XmlNode node)
    {
        _outputString.AppendLine(ReturnNameAsString(node));
        if (node.HasChildNodes)
        {
            foreach (XmlNode childNode in node.ChildNodes)
            {
                buildString(childNode);
            }
        }
    }
    public void PrintOutput ()
    {
        buildString(_root);
        Console.WriteLine(_outputString.ToString());
        Console.ReadLine();
    }
}

similar sample to what I was coding against.  but the code needed to be generic and able to go as deep as necessary depending on the document that was fed in, but the structure is always going to be similar to this.

<?xml version="1.0" encoding="UTF-8"?>
<report name="ReportName">
    <agency name="agency1">
        <office name="office1"></office>
        <office name="office2"></office>
        <office name="office3"></office>
    </agency>
    <agency name="agency2">
        <office name="office1">
            <agent name="agent Amy"></agent>
    <address name="address line"></address>
        </office>
        <office name="office2"></office>
        <office name="office3"></office>
    </agency>
    <agency name="agency3">
        <office name="office1">
        <agent name="agent Bettie">
            <subagent name="sub-agent bob">
            <phone name="456-789-1230"></phone>
            </subagent>
            <subagent name="sub-agent billy"></subagent>
        </agent>
        <address name="address line">
            <faxnumber name="1234567890"></faxnumber>
        </address>
        </office>
        <office name="office2"></office>
        <office name="office3"></office>
    </agency>
</report>

The Results:

Naming
There are .NET Naming Guidelines which state that methods should be named using PascalCase casing. You haven't done this for buildString().
The code

The InnerXml property returns a string hence there is no need to call ToString() on the property.
Because all methods are public you should do proper parameter validation. At least you should check if the passed parameter is null and if yes throw an ArgumentNullException.
The PrintOutput () method does way too much. It builds the output, writes it to the console and reads a line from the console.
Because _root won't change you should make it readonly.
Calling PrintOutput twice results in doubling the printed value because you never reset/clear the StringBuilder.
The method name of ReturnNameAsString is too much. Because the method isn't void it is clear that it returns something. From looking at the returned type (string) it is clear what the method returns. Just call it GetNodeName instead.
The class name XMLRecursionReader exposes too much information about the implementation. A user of your code won't need to know that you solve the task by using recursion.
Both methods buildString and ReturnNameAsString should be private.
The if (node.HasChildNodes) in the buildString method is superflous because you are using a foreach to iterate over the ChildNodes. If there aren't any ChildNodes the foreach won't iterate.
The constructor assumes that the passed XmlDocument containes ChildNodes which may not be the case.


Better method of a timer in Python I am currently using:

class RepeatedTimer(object):
    def __init__(self, interval, function, *args, **kwargs):
        self._timer = None
        self.interval = interval
        self.function = function
        self.args = args
        self.kwargs = kwargs
        self.is_running = False
        self.start()

    def _run(self):
        self.is_running = False
        self.start()
        self.function(*self.args, **self.kwargs)

    def start(self):
        if not self.is_running:
            self._timer = Timer(self.interval, self._run)
            self._timer.start()
            self.is_running = True

    def stop(self):
        self._timer.cancel()
        self.is_running = False

to run a section of code every 24 hours, however, this looks messy and I think it can be simplified.

I want to run a function every 24 hours.  Is there a simpler and cleaner method?

If the goal is to run a task at the same time each day, it is better
to capture the value of the real-time clock, because using successive 24
hour intervals can give you a "time creep".  Something along these lines
would work fine (you'd probably put this in a separate thread):

import time

def time_of_day:
     return time.strftime("%H:%M:%S")

target_time = "18:00:00"

while True:
    while time_of_day() != target_time:
         time.sleep(.1)
    call_function()
    while time_of_day() == target_time:
         time.sleep(.1)


Removing Substrings from List I have a list of palindromes

List<string> palindromes = new List<string>()
{
    hijkllkjih,
    ijkllkji,
    jkllkj,
    kllk,
    ll,
    defggfed,
    efggfe,
    fggf,
    gg,
    abccba,
    bccb,
    cc,
    qrrq,
    rr,
    mnnm,
    nn,
    pop,
    o,
    s,
    q,
    r
};

But I want to remove palindromes that are substring of other palindromes. So, the list would become

List<string> palindromes = new List<string>()
{
    hijkllkjih,
    defggfed,
    abccba,
    qrrq,
    mnnm,
    pop,
    s,
    q,
    r
};

To do this I have written the following code

List<int> indexList = new List<int>();
for (int i = 0; i < palindromes.Count; ++i)
{
    if (indexList.Contains(i))
        continue;

    string tmp = palindromes[i];
    for (int j = 0; j < palindromes.Count; ++j)
    {
        if (i == j
            || indexList.Contains(j)
            || palindromes[j].Length > palindromes[i].Length)
            continue;

        if (tmp.Contains(palindromes[j]))
            indexList.Add(j);
    }
}

foreach (var index in indexList.OrderByDescending(l => l))
    palindromes.RemoveAt(index);

Can this be improved?

You can simplify your code to this:

var palindromesCopy = palindromes.ToList();
palindromes.RemoveAll(x => palindromesCopy.Any(y => x != y &&
                                               y.Contains(x)));

If you want to remove duplicated strings as said by @MoonKnight in comments, just call Distinct on palindromes after removing.


Count sort implementation Is this a proper Count sort implementation?
Code and style improvements?

It does not support negative and that should be improved.  A large input could cause out of memory.

Some of the array index is different as the WIKI is 1 based and these arrays are zero based.

public static void CountSort(int[] arr)
{
    int max = -1;
    foreach(int i in arr)
    {
        if (i < 0)
        {
            throw new IndexOutOfRangeException(" < 0 ");
        }
        max = Math.Max(max, i);
    }

    int n = arr.Length;

    // The output character array that will have sorted arr
    int[] output = new int[n];

    // Create a count array to store count of inidividul
    // characters and initialize count array as 0
    int[] count = new int[max+1];
    for (int i = 0; i <= max; ++i)
        count[i] = 0;

    // store count of each character
    foreach (int i in arr)
        count[i]++;

    // Change count[i] so that count[i] now contains actual
    // position of this character in output array
    for (int i = 1; i <= max; ++i)
        count[i] += count[i - 1];

    // Build the output character array
    for (int i = 0; i < n; ++i)
    {
        output[count[arr[i]] - 1] = arr[i];
        count[arr[i]]--;
    }

    // Copy the output array to arr, so that arr now
    // contains sorted characters
    for (int i = 0; i < n; ++i)
        arr[i] = output[i];
}

//test
int[] ar = new int[] { 0, 0, 0, 0 };
CountSort(ar);
foreach (int i in ar)
    Debug.WriteLine(i);
Debug.WriteLine("");

ar = new int[] { 5, 5, 5, 5 };
CountSort(ar);
foreach (int i in ar)
    Debug.WriteLine(i);
Debug.WriteLine("");

ar = new int[] {};
CountSort(ar);
foreach (int i in ar)
    Debug.WriteLine(i);
Debug.WriteLine("");

ar = new int[] { 90, 90, 71, 82, 93, 75, 81, 0, 12, 54, 36, 13, 102, 99, 34, 103, 78, 196, 52, 5, 215 };
CountSort(ar);
foreach (int i in ar)
    Debug.WriteLine(i);
Debug.WriteLine("");

Limitations

Being able to sort in \$O(n)\$, counting sort can be very efficient,
but with the right kind of data.
Just keep in mind that it won't work well when the variation in values is very large.

Optimizations and extensions

The implementation checks the maximum value,
so the array of counts will use that much space.
You could reduce the amount of space used by finding the minimum too.
That way the size of the array of counts could be max - min instead of max.
That will of course require to use the min as an offset both when building the array of counts,
and when writing the output.

Related to that, the current implementation doesn't support negative values in the input.
But it could, by using the minimum value as an offset.
In fact,
by applying the optimization above,
negative numbers will be automatically supported.

Unnecessary storage and mutations

There's no need for the temporary output array,
and for modifying the content of count to contain positions.
Once you have the array of counts,
you can directly overwrite the target array,
reducing the amount of code and simplifying the implementation.

for (int countIndex = 0, outIndex = 0; countIndex < count.Length; ++countIndex)
{
    for (int i = 0; i < count[countIndex]; ++i)
    {
        arr[outIndex++] = countIndex;
    }
}

Unnecessary array initialization

int[] arrays are initialized with zeros,
no need to manually set the content to zeros.

Style tips

It's recommended to always use braces with loops and conditionals.

Testing

It's better to use a proper unit testing framework that gives you simple pass-or-fail results,
rather than reading the output and verifying manually that it's actually correct.


Set of matrix operations for summing around a matrix element I have a results of simulations, which are discrete complex numbers representing wave function on a NxN grid. I calculate phase of the wavefunction. For every point on that grid I have to perform this set of operations. Example:

|a b c|
|d e f|
|g h i|

Sum of difference between neighbours around element e:

 (a-b)+(b-c)+(c-f)+(f-i)+(i-h)+(h-g)+(g-d)+(d-a)

But every substraction δ have is subjected to this conditions

if δ > π than δ = δ - 2π
if δ < -π than δ = δ + 2π

I wrote a code in Octave, which is extremaly inefficient and slow, but I was hoping there is a way to move from buteforce calculations to faster calculations using some of the functions in image package. I know I can perfom sumation using convolution but I still have problem with other operations.

Code in Octave:

function deltaPhi = phaseDifference(phi1, phi2)
 deltaPhi = phi1 - phi2;
  if(deltaPhi > pi)
    deltaPhi = deltaPhi - 2*pi;
  endif

  if(deltaPhi < -pi)
    deltaPhi = deltaPhi + 2*pi;
  endif;
end

function [phase] = checkPhase(M)

phase = zeros(size(M)-2);

  for i = 2:size(M,1)-1
    for j = 2:size(M,2)-1
      phase(i-1,j-1) = phaseDifference(M(i-1,j-1),M(i,j-1)) + phaseDifference(M(i,j-1),M(i+1,j-1)) + phaseDifference(M(i+1,j-1),M(i+1,j)) + phaseDifference(M(i+1,j),M(i+1,j+1)) + phaseDifference(M(i+1,j+1),M(i,j+1)) + phaseDifference(M(i,j+1), M(i-1,j+1)) + phaseDifference(M(i-1,j+1), M(i-1,j)) + phaseDifference(M(i-1,j), M(i-1,j-1));
    endfor
  endfor

end

The idea is to rewrite this code in OpenCV and use some of that librarys methods.

First some simple things:

I would use end instead of endfor and endif, to keep the code compatible with MATLAB.
It is not necessary to end a function with end (though it's not harmful either). The end is only necessary when writing nested functions that have access to the parent function's workspace.
You should break up very long lines of code using ... (see an example below).
When looping through a large matrix, always make the first index your inner loop. Matrix elements M(1,j) and M(2,j) are adjacent in memory, M(i,1) and M(i,2) are not. If you switch your two nested loops, your code will be faster for large arrays, as you use the cache better.

Now for the hard part: changing your logic.

|a b c|
|d e f|
|g h i|

  Sum of difference between neighbours around element e:

(a-b)+(b-c)+(c-f)+(f-i)+(i-h)+(h-g)+(g-d)+(d-a)

Note that you compute the phase difference between two neighboring cells several times: for example, (a-b) will be computed when determining your value for e, but also in the previous loop iteration, when d was the center element; it will be computed two more times when that pair appears at the bottom of such a neighborhood.

Thus, you should pre-compute these differences and store them:

vdiff = phaseDifference(diff(M,1,1));
hdiff = phaseDifference(diff(M,1,2));

For this to work, phaseDifference must be rewritten, see below.

Now, vdiff has one fewer rows, and hdiff has one fewer columns, than M. We need to take this into account when indexing. Think of these as the elements in between the rows/columns of M.

The sum for one element at (i,j) now becomes:

phase(i-1,j-1) = hdiff(i-1,j-1) + hdiff(i-1,j) ...
               - hdiff(i+1,j-1) - hdiff(i+1,j) ...
               - vdiff(i-1,j-1) - vdiff(i,j-1) ...
               + vdiff(i-1,j+1) + vdiff(i,j+1);

The full function now becomes:

function phase = checkPhase2(M)
vdiff = phaseDifference(diff(M,1,1));
hdiff = phaseDifference(diff(M,1,2));
phase = zeros(size(M)-2);
for j = 2:size(M,2)-1
   for i = 2:size(M,1)-1
      phase(i-1,j-1) = hdiff(i-1,j-1) + hdiff(i-1,j) ...
                     - hdiff(i+1,j-1) - hdiff(i+1,j) ...
                     - vdiff(i-1,j-1) - vdiff(i,j-1) ...
                     + vdiff(i-1,j+1) + vdiff(i,j+1);
   end
end

Since Octave is slow with loops, it is likely that this vectorized version will be faster:

function phase = checkPhase3(M)
vdiff = phaseDifference(diff(M,1,1));
hdiff = phaseDifference(diff(M,1,2));
j = 2:size(M,2)-1;
i = 2:size(M,1)-1;
phase = hdiff(i-1,j-1) + hdiff(i-1,j) ...
      - hdiff(i+1,j-1) - hdiff(i+1,j) ...
      - vdiff(i-1,j-1) - vdiff(i,j-1) ...
      + vdiff(i-1,j+1) + vdiff(i,j+1);

I'm testing on MATLAB R2017a, and the vectorized form is actually slower than the version with loops, because indexing is still quite slow, whereas MATLAB has made huge strides in the last year speeding up code with loops.

Timings for an input of 2000x3000 elements:

checkPhase(M) (the OP's version) runs in 1.3s,
and swapping the loop order it runs in 1.0s,
checkPhase2(M) runs in 0.17s,
checkPhase3(M) runs in 0.25s.

Your timings on Octave will be vastly different (much slower for the code with loops).

For phaseDifference to work with an array input, it cannot use if the way that the original function does. It is possible to rewrite the same logic using logical indexing to work on a full matrix, but this version is simpler:

function deltaPhi = phaseDifference(deltaPhi)
deltaPhi = mod(deltaPhi + pi, 2*pi) - pi;

Note I replaced the two inputs with a single one, as diff already computes the difference between neighbors.


Working with pandas dataframes for stock backtesting exercise I'm attempting to apply a long set of conditions and operations onto a pandas dataframe (see the dataframe below with VTI, upper, lower, etc). I attempted to use apply, but I was having a lot of trouble doing so. My current solution (which works perfectly) relies on a for loop iterating through the dataframe. But my sense is that this is an inefficient way to complete my simulation. I'd appreciate help on the design of my code.

VTI    uppelower  sell  buy        AU     BU     BL      date    Tok order
44.58  NaN NaN  False   False   False   False   False   2001-06-15  5  0
44.29  NaN NaN  False   False   False   False   False   2001-06-18  5  1
44.42  NaN NaN  False   False   False   False   False   2001-06-19  5  2
44.88  NaN NaN  False   False   False   False   False   2001-06-20  5  3
45.24  NaN NaN  False   False   False   False   False   2001-06-21  5  4

If I wanted to run a bunch of conditions and for loops like the below and run the function below (the get row data function) only if the row meets the conditions provided, how would I do so?

My intuition says to use .apply() but I'm not clear how to do it within this scenario. With all the if's and for-loops combined, it's a lot of rows. The below actually outputs an entirely new dataframe. I'm wondering if there are more efficient/better ways to think about the design of this simulation/stock backtesting process.

Get row data simply obtains key data from the dataframe and computes certain information based on globals (like how much capital I have already, how many stocks I have already) and spits out a list. I append all these lists into a dataframe that I call the portfolio.

I've given a snippet of the code that I've already made using a for-loop.

 ##This is only for the sell portion of the algorithm
 if val['sell'] == True and tokens == maxtokens:
        print 'nothign to sell'

if val['sell'] == True and tokens < maxtokens:
    print 'sellprice', price

    #CHOOSE THE MOST EXPENSIVE POSITION AND SELL IT#
    portfolio = sorted(portfolio, key = lambda x: x[0], reverse = True)
    soldpositions = [position for position in portfolio if position[7] == 'sold']
    soldpositions = sorted(portfolio, key=lambda x: x[1], reverse = True)

    for position in portfolio:

        #Position must exist
        if position[9] == True:
            print 'position b4 sold', position

            #Position's price must be LOWER than current price
            #AND the difference between the position's price and current price must be greater than sellbuybuffer
            if abs(position[0] - price) <= sellbuybuffer:
                print 'does not meet sellbuybuffer'

            if position[0] < price and abs(position[0] - price) >= sellbuybuffer:
                status = 'sold'

                #This is if we have no sold positions yet
                if len(soldpositions) == 0:

                   ##this is the function that is applied only if the row meets a large number of conditions
                   get_row_data(price, date, position, totalstocks, capital, status, tokens)

Pandas allows you to filter dataframes efficiently using boolean formulas.

Instead of using a for loop and conditional branching, use the following syntax:

df = portfolio[(portfolio['sell'] == True) & (portfolio['Tok'] < maxtokens)]

To sort a dataframe, you can also simply write:

portfolio = portfolio.sort('VTI', ascending=False)
sold_positions = portfolio[portfolio['BL'] == True].sort('upperlower', ascending=True)


Lossy packing 32 bit integer to 16 bit I am working on the lossy 16 bit representation for 32 bit integers that catches all 32 bit range with precision depending on absolute value.

My idea is to store integer lv = sign(x)*ilog2(x) in first bite, and a tail that approximates the error x - 2^ilog2(x).  In order to do that I divide possible error values in 256 bins and store the index of the bin.

To reconstruction of the integer value, I calculate sign(lv) * (2^lv) + bin_size * tail.

I have initial implementation for that, but my code have several problems.

It is not as clear as I'd like to.
short_int32_t(x) should be equal to -short_int32_t(-x) but it is not.  I am pretty sure that it is possible to achieve it, but I am not sure how to do it not making code more ugly.
Any performance suggestions welcome.

Look at functions void set(int32_t) and int32_t get():

template <typename T>
int32_t signum(T x) {
    return (T(0) < x) - (x < T(0));
}

inline int8_t signum_log2(int32_t x) {
    int8_t log2 = static_cast<int8_t>(8 * sizeof(int32_t) - __builtin_clz(abs(x)) - 1);
    static_assert(8 * sizeof(int32_t) == __builtin_clz(1) + 1, "log is wrong");
    return log2 * signum(x);
}

inline int8_t abs_log2(int32_t x) {
    static_assert(8 * sizeof(int32_t) == __builtin_clz(1) + 1, "log is wrong");
    return static_cast<int8_t>(8 * sizeof(int32_t) - __builtin_clz(abs(x)) - 1);
}

class short_int32_t {
public:
    short_int32_t(int32_t x) {
        set(x);
    }

    void operator=(int32_t x) {
        set(x);
    }

    bool operator<(short_int32_t x) {
        return m_log == x.m_log ? (signum(m_log) * m_tail < signum(x.m_log) * x.m_tail) : (m_log < x.m_log);
    }

    short_int32_t operator*(short_int32_t x) {
        return short_int32_t(get() * x.get());
    }

    short_int32_t operator+(short_int32_t x) {
        return short_int32_t(get() + x.get());
    }

    short_int32_t operator-(short_int32_t x) {
        return short_int32_t(get() - x.get());
    }

    short_int32_t operator*(int32_t x) {
        return short_int32_t(get() * x);
    }

    short_int32_t operator+(int32_t x) {
        return short_int32_t(get() + x);
    }

    short_int32_t operator-(int32_t x) {
        return short_int32_t(get() - x);
    }

    void set(int32_t x) {
        if (x == 0) {
            m_log = 0;
            m_tail = 0;
            return;
        }

        int8_t abs_log = abs_log2(x);
        int32_t interval = abs_log ? 1 << abs_log : 0;
        if (interval == abs(x)) {
            m_log = x < 0 ? -abs_log : abs_log;
            m_tail = 0;
            return;
        }

        int32_t bin_size = interval < 512 ? 1 : (x < 0 ? (interval >> 9) : (interval >> 8));

        m_log = x > 0 ? abs_log : -abs_log - 1;
        int32_t tail = x > 0 ? x - interval : ((1 << -m_log) + x);

        m_tail = tail / bin_size;
    }

    int32_t get() const {
        int32_t interval = 1 << abs(m_log);
        int32_t bin_size = interval < 512 ? 1 : (m_log < 0 ? (interval >> 9) : (interval >> 8));

        return signum(m_log) * interval + static_cast<int32_t>(m_tail) * bin_size;
    }

public:
    int8_t m_log;
    uint8_t m_tail;
};

short_int32_t operator*(int32_t x, short_int32_t y) {
    return y * x;
}

short_int32_t operator+(int32_t x, short_int32_t y) {
    return y + x;
}

short_int32_t operator-(int32_t x, short_int32_t y) {
    return short_int32_t(x - y.get());
}

A few improvements:

Assignment operator (=) should return a reference to short_int32_t, returning *this in the implementation.
Operator < or any other comparison/arithmetical operator for that matter, should be const, since they only read the object's state. E.g.: bool operator < (short_int32_t x) const.
Where are the other comparison operators, BTW? I would expect at least ==,!=,<,>,<=,>=.
get/set are good enough names, but wouldn't maybe set32 and get32 be more idiomatic in this case? Or perhaps even more explicit, such as fromInt32 and toInt32.
Consider making those global inline functions member of the class, if they are never used elsewhere.


Project Euler #4 - Largest Palindrome Product Problem Statement:

  A palindromic number reads the same both ways. The largest palindrome
  made from the product of two 2-digit numbers is 9009 = 91 × 99.

  Find the largest palindrome made from the product of two 3-digit
  numbers.

This code is in C++ 11, please review my code.

#include <iostream>
#include <math.h>

using namespace std;

int palindrom(int num);

int main()
{
    auto n1 = 999u;
    auto n2 = 999u;
    unsigned int n = 0u;

    unsigned int largest = 0;

    for (; n1>=100; n2--)
    {
        if (n2 < 100)
        {
            n1--;
            n2 = 999;
            continue;
        }

        if (palindrom(n1*n2) == n1*n2)
        {
            cout << "n1 << " << n1 << " n2 " << n2 << endl;
            cout << n1*n2;

            if (largest < n1*n2)
                largest = n1*n2;
        }
    }

    cout << "Largest" << largest << endl;

    return 0;
}

int palindrom(int num)
{
    int new_num = 0;
    int digit = 0;

    while (num != 0)
    {
        digit = num % 10;
        new_num = new_num*10 + digit;
        num /= 10;
    }

    return new_num;
}

Output (project Euler solution kept secret)

   906609

Your function palindrom just "reverses" your number without actually checking it is a palindrom. It would probably be more appropriate to give this function a proper name like reverse_number() and to use it in a different function which can be called is_palindrome().

Please note that a faster implementation for this could be done differently as one could stop as soon as 2 digits do not match but we can consider that this is good enough: it's easy to test and it's easy to understand how it works.

Also, you could get rid of the magic number 10. You could for instance provide the base with a default argument.

You should always try to define your variable in the smallest possible scope.

Here's what I have for the helper function :

int reverse_number(int num, int base = 10)
{
    int new_num = 0;

    while (num != 0)
    {
        int digit = num % base;
        new_num = new_num*base + digit;
        num /= base;
    }
    return new_num;
}

bool is_palindrom(int num)
{
    return num == reverse_number(num);
}

You should compile your code with all warnings activated as they can provide you good hints :

euler3.cpp:28:18: warning: unused variable ‘n’ [-Wunused-variable]

The way you are iterating is super weird. If you want to iterate over a range with n1 and iterate over another range with n2, just use two nested for loops:

for (auto n1 = 999u; n1>=100; n1--)
for (auto n2 = 998u; n2>=100; n2--)

Also, without any loss of generality, one can assume that n1 >= n2 :

for (auto n1 = 999u; n1>=100; n1--)
for (auto n2 = n1;   n2>=100; n2--)

Because the values will get smaller, you can break when you find a value smaller that the one you have already found.

At the stage, my code looks like:

int main()
{
    unsigned int largest = 0;

    for (auto n1 = 999u; n1>=100; n1--)
    {
        for (auto n2 = n1;   n2>=100; n2--)
        {
            auto prod = n1*n2;
            if (prod < largest)
                break;

            if (is_palindrom(prod))
            {
                cout << "n1 << " << n1 << " n2 " << n2 << " -> " << prod << endl;
                largest = prod;
            }
        }
    }
    cout << "Largest" << largest << endl;
    return 0;
}


Toggling icon images using jQuery I have about 5 of these but I can't figure out how to consolidate them.

$('#togglemass').toggle(function () {
    $("#plusmass").attr("src", "minus.png");
  },

  function () {
    $(".plusmass").attr("src", "plus.png");

  });

$('#togglestar').toggle(function () {
    $(".plusstar").attr("src", "minus.png");
  },

  function () {
    $(".plusstar").attr("src", "plus.png");
  });

$(function() {

    var togglePlusMin = function(clicker, img) {
        $(clicker).toggle(function() {
            $(img).attr("src", "minus.png");
        }, function() {
            $(img).attr("src", "plus.png");
        });
    };

    // can be used by
    togglePlusMin('#togglemass', '#plusmass');

    // BONUS: this will also work:
    var star = $('#togglestar'),
        starIcon = $('img.plusstar');

    togglePlusMin(star, starIcon);
})

As a comment, its generally a bad idea to use ids without a second context parameter


Group by range and include edge duplicates I've created a method that groups Item entities below based on their Value property. The idea is to produce groups that also includes possible "edge duplicates", i.e.
[{ Value: 9 }, { Value: 10 }, { Value: 11 }]

With rangeSize set to 10 yields two groups:
[ [{ Value: 9 }, { Value: 10 }], [{ Value: 10 }, { Value: 11 }] ]

public IEnumerable<IEnumerable<DTO.Item>> GroupByValueRange(int rangeSize)
{
    var items = _itemRepository.GetAll().ToList();
    var steps = Math.Ceiling(items.Max(p => p.Value) / rangeSize);
    var groups = new List<IEnumerable<DTO.Item>>();

    for (var i = 1; i <= steps; i++)
    {
        var max = i * rangeSize;
        var min = (i - 1) * rangeSize;
        var currentGroup = items.Where(p => p.Value >= min && p.Value <= max);
        groups.Add(currentGroup.Select(p => _mapper.Map<DTO.Item>(p)));
    }

    return groups;
}

So the idea is to create groups with values like 0-10, 10-20, 20-30 etc...

Can there be empty groups?
If you pass in a rangeSize of 5, your current code will have an empty group. Is this desired?
Iterating over entire collection for each group
In addition to Peter Csala's recommendations, there is a possible performance issue here: you are iterating over the entire source for each group. The delegate passed to Where must be evaluated against all the items, to determine the contents of each group.
for (var i = 1; i <= steps; i++)
{
    ...
    var currentGroup = items.Where(p => p.Value >= min && p.Value <= max);
    groups.Add(currentGroup.Select(p => _mapper.Map<DTO.Item>(p)));
}

If you have few items, or only a small number of groups, this might not be an issue.
The following might be more efficient. Create a sequence of mapped-item / group-index pairs:
public IEnumerable<(DTO.Item mappedItem, int groupIndex)> GroupIndexes(int rangeSize) {
    foreach (var item in _itemRepository.GetAll()) {
        var groupIndex = Math.Floor(item.Value / rangeSize);
        var mapped = _mapper.Map<DTO.Item>(item);
        yield return (mapped, groupIndex);
        if (item.Value > 0 && item.Value % rangeSize == 0) {
            yield return (mapped, groupIndex - 1);
        }
    }
}

which you could then use to group:
var grouped = GroupIndexes(10).GroupBy(x => x.groupIndex, x => x.mappedItem);


Selection algorithm using merge sort and IEnumerable For educational purposes, I wrote a selection algorithm based on a Merge sort. I would like to improve performance.

public IEnumerable<T> MergeSort<T>(List<T> list, int left, int right, Comparer<T> comparer)
{
    if (left == right)
    {
        yield return list[left];
        yield break;
    }

    //divide
    int mid = (left + right) / 2;
    var firstEnumerable = MergeSort(list, left, mid, comparer);
    var secondEnumerable = MergeSort(list, mid + 1, right, comparer);

    //merge
    using (var firstEnumerator = firstEnumerable.GetEnumerator())
    using (var secondEnumerator = secondEnumerable.GetEnumerator())
    {
        bool first = firstEnumerator.MoveNext();
        bool second = secondEnumerator.MoveNext();

        while (first && second)
        {
            if (comparer.Compare(firstEnumerator.Current, secondEnumerator.Current) < 0)
            {
                yield return firstEnumerator.Current;
                first = firstEnumerator.MoveNext();
            }
            else
            {
                yield return secondEnumerator.Current;
                second = secondEnumerator.MoveNext();
            }
        }

        while (first)
        {
            yield return firstEnumerator.Current;
            first = firstEnumerator.MoveNext();
        }

        while (second)
        {
            yield return secondEnumerator.Current;
            second = secondEnumerator.MoveNext();
        }
    }
}

What it does : it recursively divide the list into smaller sequences (until the sequence has only one element). Then, it repeatedly merge sequences to produce new sorted ones until there is only 1 sequence remaining.

The main idea is to use IEnumerable<T> so there is no need to allocate arrays to merge results AND I can sort the list lazily and stop when I want. Example :

var list = ... // 1.000.000 elements
MergeSort(list, 0, list.length - 1, comparer).Take(50);

The actual performance to sort 1M integers and return the first 50 ones is 600 ms why I found to be slower than expected.  Returning only the first element give a similar performance.

My main concern is the recursive calls between Enumerators/IEnumerables. I have tried to wrote the same logic using a stack (to fully avoid recursion) but I don't know how to implement it.

I have also tried to isolate the merge code part (the code inside the two usings statements) into a separate method but it run considerably slower (about 1 sec). I don't know why.

I could easily parallelise the algorithm or use another selection algorithm (like quick select) but this is outside the scope of this question.

I was able to get a performance increase (500 ms in average instead of 600 ms) by splitting the code in two methods : one that return a sequence with a single element, one that merge IEnumerables. I think this is faster because the implementation of the yield return statements is simpler for the compiler (AFAIK it is done using a state machine) .

public IEnumerable<T> MergeSort<T>(List<T> list, int left, int right, Comparer<T> comparer)
{
    if (left == right)
    {
        return SingleValue(list[left]);
    }

    int mid = (left + right) / 2;
    var firstEnumerable = MergeSort(list, left, mid, comparer);
    var secondEnumerable = MergeSort(list, mid + 1, right, comparer);
    return Merge(firstEnumerable, secondEnumerable, comparer);
}

public static IEnumerable<T> SingleValue<T>(T value)
{
    yield return value;
}

public static IEnumerable<T> Merge<T>(IEnumerable<T> firstEnumerable, IEnumerable<T> secondEnumerable, Comparer<T> comparer)
{
    using (var firstEnumerator = firstEnumerable.GetEnumerator())
    using (var secondEnumerator = secondEnumerable.GetEnumerator())
    {
         //same as before
    }
}

Performance can be improved further by checking the range of elements to sort inside MergeSort method. Above a certain threshold, another sort can be used (eg : InsertionSort or SelectionSort)

public IEnumerable<T> MergeSort<T>(List<T> list, int left, int right, Comparer<T> comparer)
{
    if (right - left <= threshold)
    {
        return SelectionSort(list, left, right, comparer);
    }

    //...
}

I think SelectionSort is a good candidate because there is a way to implement it lazily : it can return the smallest number very early without having to sort the whole list  (eg: using a yield return). The partial merge sort now take about 15 ms to get the first 50th smallest numbers out of 1M integers.


Log writing PowerShell function I am still learning PowerShell and I would like your opinion on my log writing function. It creates a log entry with timestamp and message passed thru a parameter Message or thru pipeline, and saves the log entry to log file, to report log file, and writes the same entry to console. In Configuration.cfg file paths to report log and permanent log file are contained, and option to turn on or off whether a report log and permanent log should be written. If Configuration.cfg file is absent it loads the default values. Depending on the OperationResult parameter, log entry can be written with or without a timestamp. Format of the timestamp is "yyyy.MM.dd. HH:mm:ss:fff", and this function adds " - " after timestamp and before the main message.
function Write-Log {
    param (
        [Parameter(Position = 0, ValueFromPipelineByPropertyName)]
        [ValidateSet('Success', 'Fail', 'Partial', 'Info', 'None')]
        [String]
        $OperationResult = 'None',

        [Parameter(Position = 1, Mandatory, ValueFromPipeline, ValueFromPipelineByPropertyName)]
        [String]
        $Message
    )

    begin {

        if (Test-Path -Path '.\Configuration.cfg') {
            $Configuration = Get-Content '.\Configuration.cfg' | ConvertFrom-StringData
            $LogFile    = $Configuration.LogFile
            $ReportFile = $Configuration.ReportFile
            $WriteLog   = $Configuration.WriteLog -eq 'true'
            $SendReport = $Configuration.SendReport -eq 'true'
        }
        else {
            $LogFile    = '.\Log.log'
            $ReportFile = '.\Report.log'
            $WriteLog   = $true
            $SendReport = $true
        }

        if (-not (Test-Path -Path $LogFile)) {
            New-Item -Path $LogFile -ItemType File
        }

        if (-not (Test-Path -Path $ReportFile)) {
            New-Item -Path $ReportFile -ItemType File
        }
    }

    process {
        $Timestamp = Get-Date -Format 'yyyy.MM.dd. HH:mm:ss:fff'
        $LogEntry = $Timestamp + " - " + $Message

        switch ($OperationResult) {
            'Success' {
                $ForegroundColor = 'Green'
                break
            }

            'Fail' {
                $ForegroundColor = 'Red'
                break
            }

            'Partial' {
                $ForegroundColor = 'Yellow'
                break
            }

            'Info' {
                $ForegroundColor = 'Cyan'
                break
            }

            'None' {
                $ForegroundColor = 'White'
                $LogEntry = $Message
            }
        }

        Write-Host $LogEntry -ForegroundColor $ForegroundColor -BackgroundColor Black

        if ($WriteLog) {
            Add-content -Path $LogFile -Value $LogEntry
        }

        if ($SendReport) {
            Add-content -Path $ReportFile -Value $LogEntry
        }
    }
}

I don't see anything wrong or debatable in this so far. Just a couple nitpicks:

I would use parameter splatting to make the color part more compact. For example, from my own log function:
  switch ($Severity) {
      Debug    { $colors = @{ForegroundColor="DarkGray"                               } }
      Info     { $colors = @{                                                         } }
      Warning  { $colors = @{ForegroundColor="Black"      ; BackgroundColor="Yellow"  } }
      Error    { $colors = @{ForegroundColor="White"      ; BackgroundColor="DarkRed" } }
      Critical { $colors = @{ForegroundColor="DarkYellow" ; BackgroundColor="DarkRed" } }
  }

  Write-Host @colors "$($Severity): $Message"

Maybe you could go even further with a hash of hashs and do Write-Host @colors["error"] or something like that.

To create a file only if the file doesn't already exist, you can call New-Item directly, it will throw an exception if the file already exists which you can just ignore with a try-catch or with -ErrorAction SilentlyContinue, saving a couple lines.

Why not make all the configuration fetched from the config file optional parameters, with the values in the file as default values ? That would help if you ever have to use the function on the fly, in a shell or as a debug tool.

Very personal opinion, but I would personally put the default value of $OperationResult to Info, as this is the most common case for me and I'd appreciate just whipping out a simple Write-Log "mymessage" without extra typing.

Last one, this one depends on your environment, but Write-Log is also a function in the officiel VMWare vSphere module that is quite widely used. I wouldn't change any code because of that but remember to document that if you ever have to distribute your code.

Overall I think your cmdlet is pretty readable and produces a log that is easy enough to parse, so it ticks all the important boxes for me.


C# Am I using proper SOLID principles? I am currently practising using the SOLID principles in C#/.NET
I have made a little example, but i am not sure if i have followed it correct.
Its a simple example where i have an API controller that calls a method to get a list of users from an database and returned as DTOs.
To use the method i need to submit an IUserReader that is mainly doing the reading operation from the database and then a IMapper object is needed that will handle the mapping from database entity to DTO.
I made it that way to be available to switch out the IUserReader and IMapper so i can fit the query and mapping to match my requirements for a specific API call. Lets say an app is using the API to get users, the app will only need some details of the user, so we use a implementation that only selects the required information and maps it correct. We also have an backend to view the users, where we want all the user details from the database, here we use another implementation for that.
Here is my code example, i would like to know if it is done correct.
Data readers
public interface IDataReader<T>
{
    IEnumerable<T> Read();
}

public abstract class DataReader<T> : IDataReader<T>
{
    private protected IDataContext _context;
    public abstract IEnumerable<T> Read();

    public DataReader(IDataContext context)
    {
        _context = context;
    }
}

public interface IUserDataReader : IDataReader<IUserEntity>
{
}

//First implementation of user reader
public class UserDataReader : DataReader<IUserEntity>, IUserDataReader
{

    public UserDataReader(IDataContext context) : base(context) { }

    public override IEnumerable<IUserEntity> Read()
    {
        return _context.Users.Where(x => x.IsActive).OrderBy(x => x.Name).ToList();
    }
}

//Secound implementation of user reader
public class UserGridDataReader : DataReader<IUserEntity>, IUserDataReader
{

    public UserGridDataReader(IDataContext context) : base(context) { }

    public override IEnumerable<IUserEntity> Read()
    {
        return _context.Users.OrderBy(x => x.Name).ToList();
    }
}

Now the mappers:
public interface IMapper<I, O>
{
    O Map(I item);
}

public interface IUserMapper : IMapper<IUserEntity, UserDTO>
{

}

public class UserMapper : IUserMapper
{
    public UserDTO Map(IUserEntity item)
    {
        return new FullUserDTO
        {
            Name = item.Name,
            Email = item.Email
        };
    }
}

The API then calls this class and method to read:
public class UserReaderService
{
    private IUserDataReader _reader;
    private IUserMapper _mapper;

    public UserReaderService(IUserDataReader reader, IUserMapper mapper)
    {
        _reader = reader;
        _mapper = mapper;
    }

    public IEnumerable<UserDTO> Read()
    {
        IEnumerable<IUserEntity> userData = _reader.Read();

        IEnumerable<UserDTO> users = userData.Select(x => _mapper.Map(x));

        return users;
    }
}

API method:
    public void GetUsers()
    {
        IDataContext context = new DataContext();

        IUserDataReader userDataReader = new UserDataReader(context);
        IUserMapper mapper = new UserMapper();

        UserReaderService ur = new UserReaderService(userDataReader, mapper);

        ur.Read();
    }

Is that correctly using SOLID principles and generally abstraction?
Could i throw the IMapper dirrectly into the DataReader and make the mapping directly in the linq query instead of first returning the data result and then use mapping or would it break the SOLID principles?

I suppose the simplest thing is to go through the principles:

SRP - Cannot see any 'too big' things in your example
OCP - The example is small enough and you are not using class
inheritance that I cannot think of how this would apply
LSP - You only have one implementation of each interface so hard to
say
ISP - Your interfaces seem to serve a single purpose that might serve
the needs of several users - so no complaints there
DIP - You have pushed the more variable 'decision' making out,
keeping the API more abstract which seems reasonable.

Much of this, of course, depends on your specific aims and requirements. some thoughts:

How likely is it that the mapper and reader will vary independently?
Are you going to get any reuse out of all the different readers and mappers (actual not future-proofing)
Will each reader have a mapper in practice, in which case your comment about combining the mapper and reader is worth more thought.
Do you really need an interface for the reader, it seems like you are just wrapping what could be a function/lambda
What is the motivation in restricting the outbound data?
I appreciate this may just be an exercise, but it's good practice to think if you need to do this kind of restriction

It can be difficult to have a good instinct for principles like SOLID on toy problems due to the lack of realistic constraints and compromises.