Switching on hardware ports I have the following function for turning on some ports on a piece of hardware:

public Boolean SwitchOn(Int32 portNumber)
    {
        HttpCommandBuilder builder = new HttpCommandBuilder(_ipAddress);
        builder.Command = HttpCommandBuilder.CommandType.SetPower;

        String result;

        if (portNumber == 1)
        {
            builder.Port1 = true;
            result = SendCommand(builder.GenerateUrl());

            if (String.IsNullOrEmpty(result))
            {
                return false;
            }
            else if (result.IndexOf("p61=1") == -1)
            {
                return false;
            }
            else
            {
                return true;
            }
        }
        else if (portNumber == 2)
        {
            builder.Port2 = true;
            result = SendCommand(builder.GenerateUrl());

            if (String.IsNullOrEmpty(result))
            {
                return false;
            }
            else if (result.IndexOf("p62=1") == -1)
            {
                return false;
            }
            else
            {
                return true;
            }
        }
        else if (portNumber == 3)
        {
            builder.Port3 = true;
            result = SendCommand(builder.GenerateUrl());

            if (String.IsNullOrEmpty(result))
            {
                return false;
            }
            else if (result.IndexOf("p63=1") == -1)
            {
                return false;
            }
            else
            {
                return true;
            }
        }
        else if (portNumber == 4)
        {
            builder.Port4 = true;
            result = SendCommand(builder.GenerateUrl());

            if (String.IsNullOrEmpty(result))
            {
                return false;
            }
            else if (result.IndexOf("p64=1") == -1)
            {
                return false;
            }
            else
            {
                return true;
            }
        }
        else
        {
            throw new ArgumentException("Invalid port number: " + portNumber);
        }
    }

It has a low maintainability index and there's a lot of copy and pasting going on and I am doing a similar, repetitive action for each port.

I know it's not a very elegant solution so I am looking for any advice and suggestions on how I could re-factor this function to improve its maintainability score.

There are already two good answers here, but in addition:

Consider using the C# type keywords (bool intead of Boolean, int instead of Int32, string instead of String)
Consider using Contains instead of IndexOf
Consider introducing a method to set the port. If you can't modify HttpCommandBuilder, an extension method is one option.
Consider using object initializers

Here's an example with these suggestions put together:

public bool SwitchOn2(int portNumber)
{
    var builder = new HttpCommandBuilder(_ipAddress)
    {
        Command = HttpCommandBuilder.CommandType.SetPower
    };

    builder.SetPort(portNumber);
    var result = SendCommand(builder.GenerateUrl());
    return !string.IsNullOrEmpty(result) && result.Contains($"p6{portNumber}=1");
}

internal static class HttpCommandBuilderExtensions
{
    public static void SetPort(this HttpCommandBuilder builder, int portNumber)
    {
        if (builder == null)
        {
            throw new ArgumentNullException(nameof(builder));
        }

        switch (portNumber)
        {
            case 1:
                builder.Port1 = true;
                break;
            case 2:
                builder.Port2 = true;
                break;
            case 3:
                builder.Port3 = true;
                break;
            case 4:
                builder.Port4 = true;
                break;
            default:
                throw new ArgumentOutOfRangeException(nameof(portNumber));
        }
    }
}

For what it's worth, here are the code metrics from VS2015:

Member: SwitchOn2(int) : bool
Maintainability Index: 70
Cyclomatic Complexity: 2
Class Coupling: 2
Lines of Code: 5

Member: SetPort(this HttpCommandBuilder, int) : void
Maintainability Index: 62
Cyclomatic Complexity: 6
Class Coupling: 4
Lines of Code: 12

And for the original:

Member: SwitchOn(int) : bool
Maintainability Index: 45
Cyclomatic Complexity: 13
Class Coupling: 2
Lines of Code: 36


User registration function too slow I just wrote a PHP code to register users.

It's called through AJAX and it looks like it's too slow because it often sent me a .status = 0 that it means it took too much time to proceed.

If i replace my php function that register only by :

echo "Ok";

It always sent me a .status = 200

Could someone helps me to improve this file to make it faster ? I'm not used to code in PHP.

<?php

require_once('../config/database.php');
require_once('../config/functions.php');
$keywords = preg_split("/MyWebSite\//", getcwd());
$folder = explode('/', $keywords[1]);

if (!isset($_POST['login']) || !isset($_POST['email']) || !isset($_POST['password']) || !isset($_POST['password_check']))
{
    echo "missing some fields, did you try to edit my html?";
    die();
}
if (strlen($_POST['login']) < 5 || strlen($_POST['email']) == 0 || strlen($_POST['password']) < 5 || strlen($_POST['password_check']) < 5)
{
    echo "fieds badly filled";
    die();
}
if ($_POST['password'] != $_POST['password_check'])
{
    echo "passwords field aren't same";
    die();
}

try
{
    $db = new PDO($DB_DSN, $DB_USER, $DB_PASSWORD);
    $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); // to get an exception when caught an error :)
} catch (PDOException $e) {
    print "Erreur !: " . $e->getMessage() . "<br/>";
    die();
}

$count_login = $db->prepare("SELECT COUNT(*) FROM camagru_jgengo.user WHERE login = :login");
$count_login->bindValue(':login', $_POST['login']);
$count_login->execute();
if ($count_login->fetchColumn() > 0)
{
    echo "This username already taken";
    die();
}

$count_email = $db->prepare("SELECT COUNT(*) FROM camagru_jgengo.user WHERE email = :email");
$count_email->bindValue(':email', $_POST['email']);
$count_email->execute();
if ($count_email->fetchColumn() > 0)
{
    echo "This email already taken";
    die();
}

$insert = $db->prepare('INSERT INTO camagru_jgengo.user (email, login, password, admin, validate_link, created_at) VALUES (:email, :login, :password, 0, :validate_link, :created_at)');
$arr = array(
    ":login" => $_POST['login'],
    ":password" => hash_it($_POST['password']),
    ":email" => $_POST['email'],
    ":validate_link" => hash('md5', time()),
    ":created_at" => date('Y-m-d')
    );
$insert->execute($arr);

echo "Created!\n";

mail ($_POST['email'], "[Camagru] Active your account", "To active your account click that link : http://localhost:8080/".$folder[0]."/?p=activate&hash=".$arr[':validate_link']);

?>

isset can take multiple inputs, and fails if any of them is not set. So, the first if clause could become:

if (!isset($_POST['login'], $_POST['email'], $_POST['password'], $_POST['password_check']))

I personally prefer using !== so that type juggling will not happen.

if ($_POST['password'] !== $_POST['password_check'])

Instead of using SELECT COUNT(*) query to fetch existing username/email in your DB, you can instead set those columns to be UNIQUE and just go to the INSERT statement. The INSERT will fail with error code 1062.
Instead of using camagru_jgengo.user everywhere, you can set the dbname in the DSN string.
While not necessary, you can switch to using crypt() instead of md5 hashing. An MD5 has a higher collision chance, and since you are using it for sending emails with verification links. Using some form of salt will protect you from sending the same verification links to multiple users.


Finding and returning a loopy path in a directed graph I wrote a function in Scala to find out and return a loopy path in a directed graph.

One of the arguments is a graph presented in an adjacent list, and the other is a start node. It returns a pair including a loopy path by a list of nodes.

I wonder if there are more elegant ways of doing this.

  def GetACycle(start: String, maps: Map[String, List[String]]): (Boolean, List[String]) = {
    def explore(node: String, visits: List[String]): (Boolean, List[String]) = {
      if (visits.contains(node)) (true, (visits.+:(node)).reverse)
      else {
        if (maps(node).isEmpty) (false, List())
        else {
          val id = maps(node).indexWhere(x => explore(x, visits.+:(node))._1)
          if (id.!=(-1))
            explore(maps(node)(id), visits.+:(node))
          else
            (false, List())
        }
      }
    }
    explore(start, List())
  }

I felt I had to use the indexWhere in this situation, but I suppose it would have other ways to do that.

You should use an array to check if you have already visited a node and not visits.contains(node), it would give you the answer in constant time instead of linear time.

The overall complexity of your algorithm is exponential. For instance, if you run your algorithm on this graph:

0 -> 1, 2, ..., n
1 -> 2, ..., n
...

where there are n nodes and there are edges from i to j iff i<j then the node i will be explored 2^i times.

Again you can solve this problem using an array (one array for all nodes) to ensure that each node is explored at most one time.


Restful CRUD API with Node.js and Express I build a RESTful CRUD API with Node.js and Express. This application allows, for example, to create a table on the client side with the ability to add, update and remove data by ID.
I generate an array of random data for the availability of data when you start working on the client side with the table.
I will be grateful for tips on improving the style.

const express = require('express')
const bodyParser = require('body-parser')
const crypto = require("crypto");
const cors = require('cors')

const getId = () => crypto.randomBytes(16).toString("hex");
const getRandomInt = (min, max) => Math.floor(Math.random() * (max - min)) + min;
const nameList = ['Andrew', 'John', 'Sebastian', 'Owen', 'Luke', 'Anthony', 'Dylan']
const cityList = ['Riga', 'Berlin', 'Paris', 'Tokyo', 'London', 'Milan', 'Madrid']
const colorList = ['red', 'blue', 'green']
const getRandomStr = (str) => (str[getRandomInt(0, str.length)])

const app = express()

const names = [
  { id: getId(),
    name: 'Li',
    city: 'Tokyo',
    color: 'blue',
    answers: '10',
    reputation: '1725' }
]

const generateNames = () => {
  for (let i=0; i<50; i++){
  const newId = getId()
  const newName = getRandomStr(nameList)
  const newCity = getRandomStr(cityList)
  const newColor = getRandomStr(colorList)
  const newAnswers = getRandomInt(10, 20).toString()
  const newReputation = getRandomInt(1500, 2000).toString()
  names.push({ id: newId, name: newName, city: newCity,
  color: newColor, answers: newAnswers, reputation: newReputation })
  }
}
generateNames()

app.use(bodyParser.json())

app.use(cors())

app.get('/names', (req, res) => {
  res.json(names)
})

app.post('/names', (req, res) => {
  const newId = getId()
  const newName = req.body.name
  const newCity = req.body.city
  const newColor = req.body.color
  const newAnswers = req.body.answers
  const newReputation = req.body.reputation
  names.push({ id: newId, name: newName, city: newCity,
  color: newColor, answers: newAnswers, reputation: newReputation })
  res
    .status(201)
    .json({ id: newId })
})

app.put('/names', (req, res) => {
  const { id } = req.body
  const indexToEdit = names.findIndex(obj => obj.id === id )
  names[indexToEdit].name = req.body.name
  names[indexToEdit].city = req.body.city
  names[indexToEdit].color = req.body.color
  names[indexToEdit].answers = req.body.answers
  names[indexToEdit].reputation = req.body.reputation
  const editName = names[indexToEdit].name
  const editCity = names[indexToEdit].city
  const editColor = names[indexToEdit].color
  const editAnswers = names[indexToEdit].answers
  const editReputation = names[indexToEdit].reputation
  res
    .status(200)
    .json({ name: editName, city: editCity,
      color: editColor, answers: editAnswers, reputation: editReputation})
})

app.delete('/names', (req, res) => {
  const { id } = req.body
  const indexToRemove = names.findIndex(obj => obj.id === id )
  names.splice(indexToRemove, 1)
  res
    .status(200)
    .json({ status: 'success'})
})

