Maximum possibility of banknotes to keep into the given integer value Banknotes are given specifically 100, 50, 20, 10, 5, 2, 1. Required to check and find out: What is the maximum possibility to keep these individually into the given integer value.
For example:
input: 576
output: 576
        5 nota(s) de R$ 100,00
        1 nota(s) de R$ 50,00
        1 nota(s) de R$ 20,00
        0 nota(s) de R$ 10,00
        1 nota(s) de R$ 5,00
        0 nota(s) de R$ 2,00
        1 nota(s) de R$ 1,00

I have solved this problem by following this strategy->
#include <stdio.h>
int main(){

    int number, count = 0, occupied = 0, number_update, i;
    int a = 100;
    scanf("%d",&number);
    number_update = number;
    printf("%d\n",number);

    //Getting the number that can be stored maximum in the given integer.
    for (i = 0; i < 7; i++){
        count = 0;

        while (a <= number_update){
        count++;
        occupied = occupied + a;
        number_update = number_update - a;
        }
        printf("%d nota(s) de R$ %d,00\n",count,a);
        number_update = number - occupied;

        if (i == 0){
         a = a-50;

        }

        else if (i == 1){
            a = a-30;
        }

        else if (i == 2){
            a = a-10;
        }

        else if (i == 3){
            a = a-5;
        }

        else if (i == 4){
            a = a-3;
        }

        else {
            a = a-1;
        }

       }
        return 0;
}

I am here for:
Would you propose any simplification from my existing solution?
Will I face any problem with some conditions?
The way I followed to solve this problem. Is it ok or not?

Don't use the for-case antipattern
You are using the for-case antipattern, which you should almost always avoid. Instead, find some way to simplify your code. For example, instead of hardcoding the denominations in the if-statements, put them in an array:
static const int denominations[] = {100, 50, 20, 10, 5, 2, 1};
static const int n_denominations = sizeof denominations / sizeof *denominations;

Then use it like so:
for (int i = 0; i < n_denominations; i++) {
    count = 0;

    while (number >= denominations[i]) {
        count++;
        number -= denominations[i];
    }

    printf("%d nota(s) de R$ %d,00\n", count, denominations[i]);
}


Finding the postion of a substring in a string I am given two strings and return the position if in the larger string the smaller string is found.  If not I am returning -1 for position.  Any suggestions for improving my code.

using System;
using System.Collections.Generic;
using System.Linq;

namespace Reverse
{
    class MainClass
    {
        static int IsFound (string stringOne, string stringTwo){
            int j = 0;
            int position = -1;
            int length = stringTwo.Length - stringOne.Length;
            for (int i = 0; i < length; i++) {
                if (stringOne [j] == stringTwo [i]) {
                    position = i;
                    while (stringOne [j] == stringTwo [i]) {
                        j++;
                        i++;
                        if (stringOne.Length == j) {
                            return position;
                        }
                    }
                    j = 0;
                    position = -1;
                }
            }
            return position;
        }
        public static void Main (string[] args)
        {
            string stringOne = "charter";
            string stringTwo = "cstarchabsccharldmscharterfadgafga";
            int test = IsFound (stringOne, stringTwo);
            Console.WriteLine(test);
            string stringThree = "atfc";
            test = IsFound (stringThree, stringTwo);
            Console.WriteLine(test);
        }
    }
}

Small improvement here:

Your IsFound method is making an assumption that stringTwo is longer than stringOne. Might I suggest:

Math.Abs(stringOne.Length - stringTwo.Length)

This way length will never be negative and the for loop will always execute.


Database input script Is this code secure and valid?  If so, can it be improved?

<?php
        // conect to the data base
        $mysqli =  new mysqli('localhost','user','pass','dbname');
         if($mysqli->connect_errno >0){
            die( "problem with the connection");
        }

        // creat a function to get table information
        function fetchTableFeildNames($mysqli , $databaseName , $tableName ){
            $sql = "SELECT `COLUMN_NAME` FROM `INFORMATION_SCHEMA`.`COLUMNS` WHERE `TABLE_SCHEMA`= '$databaseName' AND `TABLE_NAME`= '$tableName' ";
     if(!$stmt =$mysqli->prepare($sql)){
          die("There is an error with mysql preperation staetment".$mysqli->error);
        }
     if (!$stmt->execute()) {
          echo "Execute failed: (" . $stmt->errno . ") " . $stmt->error;
         }
      $res = $stmt->get_result();
      //looping throught the result
       while ($array = $res->fetch_array()){
           $newArray[] =$array;
           }
        return $newArray;
        }

    //insert function
    function insert($mysqli,$tableName,$databaseName,$array){
        //fetching table columns information
        $tableColumns      = fetchTableFeildNames($mysqli , $databaseName , $tableName );
        // geeting table columns count
        $tableColumnsCount = count($tableColumns);
        //geting data array count
        $dataArrayCount    = count($array);
        //see if the table columns count dosenot mutch array data count we kill the script
        if  ($tableColumnsCount != $dataArrayCount){
            die("Data array dosenot mutch table columns count");
        }
        //forming the query depending on input array at same time we are geting data typs for the data array
        $sql = "INSERT INTO `{$databaseName}`.`{$tableName}` VALUES (";
        for($i=0;$i<$dataArrayCount;$i++){
            $string         =  gettype($array[$i]);
            //takeing the first litter example string= s ,integer =1
            $a_param_type[] =  $string[0];
            $sql .= "?,";
            }
            //remove the last char ","
            $sql  = rtrim($sql , ",");
            $sql .= ")";

            $a_params   = array();
            $param_type = '';
            $n          = count($a_param_type);
            //merging all types in one string
            for($i = 0; $i < $n; $i++) {
                $param_type .= $a_param_type[$i];
            }
            //passing the types to the array by refrence
           $a_params[]  = & $param_type;
           //passing the data array  by refrence
           for($i = 0; $i < $n; $i++) {
             $a_params[] = & $array[$i];
            }
             //preperaing the statment
           if(!$stmt = $mysqli->prepare($sql)){
                die("error in preperation " . $mysqli->error);
            }
           //using   call_user_func_array() because mysqli::bind dont work with dynamic parameters
           call_user_func_array(array($stmt, 'bind_param'), $a_params);

           //executing the query
           if(!$stmt->execute()){
                die("error in executeing the query".$mysqli->error);

           }
}

        $array = array ("","catName" ,"catDesecription","","","","");

        insert($mysqli,"items","yaztor",$array);
?>

There are some formatting and spelling issues.  I fixed those (somewhat arbitrarily changing to my style), removed redundant comments, and made a few other changes:

<?php
  function fetchTableFieldCount($mysqli , $databaseName , $tableName ) {
    $sql = "SELECT COUNT(`COLUMN_NAME`) AS FieldCount FROM `INFORMATION_SCHEMA`.`COLUMNS` WHERE `TABLE_SCHEMA`= ?  AND `TABLE_NAME`= ? ";

    if ( ! ( $stmt = $mysqli->prepare($sql) ) ) {
      die("There is an error with mysql preparation statement".$mysqli->error);
    }

    $stmt->bind_param($databaseName, $tableName);

    if ( ! $stmt->execute() ) {
      echo "Execute failed: (" . $stmt->errno . ") " . $stmt->error;
    }

    $res = $stmt->get_result();

    if ( $row = $res->fetch_array() ) {
      return $row['FieldCount'];
    }

    return 0;
  }

  function insert($mysqli, $tableName, $databaseName, $array) {
    //see if the table columns count does not match array data count we kill the script
    $tableColumnsCount = fetchTableFieldCount($mysqli, $databaseName, $tableName);
    $dataArrayCount    = count($array);
    if ( $tableColumnsCount != $dataArrayCount ) {
      die("Data array does not match table columns count");
    }

    if ( 0 == $dataArrayCount ) {
      return;
    }

    $sql = "INSERT INTO `{$databaseName}`.`{$tableName}` VALUES (";
    $sql .= implode(', ', array_fill(0, $dataArrayCount, '?'));
    $sql .= ')';

    for ( $i = 0; $i < $dataArrayCount; $i++ ) {
      $string         =  gettype($array[$i]);
      //taking the first letter; example:  string = s, integer = i
      $a_param_type[] =  $string[0];
    }

    if ( ! $stmt = $mysqli->prepare($sql) ) {
      die("error in preparation " . $mysqli->error);
    }

    // make the first element of the array a string representing the types
    // of the values of the rest of the array
    array_unshift($array, implode($a_param_type));
    // use call_user_func_array to pass the variable number
    // of elements in the $array as arguments
    call_user_func_array(array($stmt, 'bind_param'), $array);

    if ( ! $stmt->execute() ) {
      die("Error in executing the query:  " . $mysqli->error);
    }
  }

  $mysqli =  new mysqli('localhost','user','pass','dbname');
  if ( $mysqli->connect_errno > 0 ){
    die( "problem with the connection");
  }

  $array = array("", "catName" ,"catDescription", "", "", "", "");
  insert($mysqli, "items", "yaztor", $array);

First, I moved the connection info from the top to the bottom.  This puts all the commands outside of a function together.  Normally we'd move the functions into a different file and require it, but I skipped that here.

Next, I removed the first redundant comment.  A comment like "creat [sic] a function to get table information" doesn't tell me anything that function fetchTableFeildNames [sic] doesn't already tell me.  Save comments for explaining why you chose to do something.  That's what you'll forget before you have to go back to the code.

I changed the first function to get just the column count, as that's all we're using.  No need to ship the column names across the wire.

I changed $array to $row.  Row describes what the variable holds.  Array doesn't differentiate it from any other array.  I dropped $newArray entirely, but if needed, I would have called it $columnNames.

I added a check for an array with no elements.  This avoids a failed SQL query in that case.

I took the $sql concatenation out of the for loop.  This allows for the more efficient implode rather than a series of string concatenations.

I took out the extraneous for loop that copied the array.  The $array variable itself is a copy, so I just used that.  I also took out the manual splice in favor of array_unshift.  I would expect a built-in function to be faster than a for loop in PHP.

I didn't remove your column count check, but I don't know that it's necessary.  It removes one thing that can go wrong but leaves others.  It's not like you recover from the mistake.  It ends the program.  The same thing would happen with an invalid INSERT to the database.  I don't know that you gain much.

The code that you show here is reasonably safe and secure.  However, it was not reliably so.  It relied on the caller to secure $databaseName and $tableName.  If a caller bases either of those on user input, there's risk.


Cardinal direction enum from range input Still learning JS and I made the following function to convert degrees into cardinal directions respectively. My first iteration of the code was over 50 lines and I was able to get it down to 13 using a for loop.

Is there a way to optimize or simplify the code down even more? Is my logic and implementation ideal?

https://codepen.io/bbbenji/pen/JjPGNmY

function setCardinalDirection() {
  value = document.querySelector('#orientation').value // Get current value from range input
  document.querySelector(".degrees").textContent = value // Inject current input value into label
  direction = document.querySelector(".direction") // Define intercardinal direction display element

  degrees = 22.5 // Define range between two intercardinal directions
  cardinal = ["N", "NNE", "NE", "ENE", "E", "ESE", "SE", "SSE", "S", "SSW", "SW", "WSW", "W", "WNW", "NW", "NNW", "N"]

  for ( i = cardinal.length; i >= 0; i-- ) { // Iterate through cardinal array backwards
    if ( value < degrees/2 + degrees * i ) {
      cardinalOut = cardinal[i]
    }
  }

  direction.textContent = cardinalOut // Inject current cardinal value into label
}

