Loop function using an array I have a jQuery function using a series of if and else statements. I am wondering if there is a more elegant way I can code this. I am thinking I could create an array with my "hashtag names" and then loop it through a function. However, I am not quite sure how to go about doing that.

setTimeout(function() {
    var i = 0;
    if(window.location.hash == "#" + "somename"){
        var i = 0;
        return scroll(i);
    }

    else if(window.location.hash == "#" + "somethingelse"){
        var i = 1;
        return scroll(i);
    }

    else if(window.location.hash == "#" + "someotherword"){
        var i = 2;
        return scroll(i);
    }

    else if (window.location.hash == "#" + "sometext"){
        var i = 3;
        return scroll(i);
    }
}, 500 );

function scroll(i){
    var title = $('.title').eq(i);
    var top =  $(title).offset().top - 50 ;
    $('html,body').animate({ scrollTop: top }, 'slow');
}

I'm checking to see what hashtag is in the URL, and then scrolling to the part of the page that is related to that hashtag. This method works, but I would like to optimize the if/else part of my code if possible. An array with a loop may not be the best approach. I am open to any ideas.

You can just parse the number right out of the location like this:

setTimeout(function() {
    var matches = window.location.hash.match(/^#page(\d+)$/);
    if (matches) {
        scroll(+matches[1] - 1);
    }
}, 500);

OK, now that you've changed to random strings, you could use a table lookup like this:

setTimeout(function() {
    var values = {
        "#somename": 1,
        "#somethingelse": 2,
        "#someotherword": 3,
        "#sometext": 4
    };
    var val = values[window.location.hash];
    if (val) {
        scroll(val - 1);
    }
}, 500 );

P.S. Note, there is no reason to return anything from the setTimeout() callback as the return value is not processed by anything which is why I removed the return statement.


Classes to manage perks to be added and removed I am looking for code correctness and design usage as I think I might be over doing it in the class department.

There are two things I'm mostly concerned with.

The possible redundancy of classes derived from IPerks. As
CommonPerks, UncommonPerks, etc, all look identical except for
the rows/columns in its internal perk list. I'm thinking I should
just have a concrete PerksList class instead.
The use of an enum to keep track of constant values for the
    different indices associated with the perkList. All perkList's
    will follow the same order, so the indices will always mean the same
    thing regardless of which IPerks derived class is being used.

PerkSlotEnums.h

enum class PERK_SLOTS
{
    COMMON_SLOT_COLUMNS = 1,
    COMMON_SCOPE_PERK_COUNT = 2,
    SCOPE_SLOT_INDEX =0,
    STATS_UPGRADE_A_SLOT_INDEX = 1,
    UNIQUE_PERK_A_SLOT_INDEX = 2,
    STATS_UPGRADE_B_SLOT_INDEX = 3,
    UNIQUE_PERK_B_SLOT_INDEX = 4,
    STATS_UPGRADE_C_SLOT_INDEX = 5,
    UNIQUE_PERK_C_SLOT_INDEX = 6
};

IPerks.h

#pragma once
#include "IPerk.h"
#include "IComponent.h"
#include <vector>

class IPerks : public IComponent
{

public:
    IPerks(void);
    virtual ~IPerks(void);
    virtual void AddPerk(IPerk* perk, int column, int row) = 0;
    virtual void RemovePerk(unsigned int index_1, unsigned int index2) =0;
    virtual IPerk* GetPerk(unsigned int perkId) const = 0;
};

CommonPerks.cpp

#include "CommonPerks.h"
#include "PerkSlotEnums.h"

CommonPerks::CommonPerks(void)
{
    perkList.resize((int)PERK_SLOTS::COMMON_SLOT_COLUMNS);
    perkList[(int)PERK_SLOTS::SCOPE_SLOT_INDEX].resize((int)PERK_SLOTS::COMMON_SCOPE_PERK_COUNT);
}

CommonPerks::~CommonPerks(void)
{
}

void CommonPerks::AddPerk(IPerk* perk, int column, int row)
{
    perkList[column][row] = perk;
}

void CommonPerks::RemovePerk(unsigned int index_1, unsigned int index_2)
{
    if(index_1 < perkList.size())
    {
        if(index_2 < perkList.at(index_1).size())
        {
            perkList.at(index_1).at(index_2) = nullptr;
        }
    }
}

IPerk* CommonPerks::GetPerk(unsigned int index_1, unsigned int index_2) const
{
    if(index_1 < perkList.size())
    {
        if(index_2 < perkList.at(index_1).size())
        {
            return perkList.at(index_1).at(index_2);
        }
    }

    return nullptr;
}

It is hard to understand what this code does without context.

Strange code

I am looking at this enum and I am asking myself - why?
Is there any way to divide this enum into maybe 3 different enums? That's really strange that you use the same numbers for different elements.

enum class PERK_SLOTS
{
    COMMON_SLOT_COLUMNS = 1,
    COMMON_SCOPE_PERK_COUNT = 2,
    SCOPE_SLOT_INDEX =0,
    STATS_UPGRADE_A_SLOT_INDEX = 1,
    UNIQUE_PERK_A_SLOT_INDEX = 2,
    STATS_UPGRADE_B_SLOT_INDEX = 3,
    UNIQUE_PERK_B_SLOT_INDEX = 4,
    STATS_UPGRADE_C_SLOT_INDEX = 5,
    UNIQUE_PERK_C_SLOT_INDEX = 6
};

C++ is not C

Next, you have:

CommonPerks::CommonPerks(void)

Remove void, it is totally unneccesary. It should be simply

CommonPerks::CommonPerks()

Conventions

Your methods starts with capital letters, that mostly not popular among C++ developers.

virtual void AddPerk(IPerk* perk, int column, int row) = 0;

I suspect that IComponent has a virtual destructor. You do not need to do this.

virtual ~IPerks(void);

Bad naming

Your parameters are badly named, give some more meaningful names.

void CommonPerks::RemovePerk(unsigned int index_1, unsigned int index_2)

Pragma

Pragma is compiler dependent. I would suggest to use headers guards.


Video/article slider. I have created a video/article slider. It works fine, but I have a feeling it's not quite a standard solution.

I would appreciate it if someone could review my code, so that if there are mistakes, I could learn from them.

Article 0

Article 1

Article 2

Article 3

I would consider having the css be in an external css file and javascript in an external js file since there is quite a bit of non-HTML code, but that is just my own preference.

I would get rid of extra whitespace and a lot of rework can be done with indentation to make the code more readable.

You can also group variable assignments together:

var video1 = document.createElement('video');
video1.src = 'images/video/trx.mp4';
video1.id = 'video1';
video1.autoplay = true;
video1.controls = true;

And group methods together:

document.getElementById("vidcont_2").appendChild(video2);
slide6();

I would consolidate

Article 0

Article 1

Article 2

Article 3

Additional Note: I would also improve the comments to be more descriptive and helpful.

 is unnecessary if you have proper indentation
//slide 5 right before var slide4 is confusing
If you keep HTML, CSS, and JavaScript in the same file I would comment each section as such.


A todo application in React using hooks I am learning React and wanted to implement a todo app using function components. Any suggestions and criticism is welcome, especially regarding best practices.

import React, {useState} from 'react';

class Todo {
  constructor(id, description, complete) {
    this.id = id;
    this.description = description;
    this.complete = complete;
  }
}

export default () => {

  const [idCounter, setIdCounter] = useState(1);
  const [todoList, setTodoList] = useState([]);
  const [todoDescription, setTodoDescription] = useState('');

  function addTodo() {
    setTodoList(
      [...todoList, new Todo(idCounter, todoDescription, false)]
    );
    setIdCounter(idCounter + 1);
    setTodoDescription('');
  }

  function markTodoItemComplete(id) {
    const todoListClone = todoList.slice();
    todoListClone[id - 1].complete = true;
    setTodoList(todoListClone);
  }

  return <>
    {todoList.map((todoItem) => {
      return (
        <React.Fragment key={todoItem.id}>
          {todoItem.id} - {todoItem.description} - {todoItem.complete ? 'Completed' : 'Waiting'}
          <button
            onClick={() => markTodoItemComplete(todoItem.id)}
            disabled={todoItem.complete}>
            Mark as complete
          </button>

        </React.Fragment>
      );
    })}

    <input
      onChange={e => setTodoDescription(e.target.value)}
      value={todoDescription}
    />

    <button
      disabled={!todoDescription}
      onClick={addTodo}>Add Todo
    </button>
  </>
}

function markTodoItemComplete(id) {
    const todoListClone = todoList.slice();
    todoListClone[id - 1].complete = true;
    setTodoList(todoListClone);
  }

This seems me not good practice, what you could have done is just getting id(key) of array and just set it to true.
like

 function markTodoItemComplete(id) {
    todoListClone[id].complete = true;
  }


Common interface with different return types I am creating a small app working on file system. The user can choose a directory and than, application get one of the possible result (examples):

return number of lines in all *.java files
return number of lines in all *.java files without comments
return number of all classes
return number of ifs etc...

Because I want to follow the single responsibility principle, and because all these functionalities have something in common, I want to create one class for each functionality (using commons.io DirectoryWalker) under a common interface. That's why I find such an interface useful:

**
 * Common interface for counting classes (lines, words, classes, etc...)
 */
public interface Counter {
    /**
     * count required things starting from startDirectory, recursively or not,
     * and consider only specified extensions
     */
    int count(File startDirectory, boolean recursive, List<String> fileExtensions);
    int count(File startDirectory, boolean recursive);
}

Now, the user can simply create some collection with counters, create and add new counter easily, iterate etc.

Questions:

Of course it is just a simplification, but do you think that the main idea of using an interface here is reasonable?
Maybe I will have some methods which will return something different than int - for example, Map with pairs, or a list of the ten longest files. In this case my interface is not correct. What should I do in such case? Maybe the interface's method should return Object like this:

Object count(File startDirectory, boolean recursive);

But this seems bad to me as it will lead to some unfriendly casting when parsing the returned value (Object). I can create some special DTO object like the below which will contain all possible "answers":

public class CounterDTO {
    public int numerOfLines; // used for counters counting lines
    public int numberOfIfs;  // used for counters counting if statements
    public Map<File, Integer> linesInFiles;  // ... File -> number of lines
    public List<File> longestFiles; // used for indicating files with maximal number of lines
    // and so on...
}

Then the interface method would look like:

CounterDTO count(File startDirectory, boolean recursive);

Is this OK or is there a better way?

Normally when you need "different return types", you can use generics. However, since you want these Counters to be in a List of some sort, using generics goes out the window since a List cannot be defined properly to contain both Counter<List<File>> and Counter<Integer>, so the only solution would be to have a List<Counter<?>> in which case the reason for using generics in the first place is lost.

Your CounterDTO class makes sense if a Counter counts more than one of the things. If the counter is only meant to return an int or a list of files, then using a CounterDTO is quite useless since many fields will be null.

Regarding Banthar's comment to your question, you could modify your existing interface to

int count(List<File> files);

It would then be the job for a single utility method (or two) to create a list of files from these parameters: File startDirectory, boolean recursive, List<String> fileExtensions.

As for the question about return type... If you need an interface for this or not depends on how you are using these counters. And also: Which possible counters are there? If you have one counter for returning total line count and one for returning a Map<File, Integer> of line counts, then you don't need two different counters. Since you need to know all the line counts in all the files to know the total line count, using a CounterDTO makes perfect sense here.


Faster way to 3 nested for loop python I am calculating parameters for a time series and the data size is very big. How can I make my function faster? The following function, param, calculates parameters for a time series model.

Input:

import pandas as pd
import statsmodels.api as sm

  data = [['01-01-2018', 150,661,396,286,786],['01-02-2018',231,341,57,768,941], ['01-03-2018',486,526,442,628,621],
    ['01-04-2018',279,336,140,705,184],['01-05-2018',304,137,800,94,369],['01-06-2018',919,340,372,494,117],
    ['01-07-2018',947,920,848,716,719],['01-08-2018',423,20,313,368,909],['01-09-2018',422,678,656,604,674],
    ['01-10-2018',422,678,656,604,674],['01-11-2018',337,501,743,606,991],['01-12-2018',408,536,669,903,463]]
df = pd.DataFrame(data, columns = ['date', 'A','B','C','D','E'])
df.index = df.date

def param(data_param):
    w = []
    x = []
    y = []
    z = []
    p = d = q = range(0, 2) # Define the p, d and q parameters to take any value between 0 and 2
    pdq = list(itertools.product(p, d, q)) # Generate all different combinations of p, q and q triplets
    seasonal_pdq = [(x[0], x[1], x[2], 12) for x in list(itertools.product(p, d, q))] # Generate all different combinations of seasonal p, q and q triplets

    for i in data_param:
        for param in pdq:
            for param_seasonal in seasonal_pdq:
                try:
                    results = sm.tsa.statespace.SARIMAX(data_param[i],
                                                        order=param,
                                                        seasonal_order=param_seasonal,
                                                        enforce_stationarity=False,
                                                        enforce_invertibility=False).fit()
                    #print(i,param, param_seasonal,results.aic)
                    w.append(i)
                    x.append(param)
                    y.append(param_seasonal)
                    z.append(results.aic)
                    mod_score_param = pd.DataFrame({
                        'id': w,
                        'param': x,
                        'param_seasonal': y,
                        'results_aic': z,
                        })
                except:
                    continue
    mod_score_param = mod_score_param.sort_values(by='results_aic')
    mod_score_param = mod_score_param.dropna()
    mod_score_param = mod_score_param[mod_score_param['results_aic']>3.0]
    mod_score_param = mod_score_param.sort_values(['id','results_aic'])
    mod_score_param = mod_score_param.drop_duplicates(['id'],keep='first')
    return(mod_score_param)

Output= param(df)

Output:

+-----+----+-----------+----------------+-------------+
|     | id |   param   | param_seasonal | results_aic |
+-----+----+-----------+----------------+-------------+
| 199 | A  | (1, 0, 1) | (1, 0, 0, 12)  |       4.565 |
| 197 | B  | (1, 0, 0) | (1, 0, 0, 12)  |      21.752 |
|  30 | C  | (0, 1, 1) | (1, 0, 0, 12)  |      87.847 |
|  22 | D  | (1, 1, 1) | (0, 1, 0, 12)  |      91.183 |
|  50 | E  | (0, 1, 1) | (0, 1, 0, 12)  |       92.87 |
+-----+----+-----------+----------------+-------------+

Without really understanding what you're attempting to do (and sorry, not enough time to dig into the subject matter), I've rewritten you code a little and profiled it.
The biggest bottleneck I see have to do with the panda/numpy definitions, there is a lot of code querying what your data structure is to determine how to handle it. Perhaps review all your cells in the data frame and ensure they're defined correctly?

Full image (but I'm pretty sure it's not helpful): https://i.imgur.com/nSiOrqu.png

A lot of those hints come from the exceptions which are raised - something you could have found easily if you didn't disable them with continue. More on that later.

Onto the code. Firstly was the imports, make sure you have them defined - when I loaded up your code I had a missing import, and that could have been either your paste (missing) or settings on your computer.

import pandas as pd
import statsmodels.api as sm
import itertools

Next is you're missing the entry point for your python code, always have this as it performs several things. First, it lets people reading your code understand where it begins and ends, secondly if you use tools like an auto-documentor, they load your code to perform reflection on all the objects.
Currently your code would just execute immediately on loading (that's loading, not executing), and that's not good - think of it like your car taking off in gear as soon as you turn the engine on.

if __name__ == "__main__":

Is your standard entry point for python code. Next, we have data initialisations at the top of the code - again, they should be after the entry point, or better yet, external (like an .ini file) and imported at runtime. The code comes out to:

if __name__ == "__main__":
    data = [['01-01-2018', 150, 661, 396, 286, 786], ['01-02-2018', 231, 341, 57, 768, 941],
            ['01-03-2018', 486, 526, 442, 628, 621],
            ['01-04-2018', 279, 336, 140, 705, 184], ['01-05-2018', 304, 137, 800, 94, 369],
            ['01-06-2018', 919, 340, 372, 494, 117],
            ['01-07-2018', 947, 920, 848, 716, 719], ['01-08-2018', 423, 20, 313, 368, 909],
            ['01-09-2018', 422, 678, 656, 604, 674],
            ['01-10-2018', 422, 678, 656, 604, 674], ['01-11-2018', 337, 501, 743, 606, 991],
            ['01-12-2018', 408, 536, 669, 903, 463]]
    df = pd.DataFrame(data, columns=['date', 'A', 'B', 'C', 'D', 'E'])
    result = param(df)
    print(result)

When looking at the function def param(data_param):, the first thing that my IDE highlighted was mod_score_param was used inside a loop on a conditional path - but never initialised outside the loop.
When you write code, you need to be aware of what scope your variables have. Usually variables used inside loops are discarded at the end of the loop. This changes the start of def param(data_param): slightly to:

    seasonal_pdq = [(x[0], x[1], x[2], 12) for x in list(
        itertools.product(p, d, q))]  # Generate all different combinations of seasonal p, q and q triplets

    mod_score_param = pd.DataFrame({
        'id': w,
        'param': x,
        'param_seasonal': y,
        'results_aic': z,
    })

    for i in data_param:

The next modification I did was to extract the call to the stats package into it's own function, this is because we try to adhere to the Single Responsibility Principle (from S.O.L.I.D) where each function should only perform a single action - it makes it much easier to track down bugs, and we usually only change a single line of code when fixing problems (great when our single change doesn't work - we don't need to debug 10 new lines of code to find out what went wrong on top of the original bug).
Much easier in the long-run (and the code looks very clean). Here is the function:

def model_params(data_param, i, param, param_seasonal):
    results = sm.tsa.statespace.SARIMAX(data_param[i],
                                        order=param,
                                        seasonal_order=param_seasonal,
                                        enforce_stationarity=False,
                                        enforce_invertibility=False).fit()
    return results.aic

So the caller becomes:

            try:
                results = model_params(data_param, i, param, param_seasonal)
                w.append(i)
                x.append(param)
                y.append(param_seasonal)
                z.append(results)

The exception was modified slightly too:

            except Exception as e:
                print("Exception reached: ", e)
                continue

So we can see immediately what is broken and needs fixing. It's important with any coding - you should never let your exceptions be silenced, because it will lead to bigger problems in the future.
If they are known, catch them with the appropriate exception handler, such as ZeroDivisionError or TypeError, and handle them at the time of the exception.
If they don't match any existing error type, create a custom error handler and handle that particular error before continuing on.

That wraps up my code review and attempt to help you find the slow down in your code. Please apply these changes and see if the modifications to your data frame definitions (such as defining the frequency of the date strings - that's one of the warnings I saw) make the intended improvements.
Good Luck!


Search for products based on criteria The method must search Products found by criteria and store them in @products.

The criteria are as follows:

If the user enters something in text_field (:search_keywords) then the product's name needs to be matched with that word
If the user specifies product_type (:product[:type]) then all the found products must be of that type.

def search
  if params[:search_keywords] && params[:search_keywords] != ""
    @products = Product.where("name LIKE '%" + params[:search_keywords] + "%'")
    if params[:product] && params[:product][:type] && params[:product][:type] != ""
      @products = Product.where("name LIKE '%" + params[:search_keywords] + "%' AND product_type_id = " + params[:product][:type])
    end
  else
    if params[:product] && params[:product][:type] && params[:product][:type] != ""
      @products = Product.find_all_by_product_type_id(params[:product][:type])
    else
      @products = Product.all
    end
  end
end

Try to use scopes, its a lot nicer

def search
  @products = Products.scoped
  if !params[:search_keywords].blank?
    @products = @products.where("name LIKE ?", "%#{params[:search_keywords]}%")
  end
  if !params[:product].try(:[], :type).blank?
    @products = @products.where(:product_type_id => params[:product][:type])
  end
  @products # maybe @products.all if realy needed
end


"Human" interfaces with two implementation classes The Helloworld class executes all of the methods. Human is the interface, and Nate & Ryan are two classes that implement the Human interface. Are there any deficiencies or faults that my code contains that could help me in the long run?

public class Helloworld{

    public static void main(String[] args) {

        Human ryan = new Ryan();
        ryan.setHeight(12);
        System.out.println("Ryan's height is " + ryan.getHeight());

        Human nate = new Nate();
        nate.setHeight(32);
        System.out.println("Nate's height is " + nate.getHeight());

        nate.hit(ryan);

        /*
         * OUTPUT --------------------------------------
         *
         * Ryan's height is 12
         * Nate's height is 32
         * Nate has hit Ryan
         *
         */

    }

}

interface Human{

    public void sleep();
    public void eat();
    public void wakeUp();
    public void walk();
    public void hit(Human name);
    public void setHeight(int height);
    public int getHeight();
    public String getName();

}

class Nate implements Human{

    int height = 0;
    final String name = "Nate";

    public String getName() {
        return this.name;
    }

    public void setHeight(int height) {
        if(height > 0) {
            this.height = height;
        }else {
            System.out.println("You can't set Nate's height to less than 0.");
        }
    }
    public int getHeight() {
        return this.height;
    }
    public void sleep() {
        System.out.println("Nate has fallen asleep");
    }
    public void eat() {
        System.out.println("Nate has eaten");
    }
    public void wakeUp() {
        System.out.println("Nate has woken up");
    }
    public void walk() {
        System.out.println("Nate is walking");
    }
    public void hit(Human name) {
        System.out.println("Nate has hit " + name.getName());
    }

}

class Ryan implements Human{

    int height = 0;
    final String name = "Ryan";

    public String getName() {
        return this.name;
    }

    public void setHeight(int height) {
        if(height > 0) {
            this.height = height;
        }else {
            System.out.println("You can't set Ryan's height to less than 0.");
        }
    }
    public int getHeight() {
        return this.height;
    }
    public void sleep() {
        System.out.println("Ryan has fallen asleep");
    }
    public void eat() {
        System.out.println("Ryan has eaten");
    }
    public void wakeUp() {
        System.out.println("Ryan has woken up");
    }
    public void walk() {
        System.out.println("Ryan is walking");
    }
    public void hit(Human name) {
        System.out.println("Ryan has hit " + name.getName());
    }

}

A class is a blueprint or template or set of instructions to build a specific type of object. Every object is built from a class.
  An instance is a specific object built from a specific class. It is assigned to a reference variable that is used to access all of the instance's properties and methods.

Ryan and nate should both be an instance. They have the same properties. From the 2 classes you can extract one template and make it a Human class.

What you are doing now is hard coding their properties into the methods. Try to make it more dynamic by creating one class.

public void eat() {
    System.out.println("Nate has eaten");
}

Can then be tranformed into:

public void eat() {
    System.out.println(this.name + " has eaten");
}


Gives a live view to a changing file on server I have a file on my server which contains a running output of a script. This file is constantly changing and fluctuates in size between 0 - 1Mb. Now I want to take this file and make it viewable from the web. Currently I am using jQuery to get the file then I set a textarea's value to be the file content. In order to make this a "live" window I repeat the code every 5 seconds.

This was working meh at first but now is causing high cpu usage and essentially crashing my webpage.

Here is the code I am currently using.

function getUtilLog(){
    $("#livewindow").resizable();
    $.ajaxSetup({
        // Disable caching of AJAX responses
        cache: false
    });

    readUtilLog();
    setInterval(function(){readUtilLog();}, 5000);

    function readUtilLog(){
        $.get("UtilRun.log", function(data){
            if (data.length < 256000){
                $.get("UtilRun.log.1", function(data1){
                    d=data1 + data;
                    $('textarea#livewindow').val(d);
                });
            }
            else{
                d=data;
                $('textarea#livewindow').val(d);
            }
        });
    }
}

I want to know a better way of doing this which will be less cpu intensive. What is the correct way of accomplishing this?

Global variables
In side your function readUtilLog, in both cases of your if conditional, you have this:
d=data1 + data;

or this:
d=data;

Why are you using the d variable here? No where else in your code did you write var d, so when one of these conditional cases first runs, you are going to create a global d variable, which is just bad practice.
Since all you are doing with that d variable is setting an element's value to it in the very next line, then why not just set the element's value immediately?
That would look like this:
$('textarea#livewindow').val(data1 + data);

and:
$('textarea#livewindow').val(data);

OC$ (obsessive compulsive $)
Every 5 seconds, no matter what conditional passes, you do this:
$('textarea#livewindow')...

This magical $ function does quite a lot work under the hood to find an element. Therefore, this can be a pretty huge performance impact, especially if you are calling it every 5 seconds.
Your code would benefit a lot speed and efficiency wise if you store the this element in a variable, and then access it's .val through the variable.
var liveWindow = $("textarea#livewindow");

Now, when you are accessing the variable(for example):
liveWindow.val(data);

setTimeout vs setInterval
I have two thoughts on why this is bad.

Possible bad way 1
It's quite dangerous that you are using setInterval as a timer for calling readUtilLog every 5 seconds, and here's why:
After you have created an interval with setInterval, the interval now is going to create a new thread running the readUtilLog every 5 seconds even if the last readUtilLog has not yet completed.
And that's the problem: completing the last readUtilLog. The readUtilLog function has two very intensive and performance reducing tasks: XHR requests.
XHR requests can take quite a while. That means that, as these requests are taking their time, the interval could be spawning new threads background, which will eventually run your browser dry of threads and possibly cause it to crash.

Possible bad way 2
The same thing as above, only a thought on $.get.
In vanilla JavaScript, XHR requests occur asynchronously. That means that, every time you launch an XHR request, you are creating a new thread for the return.
Now you have two sources spawning threads: your interval, and the function that your interval runs.
That means that every 5 seconds, your interval is going to create a thread running readUtilLog, which is going to create a thread to handle the XHR request, which could potentially create another thread for another XHR request.
That is just complete madness, and could easily run your browser dry of threads potentially crashing it.

Solution
Use setTimeout.
As an example, this is what your code would look like if you were to switch to setTimeout:
function foo() {
    ...
    setTimeout(foo, 5000);
}

foo();

Why is this better? This will wait for the last thread to complete before starting a new one.
If there is an intensive function running in foo that will take longer than 5 seconds, who cares? That setTimeout isn't going to run until the end of the function.
This prevents thread build-up.

Nit-picks
There are just small things in your code that I'd like to point out.

setInterval(function(){readUtilLog();}, 5000);

Since you aren't trying to pass any parameters to readUtilLog, wrapping it an anonymous function is unnecessary:
setInterval(readUtilLog, 5000);

if (data.length < 256000){

Where did the 256000 come from?. You should create a constant at the top of your code and use that constant instead of just the straight-up value 256000.
var CONSTANT_NAME = 256000;

CONSTANT_NAME is not a good name; I could not derive a good name out of context.


Quiz form with random questions How could I make this code more efficient?  Instead of using many if-statements, should I make a function, array, or switch?  How would I do that?

<form method="post" action="trivia2.php">

How many days are in a week? <input type="text" name="daysweek">

How many days are in a year? <input type="text" name="daysyear">

How many weeks are in a year? <input type="text" name="weeksyear">

What color is the sky? <input type="text" name="colorsky">

what is us president name? <input type="text" name="presidentname">

How many sides does a triangle have? <input type="text" name="sidestriange">

What is the first element on the periodic table of elements? <input type="text" name="firstelement">

Bees create what sweet substance? <input type="text" name="beesubstance">

How many stripes are displayed on the American flag?<input type="text" name="stripes">

<input type="submit" value="Done!"/>
</form>
<?php
$daysweek=$_POST['daysweek'];
$daysyear=$_POST['daysyear'];
$weeksyear=$_POST['weeksyear'];
$colorsky=$_POST['colorsky'];
$presidentname=$_POST['presidentname'];
$sidestriangle=$_POST['sidestriange'];
$firstelement=$_POST['firstelement'];
$beesubstance=$_POST['beesubstance'];
$stripes=$_POST['stripes'];
$wrong=0;
if($daysweek==7){
echo "1. you got the right answer: $daysweek
";
}else {
    $wrong++;
    echo "number 1 is wrong
";}

if($daysyear==365){
echo "2. you got the right answer: $daysyear
";
}else {
    $wrong++;
    echo "number 2 is wrong
";}

if($weeksyear==52){
echo "3. you got the right answer: $weeksyear
";
}else {
    $wrong++;
    echo "number 3 is wrong
";}

if($colorsky=='blue'){
echo "4. you got the right answer: $colorsky
";
}else {
    $wrong++;
    echo "number 4 is wrong
";}

if($presidentname=='obama'){
echo "5. you got the right answer: $presidentname
";
}else {
    $wrong++;
    echo "number 5 is wrong
";}

if($sidestriangle==3){
echo "6. you got the right answer: $sidestriangle
";
}else {
    $wrong++;
    echo "number 6 is wrong
";}

if($firstelement=='hydrogen'){
echo "7. you got the right answer: $firstelement
";
}else {
    $wrong++;
    echo "number 7 is wrong
";}

if($beesubstance=='honey'){
echo "8. you got the right answer: $beesubstance
";
}else {
    $wrong++;
    echo "number 8 is wrong
";}

if($stripes==13){
echo "9. you got the right answer: $stripes
";
}else {
    $wrong++;
    echo "number 9 is wrong
";}
echo "
You have $wrong wrong answers
";
?>

Theres nothing particularly unefficient about what you have, but there are a couple of things. instead of storing the variables like

$daysweek=$_POST['daysweek'];

just leave them in $_POST like

if($_POST['daysweek']==7){//..

and then instead of

echo "1. you got the right answer: $daysweek
";

you already know the answer is 7 so just

echo "1. you got the right answer: 7
";

but based on the content of your application you are not yet in a place where you need to be worrying about optimization


Reading and classifying lines from a file I'm new to C which I'm learning in university now, and I'm not sure if the following is considered in C good practices or not.

For an assignment in a simple Classification problem I wrote the following code:

int train(FILE *file, const dim dim, const int m, vector *linearSeparator)
{
    size_t len = 0;
    char *line;
    bool stop = false;

    Sample *sample = NULL;
    for(int i = 0; i < m && getline(&line, &len, file) != -1 && !stop; i++)
    {
        if (strToSample(line, SEPARATOR, dim, sample) != EXIT_SUCCESS ||
            calibrateLinearSeparator(sample, linearSeparator, dim) != EXIT_SUCCESS)
        {
            printError(TRAINING, "Failed processing line");
            stop = true;
        }
        free(line);
        freeSample(sample);
    }
    line = NULL;
    return stop ? EXIT_FAILURE : EXIT_SUCCESS;
}

The functions:

strToSample gets a char* line and constructs a Sample object out of it, setting the *sample pointer. It returns EXIT_SUCCESS if succeeded and an error code if failed.
calibrateLinearSeparator uses the sample set by the previous function to manipulate the linearSeparator object (vector - a custom struct to represent a double[]). This function too returns EXIT_SUCCESS if succeeded and an error code if failed.

Both these functions print details about their internal errors and at this level I'm printing in what sample id that internal error occurred.

My question is:

Is this way of writing the if statement a good practice? Is it readable? Should I do it differently?

It feels as if these functions are having side effects. They get pointers to set but are used in an if statement to check if they succeeded in a way that order matters.

The idea behind writing it this way is that if either of these functions fail I want to print an error and not continue the loop. If the first one fails then the second is not performed (which is what I want). In any case of success or failure I want to free the allocated memory.
Is it a good practice in C to write the for condition the way I did? Having 3 conditions in it? (checking i, trying to bring next line and !stop?) It seems to me that this helps making the code cleaner as there are less "details" lines (of increasing i++ for example if the loop was a while loop on getLine)

Code has bug. After free'ing line, set len = 0; line = NULL;

    free(line);
    line = NULL; // add
    len = 0; // add

Further, it would be better to initialize line and move this free();  ... to outside the for()   loop.  Also line = NULL; len = 0; would not be needed there.

size_t len = 0;
char *line = NULL;
...
for(...; ... getline(&line, &len, file) ...; ...) {
  ...
}
free(line);

  Is this way of writing the if statement a good practice? Is it readable? Should I do it differently?

It is not clear that the  { printError(... true; } block is part of if() and not a free standing block.  Consider the below.  The first lacks clarity that the first block is part of the if() and the 2nd block simply a stand-along block

    if (strToSample(line, SEPARATOR, dim, sample) != EXIT_SUCCESS ||
        calibrateLinearSeparator(sample, linearSeparator, dim) != EXIT_SUCCESS)
    {
        printError(TRAINING, "Failed processing line");
        stop = true;
    }
    {
        printError(TRAINING, "Status");
    }
    free(line);
    freeSample(sample);

vs. this form.  The 1st block is part of the if() and the 2nd block is not nested like the first.

    if (strToSample(line, SEPARATOR, dim, sample) != EXIT_SUCCESS ||
        calibrateLinearSeparator(sample, linearSeparator, dim) != EXIT_SUCCESS)
      {
        printError(TRAINING, "Failed processing line");
        stop = true;
      }
    {
        printError(TRAINING, "Status");
    }
    free(line);
    freeSample(sample);

  Is it a good practice in C to write the for condition the way I did?

Why read a line if stop is true?  Certainly this is surprising functionality.  Given 3 stopping conditions, it appears to me that the combination of them occurring in groups, the side effects of getline() , and the short-circuit nature of && led to weak function flow.

for(int i = 0; i < m && getline(&line, &len, file) != -1 && !stop; i++)
// vs.
for(int i = 0; i < m && !stop && getline(&line, &len, file) != -1; i++)

IMO, a for() loop is best as for (initialize_index, index_test; index_delta).  Another other stopping condition belongs in the body.  Example:

for (int i = 0; i < m; i++) {
  if (stop || getline(&line, &len, file) == -1) {
    break;
  }
  ...
}

Notice this form avoid ! in 2 places.  Negation adds to complexity.  Don't you not think it is not so and not so when not done?


Pagination using livequery in jQuery I have created a piece of code for a live pagination, but I don't like the code. It works it does the job, but I guess I could done it better.

$('.show-content').livequery(function() {
    $(this).find('.page-me:not(:first)').hide();
    var totalPage = $(this).find('.page-me').length;

    for (var i = 0; i <= totalPage - 1; i++) {
        $('.modal-pagination').append('Page ' + (i + 1) + '')
    }

    $('.modal-pagination a').live('click', function() {
        var thePag = $(this).attr('id');

        $('.show-content').find('.page-me').hide();
        $('.show-content').find('.page-me:nth(' + thePag + ')').show();

        return false;
    });
});

The code looks fine to me. The only things I would personally change is the use of the live() method (which has been deprecated, and was slow anyway) todelegate() and the use of the :nth() selector, which again I believe is not very fast - although I've not specifically tested it. In place of that I'd use eq().

$('.modal-pagination').delegate('a', 'click', function(e) {
    e.preventDefault();
    var thePag = $(this).attr('id');
    $('.show-content').find('.page-me').hide().eq(thePag -1).show();
});


Word Square generation in Python For reasons unknown I've recently taken up generating word squares, or more accurately double word squares. Below you can see my implementation in Python, in about 40 lines. The code uses this word list. It takes around 10ms for dim=2 (2x2 word squares) and around 13 seconds for dim=3 (3x3 squares). For dim=4 it explodes to something like 3.5 hours in cpython, and causes a MemoryError in pypy. For dim=20 and dim=21 it returns 0 solutions in a few seconds, but for dim=18 and dim=19 it causes a MemoryError in both pypy and cpython.

So, I am looking for improvements that will allow me to explore values of dim >4, but also to explore ways of solving the MemoryErrors for the values <20. Small improvements of a few %, improvements in how things are expressed, as well as large improvements in the algorithm are all welcome.

import time
start = time.clock()

dim = 3 #the dimension of our square
posmax = dim**2 #maximum positions on a dim*dim square

words = open("word.list").read().splitlines()
words = set([w for w in words if len(w)==dim])
print 'Words: %s' % len(words)

prefs = {}
for w in words:
    for i in xrange(0,dim):
        prefs[w[:i]] = prefs.get(w[:i], set())
        prefs[w[:i]].add(w[i])

sq, options = ['' for i in xrange(dim)], {}

for i in prefs:
    for j in prefs:
        options[(i,j)] = [(i+o, j+o)
            for o in prefs[i] & prefs[j]]

schedule = [(p/dim, p%dim) for p in xrange(posmax)]

def addone(square, isquare, position=0):
    if position == posmax: yield square
    else:
        x,y = schedule[position]
        square2, isquare2 = square[:], isquare[:]

        for o in options[(square[x], isquare[y])]:
            square2[x], isquare2[y] = o
            for s in addone(square2, isquare2, position+1):
                yield s

print sum(1 for s in addone(sq, sq[:]))

print (time.clock() - start)

Am working on a few minor improvements, but I think the major saving to be had is in removing the line

square2, isquare2 = square[:], isquare[:]

Instead, do

sofar = square[x], isquare[y]
for o in options[sofar]:
    square[x], isquare[y] = o
    for s in addone(square, isquare, position+1):
        yield s
square[x], isquare[y] = sofar


Clicking outside a form On click on a div, a form replace this div. When the user clicks outside the form, the form is hidden and the div is re-show. For this, I implemented this process and I want your feedback, please.

function showForm(){
   hide the div
   show the form, and set value of a form textarea
    textarea.focus();
}

function hideForm(id_form) {
    hide the form
    show the div
}

$(document).ready(function() {
    $("form[name=theform] :submit").mousedown(function() {
         myprocess with ajax (not important)
    });
});

$(document).ready(function() {
    $("form[name=theform] textarea").focusout(function(e){
        When a form textarea loses the focus, the form is hide with this function :
        hideForm($(this).closest("form").attr('id'));
    });
});

// The process beginning
$(document).ready(function(){
    $('div[name=thediv]').click(showForm);
});

I have used the function blur() at the beginning, but it was difficult with the submit button, so I chose this process containing mousedown and focusout.

Can these 2 functions (mousedown and focusout) provoke problems? What do you think about this process?

I would move all of the functionality into a single document ready handler. I will use the short-hand notation.

$(function () {
    /* All code goes in here. */
});

I will use less verbose selectors for creating my jQuery objects and I will cache them by assigning them to variables.

var $div = $('#TheDiv');
var $form = $('#TheForm');
var $window = $(window);

I think that hiding the form on the textarea's focusout event is a bad idea because this event could be triggered by the user tabbing out of the textarea. Hiding the form at this point would prevent the user from submitting the form.

I think a better approach is to take our direction directly from your requirements and hide the form when the user clicks outside of it. We can do this by attaching a click handler to the $window. In this handler we will call hideForm only if the clicked target was neither the form element nor an element within the form.

$window.on('click.TheForm', function (e) {
    if ($(e.target).is($form)) { return; }
    if ($.contains($form.get(0), e.target)) { return; }
    hideForm();
});

Notice above that the click event has been namespaced with '.TheForm'. Namespacing will allow us to easily remove the event handler without removing any other click events that may have been attached to the window. I will wrap the $window click handler code from above into a function so that we can re-attach this handler as we please. I will also create a function for removing the handler.

var attachWindowClick = function () {
    $window.on('click.TheForm', function (e) {
        if ($(e.target).is($form)) { return; }
        if ($.contains($form.get(0), e.target)) { return; }
        hideForm();
    });
};

var removeWindowClick = function () {
    $window.off('click.TheForm');
};

If we want to hide the form when it is submitted, it will be better to listen for the submit event on the form rather than the mousedown event on the submit button, because the user can submit the form without the mouse, for example, by pressing Enter.

$form.on('submit', function (e) {
    /* Do AJAX stuff here. */
    hideForm();
    e.preventDefault();
});

In the showForm function, we will attach our window click listener so that we can capture clicks while the form is displayed. Note that we do not want our click of the #TheDiv to bubble-up and trigger our handler, so we will ensure our call to attachWindowClick is outside of the current call stack by using setTimeout with a delay of 0 milliseconds.

var showForm = function () {
    $form.show();
    $div.hide();
    $form.find('textarea').focus();
    setTimeout(attachWindowClick, 0);
};

The final code looks as follows:

$(function () {
    var $div = $('#TheDiv');
    var $form = $('#TheForm');
    var $window = $(window);

    var attachWindowClick = function () {
        $window.on('click.TheForm', function (e) {
            if ($(e.target).is($form)) { return; }
            if ($.contains($form.get(0), e.target)) { return; }
            hideForm();
        });
    };

    var removeWindowClick = function () {
        $window.off('click.TheForm');
    };

    var hideForm = function () {
        $form.hide();
        $div.show();
        removeWindowClick();
    };

    var showForm = function () {
        $form.show();
        $div.hide();
        $form.find('textarea').focus();
        setTimeout(attachWindowClick, 0);
    };

    $form.on('submit', function (e) {
        console.log('do ajax');
        hideForm();
        e.preventDefault();
    });

    $div.on('click', showForm);
});


Defining routes and controllers relationship for given pages I'm building a CMS app with the help of a custom framework. The back-office is composed of the following pages:

post list
page list
user list
post edit
page edit
user edit

Actually, my routes are defined as follows (I don't put regex here for clarity sake):

return array(
    '/post-list/' => array(
        'controller' => 'PostListController',
        'action'     => 'view'
    ),
    '/page-list/' => array(
        'controller' => 'PageListController',
        'action'     => 'view'
    ),
    '/user-list/' => array(
        'controller' => 'UserListController',
        'action'     => 'view'
    ),
    '/post-edit/(params)' => array(
        'controller' => 'PostEditController',
        'action'     => 'view'
    ),
    '/page-edit/(params)' => array(
        'controller' => 'PageEditController',
        'action'     => 'view'
    ),
    '/user-edit/(params)' => array(
        'controller' => 'UserEditController',
        'action'     => 'view'
    ),
);

With this approach, there are many little controllers that share almost the same lines of code (not DRY code). For example:

PostListController:

public function view()
{
    $postMapper = $this->repository->getMapper('PostMapper');
    $postSlugs  = $postMapper->fetchPostSlugs();
    $posts = array();

    foreach ($postSlugs as $postSlug) {
        $posts[] = $this->repository->getModel('PostModel', $postSlug);
    }

    if ($view = $this->repository->getView('PostListView')) {
        $models = array('posts' => $posts);
        $body   = $view->render($models);

        $this->response->setBody($body);
        $this->response->send();

        return;
    }
    // ...404 handling
}

... will be nearly identical to the PageListController, the only things that change are View and Model class names.

What would be the MVC best practice or which make more sense?

Keeping this approach: 1 controller for 1 page?
Making a controller that handles Lists having for action: posts(), pages(), users() and a controller handling Edit the same way?
Making a controller handling Post having for actions: edit() and list(), same for Page and User? (note that in this case I might still have some "unDRY" code).
Something else..?

I asked myself today again the same question for my current project where I have to implement a bunch of entities that will need their own controllers implementing usual actions such as:

show()
list()
edit()
save()
delete()
etc...

I believe we can extract from each of these actions a logic/control flow pattern common to all entities.

Thus, an abstract EntityController implementing these actions seems to me the way to go... Of course we can inject in child controllers extending EntityController specific parameters and services.


Analyse an Nginx access log with awk and produce a report I'm looking for your opinions on anything that I am doing wrong in the application below, such as best practices, glaringly horrible errors or even just your own personal opinion.

Task: Grab an Nginx access log, ask it some questions and dump responses out to a templated report.

I have removed all comments from this post to keep the reading to a minimum.  You can see the full version at GitHub.

#!/usr/bin/python
import datetime
from os import remove
from subprocess import Popen
from subprocess import PIPE
from subprocess import check_output
from string import Template

class NginxReporter():

    location = None
    template = 'templates/default.html'
    html = None

    def __init__(self, location=None, template=None, execute=False):
        self.location = location

        if template:
            self.template = template

        if execute:
            self.parse()
            self.output()
            self.delete()

    def scpdownload(self, remotelocation=None):
        if remotelocation:
            check_output(['scp', remotelocation, self.location])

    def delete(self):
        remove(self.location)

    def parse(self):
        html = {}

        codes = Popen("awk '{print $9}' " + self.location +
                       " | sort | uniq -c | sort -r", shell=True, stdout=PIPE)

        html['codes'] = self.build(codes)

        statuses = [206, 301, 302, 400, 404, 408, 499, 500]

        for status in statuses:
            statuslist = Popen("awk '($9 ~ /" + str(status) + "/)' "
                               + self.location + " | awk '{print $7}' " +
                               " | sort | uniq -c | sort -r",
                               shell=True, stdout=PIPE)

            html["http_"+str(status)+"_code"] = self.build(statuslist)

        html['generated'] = datetime.datetime.today().strftime('%Y-%m-%d %H:%M')

        self.html = html

    def build(self, output):
        block = ""

        while True:
            out = output.stdout.read()

            if out == '' and output.poll() != None:
                break
            if out != '':
                block = out

        return block

    def output(self):
        file = open(self.template)
        block = Template(file.read())
        print block.substitute(self.html)

Usage

report = NginxReporter('access.log')
""" report.scpdownload("sshuser@server.com:/path/to/nginx/access.log") """
report.parse()
report.output()

The biggest problem, and the hardest to fix, is calling awk, sort | uniq -c | sort -r, sometimes within a loop, when you could implement all that more efficiently in Python. It's ugly to do this in multiple sub-shells and multiple processes. But that would take a substantial rewrite.

You are not using the class attributes here:

class NginxReporter():

    location = None
    template = 'templates/default.html'
    html = None

So you could just delete these from your class.

The constructor is not well designed. You're trying to do too much, for two very different use cases:

In one use case, you use it just to set a logfile location, and then you call other methods to do the real work
In another use case, the constructor would call the other methods

It's better to decide one of these ways, stick with it, and simplify the code accordingly.

This method is strange:

def scpdownload(self, remotelocation=None):
    if remotelocation:
        check_output(['scp', remotelocation, self.location])

You allow calling it without params, or null param, to do nothing? This is messy and confusing. It would be better to require a param.

Btw, downloading a file with scp doesn't sound like it belongs to the class. It violates the single responsibility principle: a class should have one clear purpose (parse nginx log files), and nothing else.

This could use some reworking:

if out == '' and output.poll() != None:
    break
if out != '':
    block = out

Like this:

if not out:
    if not output.poll():
        break
else:
    block = out

You open a file for reading but you're not closing it:

file = open(self.template)
block = Template(file.read())
print block.substitute(self.html)

The right way to do is using with:

with open(self.template) as fh:
    block = Template(fh.read())
    print block.substitute(self.html)

I also renamed file to fh because file is a class in Python 2.x

If you want to temporarily comment out one line,
it's better to use # instead of """ ... """ which is recommended for multi-line comments and docstrings:

report = NginxReporter('access.log')
#report.scpdownload("sshuser@server.com:/path/to/nginx/access.log")
report.parse()
report.output()


Finding number of integer partitions I am trying to find number of integer partitions of given n - number. If I have n == 4, the answer should be 5 because:

\$4 = 1+1+1+1\$
\$4 = 2+1+1\$
\$4 = 3+1\$
\$4 = 2+2\$
\$4 = 4\$

My code works properly but the matter is that it counts big numbers for a very long time. I have no idea how to optimize my code. Maybe you can help me to make it faster?

def get_answer(n):
    if n == 0:
        yield []
        return
    for p in get_answer(n-1):
        yield [1] + p
        if p and (len(p) < 2 or p[1] > p[0]):
            yield [p[0] + 1] + p[1:]
number_of_partitions=lambda n:sum(1 for _ in get_answer(n))

A slightly more efficient dynamic programming approach means you only need O(n) space:

def partitions(n):
    parts = [1]+[0]*n
    for t in range(1, n+1):
        for i, x in enumerate(range(t, n+1)):
            parts[x] += parts[i]
    return parts[n]

In []:
partitions(50)

Out[]:
204226

Note: this is trivial to extend to the coin change problem (the number of ways you can make change with certain coins: Ways to make change for a dollar), by restricted the values in the outer loop, e.g.:

def coin_change(n, coins):
    parts = [1]+[0]*n
    for c in coins:
        for i, x in enumerate(range(c, n+1)):
            parts[x] += parts[i]
    return parts[n]

In []:
coin_change(100, {1, 5, 10, 25, 50, 100})

Out[]:
293


Communicating between GUI and controller class using Mediator pattern I am writing a program that allows a user to interact with MIDI-connected devices. Upon connecting a MIDI device, interactions with this device will update a GUI. Updates to the GUI include what keys are being pressed, if the volume is turned up/down, etc. In addition, the user can "request" the status of MIDI components through the GUI (how many MIDI devices are available, opening/closing MIDI devices).

All of this is set up using the Mediator pattern, and I would like to ask you all the best way of sending "requests" to each of these classes; MidiController (controls MIDI-related tasks) and GUI (which shows MIDI-related events).

A button exists on the GUI to search for MIDI-connected devices, upon clicking it, the MidiController will get some message (from the Mediator pattern) to find MIDI-connected devices.

My code currently looks like this for this function:

@Override
public void receive(String message) {
    System.out.println("Message from GUI: " + message);

    switch(message){
    case "FIND_ALL_MIDI_DEVICES":

        devices = MidiSystem.getMidiDeviceInfo();
        break;
    default:
        break;
    }
}

I've opted to use a switch statement, but I feel there is a better option. I am expecting this class, MidiController to respond to many different messages. Is there a clean way I might communicate sending data / requests / updates between these two classes?

Since you only support a limited subset of messages you might want to consider using an enum to represent the different kinds of messages.

I think you're building yourself a god-class though. Instead of having everything running through the MidiController (which makes the Mediator Pattern basically useless) you could just as well couple GUI and Controller directly.

Interestingly that's the more often used approach, to have the Controller take the role described as Mediator in the link you provided. This is especially interesting because it is not guaranteed that you have similar "suppliers of information" for your GUI.

I think the Mediator Pattern overall is the wrong pattern for your situation :/


Simulation of Central Limiting Theorem (CLT) The following is the simulation of Central Limiting Theorem from Statistics:

class MyRandom:
    m = 4294967296;# modulus
    a = 1664525; # multiplier
    c = 1013904223; # increment
    seed = 1; #initial seed
    def NextInt(self):
        self.seed = (((self.a * self.seed + self.c) % self.m));
        return self.seed;

    def NextIntMm(self, mins, maxs):
        temp = self.NextInt();
        ddd = float(temp) / float(self.m);
        returns =  int((maxs - mins) * ddd + mins);
        return returns;

    def NextDouble(self):
        temp = self.NextInt();
        return float(temp) / float(self.m);

    def NextDoubleMm(self, mins, maxs):
        temp = self.NextInt();
        fraction = float(temp) / float(self.m);
        return (maxs - mins) * fraction + mins;

#################################################################
N = 999999;
r = MyRandom();
population = [];
population_sum = 0;

for i in range(0, N):
    x = r.NextDouble();
    population.append(x);
    population_sum = population_sum + x;

population_mean = population_sum / len(population);

print("Population mean = ", population_mean);

##################################################################
rnd_num_count = 30;
sample_size = 12;
sample_means = [];
sum_of_sample_mean = 0;

for i in range(0, rnd_num_count):
    sums = 0;
    for j in range(0, sample_size):
        index = r.NextIntMm(0, N - 1);
        element = population[index];
        sums = sums + element;

sample_mean = sums / sample_size;
sum_of_sample_mean = sum_of_sample_mean + sample_mean;
sample_means.append(sample_mean);

print("Mean of sample mean = ", sum_of_sample_mean/ len(sample_means));

Here, Normal Distribution of population is used.

Can you review this and point out any possible improvement needed?

N.B. Defining a random number generator is part of the requirement.

Even though you are implementing your own random number generator, I would stick to the interface defined in the standard library module random. You could have a random, a uniform, a randint (or randrange if it is inclusive on the end) and a choices method.

Your class should probably also allow setting the seed on creation.

Python has an official style-guide, PEP8, which recommends not using trailing ; and using lower_case both for variables as well as functions/methods.

Even in Python 2 (which you should no longer use), if either the numerator or the divisor is a float, the result is also a float with the division operator /, so you only need to cast one of them. In Python 3 / is always float division and // is always integer division, so no need for the casts to float.

You should always add a dosctring to your code explaining how to use it.

For range, the start is implicitly 0. Also _ is conventionally used for unused loop variables.

Wrap the code that uses your class in a if __name__ == "__main__": guard to allow importing from this script without it running.

class Random:
    """A pseudo-random number generator using a LCG."""
    m = 4294967296 # modulus
    a = 1664525    # multiplier
    c = 1013904223 # increment

    def __init__(self, seed=1):
        """Initialize the pseudo-random number generator with a seed (default: 1)."""
        self.state = seed

    def randint(self, a=None, b=None):
        """Return either a random integer between `0` and `self.m`
        or between `a` and `b` (exclusive?)."""
        if a is None and b is None:
            self.state = (self.a * self.state + self.c) % self.m
            return self.state
        elif a is not None:
            if b is not None:
                return int(self.uniform(a, b))
            else:
                return int(self.uniform(0, a))
        else:
            raise ValueError("Need to also set `a` if you set `b`.")

    def random(self):
        """Return a random float between 0 and 1 (exclusive?)."""
        return float(self.randint()) / self.m

    def uniform(self, a, b):
        """Return a random float between `a` and `b` (exclusive?)."""
        return (b - a) * self.random() + a

    def choices(self, population, k=1):
        """Return a k sized list of population elements chosen with replacement."""
        n = len(population)
        return [population[self.randint(n - 1)] for _ in range(k)]

The usage of this is similar to your code, just with renamed methods.

You can also greatly shorten your examples using list comprehensions and a utility function mean:

random = Random()

def mean(x):
    return float(sum(x)) / len(x)

if __name__ == "__main__":
    N = 999999
    population = [random.random() for _ in range(N)]
    print("Population mean = ", mean(population))

    rnd_num_count = 30
    sample_size = 12
    sample_means = [mean(random.choices(population, k=sample_size))
                    for _ in range(rnd_num_count)]
    print("Mean of sample mean = ", mean(sample_means))

This script has the advantage that you can easily test it with the standard library random module by replacing the line random = Random() with import random. This way you can see if your class produces random numbers somewhat correctly.

If implementing so many different random functions gets tedious, you can also subclass random.Random:

  Class Random can also be subclassed if you want to use a different
  basic generator of your own devising: in that case, override the
  following methods:  random(), seed(), getstate(), and setstate().
  Optionally, implement a getrandbits() method so that randrange() can
  cover arbitrarily large ranges.

This of course means that you are using the standard library and not your own implementations as much. In that case you probably also want to use statistics.mean.


Taking text from a file and formatting it My code takes numbers from a large text file, then splits it to organise the spacing and to place it into a 2-dimensional array. The code is used to get data for a job scheduler that I'm building.

#reading in workload data
def getworkload():
    work = []
    strings = []
    with open("workload.txt") as f:
        read_data = f.read()
        jobs = read_data.split("\n")
    for j in jobs:
        strings.append(" ".join(j.split()))
    for i in strings:
        work.append([float(s) for s in i.split(" ")])
    return work

print(getworkload())

The text file is over 2000 lines long, and looks like this:

    1        0 1835117 330855  640   5886   945   -1     -1    -1  5   2   1   4  9 -1 -1 -1
    2        0 2265800 251924  640   3124   945   -1     -1    -1  5   2   1   4  9 -1 -1 -1
    3        1 3114175     -1  640     -1   945   -1     -1    -1  5   2   1   4  9 -1 -1 -1
    4  1813487   7481     -1  128     -1 20250   -1     -1    -1  5   3   1   5  8 -1 -1 -1
    5  1814044      0    122  512   1.13  1181   -1     -1    -1  1   1   1   1  9 -1 -1 -1
    6  1814374      1     51  512     -1  1181   -1     -1    -1  1   1   1   2  9 -1 -1 -1
    7  1814511      0     55  512     -1  1181   -1     -1    -1  1   1   1   2  9 -1 -1 -1
    8  1814695      1     51  512     -1  1181   -1     -1    -1  1   1   1   2  9 -1 -1 -1
    9  1815198      0     75  512   2.14  1181   -1     -1    -1  1   1   1   2  9 -1 -1 -1
   10  1815617      0    115  512   1.87  1181   -1     -1    -1  1   1   1   1  9 -1 -1 -1
    …

It takes 2 and a half minutes to run but I can print the returned data. How can it be optimised?

You are doing a lot of unnecessary work. Why split each row only to join it with single spaces and then split it again by those single spaces?

Instead, here is a list comprehension that should do the same thing:

def get_workload(file_name="workload.txt"):
    with open(file_name) as f:
        return [[float(x) for x in row.split()] for row in f]

This uses the fact that files are iterable and when iterating over them you get each row on its own.

If this is still too slow (or e.g. too large to fit into memory), then you need to process each row separately. For this you would make this a generator of processed lines:

def get_workload(file_name="workload.txt"):
    with open(file_name) as f:
        for row in f:
            yield [float(x) for x in row.split()]


Improving a method to guarantee uniqueness I have just written something to guarantee a unique URL when creating pages within an old legacy system (sounds odd doing this retrospectively but we've just changed how the routing works). I don't often end up using while and I think this may be why I feel it can be improved.

private string GetUniqueUrl(Page parentPage, Page newPage)
{
    var isNotDuplicatateUrl = false;
    var version = 0;
    var baseUrl = parentPage != null ? parentPage.FullUrl.Trim() + "/" : string.Empty;

    while (isNotDuplicatateUrl == false)
    {
        var friendlyUrl = version > 0 ? (newPage.FriendlyURLName + "-" + version) : newPage.FriendlyURLName;
        var fullUrl = baseUrl + friendlyUrl;

        var duplicateUrlPage =
            Site.Pages.FirstOrDefault(x => x.FullUrl != null && string.Equals(x.FullUrl.Trim(), fullUrl.Trim(), StringComparison.CurrentCultureIgnoreCase));

        if (duplicateUrlPage == null)
        {
            isNotDuplicatateUrl = true;
        }
        else
        {
            version++;
        }
    }

    return newPage.FriendlyURLName + "-" + version;
}

NB: I know Page.FriendlyURLName should be Page.FriendlyUrlName but I'm only looking for improvements within the scope of this method.

Having a loop to check each version of the URL could become a problem if there end up being a lot of URL's. There many be a better way to do it using a hash, or some other means of persisting data, and retrieving the most recently used version.

Still, even with your loop code, there's a way to do it in a simpler fashion, using a helper method to check for a duplicate URL is a start.....

private bool PageFound(string baseUrl, string friendlyURL)
{
    var fullUrl = baseUrl + friendlyUrl;
    var duplicateUrlPage = Site.Pages.FirstOrDefault(
                 x => x.FullUrl != null
              && string.Equals(x.FullUrl.Trim(), fullUrl.Trim(),
                       StringComparison.CurrentCultureIgnoreCase));
    return duplicateUrlPage != null;
}

private string GetUniqueUrl(Page parentPage, Page newPage)
{
    var version = 0;
    var baseUrl = parentPage != null ? parentPage.FullUrl.Trim() + "/" : string.Empty;
    var friendlyUrl = newPage.FriendlyURLName;
    while (PageFound(baseUrl, friendlyURL))
    {
        version++;
        friendlyUrl = newPage.friendlyURLName + "-" + version;
    }
    return newPage.FriendlyURLName + "-" + version;
}

There are two things to pay attention to here.

Note how the first time the duplicate is checked, there's no 'version' on the URL? The version only gets added for version >=1. This is 'managed' by doing the first URL construction outside the loop, and the rest inside the loop. This saves the conditional on the version == 0
I believe there's a bug in your code (which I have reproduced in my suggestion). If you search for the page without the version (version = 0) and you do not find one, you return the URL with the version anyway (you append "-0"). Are you sure this is what you want to do? If it is not what you want, you can just return friendlyUrl instead of newPage.FriendlyURLName + "-" + version

Using a helper function to check for the duplicate is a good way to put a code block in to a conditional clause for a loop. In essence, by creating a function, we have also created a powerful while loop check. The 'problem' with your code is that it was trying to do condition-loop logic inside the execution block, instead of inside the loop clause
2.


Dump of ColdFusion cache I am trying to give show a UI friendly dump of the cache status

Cache Status

  Cache
  Created Time
  Last Updated
  Last Hit
  Hit Count
  Hit Count %
  Size

<cfoutput>
  <cfloop index="cache" array="#cacheGetAllIds()#">
    <cfset metaData = cacheGetMetadata(cache)>

      #cache#
      #LSTimeFormat(metadata.createdTime, "h:mm:ss")#
      #LSTimeFormat(metadata.lastHit, "h:mm:ss")#
      #LSTimeFormat(metadata.lastupdated, "h:mm:ss")#
      #metadata.hitcount#
      <cfset percent_of_hits = 100.0 * metadata.hitcount / metadata.cache_hitcount>
        #LSNumberFormat(percent_of_hits, '999.9')# %

      #LSNumberFormat(metadata.size)#

  </cfloop>
</cfoutput>

Bootstrap and Datatables formatting is applied. And it looks like this:

I want to make sure this is a good format and that I am not missing any easy to get at useful information

You have the column definitions backwards.  On your header row you have:

Last Updated | Last Hit

but in the actual cf it looks like it's displaying in this order

Last Hit | Last Updated

Other than that it looks great!


BDD tests in PHPUnit No one in my team knows how to write BDD tests, so I started writing some and it's quite working well. I think the time has come to improve the code quality.

This contains a lot of duplicated code and unused features:

<?php
require_once ('core/v3/engine.php');
require_once 'PHPUnit/Extensions/Story/TestCase.php';

class cHelperValidationSpec extends PHPUnit_Extensions_Story_TestCase {

    /**
     * Check all the output for the right cases
     * @scenario
     */
    public function specForJobId() {
        $this->given('For a mValue (mixedValue) and we want to validate if it is an integer')
            ->when('sJobId is a positive integer', 123)
            ->then('return of positive integer should be', 'TRUE')
            ->when('sJobId is a negative integer', -123)
            ->then('return of a negative integer should be', 'FALSE')
            ->when('sJobId is a float', 1.23)
            ->then('return of a float should be', 'FALSE')
            ->when('sJobId is a string containing only digits', '123')
            ->then('return of a string (containing only digits) should be', 'TRUE')
            ->when('sJobId is a string containing not only digits', '123abc-?')
            ->then('return of a string (containing not only digits) should be', 'TRUE');
    }

    public function runGiven(&$world, $action, $arguments) {
        switch($action) {
            case 'For a mValue (mixedValue) and we want to validate if it is an integer': {
                $world['helper'] = new cHelperValidation();
            }
            break;

            default: {
                return $this->notImplemented($action);
            }
        }
    }

    public function runWhen(&$world, $action, $arguments) {
        switch($action) {
            case 'sJobId is a positive integer': {}
            case 'sJobId is a negative integer': {}
            case 'sJobId is a float': {}
            case 'sJobId is a string containing only digits': {}
            case 'sJobId is a string containing not only digits': {}
            break;

            default: {
                return $this->notImplemented($action);
            }
        }
    }

    public function runThen(&$world, $action, $arguments) {
        switch($action) {
            case 'return of positive integer should be': {
                $this->assertEquals(TRUE, $world['helper']->isId(123));
            }
            case 'return of a negative integer should be': {
                $this->assertEquals(FALSE, $world['helper']->isId(-123));
            }
            case 'return of a float should be': {
                $this->assertEquals(FALSE, $world['helper']->isId(1.23));
            }
            case 'return of a string (containing only digits) should be': {
                $this->assertEquals(TRUE, $world['helper']->isId('123'));
            }
            case 'return of a string (containing not only digits) should be': {
                $this->assertEquals(FALSE, $world['helper']->isId('123abc-?'));
            }
            break;
            default: {
                return $this->notImplemented($action);
            }
        }
    }

}

The only reference I'm using for BDD is here, so there is much potential for improving this code.

Your switch statements will run through until they find a break statement.  Having the braces {} only defines a block of code and has no effect on breaking out of the case.

switch (1)
{
case 1: {
   echo 'One';
}
case 2: {
   echo 'Two';
}
default: {
   echo 'Also default';
      }
}

The one above prints out "OneTwoAlso default" whereas the following prints out "One".

switch (1)
{
case 1:
   echo 'One';
   break;
case 2:
   echo 'Two';
   break;
default:
   echo 'Also default';
}

I think your testing would be cleaner if you didn't use then and when methods.  Normally I create an array of test data and expected results and loop over them.  As you are writing the test for the isId method I would have something like this:

public function test_isID()
{
   $object = new Object_That_You_Are_Testing();

   $tests = array(
      array('Data'            => 123,
            'Expected_Result' => true,
            'Test_Name'       => 'Positive Integer'),
      array('Data'            => -123,
            'Expected_Result' => false,
            'Test_Name'       => 'Negative Integer'));

   foreach ($tests as $test)
   {
      $this->assertEquals($test['Expected_Result'],
                          $object->isID($test['Data']),
                          $test['Test_Name'] . ' Failed');
   }
}

You could do more clever things than this.  Also a test will not always be so simple.  Normally I find that I have mocked objects and am ensuring that they get called appropriately.


Using jQuery 1.7 .on() and event.stopPropagation to close boxes when clicking in document body I have the following code which works but seems like it could be improved. I'm not sure if I'm using event.stopPropagation() correctly or if it is the best solution.

The elements are loaded via AJAX and so need to have .on() (previously .live(), which is now deprecated in jQuery 1.7).

I've used the example given here as a basis.

$('html').on('click', '.poster:not(.active) .image-effect', function (event) {

        var obj = $(this).parent();

        // If there are no .close spans
        if (obj.find('.close').length === 0) {

            // Add the close element by javascript to remain semantic
            obj.find('.quick-view').append('&times;');
        }

        // Close any open Quick Views
        closeQuickView();

        // Add the active class (controls opacity)
        obj.addClass('active');

        // Fade in the Quick View
        obj.find('.quick-view').fadeIn(200, 'easeInOutQuint');

        event.preventDefault();
        event.stopPropagation();

    });

    $('html').on('click', '.active', function () {
        return false;
    });

    $('html').on('click', '.close', function () {
        closeQuickView();
    });

    $('html').on('click', '.quick-view', function (event) {
        event.stopPropagation();
    });

    // Close the QuickView with a button
    $('html').on('click', function () {
        closeQuickView();
    });

    function closeQuickView() {
        $('.poster').removeClass('active');
        $('.quick-view').fadeOut(200, 'easeInOutQuint');
    }

For some reason clicking the document isn't working in Chrome (the closeQuickView is not being called) which may be an indicator that the code is not functioning 100%.

My markup is as follows:

        Content

Is this the right way to approach the scenario? Are there any conflicting functions?

It did work in my Chrome. However, I felt your code was confusing with all those handlers. Let me suggest the following:

$('html').on('click', '.poster', function(e) {

  var target = $(e.target);
  var poster = $(this);

  if (!poster.hasClass('active')) {
    closeQuickView();
    poster.addClass('active').find('.quick-view').fadeIn(200);
  }
  if (target.hasClass('quick-view') || target.hasClass('image-effect')) e.stopPropagation(); // prevent bubbling and thus closeClickView call below
});

$('html').on('click', closeQuickView);

This way, it's more concise and hopefully clearer what's handling what.

PS: I threw out the conditional '.close' appendage here, nothing wrong with having it in the vanilla HTML I'd say.


Reverse Polish Notation calculator in Python After reading the definition, I made a simple Reverse Polish Notation (RPN) calculator in Python.

Originally it had just 4 operators (using import operator and a lookup table) and only did integers. After looking at some example calculations, I amended it to work on floats and added a raising to powers. Then I saw some more advanced operations here and added math functions.

Owing to the large amount of math functions available, is there an easier way to include them without having to have math.<function> in the operators lookup?

# Reverse Polish Notation calculator
# based on http://en.wikipedia.org/wiki/Reverse_Polish_notation

import math
import operator
ops = {'+':operator.add,
       '-':operator.sub,
       '*':operator.mul,
       '/':operator.div,
       '^':operator.pow,
       'sin':math.sin,
       'tan':math.tan,
       'cos':math.cos,
       'pi':math.pi}

def is_number(s):
    try:
        float(s)
        return True
    except ValueError:
        pass

def calculate(equation):
    stack = []
    result = 0
    for i in equation:
        if is_number(i):
            stack.insert(0,i)
        else:
            if len(stack) < 2:
                print 'Error: insufficient values in expression'
                break
            else:
                print 'stack: %s' % stack
                if len(i) == 1:
                    n1 = float(stack.pop(1))
                    n2 = float(stack.pop(0))
                    result = ops[i](n1,n2)
                    stack.insert(0,str(result))
                else:
                    n1 = float(stack.pop(0))
                    result = ops[i](math.radians(n1))
                    stack.insert(0,str(result))
    return result

def main():
    running = True
    while running:
        equation = raw_input('enter the equation: ').split(' ')
        answer = calculate(equation)
        print 'RESULT: %f' % answer
        again = raw_input('\nEnter another? ')[0].upper()
        if again != 'Y':
            running = False

if __name__ == '__main__':
    main()

Simple/straightforward test:

enter the equation: 6 4 5 + * 25 2 3 + / -
stack: ['5', '4', '6']
stack: ['9.0', '6']
stack: ['3', '2', '25', '54.0']
stack: ['5.0', '25', '54.0']
stack: ['5.0', '54.0']
RESULT: 49.000000

Math functions test:

enter the equation: 5 8 2 15 * sin * + 2 45 tan + /
stack: ['15', '2', '8', '5']
stack: ['30.0', '8', '5']
stack: ['0.5', '8', '5']
stack: ['4.0', '5']
stack: ['45', '2', '9.0']
stack: ['1.0', '2', '9.0']
stack: ['3.0', '9.0']
RESULT: 3.000000

In Python, list is designed to grow efficiently from the end. Implementing a stack using append and pop at the end is therefore a better approach.
I would expect is_number to return False instead of the implicit None which usually stands for a missing value.
An explicit math. in front of functions makes clear where they come from. If you want to support more functions you have other things to worry about. For example, the forced conversion from degrees to radians only makes sense with certain functions.
You need to store explicitly the number of arguments each operator expects. Your current solution is to infer that from the length of the operator's name. That is not an extensible approach.


Improving efficiency and flow by changing subroutine orders A batch file executes WinSCP.exe's console mode to download files and log the actions. Upon exit, WinSCP returns:

ERRORLEVEL 0 if the files are successfully retrieved or
ERRORLEVEL 1 if there is a problem with the connection -
time outs, etc. - or if no file is found
Unexplainably, WinSCP will occasionally return 0 when no files are found

Due to this, to meet audit requirements, I must check for four possibilities:

ERRORLEVEL 1 and a connection problem
ERRORLEVEL 0 and the file(s) exist(s)
ERRORLEVEL 1 or ERRORLEVEL 0 and no file found

And respond with one of three options (respectively):

Send email indicating error
Send email indicating success
Send email indicating no file

Note, the same group who receives the success notice, should also receive the no file notice as that is not download failure. The code reflects this.

I wrote the fully functional code below but am not sure it's as streamlined as it could be. My first concern is that I can improve flow by using GOTO and perhaps CALL or :EOF commands in more appropriate ways as I can't seem to find a better 'structure' for the conditions/responses. My goal is to eliminate un-necessary spaghetti-ing. Second, I am troubled to find a better way to reset the ERRORLEVEL to zero so I can use an ERRORLEVEL check twice (currently the %comspec%... line). I'm not even sure I need two checks. I appreciate all suggestions.

The code I'm concerned with begins the line after START /WAIT... but I present it in full, for reference:

SET MailDst=successful_code@mydomain.com
SET FailMailDst=failed_code@mydomain.com
SET MailSrc=downloader@mydomain.com
SET MailSrcName=Automated FTP Process
SET OKMailSub=Files Have Been Moved
SET NoFileMailSub=Files Not Present - Maybe Next Time
SET FailMailSub=An Error Occurred!

IF EXIST WSCPLog.txt DEL WSCPLog.txt
SET LOG=WSCPLog.txt

START /WAIT WinSCP.exe /console /script=WSCP-GetC.txt /log=%log%
IF ERRORLEVEL 1 GOTO erroryes
IF NOT EXIST "\\server\path\Test_File.ext" GOTO nofilemail
GOTO okmail

:erroryes
%comspec% /c exit %0%
FINDSTR /C:"Test_File.ext: No such file or directory" %LOG%
IF ERRORLEVEL 0 GOTO nofilemail
IF EXIST %temp%\rdcto.txt DEL %temp%\rdcto.txt
FOR %%a IN (%FailMailDst%) DO ECHO %%a>>%temp%\rdcto.txt
"%~dp0sendmail.exe" /TO=%temp%\rdcto.txt /FROM=%MailSrcName% ^<%MailSrc%^> /REF=%FailMailSub% /MESSAGE=%log% /HOST=mail.mydomian.com
GOTO end

:nofilemail
IF EXIST %temp%\rdcto.txt DEL %temp%\rdcto.txt
FOR %%a IN (%MailDst%) DO ECHO %%a>>%temp%\rdcto.txt
"%~dp0sendmail.exe" /TO=%temp%\rdcto.txt /FROM=%MailSrcName% ^<%MailSrc%^> /REF=%NoFileMailSub% /MESSAGE=%log% /HOST=mail.mydomain.com
GOTO end

:okmail
IF EXIST %temp%\rdcto.txt DEL %temp%\rdcto.txt
FOR %%a IN (%MailDst%) DO ECHO %%a>>%temp%\rdcto.txt
"%~dp0sendmail.exe" /TO=%temp%\rdcto.txt /FROM=%MailSrcName% ^<%MailSrc%^> /REF=%OKMailSub% /MESSAGE=%log% /HOST=mail.mydomain.com

:end

1) What is this line supposed to do?

%comspec% /c exit %0%

I can almost guarantee it is not doing what you think. The most it can do is set the ERRORLEVEL. However, The EXIT command expects a numeric argument, but you pass %0%. The EXIT command returns 0 if it does not receive a numeric argument.

I doubt you have a variable with a name of "0", so the EXIT command sees a string literal value of %0%, which is non-numeric. It consequently returns 0 every time.

Your very next command (FINDSTR), is an external command, so it will set the ERRORLEVEL anyway. There is no need to initialize it.

2) It is generally a good idea to enclose SET assignments in quotes. This guards against poison characters, and inadvertent trailing spaces. (There are some exceptions, but it is a good rule of thumb).

3) It is generally a good idea to always enclose file paths in quotes if a portion is a variable. You never know if the variable may contain spaces or poison characters. The %TEMP% value normally does not have spaces, but it certainly could.

4) There is no need to delete "%temp%\rdcto.txt" when you immediately turn around and recreate it. You can simply redirect using overwrite mode (>) instead of append mode (>>). This will guarantee you start out with a fresh file each time.

5) Why do you write the value of %MailDst% using a FOR statement? You can simply echo the value directly.

>"%temp%\rdcto.txt" echo %MailDst%

6) You do not need to create an :end label and GOTO that label. Instead you can do either of the following: goto :eof or exit /b. They are identical, and neither form requires an explicitly declared label. The only difference is that exit /b can take an optional argument specifying the returned errorlevel.

7) You should encapsulate the logic for writing an email into a subroutine that accepts the variable portion as arguments. It reduces the amount of code, and eases maintenance.

8) I've simplified your ERRORLEVEL logic by using an extra variable to store the return code. I also make use of the conditional && and || operators with FINDSTR - I find them to be more convenient then checking the ERRORLEVEL. But I didn't use them with WinSCP because the logic is a bit complex.

Note that if the last command within a &&( successCommands ) block raises an error, then it would cause the ||( errorCommands ) block to fire. That is why my :sendMail routine explicitly exits with a return code of 0 (success).

Using the above principles, I would write the code as follows:

SET "MailSrc=downloader@mydomain.com"
SET "MailSrcName=Automated FTP Process"

SET "MailDst=successful_code@mydomain.com"
SET "FailMailDst=failed_code@mydomain.com"

SET "OKMailSub=Files Have Been Moved"
SET "NoFileMailSub=Files Not Present - Maybe Next Time"
SET "FailMailSub=An Error Occurred!"

SET "LOG=WSCPLog.txt"

IF EXIST "%LOG%" DEL "%LOG%"
START /WAIT WinSCP.exe /console /script=WSCP-GetC.txt /log="%log%"
SET "RTN=%ERRORLEVEL%"
FINDSTR /C:"Test_File.ext: No such file or directory" "%LOG%" && (
  CALL :sendMail "%MAILdST%" "%NoFileMailSub%"
) || (
  IF %RTN% EQU 0 (
    CALL :sendMail "%MAILdST%" "%OKMailSub%"
  ) ELSE (
    CALL :sendMail "%FailMailDst%" "%FailMailSub%"
  )
)
EXIT /B

:sendMail
>"%temp%\rdcto.txt" ECHO %~1
"%~dp0sendmail.exe" /TO="%temp%\rdcto.txt" /FROM=%MailSrcName% ^<%MailSrc%^> /REF=%~2 /MESSAGE=%log% /HOST=mail.mydomian.com
EXIT /B 0

Other possible changes:

I didn't make any of the changes below because you said your code was working, and I don't know those programs enough to guarantee the changes are an improvement.

A) I don't see why you run WinSCP via START. I believe you could simply use:

WinSCP.exe /console /script=WSCP-GetC.txt /log=%log%

B) Your sendmail arguments look very suspicious to me. Normally values that contain spaces should be enclosed in quotes. Note that the :sendMail routine above preserves the lack of quotes by using %~1 and %~2 instead of %1 and %2.

C) I don't know WinSCP, but I suspect it will overwrite any existing log file, in which case you could remove the prior DEL command.


String formatting of unit types I have a units of measure library that I added string formatting to. Here is sample code:

[Test]
public void FormatSpeed()
{
    var speed = Speed.FromMetresPerSecond(1.2);
    using (Thread.CurrentThread.UsingTempCulture(CultureInfo.InvariantCulture))
    {
        Assert.AreEqual("1.20\u00A0m/s", speed.ToString("F2"));
        Assert.AreEqual("1.20 m⋅s⁻¹", speed.ToString("F2 m⋅s⁻¹"));
        Assert.AreEqual("1.2\u00A0m/s", speed.ToString());
        Assert.AreEqual("1.2\u00A0m⋅s⁻¹", speed.ToString("m⋅s⁻¹"));
        Assert.AreEqual("1200\u00A0mm⋅s⁻¹", speed.ToString("mm⋅s⁻¹"));
        Assert.AreEqual("4.32\u00A0km/h", speed.ToString(SpeedUnit.KilometresPerHour));
        Assert.AreEqual("4.3\u00A0km/h", speed.ToString("F1", SpeedUnit.KilometresPerHour));
    }

    var sv = CultureInfo.GetCultureInfo("sv-SE");
    Assert.AreEqual("1,20\u00A0m/s", speed.ToString("F2", sv));
    Assert.AreEqual("1,20 m⋅s⁻¹", speed.ToString("F2 m⋅s⁻¹", sv));
    Assert.AreEqual("1,2\u00A0m/s", speed.ToString(sv));
    Assert.AreEqual("1,2\u00A0m⋅s⁻¹", speed.ToString("m⋅s⁻¹", sv));
    Assert.AreEqual("1200\u00A0mm⋅s⁻¹", speed.ToString("mm⋅s⁻¹", sv));
    Assert.AreEqual("4,32\u00A0km/h", speed.ToString(SpeedUnit.KilometresPerHour, sv));
    Assert.AreEqual("4,3\u00A0km/h", speed.ToString("F1", SpeedUnit.KilometresPerHour, sv));
}

[Test]
public void FormatAngle()
{
    var angle = Angle.FromDegrees(1.2);
    using (Thread.CurrentThread.UsingTempCulture(CultureInfo.InvariantCulture))
    {
        Assert.AreEqual("0.020943951023932\u00A0rad", angle.ToString());
        Assert.AreEqual("1.200°", angle.ToString("F3°"));
        Assert.AreEqual("1.2°", angle.ToString(AngleUnit.Degrees));
        Assert.AreEqual(" 1.2 ° ", angle.ToString(" F1 ° "));
        Assert.AreEqual(" 0.02 rad", angle.ToString(" F2 "));
        Assert.AreEqual("1.200°", angle.ToString("F3", AngleUnit.Degrees));
        Assert.AreEqual("0.02\u00A0rad", angle.ToString("F2", AngleUnit.Radians));
    }

    var sv = CultureInfo.GetCultureInfo("sv-SE");
    Assert.AreEqual("0,020943951023932\u00A0rad", angle.ToString(sv));
    Assert.AreEqual("1,200°", angle.ToString("F3°", sv));
    Assert.AreEqual("1,2°", angle.ToString(AngleUnit.Degrees, sv));
    Assert.AreEqual(" 1,2 ° ", angle.ToString(" F1 ° ", sv));
    Assert.AreEqual(" 0,02 rad", angle.ToString(" F2 ", sv));
    Assert.AreEqual("1,200°", angle.ToString("F3", AngleUnit.Degrees, sv));
    Assert.AreEqual("0,02\u00A0rad", angle.ToString("F2", AngleUnit.Radians, sv));
}

1) What overloads make sense and what are missing if any?

2) I'm really not sure about if adding composite formats like F2 m⋅s⁻¹ where F2 is for the scalar value and m⋅s⁻¹is for the unit is a good idea.

3) Is nobreaking space \u00A0 the best choice?

The only suggestion I would make is to extract each culture's tests to it's own unit tests. Each unit test should be testing one aspect of your code, whereas yours are each testing at least two.

Point 1: the only two overloads you need are .ToString(string) and .ToString(string, IFormatProvider). Everything else is just noise.

Point 2: you should not be adding composites like that, at all. It's just noise. If you really need that info to be passable to the .ToString call, then you should consider a third and fourth overload: .ToString(string format, string unit), and .ToString(string format, string unit, IFormatProvider formatProvider).

Point 3: I don't know enough about C# to be able to answer that exactly, but I would assume you should be using the literal space everywhere, or the \u00A0 everywhere. Pick one or the other, but don't mix-and-match.

Your unit tests have good content, but they need better names:

public void FormatSpeed_1p2_Invariant()
public void FormatSpeed_1p2_Sv_Se()
public void FormatAngle_1p2_Invariant()
public void FormatAngle_1p2_Sv_Se()

This makes it a little more clear as to what the tests are doing by name, so that when you look at the test explorer you can see you already have tests that cover certain aspects.


Simple spinlock for C using ASM, revision #1 Revision #1 for Simple spinlock for C using ASM

The code:

static inline void atomic_open(volatile int *gate)
{
    asm volatile (
        "jmp check\n"                // Renegade selected, lets skip the line!
        "wait:\n"                    // Honest citizens wait in line.
        "pause\n"                    // Stroke beard, check phone/watch.
        "check:\n"                   // Ok, lets do this...
        "mov %[lock], %%eax\n"       // eax = 1
        "lock xchg %%eax, %[gate]\n" // Exhange eax with gate value.
        "test %%eax, %%eax\n"        // 1 = closed, 0 = open.
        "jnz wait\n"                 // Ohhh man, here I go again...
        : [gate] "=m" (*gate)
        : [lock] "r" (1)
        : "eax"                      // Tell compiler you want to use eax register.
    );

}

static inline void atomic_close(volatile int *gate)
{
    asm volatile (
        "mov %[unlock], %[gate]\n"
        : [gate] "=m" (*gate)
        : [unlock] "r" (0)
    );
}

// Usage, example.
volatile int atomic_gate_memory = 0;
void *mymalloc(size_t size)
{
    atomic_open(&atomic_gate_memory);
    void *ptr = malloc(size);
    atomic_close(&atomic_gate_memory);
    return ptr;
}

The question is the same as before: Will atomic_[open/close] make mymalloc both threadsafe and reentrant?

If no, what is wrong?
If yes, it is still wrong; isn't it?... Give me a good rant about what to consider, what is missing or about better approach. If you want to suggest libraries, please restrict your self to C. I am not experienced enough to bind C++ stuff to other languages, so I often can't use the good stuff over there :'(

As you have no doubt noticed, given your follow up post, whilst your current lock is thread safe, it isn't reentrant.

If you perform a double call to atomic_open from the same thread, the second call will fail to enter the gate (because it's already locked) and become deadlocked against itself.

lock xchg %%eax, %[gate]\n" // Exhange eax with gate value.
"test %%eax, %%eax\n"        // 1 = closed, 0 = open.

This is because there's no tracking of which thread opened the gate.

Not checking the thread has the potential to cause another issue (which you might not care about, because it's pretty common with spinlocks) whereby if you have a bug in your code, it is possible for a thread that doesn't own the lock to unlock it (anybody can call atomic_close and it will unlock, even if they didn't call atomic_open first).


My implementation of bubble sort using Python I just started studying algorithms and since I'm most familiar with Python decided to use it to actually put the algorithms I've been learning to use. How is my implementation of Bubble Sort? Does it make sense, is it efficient, too many variables, can you understand it without comments, any other tips for me?

"""
Bubble sort algorithm.
"""
import random
ls = []
max_length = 10
while len(ls) < max_length:
    ls.append(random.randint(1,101))
print ls
def bubble_sort(items_to_sort):
    right_item = -1
    left_item = -2
    item_len = len(items_to_sort)
    number_of_passes = 0
    temp_item = 0
    max_checks = len(items_to_sort)
    if item_len > 1:
        while (number_of_passes < item_len) and (max_checks > 0):
            if items_to_sort[right_item] < items_to_sort[left_item]:
                temp_item = items_to_sort[right_item]
                items_to_sort[right_item] = items_to_sort[left_item]
                items_to_sort[left_item] = temp_item
            right_item += -1
            left_item += -1
            number_of_passes += 1
            if left_item < -(item_len):
                right_item = -1
                left_item = -2
                number_of_passes = 0
                max_checks -= 1
    return items_to_sort
print bubble_sort(ls)

Your bubble sort algorithm has way too many variables. You don't need to keep track of the right and left item. All you need to do is compare two values, and swap if one is bigger than the other.
def bubble_sort(nums):
    length = len(nums)
    for i in range(length - 1):
        for j in range(0, length - i - 1):
            if nums[j] > nums[j + 1]:
                nums[j], nums[j + 1] = nums[j + 1], nums[j]

After benchmarking, this code is substantially faster than your original code.
Benchmarks:
Array size bubble_sort_op       3000    1.87206s
Array size bubble_sort          3000    0.67218s
Array size bubble_sort_op       3200    2.24581s
Array size bubble_sort          3200    0.78276s
Array size bubble_sort_op       3400    2.41464s
Array size bubble_sort          3400    0.87018s
Array size bubble_sort_op       3600    2.69006s
Array size bubble_sort          3600    0.96319s
Array size bubble_sort_op       3800    2.99190s
Array size bubble_sort          3800    1.09713s
Array size bubble_sort_op       4000    3.48652s
Array size bubble_sort          4000    1.24830s


Depth-first traversal implementation in Java (by recursion) I implemented a recursive depth-first traversal, with all three traversal modes in this function. Is this the most succinct way to write the code?

/**
 * Node
 */
public class Node {
  private int value;
  Node left;
  Node right;

  public Node(int value) {
    this.setValue(value);
    left = null;
    right = null;
  }

  /**
   * @return the value
   */
  public int getValue() {
    return value;
  }

  /**
   * @param value the value to set
   */
  public void setValue(int value) {
    this.value = value;
  }

/**
 * Depth-First Traversal (Recursion)
 * @param root root of node-tree to be traversed
 */
public void dftRecursion(Node root) {
    if (root == null) {
      return;
    }
    switch (tMode) { // traversal mode
    case IN_ORDER:
      dftRecursion(root.left);
      System.out.print(root.getValue() + " ");
      dftRecursion(root.right);
      break;
    case PRE_ORDER:
      System.out.print(root.getValue() + " ");
      dftRecursion(root.left);
      dftRecursion(root.right);
      break;
    case POST_ORDER:
      dftRecursion(root.left);
      dftRecursion(root.right);
      System.out.print(root.getValue() + " ");
      break;
    default:
      throw new IllegalArgumentException("Mode value:" + tMode);
    }
  }

To answer your question exactly and with very little value: no. All those println's and traversal modes add unnecessary length to your code.

But lets get down to business... shortness is not always he most desirable feature of source code. Your choice of providing accessors for value but not for the left and right subtrees is questionable. Be consistent with how you expose and manipulate your fields.

You left out the context of dtfRecursion. Defining the traversal mode outside the traversal algorithm is odd. You could implment the traversal modes as separate classes. This would free you from the need to check the correctness of the tMode. Also you wouldn't need to worry about someone changing tMode during traversal (not that you do it now, eh).

It seems that you're just doing this for the sake of writing the algorithm but to make this review more useful, I'll point out that instead of hardcoding the println's you could pass a Consumer<Integer> to the traversal method and implement whatever you want to do with the values in that. And you could implement Node as a parameterized class to allow storage of types other than just integers.

Node is also a bit generic name for a class that is highly specific in it's implementation. BinaryTreeNode might be better.