app.listen(3000, () => {
  console.log('listening on 3000')
})

I know you are generating data for the sake of completion but it does complicate things on what aspect of the code you'd like to focus on. But here are a couple thoughts.

With Express, I often like to treat the routes as controllers in an MVC pattern. So any fetching of data and mutating data, I would create at least a model per resource to encapsulate that part of the code. So the interface may look something like the below:

app.get('/names', async (req, res) => {
  // assuming we are getting data from an async call to a db
  const names = await Name.all();
  res.json(names)
})

app.post('/names', async (req, res) => {
  const newName = await Name.create(req.body);

  res
    .status(201)
    .json({ id: newName.id })
})

app.put('/names', async (req, res) => {
  const { id } = req.body
  const name = await Name.findAndUpdate(id, req.body);
  res
    .status(200)
    .json(name)
})

app.delete('/names', async (req, res) => {
  const { id } = req.body
  const deleted = await Name.deleteById(id)
  res
    .status(200)
    .json({ status: 'success'})
})

Notice that we can implement "Name" class with whatever database(or file or no database) we want. As long as we keep the interface the same, we can swap different "Name" class implementations without having to change our Express routes.

One last point, since there is no "view" suggested here but we can have it if we want. For example, if for some reason, our model doesn't return us exactly what we need, maybe the key is slightly off from our API specifications, we can introduce a view layer that maps whatever the model returns us. This way we can continue to keep our Express routes very thin and handling specific to request/response related responsibilities.


Find all root to leaf paths in binary tree Description:

Given a binary tree, return all root-to-leaf paths.

Leetcode

/**
 * Definition for a binary tree node.
 * public class TreeNode {
 *     int val;
 *     TreeNode left;
 *     TreeNode right;
 *     TreeNode(int x) { val = x; }
 * }
 */

Code:

class Solution {

    public List<String> binaryTreePaths(TreeNode root) {
        List<String> paths = new ArrayList<>();
        traverse(root, new ArrayList<>(), paths);
        return paths;
    }

    private void traverse(TreeNode root, List<String> path, List<String> paths) {
        if (root == null) return;

        path.add(""+root.val);
        if (root.left == null && root.right == null) {
            paths.add(String.join("->", path));
        }

        traverse(root.left,  path, paths);
        traverse(root.right, path, paths);

        path.remove(path.size() - 1);
    }
}

regarding the translation from int to String

path.add(""+root.val);

This is both unclear and also involves unnecessary String creation.  Why not use the "official" conversion method? it clearly states the intention and is more efficient

path.add(String.valueOf(root.val));


Deleting the head node of a circular linked list Today I tackled this coding challenge:

  Given a circular linked list, implement a method to delete its head node. Return the list's new head node.

I would appreciate any feedback.

public ListNode deleteAtHead(ListNode head) {

    if(head == null){
        return head;
    }
    ListNode temp = head;

    while(temp.next != head){
        temp = temp.next;
    }

    temp.next = head.next;
    head.next = null;
    head = temp.next;

   return head;
}

Since you are dealing with a circularly linked list (meaning the tail's next points to head and the head's prev points to the tail) and assuming each node has a prev and next, you might consider this easier approach which does not require traversal of the entire list.

public ListNode deleteAtHead(ListNode head) {
    if (head == null) {
        return head;
    }

    ListNode newHead = head.next;
    newHead.prev = head.prev;
    head.prev.next = newHead;

    head.next = null;
    head.prev = null;

    return newHead;
}

This should also work for a list with only 1 node (e.g. a head) assuming it is initialized with prev and next referencing itself.


Efficiently calculate value of Pascal's Triangle using memoization and recursion So reading about functional programming, applications to dynamic programming, and learning Scala. I figured I would try it out with a popular example, Pascal's Triangle, tests against known values work out fine. Any tips for optimization? Am I doing anything totally wrong? Is there a way to do this with tail recursion that I'm missing? Any feedback appreciated.
def pascal(c: Int, r: Int): Int = {
    // use a cache; map if (c,r) pair to value
    // fill in value recursively
    var cache: Map[(Int,Int), Int] = Map()
    cache += ((0,0) -> 1)
    var i = 0
    for(i <- 1 to r){
       cache += ((0,i) -> 1)
       cache += ((i,i) -> 1)
    }

    def getPascalValue(c: Int, r: Int): Int = {
        if (cache.contains((c, r))) {
            cache((c, r))
        } else {
            cache += ((c, r) -> (getPascalValue(c, r - 1) + getPascalValue(c - 1, r - 1)))
            cache((c, r))
        }
    }
    getPascalValue(c,r)
}

Your code is clearly organized and well presented, but it suffers from a number of problems, from the trivial to the profound.

var i = 0 is never used and doesn't need to be there.
safety: There's no check for bad input. pascal(2,1) will happily java.lang.StackOverflowError.
Your code indicates a lack of familiarity with the Scala Standard Library. Initializing the cache, for example, can be accomplished with a single statement.

var cache: Map[(Int,Int), Int] =
  (0 to r).flatMap(n => Seq((0,n)->1, (n,n)->1)).toMap

Likewise, instead of an immutable Map in a var variable for the cache, if you use a mutable Map in a val variable then getPascalValue() can be reduced to a single statement.

def getPascalValue(c: Int, r: Int): Int =
  cache.getOrElseUpdate((c,r), getPascalValue(c, r-1) +
                               getPascalValue(c-1, r-1))

But the real problem here is that you're putting in a lot of overhead for no reward.

small problem: The cache is initialized with some values that may never be referenced.
big problem: The cache is updated with values that will never be referenced.

That's right. Nothing added to the cache, after initialization, serves any purpose. It's just inefficient busywork.
And the reason your cache isn't working: each iteration (recursion) starts with the original, initialized, cache. That version is updated, +=, with the newly computed pascal number, but that update is lost when the stack frame pops and execution returns to the caller.


File-based Equation Evaluation in Go My program 'Monkey', written in Go, can take a file like this:

+ 9 13
/ 10 2

And prints the output (in the case for the 'file' above, '22' and '5'.)
Variables can also be declared to an expression. In this 'file':

* 5 4 : a
+ a 2 : a

The variable a ends up as 22, because at first we assign it to 5 * 4 (20), and then add 2 (22).

Here's the source for the program, and here it is with syntax highlighting.

package main

import (
    "bufio"
    "fmt"
    "os"
    "strings"
    "strconv"
)

// error management
func check(e error) {
    if e != nil {
        panic(e)
    }
}

func openfile(filename string) (lines []string) {
    // safely open file
    source, err := os.Open(filename)
    check(err)

    scanner := bufio.NewScanner(source)
    // equivalent to a while loop
    for scanner.Scan() {
        lines = append(lines, scanner.Text())
    }

    return
}

var variables map[string]float32

// determine whether a token is a variable reference or
// an integer literal, and return it.
func getvalue(item string) float32 {
    // if item is number
    if num, err := strconv.Atoi(item); err == nil {
        return float32(num)
    }
    return variables[item]
}

func operate(operands []string, op string) float32 {
    var result float32 = 0
    // o = index in operands[]
    for o := 0; o < len(operands); o++ {
        if op == "+" {
            if o == 0 {
                result = getvalue(operands[0])
            } else {
                result += getvalue(operands[o])
            }
        } else if op == "-" {
            if o == 0 {
                result = getvalue(operands[0])
            } else {
                result -= getvalue(operands[o])
            }
        } else if op == "*" {
            if o == 0 {
                result = getvalue(operands[0])
            } else {
                result *= getvalue(operands[o])
            }
        } else if op == "/" {
            if o == 0 {
                result = getvalue(operands[0])
            } else {
                result /= getvalue(operands[o])
            }
        } else {
            return 0.0
        }
    }
    return result
}

var x, y int64 // these will be assigned to function params
var xerr, yerr error
func main() {
    variables = make(map[string]float32)
    lines := openfile(os.Args[1])
    for lineNumber, line := range lines {
        words := strings.Split(line, " ")
        operands := 0 // count of numbers that the operation will be applied to
        for _, word := range words {
            if _, err := strconv.Atoi(word); err == nil {
                operands += 1
            }
        }
        // line needs to end in ';' or ': varName' to be correct
        if len(words) >= operands + 2 {
            if words[operands + 1] == ";" {
                fmt.Println(operate(words[1:operands+1], words[0]))
            } else if words[operands + 1] == ":" {
                fmt.Println("var", words[operands + 2], "=", operate(words[1:operands+1], words[0]))
                variables[words[operands + 2]] = operate(words[1:operands+1], words[0])
            }
        } else if len(words) < 0 && len(words) < operands + 2 {
            fmt.Println("error: not enough arguments at line", lineNumber)
        }
    }
}

As a general naming convention, go methods are either named LikeThis (for public methods) or likeThis (for package private methods). For a small, single file project like this it doesn't really matter too much, but it is a good habit to get into regardless.

Your openfile method should probably be named readFile, as it doesn't just open the file, it also reads every line in it. Further, you're forgetting to close the file when you are done with it. Go has a fairly clean way to ensure this will always happen: the defer keyword:

func readFile(filename string) (lines []string) {
    source, err := os.Open(filename)
    defer source.Close()  // Make sure we close the file

    ...
}

Your operate function can be written more cleanly; firstly, by recognising that the case if o == 0 is exactly the same in each branch, secondly by using a switch statement (which is more idiomatic in go):

func operate(operands []string, op string) float32 {
    result := getvalue(operands[0])
    for o := range operands[1:] {
        switch op {
        case "+": result += getvalue(o)
        case "-": result -= getvalue(o)
        case "*": result *= getvalue(o)
        case "/": result /= getvalue(o)
        default: return 0.0
        }
    }
    return result
}

Here, we get initialise result to the first value in operands. Then, we range over the rest of the elements in operands. This uses range, which should be preferred to manual for loops when you can use them, and also slicing off the first element of the slice.

Note that go does not need break statements in a switch, rather, it requires an explicit fallthrough (which stops probably the largest class of bugs when using switch statements).


k_diff challenge in Java Puzzle Description

  Given \$N\$ numbers, \$N <= 10^5\$, count the total pairs of numbers \$(N_i, N_j)\$ that have a difference of \$K = N_i - N_j\$ where \$0 < K < 10^9\$.

  Input Format:

  1st line contains \$N\$ and \$K\$ (integers).
  2nd line contains \$N\$ numbers of the set. All the \$N\$ numbers are assured to be distinct.

  Output Format:

  One integer saying the number of pairs of numbers that have a difference \$K\$.

  Time limit is 5 seconds.

Sample Input : 5 2
               1 5 3 4 2
Sample Output: 3

First I've tried using ArrayLists and later tried using arrays.  The code is bad and does not at all follow normal conventions of Java.  But I'm not looking at that because execution time doesn't depend on it, right?

But to achieve the puzzle description I couldn't see any better logic than this:

import java.util.Scanner;

public class k_diff {