document.querySelector("#orientation").addEventListener('input', function(event) {
 setCardinalDirection()
})

window.addEventListener("load", function(){
 setCardinalDirection()
})
<form>

    Orientation: ° ()

  <input id="orientation" type="range" min="0" max="360" step="1" value="145">
</form>

Evaluation

For somebody learning Javascript this code isn't bad, however it does have some inefficiencies and practices that are frowned upon (e.g. Global variables). See the suggestions below for advice about improving it.

Suggestions

Semicolon terminators

Unless you are intimately familiar with the statements that need to be terminated by a semicolon it is wise to always terminate each line with a semi-colon.

Selecting elements by class vs id attribute

The first line selects an element from the DOM by the id attribute - i.e. document.querySelector('#orientation'). The next line uses a class selector to select an element: document.querySelector(".degrees"). If there is only one element with class name degrees that matters for this application then an id attribute would be more appropriate than class.

Also, it isn't wrong to use document.querySelector() to get elements by id but using document.getElementById() "is definitely faster" 1 (see this jsPerf test for comparison). Similarly document.getElementsByClassName() is also faster. I would suggest only using document.querySelector() when there is a complex selector (e.g. form lable[for="direction']

Global variables

Any variable not declared with a keyword like var, const or let is considered global. For a small application like this it likely wouldn't lead to any issues but in a larger application it could lead to unintentional side-effects if the same name is used in different functions.

Excess closures

The lines to add event listeners can be simplified:

document.querySelector("#orientation").addEventListener('input', function(event) {
  setCardinalDirection()
})

window.addEventListener("load", function(){
 setCardinalDirection()
})

Instead of wrapping the calls to setCardinalDirection() in an extra function, just pass the name of the function:

document.querySelector("#orientation").addEventListener('input', setCardinalDirection);
window.addEventListener("load", setCardinalDirection);


Collections.Counter in Python3 Can you suggest how I might make the following code more efficient:

from collections import Counter

a = [1,1,3,3,1,3,2,1,4,1,6,6]

c = Counter(a)

length = len(set(c.values()))

normalisedValueCount = {}
previousCount = 0
i = 0
for key in sorted(c, reverse=True):
    count = c[key]

    if not previousCount == count:
        i = i+1
        previousCount = count

    normalisedValueCount[key] = i/length

print(normalisedValueCount)

It basically gives a dictionary similar to counter, but instead of counting the number of occurrences, it contains a weighting based on the number of occurrences.

The number 1 is associated with 1.0 (4/length) because it occurs the most often.
2 and 4 occur the least often and are associated with the value 1/length.
6 is the second least occurring value and is associated with 2/length.
3 is the third least occurring value and is associated with 3/length.

Some more examples:

The list a[1,2,3] results in a normalisedValueCount of 1:1.0, 2:1.0, 3:1.0.
The list a[2,1,2] results in a normalisedValueCount of 2:1.0, 1:0.5.
The list a[2,1,2,3] results in a normalisedValueCount of 2:1.0, 1:0.5, 3:0.5.
The list a[2,2,2,2,2,2,2,2,2,2,1,2,3,3] results in a normalisedValueCount of 2:1.0, 3:0.66666, 1:0.33333.

I have no idea what you code is doing, it doesn't look like any normalization I've seen. I'd offer a more specific suggestion on restructuring if I understood what you are doing.

You:

Put a in a counter
Put that counter's values into a set
sort the keys of the counter

I'd look for another approach that doesn't involve so much moving around.

if not previousCount == count:

better as

if previousCount != count:

EDIT

Counter.most_common returns what you are fetching using sorted(c, reverse=True)
itertools.groupby allows you group together common elements nicely (such as the same count)
enumerate can be used to count over the elements in a list rather then keeping track of the counter

My code:

c = Counter(a)
length = len(set(c.values()))
counter_groups = itertools.groupby(c.most_common(), key = lambda x: x[1]))
normalisedValueCount = {}
for i, (group_key, items) in enumerate(counter_groups)
    for key, count in items:
        normalisedValueCount[key] = (i+1)/length


JavaScript Tic Tac Toe - functional but could be DRYer I've been working on a simple Tic Tac Toe game in JavaScript.  It runs the way it should, but the Board.checkWin function (check every possible winning combo) could be better, it is very repetitive in its current state.  Any ideas?

Here is my Cell class:

var Cell = function() {
this.value = "|_|";
};

Cell.prototype.setState = function(arg) {
    this.value = arg;
};

Cell.prototype.playable = function() {
    if (this.value == "|_|") {
        return true;
    } else {
        return false;
    }
};

Cell.prototype.render = function() {
    return this.value;
};

Here is my Board class:

var Board = function() {
    this.cells = [];
    this.currentPlayers = ["|X|","|O|","|X|","|O|","|X|","|O|","|X|", "|O|", "|X|"];
    this.winner = this.currentPlayers[1];
};

Board.prototype.startGame = function() {
    for (i = 0; i < 9; i++) {
        this.cells.push(new Cell);
    }
    return this.render();
};

Board.prototype.makePlay = function(position) {
    if (this.checkWin() == false) {

    if (position < this.cells.length) {
        var cell = this.cells[position];
        var currentPlayer = this.currentPlayers.shift();

        if (cell.playable() == true) {
        cell.setState(currentPlayer);
        }
    }
    return this.render();
    }
};

Board.prototype.checkWin = function() {

for(var i = 0; i < this.cells.length; i++)
    if (this.cells[0].value == "|X|" && this.cells[1].value == "|X|" && this.cells[2].value == "|X|"    ||
        this.cells[0].value == "|O|" && this.cells[1].value == "|O|" && this.cells[2].value == "|O|" ||

        this.cells[3].value == "|X|" && this.cells[4].value == "|X|" && this.cells[5].value == "|X|" ||
        this.cells[3].value == "|O|" && this.cells[4].value == "|O|" && this.cells[5].value == "|O|" ||

        this.cells[6].value == "|X|" && this.cells[7].value == "|X|" && this.cells[8].value == "|X|" ||
        this.cells[6].value == "|O|" && this.cells[7].value == "|O|" && this.cells[8].value == "|O|" ||

        this.cells[0].value == "|X|" && this.cells[3].value == "|X|" && this.cells[6].value == "|X|" ||
        this.cells[0].value == "|O|" && this.cells[3].value == "|O|" && this.cells[6].value == "|O|" ||

        this.cells[1].value == "|X|" && this.cells[4].value == "|X|" && this.cells[7].value == "|X|" ||
        this.cells[1].value == "|O|" && this.cells[4].value == "|O|" && this.cells[7].value == "|O|" ||

        this.cells[2].value == "|X|" && this.cells[6].value == "|X|" && this.cells[8].value == "|X|" ||
        this.cells[2].value == "|O|" && this.cells[6].value == "|O|" && this.cells[8].value == "|O|" ||

        this.cells[0].value == "|X|" && this.cells[4].value == "|X|" && this.cells[8].value == "|X|" ||
        this.cells[0].value == "|O|" && this.cells[4].value == "|O|" && this.cells[8].value == "|O|" ||

        this.cells[6].value == "|X|" && this.cells[4].value == "|X|" && this.cells[2].value == "|X|" ||
        this.cells[6].value == "|O|" && this.cells[4].value == "|O|" && this.cells[2].value == "|O|")

        {
            return true;
        } else {
            return false;
    }
};

Board.prototype.render = function() {
    var boardArray = [];
    for (var i = 0; i < this.cells.length; i++) {
        boardArray.push(this.cells[i].value);
    }
    for (var j = 0; j < boardArray.length; j++) {
        var boardString = "TIC TAC TOE " + boardArray[0] + boardArray[1] + boardArray[2] + "\n ";
        boardString += " PICK A BOX " + boardArray[3] + boardArray[4] + boardArray[5] + " \n ";
        boardString += "   0 THRU 8 " + boardArray[6] + boardArray[7] + boardArray[8];
    }

    if (this.checkWin() == true) {
        return boardString + "\n   WE HAVE A WINNER!";
    } else {
    return boardString;
    }
};

The most un-DRY code is checking if someone has won. You could add the following code to help you

// Row class

var Row = function{
    this.cells = [];
}

Row.prototype.allSame = function(){
    return this.cells[0] == this.cells[1] && this.cells[0] == this.cells[1];
}