    public int process() {
        Scanner scanner = new Scanner(System.in);
        int total_nums = scanner.nextInt();
        int diff = scanner.nextInt();
        int ary_nums[] = new int[total_nums];
        for (int i = 0; i < total_nums; i++) {
            ary_nums[i] = scanner.nextInt();
        }
        int len = ary_nums.length;
        int count = 0;
        for (int j = 0; j < len - 1; j++) {
            for (int k = j + 1; k < len; k++) {
                if (Math.abs(ary_nums[j] - ary_nums[k]) == diff) {
                    count++;
                }
            }
        }
        return count;
    }

    public static void main(String args[]) {
        k_diff kdiff = new k_diff();
        System.out.println(kdiff.process());
    }
}

Some things which just jump into my eyes (even if not asked, I'll tell you anyway):

Whitespaces: Use whitespaces were appropriate, f.e. for(int i=0;i<nums;i++) is harder to rad then for(int i = 0; i < nums; i++).
Meaningful names: Name your variables after what they are, not what type they are. This especially includes simple for loops. It might be taught or standard to use i, but I consider it bad practice because you never know what exactly i is at first glance. Give your variables meaningful names, please, they deserve love too! Same goes for your classes.
Split up where appropriate: if you've got only one function (main) you're most likely doing something wrong. Split it into Input, Processing and Output.


Returning an IEnumerable using Linq-to-SQL which is then bound to a GridView This function works, but it's really really slow and the generated SQL is gargantuan and horrible to look at.  It's also very expensive to run when it shouldn't be.

public static IEnumerable<TrackingComputer> TrackingBoardPCsgroup(string groupName)
{
    var pingresult = from p in db.GetTable<tblTBHealthPing>()
                     group p by p.ComputerAsset into g
                     select g.OrderByDescending(t => t.HealthPingTime).FirstOrDefault();

    var healthresult = from p in db.GetTable<tblTBHealthHeartbeat>()
                       group p by p.ComputerAsset into g
                       select g.OrderByDescending(t => t.HealthHeartbeatTime).FirstOrDefault();

    var query = (from t in TrackingBoardPCs()
                 where t.TrackingGroup == groupName
                 select new TrackingComputer
                 {
                     ComputerName = t.ComputerAsset,
                     IPAddress = t.ComputerIP,
                     Location = t.Location,
                     Pingable = (from p in pingresult where p.ComputerAsset == t.ComputerAsset select p.HealthPingResult).FirstOrDefault() ?? false,
                     PingTime = (from p in pingresult where p.ComputerAsset == t.ComputerAsset select p.HealthPingTime).FirstOrDefault() ?? DateTime.Now.AddDays(-100),
                     Username = (from p in healthresult where p.ComputerAsset == t.ComputerAsset select p.HealthCurrentUser).FirstOrDefault(),
                     CurrentWindow = (from p in healthresult where p.ComputerAsset == t.ComputerAsset select p.HealthCurrentWindow).FirstOrDefault(),
                     Uptime = (from p in healthresult where p.ComputerAsset == t.ComputerAsset select p.HealthUptime).FirstOrDefault()
                 }).OrderBy(t => t.Pingable);
    return query;
}

TrackingBoardPCs function:

public static IQueryable<tblTrackingBoardPC> TrackingBoardPCs()
    {

            var query = (from tbpcs in db.GetTable<tblTrackingBoardPC>()
                         select tbpcs);

        return query;

    }

First, the fact that your methods are static, and accessing what appears to be a static, disposable field, scares me: your application quite possibly has structural problems much more important than a slow query - but you haven't included nearly enough code for us to review that aspect.

All I can say, is that db instances should be as short-lived as possible, and properly disposed once you're done with them; you might want to look into patterns like Repository and Unit-of-Work, to wrap up your LINQ-to-SQL dependencies.

As you may or may not know, LINQ-to-SQL queries don't hit the database immediately; deferred execution makes the provider translate your expressions into proper SQL when the results are iterated... which means... your UI is actually what's triggering query execution.

Let's see...

var pingresult = from p in db.GetTable<tblTBHealthPing>()
                 group p by p.ComputerAsset into g
                 select g.OrderByDescending(t => t.HealthPingTime).FirstOrDefault();

I had to read twice and think more than I should have done so, to parse exactly what's going on here and make sure I was reading it right - the FirstOrDefault applies to each grouping, right?

Sometimes method notation is easier to read:

var pingResult = db.GetTable<tblTBHealthPing>()
                   .GroupBy(p => p.ComputerAsset)
                   .Select(g => g.OrderByDescending(t => t.HealthPingTime)
                                 .FirstOrDefault());

If I read this correctly (did I?), pingResult is an IGrouping<tblTBHealthPing> where each group contains the greatest HealthPingTime, and each group represents a ComputerAsset.

I would materialize this query immediately, into a dictionary:

var pingResults = db.GetTable<tblTBHealthPing>()
                    .GroupBy(ping => ping.ComputerAsset)
                    .AsEnumerable()
                    .ToDictionary(
                        grouping => grouping.Key,
                        grouping => grouping.OrderByDescending(t => t.HealthPingTime)
                                            .FirstOrDefault());

I'd do the same for the other one:

var healthResults = db.GetTable<tblTBHealthHeartbeat>()
                      .GroupBy(beat => beat.ComputerAsset)
                      .AsEnumerable()
                      .ToDictionary(
                          grouping => grouping.Key,
                          grouping => grouping.OrderByDescending(t => t.HealthHeartbeatTime)
                                              .FirstOrDefault());

Now you have two materialized dictionaries, and you're armed for \$O(1)\$ lookups that you don't need to query anymore - I believe your code makes a bunch of \$O(n)\$ lookups for every single row.

var query = db.GetTable<tblTrackingBoardPC>()
              .Where(t => t.TrackingGroup == groupName)
              .ToList()
              .Select(t => CreateTrackingComputerItem(t, pingResults, healthResults)
              .OrderBy(t => t.Pingable);

Keep things clean, make a separate function for the messy projection:

private TrackingComputer CreateTrackingComputerItem(
    tblTrackingBoardPC row,
    IDictionary<string,tblTBHealthPing> pingResults,
    IDictionary<string,tblTBHealthHeartbeat> healthResults)
{
    tblTBHealthPing pingResult = null;
    pingResults.TryGetValue(row.ComputerAsset, out pingResult);

    tblTBHealthHeartbeat healthResult = null;
     healthResults.TryGetValue(row.ComputerAsset, out healthResult);

    return new TrackingComputer
        {
            ComputerName = row.ComputerAsset,
            IPAddress = row.ComputerIP,
            Location = row.Location,
            Pingable = pingResult != null
                ? pingResult.HealthPingResult
                : false,
            PingTime = pingResult != null
                ? pingResult.HealthPingTime
                : DateTime.Now.AddDays(-100),
            Username = healthResult != null
                ? healthResult.HealthCurrentUser
                : null,
            CurrentWindow = healthResult != null
                ? healthResult.HealthCurrentWindow
                : null,
            Uptime = healthResult != null
                ? healthResult.HealthUptime
                : null
        };
}

Calling ToList will materialize the query - yes, server-side sorting is generally faster, but you have complex projections going on here, and it's not because LINQ-to-SQL can do it all SQL-side that it should.

With SQL doing the filtering, and LINQ-to-Objects doing the complex projection, IMO you're getting the best of both worlds, and you're returning materialized query results to your UI.

That said - avoid exposing IQueryable<T> (and unmaterialized queries in general) outside your data access code. The reason is deferred execution: because the query didn't run yet, your client code is "free" to extend the query and potentially add things that the LINQ provider can't translate into SQL, resulting in unexpected exceptions


Finding the frequency of an element in a sorted list of integers Input: Array of sorted integers and an element to check the frequency for

Output: Frequency of the element

public class FrequencyElement {
public static void main(String args[]){
    //System.out.println("Enter Input string");

    int input_array []= {2,3,4,25,99,99,19000};
    int element = 99;

    System.out.println(checkFreq(input_array,element));
}
static int checkFreq(int a[],int n){
    //returns the frequency of n in a

    int indexOfElement=elementExists(a,n);

    if(indexOfElement!=-1){
        int count =0;
        while(a[indexOfElement]==n){
            count++;
            indexOfElement++;
        }
        return count;
    }
    else
        return 0;

}
static int elementExists(int input[], int element){
    int lo=0;
    int high = input.length-1;

    while(lo<high){
        int mid = (lo + high )/2;
        if(element >input[mid] ){
            lo = mid+1;
        }
        else if(element < input[mid]){
            high= mid-1;
        }
        else if (lo !=mid)
            high = mid;
        else {
            System.out.println("Mid is: " + mid);
            return mid;
        }
    }
    return -1;
}
}

Reinventing the wheel

There is already a binary search available in JDK.

Bug

On the array {2,3,4,25,99,99,99,99,19000} your implementation returns 0. Removing the lines

else if (lo !=mid)
    high = mid;

from elementExists seems to fix the issue.

Printing from an algorithm

System.out.println("Mid is: " + mid);

It is not a good idea to print to stdout from an algorithm; at least not in the production code. Think about what happens if your friend uses your code: he gets overwhelmed with all the output + I/O is usually computationally expensive.

Summa summarum

All in all, I had this in mind:

import java.util.Arrays;

public class FrequencyElement {

    public static void main(String args[]) {
        int input_array[] = {2, 3, 4, 25, 99, 99, 19000};
        int element = 99;
        System.out.println(getElementFrequency(input_array, element));
    }

    static int getElementFrequency(final int array[], final int queryElement) {
        final int index = getElementIndex(array, queryElement);

        // If there is more than one query elements. Arrays.binarySearch does
        // not guarantee that the index returned will point to the left most
        // occurrence.
        if (index < 0) {
            return 0;
        }

        int count = 1; // Count array[index].
        int offset = 1;

        while (index - offset >= 0 && queryElement == array[index - offset]) {
            ++offset;
            ++count;
        }

        offset = 1;

        while (index + offset < array.length
                && queryElement == array[index + offset]) {
            ++offset;
            ++count;
        }

        return count;
    }

    private static int getElementIndex(final int[] array,
                                       final int queryElement) {
        return Arrays.binarySearch(array, queryElement);
    }
}

Hope that helps.


Form validation for zip codes I have a function that I intend to use for validating zip codes and I wanted to know if my way could be made better.

function zipcode (inputtxt)
{
    var zipcode = /^\+?([0-9]{2})\)?[-. ]?([0-9]{4})[-. ]?([0-9]{4})$/;

    if((inputtxt.value.match(zipcode))
        {
          return true;
        }
        else
        {
            alert("message");
            return false;
        }
}

Changes I would make: (but not necessary)

Make sure inputtext has a value, and isn't undefined, so it wouldn't throw an error in your if statement.
Replace the if statement with return instead. (This is mostly to minimize the code a bit, something your minifier won't do for you).

Except for that, it seems ok (I'm not looking into the logic of the regex, since I'm not sure how a zipcode should be parsed).

function zipcode (inputtxt)
{

    if (!inputtxt || !inputtxt.value) return false;
    var zipcode = /^\+?([0-9]{2})\)?[-. ]?([0-9]{4})[-. ]?([0-9]{4})$/;

    return ((inputtxt.value.match(zipcode));
}


Cabbage-Goat-Wolf puzzle in Haskell Could you please review the following code, and point out how I can make it cleaner, more idiomatic and easier to understand?

module Cabbage (
  solve
) where

data Place = Here | There deriving (Eq, Show)

data Pos = Pos { cabb :: Place
               , goat :: Place
               , wolf :: Place
               , farmer :: Place
               } deriving (Eq, Show)

opp :: Place -> Place
opp Here = There
opp There = Here

valid :: Pos -> Bool
valid (Pos {cabb = c, goat = g, wolf = w, farmer = f}) = (c /= g && g /= w) || g == f

findMoves :: Pos -> [Pos]
findMoves pos@(Pos {cabb = c, goat = g, wolf = w, farmer = f}) =
    filter valid $ moveCabb ++ moveGoat ++ moveWolf ++ moveFarmer where
    moveCabb | c == f = [pos {cabb = opp c, farmer = opp f}] | otherwise = []
    moveGoat | g == f = [pos {goat = opp g, farmer = opp f}] | otherwise = []
    moveWolf | w == f = [pos {wolf = opp w, farmer = opp f}] | otherwise = []
    moveFarmer = [pos {farmer = opp f}]

findSolution :: Pos -> Pos -> [Pos]
findSolution from to = head $ loop [[from]] where
    loop pps = do ps <- pps
                  let moves = filter (flip notElem ps) $ findMoves $ head ps
                  if to `elem` moves
                    then return $ reverse $ to:ps
                    else loop $ map (:ps) moves

solve :: [Pos]
solve = findSolution (setAll Here) (setAll There) where
            setAll x = Pos{ cabb = x, goat = x, wolf = x, farmer = x }

IMHO the findMoves function seems to be quite verbose, and the findSolutions function looks confusing.

Thank you!

Haskell's record system is not really good at providing you uniform access to record entries. That is the reason your findMoves implementation has to be so verbose: You cannot generalize over the fields.

There are a number of ways to get around that. One could be using a library such as fclabels that facilitates this job for you. You set it up like follows:

import Data.Record.Label

data Pos = ....

$( mkLabels [''Pos] )

This will give you "labels" with names such as lCabb that you can use with functions such as getL or modL. Without all this boilerplate, it is possible to write a much more satisfying findMoves function:

findMoves :: Pos -> [Pos]
findMoves pos = filter valid moves
  where
    moves    = [ foldr (\obj -> modL obj opp) pos objs
               | objs <- moveComb, same $ map (`getL` pos) objs
               ]
    moveComb = [[lCabb, lFarmer], [lGoat, lFarmer], [lWolf, lFarmer], [lFarmer]]
    same xs  = all (== head xs) xs


Adding a new class to HTML tag and writing it back with Beautiful Soup I am working on an HTML document to which I need to add certain classes to some elements. In the following code, I am adding class img-responsive.

def add_img_class1(img_tag):
    try:
        img_tag['class'] = img_tag['class']+' img-responsive'
    except KeyError:
        img_tag['class'] = 'img-responsive'
    return img_tag

def add_img_class2(img_tag):
    if img_tag.has_attr('class'):
        img_tag['class'] = img_tag['class']+' img-responsive'
    else:
        img_tag['class'] = 'img-responsive'
    return img_tag

soup = BeautifulSoup(myhtml)
for img_tag in soup.find_all('img'):
    img_tag = add_img_class1(img_tag) #or img_tag = add_img_class2(img_tag)

html = soup.prettify(soup.original_encoding)
with open("edited.html","wb") as file:
    file.write(html)

Both functions do same, however one uses exceptions and another has_attr from BS4. Which is better and why?
Am I doing the right way of writing back to HTML? Or shall convert entire soup to UTF-8 (by string.encode('UTF-8')) and write it?

The second option is better, because the possible error is explicit. However, in lots of case in Python, you should follow EAFP and go for the try statement. However, we can do better.

get(value, default)

In BeautifulSoup, attributes behave like dictionaries. This means you can write img_tag.get('class', '') to get the class if it exists, or the empty string if it doesn't.

def add_img_class(img_tag):
    img_tag = img_tag.get('class', '') + ' img-responsive'

You don't need to return the new img_tag as it is passed by reference. Now that your function is a one-liner, you might as well use the one-liner directly.

Multi-valued attributes

Note that the above code doesn't work! class is a multi-valued attribute in HTML4 and HTML5, so at least BeautifulSoup 4 returns a list instead of a string. The correct code becomes:

img_tag['class'] = img_tag.get('class', []) + ['img-responsive']

Wich is nicer as you don't have to worry about the extra space between the two values.

Encoding

You don't need to convert to UTF-8 before writing the file back. What's wrong with &nbsp;?


Getting count value for matched document I have a list of keywords and I want to count the number of records in a db which contain any one of these keywords. From each record, I want to consider only title and description to be checked.

Is this correctly written?

self.keyword_list = ['Buzz', 'Heard on the street', 'familiar with the development', 'familiar with', 'enters race', 'mull', 'consider', 'final stage', 'final deal', 'eye', 'eyes', 'probe', 'vie for', 'detects', 'allege', 'alleges', 'alleged', 'fabricated', 'inspection', 'inspected', 'to monetise', 'cancellation', 'control', 'pact', 'warning', 'IT scanner', 'Speculative', 'Divest', 'Buzz', 'Heard on the street', 'familiar with the development', 'familiar with the matter', 'Sources', 'source', 'Anonymous', 'anonymity', 'Rumour', 'Scam', 'Fraud', 'In talks', 'Likely to', 'Cancel', 'May', 'Plans to ', 'Raids', 'raid', 'search', 'Delisting', 'delist', 'Block', 'Exit', 'Cheating', 'Scouts','scouting', 'Default', 'defaulted', 'defaulter', 'Calls off', 'Lease out', 'Pick up', 'delay', 'arrest', 'arrested', 'inks', 'in race', 'enters race', 'mull', 'consider', 'final stage', 'final deal', 'eye', 'eyes', 'probe', 'vie for', 'detects', 'allege', 'alleges', 'alleged', 'fabricated', 'inspection', 'inspected', 'monetise', 'cancellation', 'control', 'pact', 'warning', 'IT scanner', 'Speculative', 'Divest']
fetch_record = self.collection1.find()
for record in fetch_record:
    count2 += 1
    for item in self.keyword_list:
        if item.lower() in u'{} {}'.format(record['title'], record['description']).lower():
            #print "Matched for item : ", item
            count1 += 1
            break
print "Matched records are : ", count1
print "Total records are : ", count2

A few things that meet my eye:
Value Casing
You run item.lower() for each item in the keyword list when you iterate over the records. You could ensure the lower-case of all items in your keyword-list before getting to the records, then that step would be away.
Naming
count2 should be total, count1 should be matched, collection1 needs a better name. Names are vital in allowing other programmers to understand your code.
Nitpicks

You may get better performance by formatting the string before entering the loop over the keyword-list
The keyword list contains duplicate items
Remove Code you do not need instead of commenting it out. Version Control has got your back :)
keyword_list could be keywords...
printing is not really a valid return from a method.


Loading a file from a FAT32 filesystem in 16 bit real mode on x86 (Intel syntax assembly) I am writing a bootloader that needs to load a file from a FAT32 filesytem. Since I need to load both the root directory first to find the start of the file, and then the actual file, I wrote a function to load a sequence of clusters from the disk to 0x8000 in memory (since the bootloader is loaded from 0x7C00-0x7DFF and I use 0x7E00-0x7FFF to load sections of the FAT).

loadClusterChain: ; ARGS: eax = first cluster number
    pusha

    ; Set the memory location to load the file to 0x8000
    mov dx, 0x8000

    loadClusterChainLoop:

    ; Store the cluster number (eax) and additional offset (dx) for later
    push dx
    push eax

    ; Update the disk address packet
    mov word [dapDataOffset], dx

    ; Account for the cluster numbers being shifted by 2
    sub eax, 2

    ; Calculate how many sectors the cluster is from the start of the data area
    xor ebx, ebx
    mov bl, byte [bpbSectorsPerCluster]
    mul ebx

    ; Add the start of data area to the root directory offset
    add eax, dword [diskDataAreaStart]

    ; Update the disk address packet start sector
    mov dword [dapStartSector], eax

    ; Update the disk address packet sector count
    push eax
    xor ax, ax
    mov al, byte [bpbSectorsPerCluster]
    mov word [dapSectorCount], ax
    pop eax

    ; Load the file cluster
    mov ah, 0x42
    mov dl, byte [bpbDriveNumber]
    mov si, diskAddressPacket
    int 0x13
    jc diskError

    ; Retrieve the value of eax but keep it in the stack
    pop eax
    push eax

    ; Divide by 128 to calculate the FAT sector to load
    shr eax, 7

    ; Get the number of reserved sectors
    xor ebx, ebx
    mov bx, word [bpbReservedSectors]

    ; Add the reserved sectors to the FAT sector, so eax = relevant FAT sector
    add eax, ebx

    ; Update the disk address packet start sector
    mov dword [dapStartSector], eax

    ; Update the disk address packet sector count
    mov word [dapSectorCount], 1

    ; Update the disk address packet data offset to load the fat to 0x7E00
    mov word [dapDataOffset], 0x7E00

    ; Load the FAT sector
    mov ah, 0x42
    mov dl, byte [bpbDriveNumber]
    mov si, diskAddressPacket
    int 0x13
    jc diskError

    ; Get the offset of clusters into the FAT sector by getting the remainder when divided by 128
    pop eax
    and ax, 0x007F

    ; Multiply offset by 4 to get it in bytes
    shl ax, 2

    ; Calculate the memory address of the cluster in the FAT
    add ax, 0x7E00

    ; Read cluster value
    mov bx, ax
    mov eax, dword [bx]

    ; Set bx = 512*bpbSectorsPerCluster
    xor bx, bx
    mov bl, byte [bpbSectorsPerCluster]
    shl bx, 9

    ; Restore the value of dx and add bx to move dx to the memory location of the end of the file parts loaded so far
    pop dx
    add dx, bx

    ; See if this is the end of the chain
    cmp eax, 0x0FFFFFFF
    jne loadClusterChainLoop

    ; Ensure the file ends with 0, so when scanning the root entries we will definitely stop
    mov bx, dx
    mov [bx], byte 0

    popa
    ret

Here are some of the variables used. diskDataAreaStart is not 0 when the code runs as the actual value is calculated before the function is called. The other variable names are mostly self-explanatory.

diskDataAreaStart       dd 0

diskAddressPacket:
    dapSize         db 0x10
    dapReserved     db 0x00
    dapSectorCount      dw 0
    dapDataOffset       dw 0
    dapDataSegment      dw 0
    dapStartSector      dq 0

I know it's a fairly niche piece of code, but I would be very grateful if someone could give me feedback on it - if I could do something more efficiently or even feedback on how clear my comments were and how easy the code was to follow along would be highly appreciated.

Style

  feedback on how clear my comments were and how easy the code was to follow

I'm not a big fan of line comments. I think they make the code a bit harder to follow. That's why I prefer tail comments along with a nice tabular layout of the labels, instructions, operands, and tail comments.
And of course don't write redundant comments like "Update the disk address packet sector count" when looking at the names of the identifiers already tells what is happening.

You've got some pushes and pops that are fairly distant apart. If you would number these (or tag them somehow), verifying what the stack holds and whether the stack was correctly balanced becomes a bit easier.

push    dx                              ; (1)
push    eax                             ; (2)

...

push    ax                              ; (3)
movzx   ax, byte [bpbSectorsPerCluster]
mov     [dapSectorCount], ax
pop     ax                              ; (3)

...

pop     eax                             ; -(2)-
push    eax                             ; -(2)-

...

pop     eax                             ; (2)

...

pop     dx                              ; (1)

Optimizations

mov word [dapDataOffset], dx

Most assemblers will not require you to write this word size tag because the use of the DX register made already clear that this has to be a word-sized operation. Not writing the redundant size tag makes your code a bit clearer.

mov     [dapDataOffset], dx

You did this several times.

xor ebx, ebx
mov bl, byte [bpbSectorsPerCluster]

The instruction set has the MOVZX instruction for this kind of situations. It will also reduce the code's footprint which is important in a bootloader that is limited to just 512 bytes. Notice that here the byte size tag is required.

movzx   ebx, byte [bpbSectorsPerCluster]

You did this several times.

push eax
xor ax, ax
mov al, byte [bpbSectorsPerCluster]
mov word [dapSectorCount], ax
pop eax

Pushing and popping the 32-bit register EAX from real address mode uses an operand size prefix. In this case the high word was never changed and so you can shave off 2 bytes if you only push/pop AX.

Better still, the program doesn't need you to preserve AX at this point. Again 2 bytes less.

movzx   ax, byte [bpbSectorsPerCluster]
mov     [dapSectorCount], ax

pop eax
and ax, 0x007F

; Multiply offset by 4 to get it in bytes
shl ax, 2

; Calculate the memory address of the cluster in the FAT
add ax, 0x7E00

; Read cluster value
mov bx, ax
mov eax, dword [bx]

Here's another opportunity to shave off a byte. Pop the value directly in the register that you'll use to retrieve the cluster number:

pop     ebx
and     bx, 0x7F
shl     bx, 2      ; Multiply offset by 4 to get it in bytes
add     bx, 0x7E00 ; Calculate the memory address of the cluster in the FAT
mov     eax, [bx]  ; Read cluster value

Exactly the same length but less instructions:

pop     ebx
and     ebx, 0x7F
mov     eax, [ebx*4 + 0x00007E00]  ; Read cluster value

; Update the disk address packet sector count
mov word [dapSectorCount], 1

; Update the disk address packet data offset to load the fat to 0x7E00
mov word [dapDataOffset], 0x7E00

The above instructions total 12 bytes. Because dapSectorCount and dapDataOffset follow each other in memory, you can combine the writes. The total then becomes 9 bytes.

; Update dapSectorCount=1 and dapDataOffset=0x7E00 together
mov     dword [dapSectorCount], 0x7E000001

This last optimization is of course killing readability in favor of code size. Your choice!

Beware

Your loadClusterChain routine uses some 32-bit registers. Given that this is 16-bit code the pusha and popa instructions will not preserve the high words. Maybe this can become a problem for the rest of your program!


Highlight the active link in a navigation menu I wonder if my script is all right. Please review my code:

<!DOCTYPE html>

<meta charset="UTF-8">
<title>Menu</title>

Home

News

Contact

About

It should highlight the current page link in the navigation bar.

querySelectorAll

Here's another approach, if you don't mind older browsers. You can use querySelectorAll instead of document.links. That way, you end up with a smaller result set rather than all links, and save you CPU cycles for running the loop.

In this case, we get only those links that have the same href value as the current document url:

var links = document.querySelectorAll('a[href="'+document.URL+'"]');

However, note the browser compatibility and there are quirks across implementations.

Not all links need active

Now if you think about it, not all links with the document.URL need to have active. Say you have active set the font to 2em. If you get all links that point to the same page, they'd be 2em in size, regardless of where they are in the page!

Most likely, you only need it for the primary navigation. Consider adding a class to further refine the result set.

Home

News

Contact

About

// Getting only the links that are in .navigation
var links = document.querySelectorAll('.navigation a[href="'+document.URL+'"]');

// More specific CSS
.navigation a.active {background:red;}

className

Now you have to note that setting className replaces it with the value, like this example. If the links you have happen to have existing classes (and styles that come with them), your script will unintentionally remove their styles due to the replaced className.

What you can do is get the existing className value, split them by spaces (multiple class names are separated by spaces), append your class name, join them back and then change the value, like I did here:

var element = document.getElementById('change');
var classNames = element.className.split(' ');
classNames.push('huge');
element.className = classNames.join(' ');


Removing random repeated guesses from brute force program I came here to ask if there is a way to prevent already guessed guesses from being guessed again in my brute-force Java program. Basically I used a rand.nextInt in my program but the program will guess already guessed passwords again even if they were incorrect. How could I prevent this as it wastes time and processing power to guess already guessed numbers?

import java.util.*;
import java.io.*;
class pwcracker
{
public static void main (String[] args)
{
    Scanner scan = new Scanner( System.in );
    Random rand = new Random();

  String pw, choices, guess;
    long tries;
    int j, length;

    System.out.println("Enter a password that is up to 5 chars and contains no numbers: ");
    pw = "" + scan.nextLine();
    length = pw.length();

    choices = "abcdefghijklmnopqrstuvwxyz";
    tries = 0;
    guess = "";

    System.out.println("Your pw is: " + pw);
    System.out.println("The length of your pw is: " + length);

  System.out.println("for TEST- Guess: " + guess + "pw :"+pw);

  if (!guess.equals(pw)){

  while  (!guess.equals(pw))
    {
        j = 0;
        guess = "";

        while ( j < length )
        {
            guess = guess + choices.charAt( rand.nextInt( choices.length() ) );
        j = j + 1;

        if (guess .equals(pw))
     {
       System.out.println("Match found, ending loop..");
       break;
     }

        }
                System.out.println("Guess: " + guess + " pw :"+pw);
        tries = tries + 1;
    }
  }
    System.out.println("Here is your password: " + guess);
    System.out.println("It took " + tries + " tries to guess it.");
   }
}

I think that your problem is the use of randomisation - it's not really a brute-force attack if you are guessing.

If it were a 4-digit combination lock you would start at 0000 and then try 0001 and 0002 and so on. That's the brute-force approach. I suggest that you employ exactly the same approach with your password cracker. It should be the same algorithm, but rather than 10 posibilities in each slot you have 26.


Splitting many data files by quintile The program below is about finding the values below the percentage and splitting them into five different output files. This works great for small input data files. But when I am trying to run it for a big data it takes ages to compute. Any suggestions on optimizing the program so that I can get fast computation?

#seperate by col 5
import numpy as np
import os
import re
import math
import csv
import sys
import time
import multiprocessing
from scipy import stats

curdir = 'Tools/'
inputdir = 'input/'
filename = 'input'
outputdir = 'output/'

def seperate(filename):
    print "Begin seperating File: %s" % filename
    dtype = np.dtype([('id1',str,12),('id2',str,12),('c1',int),('c2',int),('c3',float)])
    data = np.loadtxt(curdir + inputdir + filename,dtype=dtype)
    col = 'c3'
    col_data = np.array(data[:][col])
    cache = {}
    def get_per(per):
            if(cache.has_key(per)):
                return cache.get(per)
            else:
                r = 100 - stats.percentileofscore(col_data,per)
                cache[per] = r
                return r
    o1 = file(curdir + outputdir+ filename + '_0_20','w')
    o2 = file(curdir + outputdir+ filename + '_20_40','w')
    o3 = file(curdir + outputdir+ filename + '_40_60','w')
    o4 = file(curdir + outputdir+ filename + '_60_80','w')
    o5 = file(curdir + outputdir+ filename + '_80_100','w')
    for line in data:
        per = get_per(line[col])
        output_format = "%s %s %d %d %.1f\n"
        output_data = (line['id1'],line['id2'],line['c1'],line['c2'],line['c3'])
        if per>=0 and per < 20:
            o1.write(output_format % output_data)
        elif per>=20 and per<40:
            o2.write(output_format % output_data)
            pass
        elif per>=40 and per<60:
            o3.write(output_format % output_data)
            pass
        elif per>=60 and per<80:
            o4.write(output_format % output_data)
            pass
        elif per>=80 and per<=100:
            o5.write(output_format % output_data)
            pass
    o1.close()
    o2.close()
    o3.close()
    o4.close()
    o5.close()

    print "Finish seperating File: %s" % filename

ps = []
print "Reading Files"
for parent,dirNames,fileNames in os.walk(curdir+inputdir):
    if fileNames:
        #multi_process
        print fileNames
        for fileName in fileNames:
            m = re.match(r'^(?!\.)',fileName)
            if m:
                ps.append(multiprocessing.Process(target=seperate,args=(fileName,)))

#runing by multiple processes
for p in ps:
    p.start()
for p in ps:
    p.join()

Calling percentileofscore for every distinct value is rather inefficient vs. the alternative of making just one call scoreatpercentile(col_data, [0,20,40,60,80,100]). You could directly compare the column values against the limits returned by that function.
Apart from the above you are mostly doing disk I/O. You may have made matters worse by using multiprocessing. In particular, because you lauch as many processes as you have input files, you may have way too many processes competing for RAM and disk access.


`new Function` for json reference I have JSON with references to other parts in that JSON file.

// the function I would like to be reviewed
function referenceToObject(json, reference){
    return (new Function(
            'json', 'return json' + reference
    ))(json);
}

// json
var example = {
    "target": "['first']['second']",
    "first": {
        second: 'hi'
    }
};

// execution
console.log(referenceToObject(example, example.target));

That function basically executes like this:

return json['one']['two'];

Is there a better way to extract the JSON reference? I'm able to change every bit of this code, as well as the JSON because we are reworking the front- and the backend. So, if you have ideas to improve the JSON instead of the Javascript that would also be great.

Assuming that you control over the value of the target attribute, I suggest to store the reference chain as a list of attribute names. Then you can easily resolve the reference without having to resort to eval-like meta-programming. A reduction would be ideal like you mentioned.

function referenceToObject(json, target)
{
 if (target === undefined)
  target = json.target;

 return target.reduce(referenceToObject.reductor, json);
}

referenceToObject.reductor = function(obj, attrib)
{
 return obj[attrib];
};

var example = {
 "target": ['first', 'second'],
 "first": {
  second: 'hi'
 }
};

console.log(referenceToObject(example));


Animation Template Method for Snake Game I have recently finished my first iteration of a template method implementation for a JavaScript canvas animation of the snake game. However, I would love for the community to critique this code and inform me where I am going wrong?

In my opinion, a bad smell for me is the number of times this is used throughout. Furthermore, the number of properties in the SnakeAnimator implementation (Note: These properties are passed upon extension).

const VectorFactory = require("./model/vector");
const _ = require("lodash");
const Animator = require('./animators/animator');

let SnakeAnimator = {
    vectors: [],
    canvas: null,
    xAdd: null,
    canvas_context: null,
    canvas_height: 0,
    canvas_width: 0,

    initialise: function (options) {
        this.canvas_context = this.canvas.getContext("2d");
        this.canvas_width = this.canvas.width = window.innerWidth;
        this.canvas_height = this.canvas.height = window.innerHeight;
        this.canvas_context.strokeStyle = options.colour;
    },

    draw: function () {
        this.canvas_context.beginPath();

        for( let i = 0; i < this.vectors.length; i++) {
            this.canvas_context.moveTo(this.vectors[i][0].x, this.vectors[i][0].y);
            this.canvas_context.lineTo(this.vectors[i][1].x, this.vectors[i][1].y);
        }

        this.canvas_context.stroke();
        this.canvas_context.closePath();
    },

    update: function() {
        for (let i = 0; i < this.vectors.length; i++) {
            this.vectors[i][0].x = this.vectors[i][0].x + this.xAdd;
            this.vectors[i][1].x = this.vectors[i][1].x + this.xAdd;
        }
    },

    clear: function() {
        this.canvas_context.clearRect(0, 0, this.canvas_width, this.canvas_height);
    },

    addSnake: function() {
        this.vectors.push([VectorFactory(0, this.canvas_height / 2), VectorFactory(this.xAdd, this.canvas_height / 2)]);
    }
};
_.extend(SnakeAnimator, Animator);

module.exports = SnakeAnimator;

The template method implementation is as follows:

let Animator = {
    animate: function() {
        this.update();
        this.clear();
        this.draw();
    }
};

module.exports = Animator;

I would greatly appreciate any feedback on how I can improve this / my overall knowledge of JavaScript applications should be structured as I have struggled with this for a while.

const VectorFactory = require("./model/vector");
const _ = require("lodash");
const Animator = require('./animators/animator');

Consider using ES6 and imports. That way, your code will be a bit more future-proof. The caveat is that you need to have a build system and a transpiler. Since you have require, I suppose you do have a build system in place. You just need the transpiler.

Now I suppose you use the code this way:

const SnakeAnimator = require('./animators/snake-animator');

const snakeAnimatorInstance = _.extend(SnakeAnimator);

snakeAnimatorInstance.canvas = referenceToCanvas

snakeAnimatorInstance.initialise();

gameLoop(snakeAnimatorInstance.animate);

Looks ok, but it would be nice to automate much of the setup using a factory function that accepts params, and pops it in to a composed object.

const SnakeAnimator = require('./animators/snake-animator');

snakeAnimatorInstance = SnakeAnimator.createInstance({
  canvas: referenceToCanvas,
  // other options for initialization
});

gameLoop(snakeAnimatorInstance.animate);

Now for the properties

vectors: [],
canvas: null,
xAdd: null,
canvas_context: null,
canvas_height: 0,
canvas_width: 0,

Looks like the only public property is canvas. All the others appear to be only used by internal functions like initialize. It would be nice to indicate that they shouldn't be modified directly. By convention, a _ is prefixed to the name.

Also, JS follows the camelCase convention in naming (notice innerWidth and innerHeight). Suggesting you follow for consistency.


In an set of integers, find three elements summing to zero (3-sum, leetcode variant) I use a dummy solution to solve the leetcode 3Sum problem.

I employ the set data type to handle duplicates, then transform back to list.

  Given an array nums of n integers, are there elements a, b, c  in nums such that a + b + c = 0?  Find all unique triplets in the array which gives the sum of zero.

  Note:

  The solution set must not contain duplicate triplets.

  Example:

  Given array nums = [-1, 0, 1, 2, -1, -4],

  A solution set is:

[
  [-1, 0, 1],
  [-1, -1, 2]
]

My codes:

from typing import List
import logging
import unittest
import random
from collections import defaultdict,Counter

##logging.disable(level=#logging.CRITICAL)
##logging.basicConfig(level=#logging.DEBUG, format="%(levelname)s %(message)s")

    class Solution:
        def threeSum(self, nums, target: int=0) -> List[List[int]]:
            """
            :type nums: List[int]
            :type target: int
            """
            if len(nums) < 3: return []

            triplets = []
            if target == [0, 0, 0]:
                triplets.append([0, 0, 0])
                return triplets # finish fast

            lookup = {nums[i]:i for i in range(len(nums))} #overwrite from the high
            if len(lookup) == 1:#assert one identical element
                keys = [k for k in lookup.keys()]
                if keys[0] != 0:
                    return []
                else:
                    triplets.append([0,0,0])
                    return triplets

            triplets_set = set()

            for i in range(len(nums)):
                num_1 = nums[i]
                sub_target = target - num_1
                # #logging.debug(f"level_1_lookup: {lookup}")

                for j in range(i+1, len(nums)):
                    num_2 = nums[j]
                    num_3 = sub_target - num_2
                    k = lookup.get(num_3) #
                    if k not in {None, i, j}: #don't reproduce itself
                        result = [num_1, num_2, num_3]
                        result.sort()
                        result = tuple(result)
                        triplets_set.add(result)

            triplets = [list(t) for t in triplets_set]
            return triplets

My solution gets this result:

  Your runtime beats 28.86 % of python3 submissions.

Could anyone please give hints to improve?

Multi-part answer: [0) these preliminaries] 1) review of code presented 2) hints for improvement excluding personal, from programming over python and accepting a programming challenge to k-sum and leetcode 3Sum.