// Expand the Board class
Board.prototype.getRow = function(num) {
    var row = new Row();

    // Horizontal rows
    if (num => 0 && num < 3) {
        row.cells.push(this.cells[3*num], this.cells[(3*num)+1], this.cells[(3*num)+2]);
        return row;
    }

    // Vertical rows
    if (num => 3 && num < 6) {
        row.cells.push(this.cells[num-3], this.cells[(num], this.cells[num+3]);
        return row;
    }

    // Diagonal rows
    if (num == 6) {
        row.cells.push(this.cells[0], this.cells[(5], this.cells[8]);
        return row;
    }

    if (num == 7) {
        row.cells.push(this.cells[2], this.cells[(5], this.cells[6]);
        return row;
    }

    return false;

};

Running on the assumtions that the first 3 horizontal rows are numbered from 0 to 2, the 3 columns from 3 to 5 and the 2 diagonals 6 and 7...

But I am not sure the amount of code you would save would be enough to make the change worth it, unless you are predicting adding bigger boards, beyond the classic 3x3.


code to check if number is prime number I have written a code to check if the number is prime number are not.

I pass the number from command line argument, and check if the number is prime.

I have written a function for it which returns the boolean value.

(function(){
    'use strict';

    main();

    function main(){
        var testNum = getCommandLineVariables();
        console.log("Is Number \""+testNum+"\" a prime number ? ",isPrimeNumber(testNum));
    }

    function isPrimeNumber(num){
        var halfNum = num/2;
        halfNum = parseInt(halfNum);

        // check if number is divisible by 2,
        // if yes then its not a prime number.
        if(num%2 === 0){
            return false;
        }

        do{
            // if number is divisible by its half value,
            // if yes, than its not prime number
            if(num%halfNum === 0){
                return false;
            }

        }while(--halfNum >= 2); // reduce the half-value by 1 and continue

        return true;
    } // end of isPrimeNumber

    // get the number from commandline argument
    function getCommandLineVariables(){
        var arg_array = process.argv.slice(2);

        if(arg_array.length !== 1){
            throw new Error("Pass a number");
        }

        return parseInt(arg_array[0]);
    } // end of getCommandLineVariables
})();

I run the program as follows,

E:\math>node isPrimeNum01.js 13
Is Number "13" a prime number ?  true

E:\math>node isPrimeNum01.js 16
Is Number "16" a prime number ?  false

E:\math>node isPrimeNum01.js 29
Is Number "29" a prime number ?  true

E:\math>node isPrimeNum01.js 113
Is Number "113" a prime number ?  true

E:\math>node isPrimeNum01.js 127
Is Number "127" a prime number ?  true

E:\math>node isPrimeNum01.js 566
Is Number "566" a prime number ?  false

E:\math>

Please review my code and leave your valuable comments. Also please check whether my logic for checking the prime number is efficient.

I will just be looking at your prime function.

You got the right idea, but you missed some edge cases, and it can also be optimized. 2 and 3 are prime but return false, since halfNum starts at 1, and do...while loops always run at least once. 1 is not prime but return true, along with some negative numbers. The easiest way to handle this is just to check for this special case at the start of the function.

if(n <= 1) return false;

You are checking for divisibility with all numbers between 2 and half the input, which is good. But it seems a bit confusing to me when you use a do...while loop and counting down, instead of just counting up with a good old for loop.

for(let i = 2; i <= halfNum; i++)

But we can optimize this, since we only need to check up to the square root of the number. For any divisible number larger than the square root, there will also be a divisible number lower than the square root. For example, 100 is divisible by 20, but also 5, since 20*5=100. I'm also going to pre-calculate it and rename halfNum as that name doesn't fit anymore.

const limit = Math.sqrt(num);

The final function looks like this.

function isPrime(num) {
    if(num <= 1) return false;
    const limit = Math.floor(Math.sqrt(num));
    for(let i = 2; i <= limit; i++) {
        if(num % i === 0) return false;
    }
    return true;
}


Checking if a list of coordinates defines a closed, exact rectangle I have to determine if an N sized list of coordinates defines a closed, exact rectangle.

I start with the question "How do I check if a list of coordinates is in the shape of a rectangle?"

The answer is:

First and last element in cordX and cordY have to be equal.
Difference between another key should be equal to 1.
Value for key <1:size-2> has to be const.

Could you tell me if this is elegant and efficient code?

public class BoardPosition {
        private int x;
        private int y;

        public int getX() {
                return x;
        }

        void setX(int x) {
                this.x = x;
        }

        public int getY() {
                return y;
        }

        void setY(int y) {
                this.y = y;
        }

        public BoardPosition(int pX, int pY) {
                y = pY;
                x = pX;
        }
}

private boolean checkifIsRectangle(ArrayList<BoardPosition> lettersPosition) {
        TreeMap<Integer, Integer> cordsX = new TreeMap<Integer, Integer>();
        TreeMap<Integer, Integer> cordsY = new TreeMap<Integer, Integer>();

        for (BoardPosition b : lettersPosition) {
            if (!cordsX.containsKey(b.getX())) cordsX.put(b.getX(), 1);
            else {
                cordsX.put(b.getX(), cordsX.get(b.getX()) + 1);
            }

            if (!cordsY.containsKey(b.getY())) cordsY.put(b.getY(), 1);
            else {
                cordsY.put(b.getY(), cordsY.get(b.getY()) + 1);
            }
        }
        boolean xIsValid = true;
        boolean yIsValid = true;

        //1
        if (cordsX.values().toArray()[0] != cordsX.values().toArray()[cordsX.values().toArray().length - 1])
            xIsValid = false;
        if (cordsY.values().toArray()[0] != cordsY.values().toArray()[cordsY.values().toArray().length - 1])
            yIsValid = false;

        //2
        int xKeyTest = -1;
        for (Integer i : cordsX.keySet()) {
            if (xKeyTest == -1) {
                xKeyTest = i;
                continue;
            }
            if ((i - xKeyTest) != 1) {
                xIsValid = false;
                break;
            }
            xKeyTest = i;
        }

        int yKeyTest = -1;
        for (Integer i : cordsY.keySet()) {
            if (yKeyTest == -1) {
                yKeyTest = i;
                continue;
            }
            if ((i - yKeyTest) != 1) {
                yIsValid = false;
                break;
            }
            yKeyTest = i;
        }

        //3
        ArrayList<Integer> xValueArray = new ArrayList<Integer>(cordsX.values());
        int xTestValue = -1;
        for (int i = 1; i < xValueArray.size() - 2; i++) {
            if (xTestValue == -1) {
                xTestValue = xValueArray.get(i);
                continue;
            }
            if (xTestValue != xValueArray.get(i)) {
                xIsValid = false;
                break;
            }
        }

        ArrayList<Integer> yValueArray = new ArrayList<Integer>(cordsY.values());
        int yTestValue = -1;
        for (int i = 1; i < yValueArray.size() - 2; i++) {
            if (yTestValue == -1) {
                yTestValue = yValueArray.get(i);
                continue;
            }
            if (yTestValue != yValueArray.get(i)) {
                yIsValid = false;
                break;
            }
        }

        return (xIsValid && yIsValid);
    }

Proposed solution

Okay BoardPosition should be an immutable data structure like this:

public class BoardPosition {
        public final int x;
        public final int y;

        public BoardPosition(int pX, int pY) {
                y = pY;
                x = pX;
        }
}

You should never specify which specific implementation of List you want but rather that you want a "list" or even a "collection" of positions. See Liskov Substitution Principle.
This is better:

private boolean checkifIsRectangle(List<BoardPosition> lettersPosition) {

This can be done better:

        for (BoardPosition b : lettersPosition) {
            if (!cordsX.containsKey(b.getX())) cordsX.put(b.getX(), 1);
            else {
                cordsX.put(b.getX(), cordsX.get(b.getX()) + 1);
            }

            if (!cordsY.containsKey(b.getY())) cordsY.put(b.getY(), 1);
            else {
                cordsY.put(b.getY(), cordsY.get(b.getY()) + 1);
            }
        }

By avoiding repeated look ups in the tree map, like this:

        for (BoardPosition b : lettersPosition) {
            Integer x = cordsX.get(b.x);
            x = (x == null) ? 1 : x + 1;
            cordsX.put(b.x, x);

            Integer y = cordsY.get(b.y);
            y = (y == null) ? 1 : y + 1;
            cordsY.put(b.y, y);
        }

Around //1 you're creating many unnecessary copies of the values. Consider calling toArray() only once.

This:

        //2
        int xKeyTest = -1;
        for (Integer i : cordsX.keySet()) {
            if (xKeyTest == -1) {
                xKeyTest = i;
                continue;
            }
            if ((i - xKeyTest) != 1) {
                xIsValid = false;
                break;
            }
            xKeyTest = i;
        }

Should be:

        //2
        int xKeyTest = -1;
        for (Integer i : cordsX.keySet()) {
            if (xKeyTest != -1 && (i - xKeyTest) != 1) {
                xIsValid = false;
                break;
            }
            xKeyTest = i;
        }

And similar for Y.

And finally //3 is rather clumsy.

        //3
        ArrayList<Integer> xValueArray = new ArrayList<Integer>(cordsX.values());
        int xTestValue = -1;
        for (int i = 1; i < xValueArray.size() - 2; i++) {
            if (xTestValue == -1) {
                xTestValue = xValueArray.get(i);
                continue;
            }
            if (xTestValue != xValueArray.get(i)) {
                xIsValid = false;
                break;
            }
        }

        ArrayList<Integer> yValueArray = new ArrayList<Integer>(cordsY.values());
        int yTestValue = -1;
        for (int i = 1; i < yValueArray.size() - 2; i++) {
            if (yTestValue == -1) {
                yTestValue = yValueArray.get(i);
                continue;
            }
            if (yTestValue != yValueArray.get(i)) {
                yIsValid = false;
                break;
            }
        }

        return (xIsValid && yIsValid);
    }

If you want to make sure all list entries have the same value, do like this:

    boolean isSame(List<Integer> list){
        Integer first = list.get(0);
        for (int i = 1; i < list.size(); i++) {
            if (first != list.get(i)) {
                return false;
            }
        }
        return true;
    }

In the end your algorithm is \$\mathcal{O}(n\log n)\$ although there is a lot of iteration going on and I would say it's on the slow side of the spectrum.

A better solution

A faster way of doing this would be:

package test;

import java.util.Collections;
import java.util.Comparator;
import java.util.Iterator;
import java.util.List;

public class CheckRectangle{

   public static class Point{
      final int x;
      final int y;

      Point(int aX, int aY){
         x = aX;
         y = aY;
      }
   }

   private static class ByX implements Comparator<Point>{
      @Override
      public int compare(Point o1, Point o2){
         int v = Integer.compare(o1.x, o2.x);
         if( v == 0 )
            return Integer.compare(o1.y, o2.y);
         return v;
      }
   }

   static boolean isRectangle(List<Point> aPoints){
      Collections.sort(aPoints, new ByX());

      int ymax = Integer.MIN_VALUE;
      int ymin = Integer.MAX_VALUE;

      {
         Iterator<Point> it = aPoints.iterator();
         Point prev = null;
         while( it.hasNext() ){
            Point next = it.next();
            if( prev != null && prev.x == next.x && prev.y == next.y )
               it.remove();
            prev = next;

            ymax = Math.max(ymax, prev.y);
            ymin = Math.min(ymin, prev.y);
         }
      }

      int xmax = aPoints.get(aPoints.size() - 1).x;
      int xmin = aPoints.get(0).x;

      if( xmax == xmin ){ // Special case the vertical stick
         return (ymax - ymin + 1) == aPoints.size();
      }

      if( ymax == ymin ){ // Special case the horizontal stick
         return (xmax - xmin + 1) == aPoints.size();
      }

      // Number of (non-overlapping) grids in either direction
      int w = (xmax - xmin + 1);
      int h = (ymax - ymin - 1);

      int numGrids = 2 * h + 2 * w;
      if( aPoints.size() != numGrids )
         return false; // Wrong number of points

      // For a rectangle, there must be exactly h+2 points with x == xmin
      if( aPoints.get(h + 1).x != xmin )
         return false;

      // And exactly h+2 points with x == xmax
      if( aPoints.get(numGrids - (h + 2)).x != xmax )
         return false;

      return true;
   }
}

This performs in \$\mathcal{O}(n\log n)\$ as well, but it has much less iteration and memory accesses.

The key here is to sort by 'x' coordinate (you can do it by 'y' too). Due to the sort the list structure is very specific for rectangles and you just need to check a few specific values in the list to be sure that you have a rectangle.

If you don't believe the correctness of my solution, you can execute the unit tests available here: https://gist.github.com/EmilyBjoerk/4f015a7e1b394a73e4cc .


Relationship between notes and tags I have 3 objects and 3 models. The note, have many tags, and every tags must have one note. So, I have a note_tag_list to store the tags and notes relationship.

The program creates a note, with some tags.

First, I create a note object, and put it into the model. And I get back the object from the model. I create the tag if it doesn't exist. Then, I read back the tag object from the DB. And use all the tags and note to create new note_tag_list object, and put this object to the model.

But I think it seems very complex for my flow. Any ways to do it more elegant? Thank you.

//load all the object
$this->load->library('obj_note');
$this->load->library('obj_tag');
$this->load->library('obj_note_tag_list');

//load all the model
$this->load->model('note_model');
$this->load->model('tag_model');
$this->load->model('note_tag_list_model');

//create a note object
$aNewUserNote    =new obj_note();

//set the object information
$user_id         =get_current_user_id();
$title           =$this->input->post('ui_title_input');
$content_url     =$this->input->post('ui_url_input');
$content         =$this->input->post('ui_textarea');
$right           =RIGHT_NOTE_PUBLIC;
$status          =STATUS_NOTE_ACTIVE;

$aNewUserNote->set_user_id($user_id);
$aNewUserNote->set_title($title);
$aNewUserNote->set_content_url($content_url);
$aNewUserNote->set_content($content);
$aNewUserNote->set_right($right);
$aNewUserNote->set_status($status);

$aNewlyCreateNoteId = 0;

if(!$this->note_model->create_note_by_note($aNewUserNote, $aNewlyCreateNoteId))
{
    //create success
    return;
}

//get back the object from the model
$aNewlyCreateObj = $this->note_model->read_note_by_note_id($aNewlyCreateNoteId);

$aTagNameString = $this->input->post('ui_tag');
$tagsStringArray = $this->convert_tags_string_to_lower_case_string_array($aTagNameString);

$aTagArray = array();
$createNewTagIsSuccess = FALSE;

//Create New Tag
for($i = 0; $i < count($tagsStringArray); $i++)
{
    $aTagObj = $this->tag_model->read_tag_by_tag_name($tagsStringArray[$i]);
    if($aTagObj == NULL)
    {
        $aNewTag =new obj_tag();
        $aNewTag->set_create_user_id($user_id);
        $aNewTag->set_tag_name($tagsStringArray[$i]);

        if(!$this->tag_model->create_tag_by_tag($aNewTag)) //create tag fail
        {
          $createNewTagIsSuccess = FALSE;
        }
    }
}

//Read all the tag
for($j = 0; $j < count($tagsStringArray); $j++)
{
    $aTagObj = $this->tag_model->read_tag_by_tag_name($tagsStringArray[$j]);
    $aTagArray[count($aTagArray)] = $aTagObj;
}

//Make the note, and tag have relationship
for($k = 0; $k < count($aTagArray); $k++)
{
    $theTagObj = $aTagArray[$k];

    $aNoteTagList = new obj_note_tag_list();
    $aNoteTagList->set_note_id($aNewlyCreateObj[0]->get_id());
    $aNoteTagList->set_tag_id($theTagObj[0]->get_id());

    $this->note_tag_list_model->create_note_tag_list_by_note_tag_list($aNoteTagList);
}

This:

//set the object information
$user_id         =get_current_user_id();
$title           =$this->input->post('ui_title_input');
$content_url     =$this->input->post('ui_url_input');
$content         =$this->input->post('ui_textarea');
$right           =RIGHT_NOTE_PUBLIC;
$status          =STATUS_NOTE_ACTIVE;

$aNewUserNote->set_user_id($user_id);
$aNewUserNote->set_title($title);
$aNewUserNote->set_content_url($content_url);
$aNewUserNote->set_content($content);
$aNewUserNote->set_right($right);
$aNewUserNote->set_status($status);

Should be rewritten as this

// create and populate note object
$note = new obj_note();
$note->set_user_id(get_current_user_id());
$note->set_title($this->input->post('ui_title_input'));
$note->set_content_url($this->input->post('ui_url_input'));
$note->set_content($this->input->post('ui_textarea'));
$note->set_right(RIGHT_NOTE_PUBLIC);
$note->set_status(STATUS_NOTE_ACTIVE);

Other than that, my comment on your question will explain the rest.


Extension method to get the Sunday date from current date based on weekOffset I want to write an extension method for getting Sunday Date based on weekOffset and current date. For example if weekOffset value is 1 that means next week sunday date and if weekOffset is 2 that means next to next week's Sunday date.
I have come up with below code
private static List<string> GetWeekRange(this DateTimeOffset date, int weekOffset = 0)
        {
            var sundayDate = new DateTimeOffset(date.Year, date.Month, date.Day, 0, 0, 0, date.Offset).AddDays(weekOffset * 7 - (int) new DateTimeOffset(date.Year, date.Month, date.Day, 0, 0, 0, date.Offset).DayOfWeek);
            var saturdayDate = new DateTimeOffset(weekStartDate.Year, weekStartDate.Month, weekStartDate.Day, 0, 0, 0, weekStartDate.Offset).AddDays(7).AddMilliseconds(-1);
            return new List<string> { weekStartDate.ToString("o"), weekEndDate.ToString("o") };
        }

I was wondering if there can be a better way to do this?

I think your code could be more succinctly and usefully (ie don't return string) written as:
    private static DateTimeOffset[] GetWeekRange(this DateTimeOffset d, int weekOffset = 0)
    {
        var weekStartDate = new DateTimeOffset(d.Date, d.Offset).AddDays(weekOffset * 7 - (int)d.DayOfWeek);
        var weekEndDate = weekStartDate.AddDays(7);
        return new [] { weekStartDate, weekEndDate };
    }

But then given that end is so easily calc'd from start, maybe just return start, forget the array


A struct for recording audio samples I am writing a program in c++ to record audio.
Here are my structure and code snippet.

typedef struct _recorder {
    const uint32_t SAMPLE_RATE = 44100;
    const uint32_t FRAME_STEP = SAMPLE_RATE / 100;
    const uint32_t STEP_BUF_SIZE = sizeof(int16_t) * FRAME_STEP;
    int16_t *step_buf;

    const uint32_t RECORD_TIME = 10;
    const uint32_t NUM_SAMPLE = RECORD_TIME * SAMPLE_RATE;
    int16_t *total_buf;

    uint32_t sample_counter;
} recorder;

void recorder_init(recorder *r)
{
    r->step_buf = new int16_t[r->STEP_BUF_SIZE / sizeof(int16_t)];
    r->total_buf = new int16_t[r->NUM_SAMPLE];
}

void recorder_free(recorder *r)
{
    delete[] r->step_buf;
    delete[] r->total_buf;
}

void main(int argc, char *argv[])
{
    recorder rec;
    recorder_init(&rec);

    // Do something to record audio

    recorder_free(&rec);
}

Does using structures and functions make sense to you? Or should I use a class? Or there are better design patterns I can follow?

EDIT -------------------------
I add a further question of this post here.

This is very much C-style code: the only thing that qualifies it as C++ is the fact that you use new and delete[] rather than malloc() and free().

I don't see why RECORD_TIME should be a constant. I would expect it to be a constructor parameter. Or, use std::vector to store the samples, and you won't even have to worry about the array capacity, or even new and delete[].

The recorder class should be responsible for initializing sample_counter to zero, which you never did in your recorder_init(). In addition, I would expect there to be a method for appending a sample, and that method would automatically increment sample_counter. Again, though, if you used a std::vector, you wouldn't need such a complication, since the vector's size would serve that purpose.


Highlight input if empty This snippets works, but I think it can be optimized:

/* highlight input, if it is empty */
$(function() {
    var highlight_if_no_val=function () {
        $(this).toggleClass('highlight-input', !$(this).val());
    }
    $('#id_index-EditForm-gpnr').keyup(highlight_if_no_val).each(highlight_if_no_val);
}
);

Is there a simpler solution?

The .each(hightlight_...) is done to set the highlight after the page loaded (it can be initially empty or initially filled).

Your code looks good enough. "optimize" is a vague action.

1. /* highlight input, if it is empty */
2. jQuery(function ($) {
3.     function toggleHighlightIfEmpty() {
4.         var $this;
5.         $this = $(this);
6.         $this.toggleClass('highlight-input', !$this.val());
7.     }
8.     $('#id_index-EditForm-gpnr').on('keyup.highlight', toggleHighlightIfEmpty).trigger('keyup.highlight');
9. });

Line 2:

I'd use the aliasing form of document.ready to prevent conflicts with other libs

Line 3:

I'd use the function declaration form over variable declaration form for the internal function. It's really a personal preference sort-of-thing. Additionally, using camelCase is relatively standard for JavaScript function names.

Lines 4-6:

I'd store a reference to $this so that you don't keep calling the jQuery factory function. It shouldn't be significant in any way for performance, but it can save some headaches with perens which can be easily misplaced

Line 8:

I'd set the event using a namespaced event name so that it can be triggered and removed simply by calling $(...).off('keyup.highlight'). Instead of passing the function to each again, I prefer to use trigger. If you need backwards compatibility, use bind instead of on.

None of these "optimizations" should lead to code that's in any significant way different as far as performance is concerned. It really comes down to personal preference.


Create an object when you need it! in PHP Well Iam trying to learn OOP for a while now, My idea here is to create a simple CMS kind of thing.And I did something like this:

App Class:

namespace System;

class App {
    private $storage = [];
    private static $_instance;

    public static function getInstance()
    {

        if (is_null(self::$_instance)) {

            self::$_instance = new self();
        }
        return self::$_instance;
    }

    public function __get($key)
    {
        return $this->get($key);
    }

    public function get($key){

        if (!$this->has($key)){
            if ($this->isCore($key)){
                $this->share($key,$this->createNewObj($key));
            }else{
                die($key.' was not found!');
            }
        }
        return $this->storage[$key];

    }

    private function has($key)
    {
        return array_key_exists($key, $this->storage);
    }

    public function share($key,$value)
    {
        $this->storage[$key]= $value;

    }

    private function isCore($class)
    {
        $coreClasses = $this->coreClasses();
        return isset($coreClasses[$class]);
    }

    private function coreClasses()
    {
        return [

            'db'        => 'System\\Database',
            'user'      => 'System\\User'
        ];
    }

    private function createNewObj($key)
    {
        $coreClasses = $this->coreClasses();
        $opj = $coreClasses[$key];
        return new $opj($this);
    }

}

User Class: (Every class I write in this app will take an object from App using Dependency Injection).

namespace System;

class User
{
    private $app = null;

    public function __construct(App $app)
    {
        $this->app = $app;
    }

    public function getAllUsers()
    {
        return $this->app->db->select('* FROM users')->fetchAll();

    }

}

init.php

if (!session_id()){
    session_start();
}

define('ROOT',dirname(__DIR__));
define('DS', /);

require ROOT.DS.'System/Autoload.php';
\System\Loader::reg();

$app = \System\App::getInstance();

an now when I want to get something from the database I use this: in users.php

require 'init.php';
//get the object from storage if does exist!
$User = $app->user->getAllUsers();

The idea was to get the an object from User class on the fly. rather doing it like:

$User = new User($database);
$User->getAllUsers();

Am I doing this right, or is it a bad idea?

Your App class seem too broad in purpose.  Right now it:

Acts as a sort of global key-value store.
Acts as a registry of "core" classes used in an application. Something typically just offloaded as an autoloading consideration in modern PHP applications.
Acts as a factory/provider for core class instances. Something that will become extremely unwieldy as your application grows in classes, each with their own sets of dependencies and logic around how they need to be set up.

It almost looks like what you are trying to do is build a dependency injection framework container, however the implementation is slightly off in that you are injecting the entire container into consuming classes rather than using the container to store instantiation logic for dependencies.  That later apporach might look something like:

// set up and configure container
$container = new \System\App();
$container->register('database', function($cont) {
    return new \System\Database();
});
$container->register('user', function($cont) {
    return new \System\User($cont->get('database'));
});

// get user, which would also instantiate new database object if needed
$user = $container->get('user');

If this is truly what you are trying to do, then I would recommend you take a look at popular PHP dependency injection frameworks/containers like PHP-DI or Pimple either for direct use in your applications or for inspiration on how you can approach the problem.

From a global key-value store standpoint, it is hard to understand how this is actually used in your application from what is shown and unclear what value it really has.

What is stored here? If application configuration, should you really provide the level of mutability you do? Would you better be served to have an immutable configuration class that exposes these values?

If storing key-values that are going to be consumed in classes, why not move these items into those classes where you can better enforce class-specific validation rules for setting these keys? Do you really need the flexibility provided by this functionality and the fragility problems this flexibility can introduce?

I would think it trivial to split this portion of functionality away from App if you decide this store truly gives you some value that is not apparent from the code shown.

I don't agree with the approach of injecting this App object along with every single potential instantiated dependency it holds into every consuming class that might need one of the dependencies.  It makes it more burdensome in consuming classes to validate that the dependency you need is actually passed.

For example, say you had a class that needs a PDO object.  A good dependency injection model might look like this:

public function __construct(\PDO $pdo) {
    $this->pdo = $pdo;
}

You enforce the dependency through type-hinting, allowing you to not have to write validation code in your class. Your class gets it's dependency in a proper state.

VS. your current approach which might require validation such as:

public function __construct(\System\App $app) {
    if($app->db instanceof \System\Database === false) {
        throw new Exception('...');
    }
    // validate other dependencies here
    // set dependencies on object as needed
    $this->app = $app;
}

You also obfuscate dependencies to someone reading/working with your classes. With the approach I suggest, the exact dependencies of the class are explicitly stated in the constructor method signature (or in other method signatures if you have static methods or load dependencies after instantiation for some reason). If someone needs to add functionality to the class that requires a new dependency, you actually have to add that dependency to the class.  This may seem like extra work, but, in fact, it is good coding behavior in that you have to be thoughtful and specific in how you consume dependencies.  You avoid someone just introducing new dependencies in your classes by simply adding calls against not-previously-used dependencies stored in App.

So, if you resolve that injecting every dependency that could possibly be needed into every class is not the way you want to go, you can then address the problem with your provider/factory behavior.

If you wanted to only pass required dependencies to consuming classes, your createNewObj() method would quickly become overly complex (it might get this way even with your current dependency injection approach if you need more complex instantiation logic for given classes).

You could imagine some horrible switch construct like:

private function createNewObj($key)
{
    switch($key) {
        case 'user':
            return new \System\User($this->get('db'));
            break;
        case 'db':
            return new \System\Database();
            break;
        // and so on
    }
}

This obviously would be a bad approach.  So how should you approach this?  Each class should have it's own mechanism for instantiation, whether that be a static method on the class itself, or whether it be a wholly separate factory or provider class.

To address the use case in your usage example.  That might look like this:

$users = UserProvider::getAllUsers($database);

Or under DI container approach:

$users = $container->get('users');

Some other thoughts:

Should App::share() be called set()?  This seems like what is happening.
Your App class singleton behavior is improperly implemented. You need a protected/private constructor or perhaps constructor that throws exception to prevent caller from instantiating the class directly.
There is nothing in your App class that can switch between singleton-type behavior for some classes vs. concrete behavior for others.  This would for example only allow a single User object to be created, which may or may not be desired behavior for your application (as it is unclear what User does).
Speaking of which, is User class properly named, or is this actually a provider class? It only has one method, which would seem to return a collection of users, not a singular user.  Should this class even have concrete context?
Don't die() in the middle of your App class, and especially don't die() with message to standard out.  The way this class is designed, you have no idea where it is being used in overall application context.  The code up the call stack is likely more better positioned to handle the error and provide meaningful end user messaging.  Consider using exceptions with this kind of functionality that is so core to your whole application framework.
You have unnecessary nesting in your App::get() method.  Unnecessary conditionals and nesting can lead to more fragile code.

Consider an approach like:

public function get($key){
    if (!$this->has($key) && !$this->isCore($key)) {
        throw new OutOfRangeException('Some message');
    }
    if (!this->isCore($key)) {
        $this->share($key,$this->createNewObj($key));
    }
    return $this->storage[$key];
}

This example also creates an early exit from the function or code path, which is almost always a good thing. Fail fast and fail loudly.

Why are your core classes defined inside a method instead of in class configuration (i.e. class constant), or better yet, in application configuration.  Look at those DI container framework examples I have shown, there is nowhere in there where they are hard coding the classes available to an application.
You are not really doing any validation against parameters passed to your public methods, which for a class as central in your application as this, might be problematic. What if invalid key values are being sent to share()?
$opj = $coreClasses[$key]; Is $opj a good variable name here?  This is really a class name right?
You should consider getting out of the habit of using SELECT * in queries against the database.  This approach oftentimes pulls across more data than necessary (for example if table has timestamps fields or other fields that are not meaningful in wider application context) and it also makes your code potentially fragile to table schema changes.


Scala Tic Tac Toe Game This is my first Scala game.  I would love some feedback on my coding style, or your brief input on how you would do it.

object tictactoe extends App {
  def tttFormat(board: Array[Char]): String =
    "|" + board(0) + "|" + board(1) + "|" + board(2) + "|\n" +
    "|" + board(3) + "|" + board(4) + "|" + board(5) + "|\n" +
    "|" + board(6) + "|" + board(7) + "|" + board(8) + "|\n"
  println("Enter the number of the square you want to occupy!\n"+
      tttFormat(GameObj.board))
  while(GameObj.atPlay) {
    println(tttFormat(GameObj.updatedStateArray))
  }
  GameObj.nextTurn
  println("game over! " + GameObj.nextTurn + "'s win!")
}

object GameObj {
  val board: Array[Char] = Array('1','2','3',
                                 '6','5','4',
                                 '7','8','9')
  var whosTurn = false

  def nextTurn: Char = {
    whosTurn = !whosTurn;
    if (whosTurn) 'X' else 'O'
  }

  def atPlay: Boolean =
    !(board(0) == 'X' && board(1) == 'X' && board(2) == 'X') &&
    !(board(3) == 'X' && board(4) == 'X' && board(5) == 'X') &&
    !(board(6) == 'X' && board(7) == 'X' && board(8) == 'X') &&
    !(board(0) == 'X' && board(4) == 'X' && board(8) == 'X') &&
    !(board(6) == 'X' && board(4) == 'X' && board(2) == 'X') &&
    !(board(0) == 'X' && board(3) == 'X' && board(6) == 'X') &&
    !(board(1) == 'X' && board(4) == 'X' && board(7) == 'X') &&
    !(board(2) == 'X' && board(5) == 'X' && board(8) == 'X') &&
    !(board(0) == 'O' && board(1) == 'O' && board(2) == 'O') &&
    !(board(3) == 'O' && board(4) == 'O' && board(5) == 'O') &&
    !(board(6) == 'O' && board(7) == 'O' && board(8) == 'O') &&
    !(board(0) == 'O' && board(4) == 'O' && board(8) == 'O') &&
    !(board(6) == 'O' && board(4) == 'O' && board(2) == 'O') &&
    !(board(0) == 'O' && board(3) == 'O' && board(6) == 'O') &&
    !(board(1) == 'O' && board(4) == 'O' && board(7) == 'O') &&
    !(board(2) == 'O' && board(5) == 'O' && board(8) == 'O')

  def updatedStateArray: Array[Char] = {
    val in: Int = Integer.parseInt(readLine()) - 1
    board.update(in, nextTurn)
    board
  }
}

Whether Scala or any other language, it's not a good sign when you have big blocks made of pasted/copied lines (tttFormat, board and atPlay).  There is some logical structure to those repeated lines and you should code that instead of copying lines.
There is a bug: the middle row is inverted (654 instead of 456).
The line asking for the player move should probably be in the while-loop.  Just my opinion.
To keep good "separation of concerns", the player input should be read in the main loop and passed to the method that updates the board.
The nextTurn followed by nextTurn at the end to get the winner is "hacky".


Loop for finding proper grouping brackets I've got this piece of code that you pass a string of special characters to and an index which points to a location in this string. The code will take the index, analyze the string, and return a new index of location of the proper grouping bracket. I.e., if index = 5, and my string is ![>[*[]^[#$]%]%]$%, I will be currently at ]. Then, I need to backtrace in the string to find its matching group bracket [. To do this, I've written the following method:

public static int jump(int current, String code){
    int count = 1;
    boolean flag = false;
    if('[' == code.charAt(current)){
      current++;
      while(flag == false){
        if('[' == code.charAt(current)) count++;
        else if(']' == code.charAt(current)) count--;
        if((']' == code.charAt(current)) && (count ==0)){
          flag = true;
          break;
        }
        current++;
      }
    }

    else{
      current--;
      while(flag == false){
        if(']' == code.charAt(current)) count++;
        else if('[' == code.charAt(current)) count--;
        if(('[' == code.charAt(current)) && (count ==0)){
          flag = true;
          break;
        }
        current--;
      }
    }
    return current;
  }

But the more I look at this code the more I feel like "there's got to be a smarter way of doing it then a bunch of if-statements, a while loop, and a flag." But I'm not good enough at this point to figure out how to simplify what I've written (or the logic) into more readable, or smarter code. So I was hoping to find ways to improve this code since I find myself writing this kind of code from time to time and would like to improve the style if possible.

The most common approach to bracket matching is using a stack.

Unmatched brackets you encounter are pushed onto the stack. When encountering a matching bracket pop the stack (and verify if it's the correct type of bracket, maybe not needed in your case). If the stack is empty after finding a match, the last match was the position you were looking for.

This approach, as you can see, also immediately allows for bracket matching validation, and can handle different types of brackets in the same String

To boot, the code is simpler.

Note

When looking for a stack implementation in Java, you may come across java.util.Stack; don't use that, use java.util.ArrayDeque instead, as the Stack javadoc itself suggests.


Partitioning a string into chunks Inspired by this question: Split a string into chunks of the same length

The code is designed to work on text elements rather than chars to avoid unicode problems.

public static class StringExtensions
{
    public static IEnumerable<string> Partition(this string value, int chunkSize)
    {
        if (value == null)
        {
            throw new ArgumentNullException(nameof(value));
        }
        if (chunkSize < 1)
        {
            throw new ArgumentOutOfRangeException(nameof(chunkSize));
        }
        var sb = new StringBuilder(chunkSize);
        var enumerator = StringInfo.GetTextElementEnumerator(value);
        while (enumerator.MoveNext())
        {
            sb.Append(enumerator.GetTextElement());
            for (var i = 0; i < chunkSize - 1; i++)
            {
                if (!enumerator.MoveNext())
                {
                    break;
                }
                sb.Append(enumerator.GetTextElement());
            }
            yield return sb.ToString();
            sb.Length = 0;
        }
    }
}

And a couple of unit tests

[TestMethod]
public void Partition_SplittingAnAsciiString_ShouldSplitTheStringIntoTheRequiredChunkSize()
{
    string input = "123456";
    string[] expected = { "123", "456" };
    string[] actual = input.Partition(3).ToArray();

    CollectionAssert.AreEqual(expected, actual);
}

[TestMethod]
public void Partition_SplittingAPartiallyDecomposedString_ShouldSplitTheStringIntoTheRequiredChunkSize()
{
    string input = "éée\u0301";
    string[] expected = { "é", "é", "e\u0301" };
    string[] actual = input.Partition(1).ToArray();

    CollectionAssert.AreEqual(expected, actual);
}

I usually use Machine Specifications for unit tests so any tips on writing MSTest unit tests would be great especially naming conventions!

The method feels slightly too long to me but I couldn't see a nice way of splitting it up.

Nice and clean implementation. I tested this with the tests (the new ones) of my implementation and they all passed (skipping the tests with chunkSize == 0).

The code could be sligthly more readable by having some vertical space to group related code, for instance after validating the input.

You could improve this a little bit by just having this

if (chunkSize == 1)
{
    yield return value;
    yield break;
}

after the validation, in this way you wouldn't need to create a StringBuilder nor having the enumerator.

A slightly different approach could be to remove the for loop. It makes the intent more clear (IMO) and removes the need to double check return value of enumerator.MoveNext().

Unfortunately this makes the code 2 lines (3 with the vertical spacing) longer

        var sb = new StringBuilder(chunkSize);
        var enumerator = StringInfo.GetTextElementEnumerator(value);
        var counter = 0;
        while (enumerator.MoveNext())
        {
            counter++;
            sb.Append(enumerator.GetTextElement());
            if (counter == chunkSize)
            {
                yield return sb.ToString();

                sb.Length = 0;
                counter = 0;
            }
        }
        if (counter > 0)
        {
            yield return sb.ToString();
        }

Regarding naming of unit tests, I usually (not in the posted question of mine) use the pattern

UnitOfWork_StateUnderTest_ExpectedBehavior

like shown in the accepted answer over here: unit test naming best practices


Squaring a tree in Clojure I am working on Problem 2.30 from Structure and Interpretation of Computer Programs. I book is in scheme, but I am doing the exercises in Clojure.

The problem is to write code that takes a tree of numbers and return a new tree with the numbers squared. I have done it in two ways:

Without higher-order functions:

(defn square-tree1 [tree]
  (lazy-seq
    (when-let [s (seq tree)]
      (if (coll? (first s))
        (cons (square-tree1 (first s))
              (square-tree1 (rest s)))
        (cons (square (first s))
              (square-tree1 (rest s)))))))

Using map

(defn square-tree2 [tree]
  (if (coll? tree)
    (map square-tree2 tree)
    (square tree)))

Where:

(defn square [x]
  (* x x))

My tests for this are:

(deftest e2.30a
  (testing "Ex 2.30a: square-tree"
    (is (=
         (square-tree1
          (list 1
                (list 2 (list 3 4) 5)
                (list 6 7)))
         '(1 (4 (9 16) 25) (36 49))))
    (is (= (square-tree1 '()) '()))
    (is (= (square-tree1 [1 2 3 4 5]) [1 4 9 16 25]))
    (is (= (square-tree1 [1 [2 [3]]]) [1 [4 [9]]]))))

I want the functions to work with lists and vector inputs.
I am unsure if 1 is the proper usage of lazy-seq and whether there is a neater way to express this.
Is (col? (first s)) the correct predicate here? I find it tricky in Clojure to test if the node in the tree is another sub-tree or if it is number. In Scheme it is easier because you have the pair? predicate. I would like to see if there is a better way.

Edit -- After a useful discussion with Timothy Pratley about clojure.walk

After looking at the source code of clojure.walk I have come up with a third square-tree function that extends the square-tree2, but now preserves the input collection types of the input tree like clojure.walk does:

    (defn square-tree3
      [tree]
      (cond
        (list? tree) (apply list (map square-tree3 tree))
        (seq? tree) (doall (map square-tree3 tree))
        (coll? tree) (into (empty tree) (map square-tree3 tree))
        :else (square tree)))

So it extends the answer to the coll? predicate with a (into (empty tree)...) which puts the result of map into whatever type of collection tree is. It also adds cases for list and seq types, handling each of them in their own special way so that they replicate into the same collection type.

You can use postwalk:

(defn maybe-square [x]
  (if (number? x)
    (* x x)
    x))

(clojure.walk/postwalk maybe-square [1 [2 [3]]])
=> [1 [4 [9]]]

Which is perhaps against the spirit of learning how to do it, so it is worth looking at the implementation code:

https://github.com/clojure/clojure/blob/2224dbad5534ff23d3653b07d9dc0a60ba076dd7/src/clj/clojure/walk.clj#L35

Which shows the correct way to test for various things you may expect in a tree (There are quite a few different tests you can take advantage of).

lazy-seq and map are cool, but it is probably more common to think of trees as data structures rather than lazy sequences; you will note that postwalk returns datastructures, not lazy sequences. So I recommend you also return datastructures (in your case vectors and lists, rather than a sequence).


Creating a structured category list in F# This takes a list of categories and organizes them into a tree-type structure based on their parentId.  This code works, but is obviously a little over-complicated. How can I optimize it?

type ValidString = | ValidString of string | NotValid

type CategoryStructure = {
    Id: ValidString;
    ParentId: ValidString;
    Name: ValidString;
    Abbreviation: ValidString;
    Description: ValidString;
    SapId: ValidString;
    Section: ValidString;
    SectionPosition: ValidString
}

type DynamicCategories = {
    Category: CategoryStructure;
    SubCategories: seq<DynamicCategories>
}

let rec private structureCategories (fullList: CategoryStructure list)
    (list: CategoryStructure list)  =

    List.fold (fun acc elem ->

                    // get all categories and details
                    let categories = fullList
                    let mainAcc =
                        [

                            for row in categories do
                                if row = elem
                                then
                                    let subs =
                                        List.fold (fun acc' elem' ->

                                                    if row.Id = elem'.ParentStructureId
                                                    then
                                                        let modifiedList = elem' :: List.empty<CategoryStructure>
                                                        let foundSubCategory =
                                                            {
                                                                Category = elem';
                                                                SubCategories = structureCategories fullList modifiedList |> Seq.ofList
                                                            }
                                                        foundSubCategory :: acc'
                                                    else acc'

                                                    ) List.empty<DynamicCategories> categories
                                        |> Seq.ofList
                                    yield{
                                        Category = elem;
                                        SubCategories = subs
                                    }

                        ]

                    mainAcc @ acc
                    ) List.empty<DynamicCategories> list

// get the initial parent categories and then call the above code to finish the organizing the list
let getStructuredCategories () =

        let categories = allCategoriesAndDetails () |> List.ofSeq
        [
            for row in categories do
                if row.ParentStructureId = NotValid
                then yield row
        ] |> structureCategories categories |> Seq.ofList

Here is an optimized version that I have discovered:

let getStructuredCategories ()  =

let fullList = allCategoriesAndDetails ()

let parentList () =
    allCategoriesAndDetails ()
    |> Seq.filter (fun p -> p.ParentStructureId = NotValid)

let rec toTree (fullList': seq<CategoryStructure>) (parent: CategoryStructure) =

    fullList'
    |> Seq.filter (fun x -> x.ParentStructureId = parent.Id)
    |> Seq.map (fun x ->
                    {
                        Category = x;
                        SubCategories =
                            toTree fullList' x
                    })
seq {
    for row in parentList () do
    yield {

        Category = row;
        SubCategories = toTree fullList row
    }

}

Can someone else do better?


Null checks in user validation I would like to know an alternative, more elegant way to write the following method.  I am especially not enthusiastic of the nested if statement.

hasUserSavedCredentials: function () {
            var userName = Storage.get(CONFIG.app.storageUserName),
                password = Storage.get(CONFIG.app.storagePassword),
                result = false;
            if (userName !== null && password !== null) {
                if (userName.length > 0 && password.length > 0) {
                    result = true;
                    // save in state for faster retrivial
                    State.xx.isSaved = result;
                    State.xx.userName = userName;
                    State.xx.password = password;
                } else {
                    result = false;
                    State.xx.isSaved = false;
                }
            }
            return result;
        },

You can remove result = false;, as result is already false.

And to get rid of the nested statements, you could use early returns:

if (userName == null && password == null) {
    return false;
}

if (userName.length <= 0 && password.length <= 0) {
    State.xx.isSaved = false;
    return false;
}
State.xx.isSaved = true;
State.xx.userName = userName;
State.xx.password = password;
return true;

Is it correct that when username is null, isSaved = false should not be set? Because if not, then you could just combine the two if statements, either in my example, or in yours.


Is there anything incorrect about this layout, semantically or otherwise? I would like to know if my chosen layout is HTML5 standards compliant, or if I'm using the heading tags incorrectly (or any other tag for that matter)?

Main Header

 for website name

 for webpage name

 for website slogan, or should this be  with styling?

Main Content/Sidebar Content

 for every first heading in each
 element (because a level 1 heading is too large), or should this really be
 with specific styling; e.g. .panel-ws section h1?
.first removes unwanted top margin.
.panel-ws adds whitespace (padding).

Example Code Extracts (OLD; Updated Below!)

Website Name

Page Title

Slogan

Content 1

Lorem ipsum...

Lorem ipsum...

Content 2

Lorem ipsum...

Lorem ipsum...

Content 3

Lorem ipsum...

Lorem ipsum...

Side content 1

Lorem ipsum...

Lorem ipsum...

Side content 2

Lorem ipsum...

Lorem ipsum...

#header-text h1
#header-text h2
#header-text h3
.panel-ws section h2
.first

Example Code Extracts (UPDATED!)

Website Name

Slogan

Page Title

Content 1

Lorem ipsum...

Lorem ipsum...

Content 2

Lorem ipsum...

Lorem ipsum...

Content 3

Lorem ipsum...

Lorem ipsum...

Related Content 1

Lorem ipsum...

Lorem ipsum...

Related Content 2

Lorem ipsum...

Lorem ipsum...

#header-text h1
#header-text p
.panel-ws #page-title-container #page-title
.panel-ws section h1
.panel-ws section h2
.panel-ws section h3

In HTML5 h1 elements can be treated as relative to their semantic container, so it is not bad practice to have an h1 as the main page header text and further h1's for each section, aside, article, etc.

As you've mentioned you would naturally style this with something like section h1 { ... }.  I'd recommend making this more general and not depending on .panel-ws because you may want to hang it off main and aside instead to allow for different header text styling in each?

I found a similar question on stackoverflow which you might want to check out too...

https://stackoverflow.com/questions/7405282/how-to-properly-use-h1-in-html5

For the webpage name and slogan, you could group h1 and h2 elements inside a parent hgroup to ensure that html readers don't parse the slogan as an implicit second level of content.


Method to separate a list of ports to scan I'm building a port scanner as an exercise. It contains a method to separate the ports that the user wants to have checked, which can include single ports and port ranges, ex: 22, 23, 80-120, 443-500, 1024, etc. It discards wrong ports such as negative ports, wrong range such as 120-80 instead of 80-120 which is the right form, and duplicate ports.

I'm wondering if there's a better way to do this, or to improve the code, make it shorter, using a more effective technique. The text box to enter the list of ports only accepts numbers, commas and dashes.

private List<int> extractPorts(string ports)                // the string argument contains a list of ports entered through a text box
    {

        string[] portList = ports.Split(',');                   // save everything between commas, including ranges like 80-120

        List<int> listOfPorts = new List<int>();                // creates a new list of integers to save the valid given ports

        for (int i = 0; i < portList.Length; i++ )              // iterates through port list including single ports and ranges
        {
            if (portList[i].Contains('-'))                      // if finds a range like 80-120...
            {
                try
                {
                    int[] range = Array.ConvertAll(portList[i].Split('-'), int.Parse);  // saves the edges of a range into an array ex: 80-120 as 80 and 120

                    for (int j = range[0]; j <= range[range.Length - 1]; j++)           // iterates through the range saved above ex: from 80 to 120 the get
                    {
                        listOfPorts.Add(j);                                             // every single port ex: in this case it gets 40 ports, from 80 to 120
                    }                                                                   // if finds an error like a negative port or a wrong range like 120-100
                }                                                                       // instead of 100-120 will discard it
                catch { }
            }
            else
                listOfPorts.Add(Convert.ToInt32(portList[i]));  // adds a single port to the list
        }

        List<int> intPortsList = listOfPorts.Distinct().ToList();   // eliminate duplicated ports

        listOfPorts.Clear();                                        // clear the previous list

        foreach (int item in intPortsList)
        {
            if (item <= 65536)                                      // discard ports out of range
              listOfPorts.Add(item);
        }

        listOfPorts.Sort();                                         // to sort the final list

        return listOfPorts;
    }

Comments

You have too many of them. Stick to comments that clarify the intent behind code instead of documenting the syntax.

When I see

List<int> listOfPorts = new List<int>();

I know already that it's a list of integers, intended to represent the ports. Comments should explain why you do what you do, for example:

// List of ints instead of strings so we can perform arithmetic logic

Whitespace

This is more personal but your code feels very spread out. You don't have to leave a space between every line of code (although I'll admit that the braces are  a major part of it).

Braces

If you omit braces, you will guaranteed end up with a logical error one day. What if you decide to add something else to this code snippet but aren't paying 100% attention?

else
    listOfPorts.Add(Convert.ToInt32(portList[i]));

This will be a pain to debug.

For situations like this it is more forgiving, but honestly is there such a difference between

if (item <= 65536)
 listOfPorts.Add(item);

and

if (item <= 65536) {
     listOfPorts.Add(item);
}

Exceptions

You have an empty catch block. That's no bueno, there should be some sort of feedback to the user that tells him what went wrong (at the very least there should be logging in place).

Abstraction

Your return statement is List<int>. Is this a conscious design choice? You might want to read up on the reasoning why I prefer IEnumerable<int>.

Naming

Two variables are respectively portList and listOfPorts. First of all: remove the underlying datastructure from the name.

In a way this can also be seen as enforcing encapsulation: it allows you to change the implementation to, for example, a Dictionary without suddenly having a discrepancy between the implementation and the variable name.

More descriptive names could be input/inputPorts/unparsedPorts and ports/parsedPorts/resultPorts, etc.

Input boundaries

I don't see any boundary checks to verify the following things:

No negative numbers
No positive numbers outside the range of ports
A range should only consist of 2 values

Loop clarity

I consider

int max = range[range.length - 1];
// ...
j <= max

to be easier to read than

j <= range[range.length - 1]

This is a minor gripe, but it can be confusing given how "<= length - 1" is usually interpreted in the condition section of a for loop (which usually gets refactored to "< length").

Duplicates

When you don't want duplicates, a List<T> is not your preferred datastructure. Instead, use a Set<T> like HashSet<T>. This will automatically take care of it for you.

If you combine it with your sorting at the end you can use a SortedSet<T> instead:

  A SortedSet<T> object maintains a sorted order without affecting performance as elements are inserted and deleted. Duplicate elements are not allowed.

Complexity

You're doing a lot of things with a lot of lists of ports (better naming would have come in handy here!).

This is what you do:

Create list (A) of input ports
Parse A into another list (B)
Create new list that holds unique values from B
Clear B
Add all ports that adhere to a boundary check to list B
Sort B

That's a lot of stuff that we can refactor.

First of all: put that boundary check earlier in your code. Once a port is added to our result list of ports, it is a valid port. We don't want to copy everything to a new list just to add that boundary check (and you need more than that!).

In fact, now it's already a lot easier: by using the SortedSet<T> you will already do the sorting and duplicate removal so that's not needed either.

Best part: SortedSet<T> implements IEnumberable<T> so you can simply return your set as-is.


URLify a given String - replace spaces with %20 I have written a code to replace all spaces in a String with %20.
Input: "Mr John Smith "
Output: "Mr%20John%20Smith"

My logic:
public class URLify {
    public static void main(String[] args) {
        String str = "Mr John Smith  ";
        str = str.stripTrailing();
        char[] ch = str.toCharArray();
        StringBuilder sb = new StringBuilder();
        for(char c : ch) {
            if(c != ' ')
                sb.append(c);
            else {
                sb.append("%20");
            }
        }
        System.out.println(sb.toString());
    }
}

The given solution:
static int MAX = 1000;
static char[] replaceSpaces(char str[]) {
    int space_count = 0, i = 0;
    for (i = 0; i < str.length; i++)
        if (str[i] == ' ')
            space_count++;
    while (str[i - 1] == ' ') {
        space_count--;
        i--;
    }
    int new_length = i + space_count * 2;
    if (new_length > MAX)
        return str;
    int index = new_length - 1;
    char[] old_str = str;
    str = new char[new_length];
    for (int j = i - 1; j >= 0; j--) {
        if (old_str[j] == ' ') {
            str[index] = '0';
            str[index - 1] = '2';
            str[index - 2] = '%';
            index = index - 3;
        }
        else {
            str[index] = old_str[j];
            index--;
        }
    }
    return str;
}

Both work correctly with a linear time complexity of () however, can someone please suggest which solution is a better one especially if asked in an interview? Thanks!

Review
Welcome to Code Review. There are some suggestions as follows.
Consider to use replaceAll
The operation you performed is what String.replaceAll do, you can use replaceAll directly instead of String.toCharArray(), whitespace comparison, and sb.append things.
Create a method URLify
In order to improve usability, it is better to create a method URLify to perform this specific operation.
Summary
Combine all the things:
public class URLify {

    public static String URLify(String input){
        return input.stripTrailing().replaceAll(" ","%20");
    }

    public static void main(String[] args) {
        String str = "Mr John Smith  ";
        System.out.println(URLify(str));
    }
}


Binary Digits to ASCII Characters I wrote this code, as a novice a to Prolog, for a challenge posted on /r/DailyProgrammer. The formal description of this challenge, as presented in the prompt reads:

  Input description

  On console input you will be given a variable number of 0's and 1's
  that correspond to letters in the alphabet [a-z] and whitespace ' '.
  These will be integers coming in, it's your job to cast them however
  you need.

  Output description

  The program should output the english translation (or other languages
  if you feel so inclined!) of the binary phrase

This is a fairly simple task so, even as a novice to Prolog, it didn't take me long to complete.

main(Words):-
    open('text.txt',read,Stream),
    readAll(Stream,Codes),
    close(Stream),
    atom_codes(Words,Codes).

readAll(Stream,[]):-
    at_end_of_stream(Stream).
readAll(Stream,Out):-
    readBinary(Stream,8,Out2),
    readAll(Stream,Out3),
    Out = .(Out2,Out3).

readBinary(_,0,0).
readBinary(Stream,Digits,Out):-
    get_code(Stream,A),
    B is (A-48) * 2 ^ (Digits-1),
    Digits2 is Digits - 1,
    readBinary(Stream,Digits2,Out2),
    Out is B + Out2.

text.txt is a text file containing the flowing binary digits encoded as characters in UTF-8:

0100100001100101011011000110110001101111001000000101011101101111011100100110110001100100

Running the code results in the correct output:

  Hello World

The code undoubtedly works, and I even find that I like how it looks. My greatest concern, however, is that this somehow does not maintain style and format standards of Prolog programs. Reading through the code, it reads as a completely imperative program, but I know that Prolog is built off the logic programming paradigm. Is this because of a fault in how I wrote the program, or is the problem that this challenge does not lend itself to logic programming? If the former is correct, how could this be written to better reflect the full capabilities of Prolog?

Your concerns about style and format are warranted, +1 for striving to make the code more elegant and declarative!

First of all, whatDoYouFindEasierToRead: mixed_caps_or_underscores?

For that reason, using underscores is the Prolog convention for naming predicates.

Second, Prolog is all about relations between things. You are using low-level moded arithmetic (is/2), which can only be used in one direction.

Instead, use constraints: They are completely general relations that can be used in all directions.

Third, since you are describing a list of bytes, consider using DCG notation. Do not use side-effects (opening a file etc.), because they prevent you from using your program in all directions. Instead, focus on a clear declarative description, and use pure features like library(pio) to apply a DCG to file contents.

All in all, consider the following program, tested with SWI-Prolog:

:- use_module(library(clpfd)).

:- set_prolog_flag(double_quotes, codes).

bytes([])     --> [].
bytes([B|Bs]) --> byte(B), bytes(Bs).

byte(Byte) -->
        [A,B,C,D,E,F,G,H],
        { foldl(pow, [A,B,C,D,E,F,G,H], 0-7, Byte-_) }.

pow(X0, Val0-Pow0, Val-Pow) :-
        X in 0..1,
        X #= X0 - 0'0,
        Val #= Val0 + X*2^Pow0,
        Pow #= Pow0 - 1.

Notice that I am not using any side-effects at all, and I don't have to, because I can do everything I need on the toplevel:

?- phrase(bytes(Bs), "0100100001100101011011000110110001101111001000000101011101101111011100100110110001100100"), maplist(char_code, Chars, Bs), maplist(write, Chars).
Hello World
Bs = [72, 101, 108, 108, 111, 32, 87, 111, 114|...],
Chars = ['H', e, l, l, o, ' ', 'W', o, r|...] .

And now the beauty of the declarative approach: In fact, the program is much more general than that, and I can also use it in the other direction:

?- phrase(bytes("Hello"), Codes), maplist(char_code, Chars, Codes), maplist(write, Chars).
0100100001100101011011000110110001101111
Codes = [48, 49, 48, 48, 49, 48, 48, 48, 48|...],
Chars = ['0', '1', '0', '0', '1', '0', '0', '0', '0'|...].

In fact, I can even leave everything unspecified and still obtain answers:

?- phrase(bytes(Bs), Codes).

Thus, the program is now much more general.

I would like to add that logic programming is very well suited for such kinds of problems. In fact, one of the very first applications of Prolog was concerned with language translation, and parsing a list of bytes like that was certainly part of that. Just stay in the pure and declarative part of Prolog (use constraints, pure input etc.), and you will obtain pretty nice programs.

EDIT: As requested, a few more comments about the code. The main part of the program, the grammar rule byte//1, is a declarative description of what we consider a byte in this notation. Now this is easily described: A byte in this particular notation is just a list of 8 bits [A,B,...,H]. And these bits are related to the byte B via the well-known relation B = A*2^7 + B*2^6 + ... + H*2^0. The pow/3 predicate defines this relation for each of the bits, using a running sum and position indicator as one of its arguments. Of course I have to subtract the ASCII code of 0 (Prolog: 0'0) from each of the given bits, because they are themselves specified in ASCII already, hence X #= X0 - 0'0 to obtain the "true" value of the bit X, 0 or 1.

Well and that's pretty much there is to it. A list of such bytes is readily described with DCG notation, and we use the phrase/2 interface to access DCGs.

I leave figuring out the rest as an easy exercise. Remember that you can always use the graphical tracer with ?- gtrace, your_goal., if you are interested in any particular execution details. In general though, I recommend you instead read the program declaratively and understand the relations that are being defined.


Euler Transform on Leibniz Series of Pi So i read about simple series for Pi named Leibniz Series for Pi. Here is its Link:-
Series . I made a program for it and soon realised that it converges very slows . So i did Euler accelerate for Alternating Series https://en.wikipedia.org/wiki/Series_acceleration#Euler's_transform and made this recursive program:-
import functools
import decimal
def LebniezTerm(n):
  return 1/(2*n + 1)

@functools.lru_cache(maxsize = 12800)
def ndiffrence(n,depth):
  if depth == 0:
    return LebniezTerm(n)
  a = ndiffrence(n,depth-1)
  b = ndiffrence(n+1,depth-1)
  return (a - b)

def EulerAccelerate(n):
  i = decimal.Decimal(1)
  pi = decimal.Decimal(0)
  while (i <= n):
    pi = pi + decimal.Decimal(ndiffrence(0,i-1))/decimal.Decimal(2**i)
    i+=1
  return pi

4*EulerAccelerate(600)

Here i did and optimisation using Functools but it is still slow. So can it become more optimised ?
How can the accuracy get more increased ?

What I found out by simply taking the time the calculation takes, is that casting to Decimal is a very costly operation. Dropping that in certain places (see my code) brings down the overall runtime to ~ 30-40%.
Besides, the Leibnitz terms can easily be precomputed (another Best Practice of optimization) as the list will be comparatively short. Surprisingly, this does not save much.
Using a module method with a local name save some time as well (import xxx from this_module as local_name instead of using this_module.xxx multiple times).
In EulerAccelerate(), the loop variable i does not need to be of type Decimal which saves a lot. Replacing 2**(i+1) with a simple addition yields another (small) saving.
Stepping back from the code analysis, I think changing the algorithm from recursive to iterative would speed up the calculation a lot, much more than those micro-optimizations.
results on my notebook: maxdepth=24, accurate to 8 places: pi=3.1415926, runtime=10 s
    import functools
    from decimal import Decimal
    import time

    ## @functools.lru_cache(maxsize = 12800)
    def ndifference(n, depth):
        if depth == 0:
            return LT[n]  #   = 1.0/(2*n + 1)
        a = ndifference(n, depth-1)
        b = ndifference(n+1, depth-1)
        return (a - b)

    def EulerAccelerate(n):
        pi = 0
        ith_power_of_2 = 2                # 2**(i+1)
        for i in range(n):
            pi += Decimal(ndifference(0, i)) / ith_power_of_2
            ith_power_of_2 += ith_power_of_2
        return pi

    # ---------------------------------
    maxdepth = 24
    # create Leibnitz terms beforehand; LT is global
    LT = [(1.0/(2.0*i+1.0)) for i in range(maxdepth+1)]

    t = time.time()
    print 4 * EulerAccelerate(maxdepth)
    print time.time()-t


Guided object builder Sometimes when designing a fluent-api it's important to build an object in a specific way/sequence.

I tried to create such a guided builder and came-up with this:

public class Query
{
    public static IFrom Build() { return new Builder(); }

    public class Builder : IFrom, IWhere, ISelect, ICreate
    {
        public IWhere From() { return this; }
        public ISelect Where() { return this; }
        public ICreate Select() { return this; }

        public Query Create() { return new Query(); }
    }

    public interface IFrom { IWhere From(); }
    public interface IWhere { ISelect Where(); ICreate Select();  }
    public interface ISelect { ICreate Select(); }
    public interface ICreate { Query Create(); }
}

Usage:

var query1 = Query.Build().From().Where().Select().Create();

or

var query2 = Query.Build().From().Select().Create();

Without manual casting you are guided through the creation process so everything stays fluent. Or maybe there are better ways to suggest the fluent path?

As written in the comment, it reminds me of LINQ. So, maybe it make sense to write a Linq QueryProvider to use the familiar LINQ syntax.

If that is not an option:

I would drop the method Build() and make the method From static instead. That simplifies the API.


One-line initialisation for a constant list with all entries except one from another list I'm looking to simplify the initialisation of the second constant, preferably to just one line. Any ideas? Anything from Guava or JDK (up to 1.6) is ok to use.

enum Box {
    PROMO_HEADER,
    FREEMIUM,
    EXTRA_STORIES,
    TOP_STORIES,
    TOP_CHARACTERS,
    TOP_THEMES,
    TOP_ARTISTS,
    TOP_ISSUES
    // several more ...
}

final static List<Box> FREEMIUM_BOXES = ImmutableList.copyOf(Box.values());
final static List<Box> DEFAULT_BOXES = initDefaultBoxes(); // All except FREEMIUM

static List<Box> initDefaultBoxes() {
    List<Box> boxes = Lists.newArrayList(FREEMIUM_BOXES);
    boxes.remove(Box.FREEMIUM);
    return ImmutableList.copyOf(boxes);
}

Granted, it's not that bad, but still I'd like to get rid of the init method. :-) (Static initialiser wouldn't be any better.)

NB: I want to define the order of boxes only once, in the enum definition itself, so ImmutableList.of(PROMO_HEADER, EXTRA_STORIES, ...) is not considered a good solution for the purposes of this refactoring.

A single-statement Guava solution would be this, using filter() and a Predicate, but arguably this is less clean than the original, because this kind of stuff is verbose in Java...

final static List<Box> DEFAULT_BOXES =
    ImmutableList.copyOf(Iterables.filter(FREEMIUM_BOXES, new Predicate<Box>() {
        @Override
        public boolean apply(Box box) {
            return box != Box.FREEMIUM;
        }
    }));

Resolution

Went with this, which is the cleanest option considering I indeed need the constants defined as Lists:

final static List<Box> FREEMIUM_BOXES = ImmutableList.copyOf(Box.values());
final static List<Box> DEFAULT_BOXES =
    Sets.immutableEnumSet(EnumSet.complementOf(EnumSet.of(Box.FREEMIUM))).asList();

Props to Xaerxess!

Use EnumSet for that. From docs:

  A specialized Set implementation for use with enum types. (...) The iterator returned by the iterator method traverses the elements in their natural order (the order in which the enum constants are declared).

It's also very efficent:

  Implementation note: All basic operations execute in constant time.
  They are likely (though not guaranteed) to be much faster than their
  HashSet counterparts. Even bulk operations execute in constant time if
  their argument is also an enum set.

So in your case:

final static Set<Box> DEFAULT_BOXES =
    EnumSet.complementOf(EnumSet.of(Box.FREEMIUM));

or if you prefer immutable one, use Sets.immutableEnumSet(Iterable):

final static ImmutableSet<Box> DEFAULT_BOXES =
    Sets.immutableEnumSet(EnumSet.complementOf(EnumSet.of(Box.FREEMIUM)));

or if you insist for List (but why would you?):

final static ImmutableList<Box> DEFAULT_BOXES =
    Sets.immutableEnumSet(EnumSet.complementOf(EnumSet.of(Box.FREEMIUM))).asList();

P.S. All constants can be represented as:

final static Set<Box> FREEMIUM_BOXES = EnumSet.allOf(Box.class); // or wrap it with Sets.immutableEnumSet to make it ImmutableSet


More efficient way of counting the number of values within an interval? Suppose I have a text file which contains 10,000 random values between 0 and 1, and I want to count the number of values within a specific interval.  An example may explain it more clearly.

Suppose my Datefile.txt contains data like:

0.0102244
0.028072
0.0144578
0.064578
0.08148
0.012749
0.12749.....and 10,000 more values

And I want to count the number of values within the interval 0.05 (i.e. it searches for the values within the range 0.0 to 0.05, then 0.05 to 0.1 and so on). For the example above, the output will be:

In range 0 to 0.05, total values 4
In range 0.05 to 0.1, total values 2
In range 0.1 to 0.15, total values 1...etc.

Here is my code:

int main(){

double area;
double a=0,b=0,c=0,d=0,e=0,f=0,g=0,h=0,i=0,k=0,l=0,m=0,n=0,o=0,p=0,q=0,r=0,s=0,t=0,u=0;

ifstream theFile ("Datafile.txt");

  while(theFile >>area){

    if (area<=0.05){
        a++;
    }

    else if (area<=0.1){
        b++;
    }
    else if (area<=0.15){
        c++;
    }
    else if (area<=0.2){
        d++;
    }
    else if (area<=0.25){
        e++;
    }
    else if (area<=0.3){
        f++;
    }
    else if (area<=0.35){
        g++;
    }
    else if (area<=0.4){
        h++;
    }
    else if (area<=0.45){
        i++;
    }
    else if (area<=0.5){
        k++;
    }

    else if (area<=0.55){
        l++;
    }
    else if (area<=0.6){
        m++;
    }
    else if (area<=0.65){
        n++;
    }
    else if (area<=0.7){
        o++;
    }
    else if (area<=0.75){
        p++;
    }
    else if (area<=0.8){
        q++;
    }
    else if (area<=0.85){
        r++;
    }
    else if (area<=0.9){
        s++;
    }
    else if (area<=0.95){
        t++;
    }
    else if (area<=1.0){
        u++;
    }

}

Though my code is working fine, it might not be the most efficient way. So I put my code here for review. Can anyone suggest a better way of doing this?

Updated (based on the answer of ChrisWue)

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

using namespace std;

int main(){

const double bucket_size = 0.05;
double area=0;
int number_of_buckets = (int)ceil(1 / bucket_size);
std::vector<int> histogram(number_of_buckets);
ifstream theFile("Datafile.txt");

while (theFile >> area) {
    int bucket = (int)floor(area / bucket_size);
    histogram[bucket] += 1;
}
for(auto loop = histogram.begin(); loop != histogram.end();++loop)
    {

cout<<bucket_size<<"\t"<<number_of_buckets<<endl;

}

return 0;
}

For the data-

0.0102244
0.028072
0.0144578
0.064578
0.08148
0.012749
0.12749.....

it gives the output like this-

0.05    20
0.05    20
0.05    20
0.05    20
0.05    20
0.05    20
0.05    20
0.05    20
0.05    20
0.05    20
0.05    20
0.05    20
0.05    20
0.05    20
0.05    20
0.05    20
0.05    20
0.05    20
0.05    20
0.05    20

What am I doing wrong?

What you want is a histogram of your values. If you want it to be flexible in the future with different bucket sizes, then you can use a std::vector and calculate how many buckets you need.

If the values are in range [0, 1] and 0 < bucket_size < 1, then the number of buckets you need obviously is ceil(1 / bucket_size).

So what I'd do would probably look something like this:

const float bucket_size = 0.05;
int number_of_buckets = (int)ceil(1 / bucket_size); // requires <cmath>
std::vector<int> histogram(number_of_buckets);

...

while (theFile >> area) {
    int bucket = (int)floor(area / bucket_size);
    histogram[bucket] += 1;
}

No big if-else cascade and easy to adjust for different bucket sizes.