One general principle to follow is do as expected, in programming, it has been formulated in many guises, including Principle of Least Surprise. Every other route leaves you under pressure to justify, at risk of being misunderstood.

In coding, document, in the program source code:
What is "everything" there for?

Such rules often have been gathered into guide lines, for Python, follow the Style Guide for Python Code.

Instead of a docstring, your module starts with imports - most of them unused.
If your code mentioned leetcode, the presentation of a class Solution with just one function definition wouldn't beg justification.
the definition of threeSum() shows (a cute addition of target&default and) a curious mix of leetcode's templates for Python 2&3, lacking a proper docstring
comparing "the int" target to the list literal [0, 0, 0] is dangerous given Python's zeal to allow operations: do you know by heart when that evaluates to True?
(Do you expect most every reader of your code to?)
(Belatedly, it occurred to me that you may have intended to compare nums to [0, 0, 0] - bogus if target != 0)
verbosity -
you can denote a tuple containing a single literal list like ([v, v, v],)
2nd early out:
with only one key→value in the dict, there's no need to access it:
every key will equal nums[0]
You need to check for 3*nums[0] != target
naming (Not your fault: Solution and threeSum are substandard.
nums/_1-3 don't feel bad enough to change):
- target, triplets, sub_target: right on!
- given Python's "duck typing", I'd stick with triplets (, never introducing triplets_set)
- lookup is a horrible name → value_to_index (or, in a suitably small context, just index)
(- there's one thing wrong with k: it gets in the way when extending three_sum() to k_sum()…)
pythonic or not (or exploiting library functions, rather?):
value_to_index = { value: index for index, value in enumerate(nums) }
(same for for i, num_1 in enumerate(nums), for j, num_2 in enumerate(nums[i+1:], i+1))
commenting
- you did comment, and I find half of the comments helpful
- I don't get #overwrite from the high (that may just be me)
- #don't reproduce itself: probably use any element/index once, at most rather than don't repeat an element/index
- you did not comment the outer for loop, arguably the most profitable place:
how is what the execution constructs a solution?:
for each value, value_to_index keeps just the last index: how does this not prevent any valid triple to show up in the solution?
checking k: nice!
(I'd go for in (None, i, j))
you don't provide a "tinker harness"

if __name__ == '__main__':
    object_disoriented = Solution()
    print(object_disoriented.threeSum([-1, 0, 1, 2, -1, -4]))
    print(object_disoriented.threeSum([1, 1, 1], 3))

(Something else to follow is make (judicious) use of every help you can: if you use an IDE supporting Python, chances are it can help you with PEP8.)
(Running low on steam, this part will start more frugal than expected.)

Real Programming is the creation of a language that simplifies the solution of the problem at hand.

With Python providing most mechanisms needed in programming challenges, that leaves:

Code readably.
Create elements easy to use correctly.

I bickered you some about having a consistent problem description.
This is another place where Test First shines:
As long as you find yourself not knowing what to test, you are not in a position to implement anything, yet. (Inconsistencies in specification tend to stand out in test design & implementation.)

Programming challenges à la leetcode.com typically build on simpler challenges - you met 2sum, which puts you in a favourable position to tackle 3sum.
The performance part of these challenges is about not doing things over, not discarding knowledge more often than about algebraic insight.

The generic solution to 2sum is to take values and find the complement (1sum?).
The extension to k-sum is to split k into smaller numbers.

Ways to have information about the order of a set or sequence (say, elements) of instances of a type with ordering reusable is to have them ordered (ordered = sorted(elements)) or sorted (histogram = Counter(elements)).
For 2sum, you could search for target/2 and work inside-out.
For 3sum, one value will be no greater, another no smaller than both others. The third one will not "lie outside" the former two.


Merging union Observables I am having a scenario where I need to execute observables that depends on the result of the first one. However I need to keep the result of the first observable. I couldn't find any extension that would help me do this. For instance SelectMany does a projection of the first observable discarding the source results. Therefore I made my own extension:

public static IObservable<T> MergeWithResultPropagation<T>(this IObservable<T> src, Func<T, IEnumerable<IObservable<T>>> elems)
{
    return src.SelectMany(result => elems(result).Union(new[] { Observable.Return(result) }))
        .SelectMany(r => r);
}

Here's a dummy example:

Observable.Range(1, 3)
.MergeWithResultPropagation(item => new[]{
    Observable.Return(item * 2),
    Observable.Return(item * 3)
}).Dump();

Is this an adequate way to solve the problem? Did I miss an extension method that does this?

Union vs Concat

As you addmited, there is no need to use the Union extension because no two items will ever have the same value. Concat would be more appropriate because it'll better show what is going on.

Difference between the two is that

[ 1, 2 ].Union([2, 3]) = [1, 2, 3]

whereas the same with Concat would be

[ 1, 2 ].Concat([2, 3]) = [1, 2, 2, 3]

because of the not clear intention I had a hard time understanding this short code. The descriptions says something else then the implementation.

Zip

I don't know any extension that could do the same job but I think this one could be expressed cleaner by first producing the results and then zipping each result with the corresponding item that lead to this result using the Zip extension. I also think that it's nicer to use Enumerable.Repeat rather then  new []{}

return
    src
        .Select(x => elems(x))
        .Zip(src, (results, x) => results.Concat(Enumerable.Repeat(Observable.Return(x), 1)))
        .SelectMany(z => z)
        .SelectMany(x => x);

Functional

To make it even cleaner I suggest encapsulating the Concat in its own method so the final extension could be:

public static IObservable<T> MergeWithResultPropagation3<T>(this IObservable<T> values, Func<T, IEnumerable<IObservable<T>>> factory)
{
    return
        values
            .Select(x => factory(x))
            .Zip(values, AppendValue())
            .SelectMany(z => z)
            .SelectMany(x => x);

    Func<IEnumerable<IObservable<T>>, T, IEnumerable<IObservable<T>>> AppendValue()
    {
        return (results, value) => results.Concat(Enumerable.Repeat(Observable.Return(value), 1));
    }
}

yield return

Alternatively to new []{} and Concat you could make the helper work with yield return

public static IObservable<T> MergeWithResultPropagation3<T>(this IObservable<T> values, Func<T, IEnumerable<IObservable<T>>> factory)
{
    return
        values
            .Select(x => factory(x))
            .Zip(values, (results, value) => AppendValue(results, value))
            .SelectMany(z => z)
            .SelectMany(x => x);

    IEnumerable<IObservable<T>> AppendValue(IEnumerable<IObservable<T>> results, T value)
    {
        foreach (var result in results) yield return result;
        yield return Observable.Return(value);
    }
}


Image downloader for a website v2 This code takes a website and downloads all .jpg images in the webpage. It supports only websites that have the img element and src contains a .jpg link. The previous version can be found here

import random

import urllib.request

import requests

from bs4 import BeautifulSoup

def Download_Image_from_Web(url):
    # variables
    k = 0
    links_list = []
    # source code to BeatifulSoup
    source_code = requests.get(url)
    plain_text = source_code.text
    soup = BeautifulSoup(plain_text, "html.parser")
    # filtering
    for link in soup.findAll('img'):

        image_links = link.get('src')
        if '.jpg' in image_links:
            for i in image_links.split("\\n"):

                links_list.append(i.split())
    # send image links and download them
    while k <= (len(links_list) - 1):
        # create a random name
        name = random.randrange(1, 1000)
        fullName = str(name) + ".jpg"
        # download each link sperated
        lines = links_list[k][0]
        urllib.request.urlretrieve(lines, fullName)

        print(lines)
        print(fullName + '\n')

        k += 1
# choose a site that has the <img> element in its html source code with src tag
Download_Image_from_Web("https://pixabay.com")

There's a lot to improve about your code, and I'll start from the top. I'll also rewrite some of the logic you had there because IMO it's a bit clumsy and hard to understand.

Imports
Don't use urllib and requests at the same time. Use either one or another. They serve the same purpose.
from bs4 import BeautifulSoup
import os
import random
import requests

As you can see, I've alphabetically ordered them so that it becomes easier to locate each of them.

Constants
You have one url which might be moved at the top of your program. That way, you'll allow your users to change it without searching through all the code.
Moving on, I realised that you didn't give the user a chance to save the pictures wherever he wants. Let's add another constant, say IMAGE_LOCATION.
URL = 'https://pixabay.com'
IMAGE_LOCATION = '/home/images/'

Functions
You can split your code into separate functions so that each of them can serve its own purpose. Read more about Single Responsability Principle:
def get_html_source():
    """Docstring here."""
    html_source = requests.get(URL).text
    return BeautifulSoup(html_source, 'html.parser')

The above function will return a BeautifulSoup object which contains the html source code of the desired URL.
We can now build another function which will generate a path containing a random name for each picture we're downloading:
def image_name():
    """Docstring here."""
    return '{}.jpg'.format(os.path.join(IMAGE_LOCATION, str(random.randrange(1, 100000))))

I've also increased the maximum number just in case there's a huge number of files.
Another helper function would be one that checks if a src URL is a valid one or not. I've modified a bit your condition and made it more strict:
def validate(image_url):
    """Docstring here."""
    return image_url.endswith('.jpg') and image_url.startswith('http')

The above, verifies if the url starts with http (implicitly https will pass) and also if it ends in .jpg.
Only now, we can finally create the download_images function, and wrap our logic into it:
def download_images(soup):
    """Docstring here."""
    for link in soup.find_all('img'):
        image_url = link.get('src')
        if validate(image_url):
            image = requests.get(image_url)
            if image.status_code == 200:
                with open(image_name(), 'wb') as f:
                    f.write(image.content)

As you can see, I've removed most of the logic you had, and simply did the following steps:

Go through all the img tags and save their src attribute (which is an URL
Check the URL if it matches our condition and build our image object
Double check to see if the status code was successful (200) and save the image to our desired location.

Next, wrapping all our code within a main function is as simple as:
def main():
    """Docstring here."""
    soup = get_html_source()
    download_images(soup)

All the code
from bs4 import BeautifulSoup
import os
import random
import requests

URL = 'https://pixabay.com'
IMAGE_LOCATION = '/home/images/'

def get_html_source():
    """Docstring here."""
    html_source = requests.get(URL).text
    return BeautifulSoup(html_source, 'html.parser')

def image_name():
    """Docstring here."""
    return '{}.jpg'.format(os.path.join(IMAGE_LOCATION, str(random.randrange(1, 100000))))

def validate(image_url):
    """Docstring here."""
    return image_url.endswith('.jpg') and image_url.startswith('http')

def download_images(soup):
    """Docstring here."""
    for link in soup.findAll('img'):
        image_url = link.get('src')
        if validate(image_url):
            image = requests.get(image_url)
            if image.status_code == 200:
                with open(image_name(), 'wb') as f:
                    f.write(image.content)

def main():
    """Docstring here."""
    soup = get_html_source()
    download_images(soup)

if __name__ == '__main__':
    main()

If you don't know what's with if __name__ == '__main__' you can read more about it here
More, note that all the functions have a docstring which contain a placeholder like "Docstring here.". You can replace that placeholder with a summary that describes what each function does.


Reading a file chunk by chunk in QT 4.3.3 I have following  code in QT in

// myEditor.h
class myEditor : QScintilla {
    public:
        readFile();
};

#include "myEditor.h"
// myEditor.cc
myEditor::readFile() {
   FILE* fp = fopen("mynew.v","r"):
   QTextStream ts(fp, QIODevice::ReadOnly);
   /* reading the  text stream  buffer by buffer
   bufferSize is calculated using following formula
   2 to power(k) *  n  =  2 to power 31*
   where n is  size of each block in linux filesystem*/

   int bufferSize =(1024* 1024)/2;
   do {
      QString s = ts.read(bufferSize);
      append(s);
    } while(!ts.atEnd());
}

I calculated the bufferSize as per comments .It will be helpful if someone can review the code and let me know if there are issues.

As you're storing the whole result into memory anyway for display in a text editor (so you're not expecting to load hundreds of MB of text), why not just use what Qt provides to read the entire file into a string in one call?

It's probably more than enough optimized for general purpose usage. Displaying the entire result in the editor might also be faster than constantly appending parts.

I also see that you're using the C API for opening the file (which is never closed) and then passing it to Qt. Qt has QFile for this, as shown in the link mentioned above.


Remove parameters from string containing URL Is this the best way to remove URL parameters from a string containing an URL?

$url = "http://www.test.com/test.html?parameter=hey&parameter2=ho";
if (strstr($url, "?")) {
   $url = strstr($url, "?", true);
}

Is there a better one-line solution? It seems so bad to run the same function twice.

The simplest way, a true one-liner, would be to use strtok, an ancient but for some reason obscure function that does exactly what you need:

$url = "http://www.test.com/test.html?parameter=hey&parameter2=ho";
$url = strtok($url, "?");

it will give you the part before the token if it is found, or the whole string otherwise


Optimization of algorithm performance I wrote the following code. A user from Stack Overflow recommended me to write my code here in order to be reviewed for optimization.

How could I optimize it?
Could I have this code in a professional, better form with increased performance?

Explanation: This code calculate multiplication of three matrix in c() function. Then, using Omo as old and Omn as new into c() calculate L(). Finally, defining two if statement accept Ls which are bigger than 1 and R3 which is random number. This code is MCMC fitting data. Here I fit omn and Mn.

import numpy as np
import emcee
import matplotlib.pyplot as plt
from math import *
from scipy.integrate import quad
from scipy.integrate import odeint
O_m=chi=None
xx=np.array([0.01,0.012])
yy=np.array([32.95388698,33.87900347])
Cov=[[137,168],[28155,-2217]]     # this is a covariance matrix for simplification I chose the 2*2 one

temp=1e5
z0=0
M=2
Omo = 0.3
Odo=0.7
H0=70

def ant(z, Om, Od):
    return 1/sqrt(((1+z)**2)*(1+Om*z)-z*(2+z)*Od)
def dl(n, Om, Od, M):
    Od=1-Om
    q=quad(ant,0,xx[n],args=(Om,Od))[0]
    h=5*log10((1+xx[n])*q)
    fn=(yy[n]-M-h)
    return fn

def c(Om, Od, M):
    f_list = []
    for i in range(2):  # the value '2' reflects matrix size
        f_list.append(dl(i,Om,Od,M))
    rdag=[f_list]
    rmat=[[f] for f in f_list]
    a=np.dot(rdag,Cov)
    b=np.dot(a,rmat)
    Matrix=np.linalg.det(b)*0.000001
    return Matrix
N=2000
with open('txtfile.txt', 'w') as f:
    for i in range (1,N):
        R1=np.random.uniform(0,1)
        R2=np.random.uniform(0,1)
        R3=np.random.uniform(0,1)
        R4=np.random.uniform(0,1)
        def delta(r1, r2):
            sig=0.04
            d=sig*(np.sqrt(-2*np.log(r1))*np.cos(np.radians(r2)))
            return d
        Omn=Omo+delta(R1, R2)
        Odn=1-Omn
        Mn=M+delta(R3,R4)
        R3=np.random.uniform(0,1)

        def L():
            l=np.exp(-0.5*(c(Omn,Odn,Mn)-c(Omo,Odo,M)))
            return l
        if L()>1:
            O_m=Omn
            chi=c(Omn,Odn,Mn)

        elif L()>R3:
            O_m=Omn
            chi=c(Omn, Odn, Mn)

        f.write("{0}\t{1}\n".format(chi, O_m))
    print("Minimum of chi squre is")
    if chi<temp:
        temp=chi
        chimin=temp
    print(chimin)
    print(input("Press any key to exit... "))

I appreciate your help and your attention

import *

Since you only need sqrt and log10 from math, I would change this:

from math import *

to:

from math import sqrt, log10

pep8

Use descriptive method names. I have no idea what dl or c mean
Valiable (and argument) names should be descriptive. It is very hard to determine what Omo is.
use lower_case for arguments and variable names
Spacing between operators

unused arguments

def dl(n, Om, Od, M):
    Od = 1 - Om

Why pass in Od if you overwrite it immediately afterwards?

In fact, Od is only used in ant, so why not just calculate it there and omit it for the rest?

global variables

dl uses xx while it's not in the argument list. It also only uses n to index xx, why not pass in the relevant value from xx as argument immediately?

Using this global variable makes it harder to use this method for other values

inner functions

Functions delta and L gets defined all 2000 iterations of the for-loop. This is unnecessary

L uses global state, and is calculated multiple times , why not just do

l = np.exp(-0.5 * (c(Omn, Odn, Mn) - c(Omo, Odo, M)))

making it a variable instead of a function?

Seperate the functions

Even in a simple script, you can seperate the production of the results from the presentation of the input

use if __name__ == '__main'__:

so your program can be run a script, but also imported as a module

R3

Why generate a new random number with the same name? Either it's the same number, so no need to generate it, or it's a different number, so give it a new name

Besides that, if L() is larger than a number between 0 and 1, it will be larger than 1, so if .. elif is unnecessary

Magic numbers

Where come the 0.000001 in c and the 0.04 in delta come from? If these a constants, better name them so

avoid repetition

The way your program is setup, Cov is converted to a numpy array a lot, and c(Omn, Odn, Mn) and c(Omo, Odo, M) are calculated over and over without any change in arguments, better to do those things once

f_list

instead of appending,  I would use a list comprehension.

One step further, since you need it as a np.array anyway, why not use np.fromiter and a generator expression?
Why are you taking the determinant from a two-dimensional array, instead of doing the calculations on a one-dimensional array?

All in all, I would end up with something like this:

import numpy as np
import emcee
import matplotlib.pyplot as plt
from math import sqrt, log10
from scipy.integrate import quad
from scipy.integrate import odeint
from io import StringIO

def ant(z, Om):
    Od = 1 - Om
    return 1 / sqrt(((1 + z) ** 2) * (1 + Om * z) - z * (2 + z) * Od)

def dl(x, y, Om, M):
    q = quad(ant, 0, x, args=(Om,))[0]
    h = 5 * log10((1 + x) * q)
    return y - M - h

def c(xx, yy, cov, Om, M):
    f_list = np.fromiter(
    (dl(x, y, Om, M) for x, y in zip(xx, yy)),
    dtype=float,
    count=len(xx),
    )
    a = np.dot(f_list, cov)
    b = np.dot(a, f_list.T)
    return b * 0.000001

def delta(r1, r2):
    sig = 0.04
    return sig * (np.sqrt(-2 * np.log(r1)) * np.cos(np.radians(r2)))

def calculation(xx, yy, cov, M, Omo, N):

    c0 = c(xx, yy, cov, Omo, M)
    for i in range(1, N):
        R1 = np.random.uniform(0, 1)
        R2 = np.random.uniform(0, 1)
        R3 = np.random.uniform(0, 1)
        R4 = np.random.uniform(0, 1)
        R5 = np.random.uniform(0, 1)

        Omn = Omo + delta(R1, R2)
        Mn = M + delta(R3, R4)

        c_ = c(xx, yy, cov, Omn, Mn)
        l = np.exp(-0.5 * (c_ - c0))

        if l > R5:
            O_m = Omn
            chi = c_

        yield chi, O_m

def main(filehandle):
    temp = 1e5
    z0 = 0
    M = 2
    Omo = 0.3
    H0 = 70

    N = 2000
    xx = np.array([0.01,0.012])
    yy = np.array([32.95388698,33.87900347])
    cov = np.array([[137,168],[28155,-2217]])

    for chi, O_m in calculation(xx, yy, cov, M, Omo, N):
        filehandle.write("{0}\t{1}\n".format(chi, O_m))

    if chi < temp:
        temp = chi
        chimin = temp
    return chimin

if __name__ == '__main__':
    # with StringIO() as filehandle:
    with open('txtfile.txt', 'w') as filehandle:
        chimin = main(filehandle)
        # print(filehandle.getvalue())
        print("Minimum of chi squre is")

        print(chimin)
        print(input("Press any key to exit... "))

I don't know whether this will spectacularly speed up your algorithm, but it will help, especially the more simple c. For this simple input, it needed about 33% less time on my machine

Larger gains will be made by vectorizing more, but for that I would need to have a better mathematical understanding of what's going on


IComparable implementation for a class representing a version I have an UpdateId class which represents some kind of version (like AssemblyVersion). UpdateId has three int values (listed in their compare priorities order): Year, ReleaseNumber and Revision.

How can I make my CompareTo method better? It looks clumpy to me.

public int CompareTo(object obj)
{
   if(obj == null) return 1;

   var updateId = obj as UpdateId;
   if (updateId != null)
   {
       int yearCompare = this.Year.CompareTo(updateId.Year);
       if (yearCompare == 0)
       {
           int releaseNumberCompare = this.ReleaseNumber.CompareTo(updateId.ReleaseNumber);
           if (releaseNumberCompare == 0)
           {
               return this.Revision.CompareTo(updateId.Revision);
           }

           return releaseNumberCompare;
        }

        return yearCompare;
    }
    else
    {
        throw new ArgumentException("Object is not an UpdateId");
    }
}

You could change it to reduce the "arrow style" like so

public int CompareTo(object obj)
{
   if(obj == null) { return 1; }

   var updateId = obj as UpdateId;
   if (updateId == null)
   {
       throw new ArgumentException("Object is not an UpdateId");
   }

   int compareResult = this.Year.CompareTo(updateId.Year);
   if (compareResult != 0) { return compareResult; }

   compareResult = this.ReleaseNumber.CompareTo(updateId.ReleaseNumber);
   if (compareResult != 0) { return compareResult; }

   return this.Revision.CompareTo(updateId.Revision);

}

which will save you some horizontal spacing and some not needed variables, because they all meant the same so I changed them to compareResult.


Calculating angle in isometric view I have an angle where I need to convert a 'normal' angle to an isometric angle (116 degrees), and I came up with this function. It works, but I was wondering if the math could be optimized/simplified, or if this is the way to go. It's for a mobile game.

public static inline var ISO:Float = 0.45378560551; // (116-90) / 180 * PI=;

function convert(angle:Float):Float
{
  angle -= Math.PI; // corrected angle

  var randomChoosenDistance = 2.0; // could be anything, I'm only interested in final angle.

  // calculate new line, using isometic angle.
  var x1 = Math.cos(angle - ISO);
  var x2 = Math.cos(angle - ISO) * randomChoosenDistance;
  var y1 = Math.sin(angle);
  var y2 = Math.sin(angle) * randomChoosenDistance;

  var dx = x1 - x2;
  var dy = y1 - y2;

  angle = Math.atan2(dy, dx);
  return angle;
}

Can I get the new angle, without calculating the angle between the temporary points I'm creating at the moment?

@neo pointed out that dx can be calculated as (1 - r) * Math.cos(a - b).  Similarly, dy = (1 - r) * Math.sin(a).  However, when you take dy / dx, which is what Math.atan2(dy, dx) implicitly does with its arguments, the (1 - r) factor cancels out.  You can also see, using a geometric argument, that (x2, y2) is pointless (pardon the pun).

Therefore, Math.atan2(y1, x1) would work just the same as Math.atan2(dy, dx).

So far, your function can be simplified to the following.  Since you repurposed angle twice, I need to disambiguate them as alpha, beta, and theta for this discussion.

public static inline var ISO:Float = 0.45378560551; // (116-90) / 180 * PI=;

function convert(alpha:Float):Float
{
  var beta = alpha - Math.PI; // corrected angle

  // calculate new line, using isometic angle.
  var x1 = Math.cos(beta - ISO);
  var y1 = Math.sin(beta);

  var theta = Math.atan2(y1, x1);
  return theta;
}

But wait, there's more!  There's a mysterious correction from alpha to beta, and the cosine expression is complicated.

Let's start with Math.sin(beta).  That's Math.sin(-alpha), or -Math.sin(alpha).

It would be nice to say Math.atan2(Math.sin(alpha), something) instead of Math.atan2(-Math.sin(alpha), something).  Let's move the negation into the denominator then, for var theta = Math.atan2(Math.sin(alpha), -x1).

Can we simplify -x1?

-x1 = -Math.cos(beta - ISO)
    = Math.cos(beta - ISO + 180°)
    = Math.cos(alpha - 180° - ISO + 180°)
    = Math.cos(alpha - ISO)

So, your function simplifies to:

public static inline var ISO:Float = 0.45378560551; // = (116-90) / 180 * PI

function convert(angle:Float):Float
{
    return Math.atan2(Math.sin(angle), Math.cos(angle - ISO));
}

Not only is the code more efficient, it's also less mysterious: the transformation is taking the x-coordinate of each point as if it were rotated 26° clockwise!

I have to question the motivation behind this function, though.  This isn't an angle-preserving transformation, so why are you operating on an angle?  Normally, you transform points' coordinates using matrix multiplication.


Check if a binary tree is a binary search tree Given a binary tree, I need to check if it satisfies the binary search tree property i.e. left child is less than or equal to the current node and right child is greater than or equal to the current node.

from collections import namedtuple

Node = namedtuple('Node', 'key, left, right')

def is_bst(node, key_min=None, key_max=None):
    """Checks is the subtree rooted at this node is a BST."""
    if node is None:
        return True # By definiton
    if key_max is not None and node.key >= key_max:
        return False
    if key_min is not None and  node.key <= key_min:
        return False
    return is_bst(node.left, None, node.key) and is_bst(node.right, node.key, None)

import unittest

class TestBst(unittest.TestCase):
    def test_none(self):
        self.assertTrue(is_bst(None))

    def test_left_skewed(self):
        root = Node(8, Node(3, Node(1, None, None), None), None)
        self.assertTrue(is_bst(root))

    def test_right_skewed(self):
        root = Node(8, None, Node(10, None, Node(14, None, None)))
        self.assertTrue(is_bst(root))

    def test_bst(self):
        root = Node(8, Node(3, None, None), Node(10, None, None))
        self.assertTrue(is_bst(root))

    def test_non_bst(self):
        root = Node(8, Node(10, None, None), Node(6, None, None))
        self.assertFalse(is_bst(root))

if __name__ == '__main__':
    unittest.main()

I want to know if I can convert it to an iterative version. If yes, then how?

Time complexity

The algorithm visits each node exactly once, hence the complexity should be \$\Theta(n)\$, where \$n\$ is the number of node in the tree.

I think one of reasons that this question is on the verge of getting closed, is that the code presented doesn't make much sense combined with a question of iterative version. But let us come to back that.

Code and style review

Good naming of methods and variables – You are trying to follow the PEP8 and that is good
Combine similar if statements – When you have similar if condition, and they do the same, then combine them using the or operator
Use \ (line continuation) or parenthesis to break long if conditions – This is a matter of personal preferences, and one area where I'm not sure what I prefer to do in all cases. But if you have a long if condition, you can either break it using a single \ at the end of the line, or add the otherwise unneccessary parenthesis around the entire condition.
Add vertical space between non-similar if statements – In some places it can be good to introduce a little vertical space. This is especially true with following if/elif/else blocks. If not vertically spaced, one might wonder if they should have been using elif or else instead. And if they really are meant to be separate if statements, then separate them using extra newlines. This enhance readability, and code understanding
(optional) Shorten None conditions – Consider whether you want to test directly on not node or if you want to use the more verbose node is None. In most cases they are identical, but I tend to prefer the not node version my self.
Bug: Not keeping the key_min and key_max whilst traversing tree – Your code sets these to None, which would introduce errors if you have a tree with a little more levels then your test case, and you first traverse a left tree, and then go down the right tree. That value could now be higher than the root value, and you'll be happy with that. So extend test cases, and then afterwards correct the bug.

With these options implemented you'll get the following (untested) code:

def is_bst(node, key_min=None, key_max=None):
    """Checks is the subtree rooted at this node is a BST."""
    if node is None:
        return True

    if (not key_max and node.key >= key_max) \
        or (not key_min and node.key <= key_min):

        return False

    return is_bst(node.left, key_min, node.key) \
           and is_bst(node.right, node.key, key_max)

Algorithmic comments

Your implementation is good, and should do what it is supposed, with correction of bug, and does so in a recursive manner. You can't do it entirely iterative as retrieving the values of a tree is not an iterative procedure.

However, you do have the option of creating/returning/yielding something which you can use in an iterative manner. Either iterating over your returned list from inorder(), or by implementing an iterative version of the traversal, as I've done in my answer for values_in_order().

If having some you can iterate over you could do either of the following variants:

# First variant: Manually keeping the previous_node
previous_node = None
for node in inorder(root):
    if previous_node and previous_node.key > node.key:
        return False
    previous_node = node

# Second variant: Using the itertools.pairwise iterator
return any(prev.key > node.key for prev, node in pairwise(inorder(root)))

The last one depending on the itertools.pairwise recipe, which returns all pairs of nodes from the start through end.

Do however note, that neither of these methods are tested for efficiency or speed, but are mostly presented as alternate methods on how to do the check whether all the values are in sorted order which seems to the criteria for a binary search tree.