Header and navbar animations on a website I was wondering if there was a way to rewrite this to be a bit more clean. It seems a bit odd to have a mouseenter and a mouseleave for this. Surely there's a toggle of some kind?

$(document).ready(function() {

// Post header animation
$('h1.post').mouseenter(function() {
    $(this).animate({ paddingLeft: 10 }, 300);
});
$('h1.post').mouseleave(function() {
    $(this).animate({ paddingLeft: 0 }, 300);
});

// Navbar hover animation
$("h1#button").mouseenter(function() {
    $(this).animate({opacity: 0.60}, 100);
});
$("h1#button").mouseleave(function() {
    $(this).animate({opacity: 1}, 100);
});

// Contact div toggle
$("h1.icons").click(function() {
    $(".icon-wrapper").slideToggle("slow");
});
});

My website is http://gamer-simms.herokuapp.com if you want to look.

You can condense this a bit by using jQuery's hover method:

$('h1.post').hover(function() {
    $(this).animate({ paddingLeft: 10 }, 300);
}, function() {
    $(this).animate({ paddingLeft: 0 }, 300);
});

$("h1#button").hover(function() {
    $(this).animate({opacity: 0.60}, 100);
}, function() {
    $(this).animate({opacity: 1}, 100);
});


Sudoku solving algorithm - Revision needed I was preparing myself for an interview at a well known .com firm. One of the question that they often ask is an algorithm to solve sudokus (that have one solution). Here is what came to my mind. Any hints criticisms or suggestions to tune it up?

import itertools
sudoku_str="""003020600
900305001
001806400
008102900
700000008
006708200
002609500
800203009
005010300"""

sudoku=[[int(i) for i in j] for j in sudoku_str.splitlines()]
while not any([0 in line for line in sudoku]):
    for x,y in itertools.ifilter(lambda x: sudoku[x[1]][x[0]]==False, itertools.product(*[range(9)]*2)):
        #Find the elements in the line
        line=set([i for i in sudoku[y] if i])
        #Find the elements in the column
        column=set([xline[x] for xline in sudoku if xline[x]])
        #Create some shifts to get the start (x,y) position for the area computation e.g. for 1,1 => 0,0 and for 3,8=>1,3
        shifts=dict(zip(range(9),[0]*3+[3]*3+[6]*3))
        #Find the elements in the area
        area=filter(None,reduce(lambda x,y: x.add(y), sudoku[shifts[y]:shifts[y]+3], set()))
        #What could be in that position?
        outcomes=set(range(1,10))-line-column-area
        if len(outcomes)==1:
            #One outcome? replace the zero
            sudoku[y][x]=outcomes.pop()

print "\n".join([" | ".join(str(k) for k in i) for i in sudoku])

This is far too complicated:

for x,y in itertools.ifilter(lambda x: sudoku[x[1]][x[0]]==False, itertools.product(*[range(9)]*2)):

It is equivalent to this:

[(x, y) for x in range(9) for y in range(9) if not sudoku[y][x]]

More generally, this code feels like it is trying too hard to be clever at the expense of both clarity and potentially accuracy.

Edit:

To answer the question of speed posed by the op:

$ python -m timeit 'sudoku_str="""003020600900305001001806400008102900700000008006708200002609500800203009005010300""";sudoku = [[int(i) for i in sudoku_str[j:j+9]] for j in range(0, 81, 9)];b = [(x, y) for x in range(9) for y in range(9) if not sudoku[y][x]];'
10000 loops, best of 3: 68.1 usec per loop

$ python -m timeit 'import itertools; sudoku_str="""003020600900305001001806400008102900700000008006708200002609500800203009005010300""";sudoku = [[int(i) for i in sudoku_str[j:j+9]] for j in range(0, 81, 9)];a =[(x, y) for x, y in itertools.ifilter(lambda x: sudoku[x[1]][x[0]]==False, itertools.product(*[range(9)]*2))];'
10000 loops, best of 3: 91.9 usec per loop

A simple list comprehension is both clearer and faster.

Here's another example of unnecessary cleverness:

>>> shifts=dict(zip(range(9),[0]*3+[3]*3+[6]*3))
>>> shifts
{0: 0, 1: 0, 2: 0, 3: 3, 4: 3, 5: 3, 6: 6, 7: 6, 8: 6}
>>> clear_shifts = {x:(x/3)*3 for x in range(9)}
>>> clear_shifts
{0: 0, 1: 0, 2: 0, 3: 3, 4: 3, 5: 3, 6: 6, 7: 6, 8: 6}


Node.js put records to Kinesis with infinite retry strategy. My application is really easy. It's going to add 500 records at a time to AWS Kinesis and if there's an error occur I'll just add that back to the queue and it's going to be retry. It's going well but I feel like there's a bug somewhere that I don't see.

'use strict';

const _ = require('lodash');
const moment = require('moment');

const config = require('../config');
const logger = require('../logger');
const numbers = require('../helpers/numbers');

const streamName = config.kinesis.streamName;

let records = [];
module.exports = (kinesis) => {
  let sendRecords = () => {
    let payloadRecords = {
      Records: records,
      StreamName: streamName
    };

    if (records.length >= 500) {
      const pushingToKinesis = records.splice(0, 500);
      payloadRecords.Records = pushingToKinesis;
    } else {
      records = []
    }

    kinesis.putRecords(payloadRecords, (err, data) => {
      if (err) {
        logger.error(err);
      }

      const failedRecord = _.get(data, 'FailedRecordCount', 0);

      if (failedRecord > 0) {

        logger.warn(`There are ${data.FailedRecordCount} failed`);

        data.Records.forEach((record, index) => {
          if (_.has(record, 'ErrorCode') || _.has(record, 'ErrorMessage')) {
            logger.debug(record);
            logger.debug(payloadRecords.Records[index]);

            records.push(payloadRecords.Records[index]);
          }
        });
      }
    });
  };

  let putRecord = (record) => {
    let payload = {
      Data: JSON.stringify(record),
      PartitionKey: String(numbers.random() * 100000)
    };
    records.push(payload);

    if (records.length >= config.kinesis.maxRecords) {
      sendRecords();
    }
  };

  return {
    putRecord: putRecord,
    recordCount: () => {
      return records.length;
    },
    clearRecord: () => {
      records = [];
    },
    init: () => {
      setInterval(() => {
        if (!_.isEmpty(records)) {
          sendRecords();
        }
      }, 200);
    }
  };
};

There is no obvious bug but;

You are defining functions both within the return structure and outside of it, which makes it hard to read/parse, just keep it clean and define everything outside of it
You are sending records in a function called putRecord, that is not good.
500 should probably be a constant retrieved with config, same thing for 200
Using numbers.random() sounds like a terrible idea, you should use a library that generates GUID's (https://www.npmjs.com/package/guid)
Why assign config.kinesis.streamName to streamName, just assign it straight in the structure. It is one less line of code, and the reader does not have to wonder where it comes from
I would have re-ordered the 500 check and the assignment:

records  = records.splice(0, 500);

let payloadRecords = {
  Records: records,
  StreamName: streamName
};

instead of

let payloadRecords = {
  Records: records,
 StreamName: streamName
};

if (records.length >= 500) {
  const pushingToKinesis = records.splice(0, 500);
  payloadRecords.Records = pushingToKinesis;
} else {
  records = []
}

personally I would even go for

//Make sure we never send more than 500 records, splice keeps the rest in records
let payloadRecords = {
  Records: records.splice(0, 500),
  StreamName: streamName
};

Finally, I am not a big fan of perpetually re-sending the data. If the data gets rejected because of data-content, then at some point if you reach 500 entries that will never be stored, your pipe will filled with junk and you will stop transmitting store-able data.


Simplifying Dr.Racket drawing code I am using Dr.Racket, Intermediate Student with Lambda. I was wondering if there was any way I can simplify this code using any sort of method like lambda, abstraction, map, filter, etc.

 ; DRAWING FUNCTIONS

 ; draw-rocket: rocket scene --> scene
 ; Purpose: To draw the given rocket in the given scene
 (define (draw-rocket a-rocket a-scene)
 (place-image rocket-img a-rocket ROCKET-Y a-scene))

 ; draw-alien: alien scene --> scene
 ; Purpose: To draw the given alien in the given scene
 (define (draw-alien an-alien a-scene)
 (place-image alien-img
   (posn-x an-alien)
   (posn-y an-alien)
   a-scene))

    ; draw-aliens: loa scn --> scene
    ; Purpose: To draw the aliens in the given scene
    (define (draw-aliens a-loa scn)
    (cond [(empty? a-loa) scn]
    [else (draw-alien (first a-loa) (draw-aliens (rest a-loa) scn)
                  )]))
;

; draw-shot: shot scene --> scene
; Purpose: To draw the given shot in the given scene
(define (draw-shot a-shot scn)
(place-image SHOT-IMG (posn-x a-shot) (posn-y a-shot) scn))

; draw-aliens: loa scn --> scene
; Purpose: To draw the aliens in the given scene
(define (draw-shots a-los scn)
(cond [(empty? a-los) scn]
[else (draw-shot (first a-los) (draw-shots (rest a-los) scn)
             )]))

; draw-world: world --> scene
; Purpose: Draw the world in the empty scene
(define (draw-world a-world)
(draw-rocket (world-rocket a-world)
(draw-aliens (world-aliens a-world)

There are significant issues here:

; draw-rocket: rocket scene --> scene
; Purpose: To draw the given rocket in the given scene
(define (draw-rocket a-rocket a-scene)
(place-image rocket-img a-rocket ROCKET-Y a-scene))

Why break the pattern from drawing code for the others? Use (pos-x a-rocket), (pos-y a-rocket) instead of a-rocket ROCKET-Y. That will help you abstract the code later on.

; draw-aliens: loa scn --> scene
; Purpose: To draw the aliens in the given scene
(define (draw-shots a-los scn)
(cond [(empty? a-los) scn]
[else (draw-shot (first a-los) (draw-shots (rest a-los) scn)
         )]))

This method has the wrong purpose and signature. This should be:

; draw_shots: los scn --> scn
; Purpose: To draw the shots in the given scene

This code seems to have multiple issues:

; draw-world: world --> scene
; Purpose: Draw the world in the empty scene
(define (draw-world a-world)
(draw-rocket (world-rocket a-world)
(draw-aliens (world-aliens a-world)

The parentheses are mismatched, as mentioned in the comments.
Why are the draw-shots methods not called here?

I also think that you want to nest the calls so that the results are cumulative.
This is done like so:

; draw-world: world --> scene
; Purpose: Draw the world in the scene (why would does it have to be empty?)
(define (draw-world a-world)
(draw-shots (world-shots
(draw-rocket (world-rocket
(draw-aliens (world-aliens a-world)))))))

As for using higher-order functions:

Try using foldr like so:

(define (draw-shots a-los scn)
(foldr draw-shot (first a-los) (rest a-los)))

Similarly, you can apply this to all the functions on lists.

Finally, using abstraction is very easy here. Create a new type, called Sprite or Actor or something similar, and have Aliens, Shots and Rocket all extend it. That way you only need 2 functions: 1 for singular, 1 for plural.


Responsive / CSS fixed and variable widths CSS and responsiveness in multiple columns with fixed and scaleable elements can be done in many ways.

I have created a solution that seems to work, though I have no idea whether this is best practice.

Fiddle

CSS

html, body { margin:0 auto; padding:0;  background: #fff; text-align: center; }

/* Clearfix
============================================================================ */
.CF { display:inline-block;overflow:hidden; }

/* Elements
============================================================================ */
div#container {max-width: 1140px;  min-width: 960px; margin:0 auto; margin-top: 10px; padding:0;  background:#0F9;  position:relative;}
    div#left-menu {width: 100px; background:#F30; position: absolute; top:0; left:0;   }
    div#information {padding: 10px 10px 25px 10px; background:#39C;  margin-left:100px;}
        div#information-wrapper {position:relative; background:#3FF; }
            div#information-left-menu {width: 125px;  background:#C30; position: absolute; top: 0; left:0;}
            div#content {background:#FC0; margin-left: 125px; text-align:left;}

HTML

Left 100px wide

Fixed width of 125px

text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text text

To have a responsive web design, you need to do more than have no horizontal scrolling when viewed with a desktop browser.  You have to adapt to the viewport of any device, from the really small (phone) to the really big (desktop).  This code does not, sorry.

Responsive web design is typically achieved by using media queries (there are other ways, but they're unavailable in IE versions older than 10), which are completely absent from your code.  I suggest you take the time to learn about what responsive web design is:

http://bradfrostweb.com/blog/post/7-habits-of-highly-effective-media-queries/
http://www.smashingmagazine.com/responsive-web-design-guidelines-tutorials/
http://www.html5rocks.com/en/mobile/responsivedesign/
http://thesiteslinger.com/blog/responsive-design-why-youre-doing-it-wrong/
http://blog.cloudfour.com/the-ems-have-it-proportional-media-queries-ftw/

That said, there are other things that are not good here:

Absolute positioning of the "left menu".  In general, absolute positioning should be avoided unless it is absolutely necessary (eg. drop menus, etc.).  Absolutely positioned elements can become cut off if there's not enough surrounding content to prevent it from overflowing its ancestor elements.  Multi-column layouts can easily be done using floats or the table display properties (eg. display: table-cell) and the content won't get cut off.
Using px to restrict the width of text containing elements.  If the user needs to increase their font-size for accessibility reasons, 100-125px is no longer an appropriate sized container for that text.  You should be using ems or other relative units instead.
No semantic markup.  With HTML5 (see: http://html5doctor.com/), a whole slew of new semantic container elements have been added (eg. article, nav, aside, section) which may or may not be more appropriate than the general purpose div (I can't tell because there's no real content here).  Markup should be chosen to describe the content first, then you can worry about how to make it pretty.


Cycle through an array of available colors repeatedly I have an array of colors for my app to choose from. My goal was to have the app cycle through the array of colors, apply it to my object, and reset to the beginning once each color has been used. This is simple enough to implement, but I am looking to see if it's possible to clean it up slightly. The one complication I have is that I am trying to prevent duplicate colors.

E.g. If I choose to use the size of the array of the object that has the color affected - when an item (or multiple) is deleted and a new one is added, it might have a duplicate color as the previous card leading to a visual design that I don't want.

To prevent this I decided to make a new counter variable that counts each time a new card is created, regardless of deletions. I feel like there might be a more elegant solution that I am not thinking of so any feedback or alternative approaches would be very appreciated!

All of the necessary code can be seen here:

class BrowseHabitsActivity : Activity() {
    private lateinit var recyclerView: RecyclerView
    private lateinit var addHabitButton: FloatingActionButton
    private val habits: ArrayList<Habit> = Habit.createHabitList(25)

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(R.layout.activity_browse_habits)

        recyclerView = findViewById(R.id.rvHabits)
        recyclerView.layoutManager = LinearLayoutManager(this)
        recyclerView.adapter = BrowseHabitsAdapter(habits)

        var cardCount = 0

        addHabitButton = findViewById(R.id.addHabitButton)
        addHabitButton.setOnClickListener {
                habits.add(Habit("New Habit", 1234, chooseCardColor(cardCount++)))
                (recyclerView.adapter as BrowseHabitsAdapter).notifyItemInserted(habits.size)
        }
    }

    private fun chooseCardColor(cardCount: Int): Int {
        val colors = resources.getIntArray(R.array.cardColors)
        return colors[cardCount % colors.size]
    }
}```

The code you've written seems to be reasonably elegant (to me at least!). You can make this slightly simpler by:

storing the colors array rather than getting each time (I've used a by lazy but you could use a nullable var or a lateinit var if you prefer)
Store the cardCount elsewhere rather than locally and incrementing it each time

I've put an example of these changes below. I would strongly recommend extracting out the color logic into its own class in order to make it more testable, reusable and better separation of concern.
private val colors: IntArray by lazy{
    resources.getIntArray(R.array.cardColors)
}

private val cardCount = 0

private fun getCardColor(): Int {
    val color = colors[cardCount % colors.size]
    cardCount.inc()
    return color
}

Also, you could use the @ColorInt annotation to make it clear that the Int being returned is a ColorInt.


Extract Students with All Enrolments in the Past I am trying to extract the students where all of their enrolments are in the Past.

Some students can have more than one Enrolment and one of its End Date is in the future while the other one is in the past

In the above Sample Data, I want to get only one Record 'BBB' in my result.

I managed to achieve it by using CTE by comparing no. of Total Records vs no. of Total Records in the Past.

WITH T1 AS
(SELECT
    RefNo
    , Total = COUNT(RecordID)
    , TotalPastRecords = SUM(IIF(EndDate < GETDATE(), 1, 0))
FROM
    Enrolments
GROUP BY
    RefNo)
SELECT
  *
FROM
  T1
WHERE
  Total = TotalPastRecords

Could you guy please review the above code and let me know if there is a way to improve it?

Our Enrolment Table contains around 200,000 rows and this query is going to join to another 4-5 tables to get the final Result.

Well, since you said that you are going to join to some other tables it seems like you only need the RefNo that meets your criteria. You can use HAVING and remove the CTE all together.

DECLARE @Enrollments TABLE(RecordID int, RefNo char(3), EndDate datetime)
INSERT INTO @Enrollments VALUES
(1,'AAA','1/1/2018'),
(2,'AAA','1/2/2018'),
(3,'BBB','1/1/2016'),
(4,'BBB','1/5/2016'),
(5,'CCC','1/5/2016'),
(6,'CCC','1/1/2018')

--Get the RefNo's that don't have a future date
SELECT
    RefNo
FROM
    @Enrollments
GROUP BY
    RefNo
HAVING MAX(EndDate) < CAST(GETDATE() AS DATE)

--Get all rows for RefNo's that don't have a future date
SELECT
    RecordID
    ,RefNo
    ,EndDate
FROM
    @Enrollments
WHERE
    RefNo IN(
             SELECT RefNo
             FROM @Enrollments
             GROUP BY RefNo
             HAVING MAX(EndDate) < CAST(GETDATE() AS DATE)
             )

If you are just needing the RefNo's to limit results of another table. Then you can use the first query with IN similarly to how the second query is written.

You can see the execution plan here comparing the CTE method you have compared to the sub-query I provided in the second query. On my instance, the plan broke down as follows:

Query Cost (Relative to the Batch)
INSERT INTO @Enrollments....24%
SUBQUERY Example using IN...16%
INSERT INTO @Enrollments....24%
Query with CTE Example......36%

So, the CTE was slower. Granted a lot is going to come into play in your live environment with indexes, what joins you are doing etc. But at least this is another option you can consider.

Lastly, depending on your indexes and joins it's sometimes faster to bring stage a temporary result set in a CTE, Table Variable or Temp Table. In most of my cases, Temp Tables have been the fastest but this could be different for your environment. You already have the CTE method down, though I would do it slightly different. Here are some examples.

--stage the RefNo in a temp table
IF OBJECT_ID('tempdb..#RefNo') IS NOT NULL DROP TABLE #RefNo

SELECT
    RefNo
INTO #RefNo
FROM
    @Enrollments
GROUP BY
    RefNo
HAVING MAX(EndDate) < CAST(GETDATE() AS DATE)

SELECT
    ...
FROM
    YourTable t
    INNER JOIN
    #RefNo n on t.RefNo = n.RefNo

--use a CTE to limit the RefNo. Understand the optimizer may not logically execute this how it is written
WITH cte AS(
    SELECT
        RefNo
    FROM
        @Enrollments
    GROUP BY
        RefNo
    HAVING MAX(EndDate) < CAST(GETDATE() AS DATE))

SELECT
    ...
FROM
    YourTable t
    INNER JOIN
    cte c on t.RefNo = c.RefNo

--using a table variable, which I wouldn't do in most cases
DECLARE @RefNo TABLE (RefNo CHAR(3))

INSERT INTO @RefNo
    SELECT
        RefNo
    FROM
        @Enrollments
    GROUP BY
        RefNo
    HAVING MAX(EndDate) < CAST(GETDATE() AS DATE)

SELECT
    ...
FROM
    YourTable t
    INNER JOIN
    @RefNo rn on t.RefNo = rn.RefNo


Need help optimizing & improving this php form script I usually program in python/javascript and am very new to php. I would appreciate some advice on how to improve the following form script.

Since this is for a highly trafficked website, I want it to be extremely secure, reliable and optimized.

Feel free to point out minor issues as well.

<?php

// avoid errors when in production
// ini_set("display_errors", "0");
error_reporting(-1);

# get value from array
function get_string($array, $index, $default = '') {
    if (isset($array[$index]) && strlen($value = trim($array[$index])) > 0) {
        return get_magic_quotes_gpc() ? stripslashes($value) : $value;
    } else {
        return $default;
    }
}

function test($field) {
    global $errors, $required_error;
    if ($field['value'] == '') {
        if ($field['required']) {
            $required_error = true;
        }
        return;
    }
    elseif ($field['validation'] == 'email' && !filter_var($field['value'], FILTER_VALIDATE_EMAIL)) {
        array_push($errors, $field['value'] . ' is an invalid email address.');
    }
    elseif ($field['validation'] == 'number' && !preg_match('/^[0-9 ]+$/', $field['value'])) {
        array_push($errors, $field['value'] . ' is an invalid number.');
    }
    elseif ($field['validation'] == 'alpha' && !preg_match('/^[a-zA-Z ]+$/', $field['value'])) {
        array_push($errors, $field['value'] . ' contains invalid characters. This field only accepts letters and spaces.');
    }
}

$post = filter_input_array( INPUT_POST, FILTER_SANITIZE_SPECIAL_CHARS );
$errors = array();
$required_error = false;
$ajax = get_string($post, "request_method") == 'ajax';

# select data that needs validation
$fields = array(
    'fullname' => array(
        'value' => get_string($post, "full-name"),
        'validation' => 'alpha',
        'required' => true,
    ),
    'emailaddress' => array(
        'value' => get_string($post, "email-address"),
        'validation' => 'email',
    ),
    'activites' => array(
        'value' => get_string($post, "activites"),
        'validation' => 'default',
    ),
    'country' => array(
        'value' => get_string($post, "country"),
        'validation' => 'default',
    ),
    'contactpreference' => array(
        'value' => get_string($post, "contact-preference"),
        'validation' => 'default',
    ),
    'message' => array(
        'value' => get_string($post, "message"),
        'validation' => 'alpha',
    ),
);

// test each field
foreach ($fields as $field) {
    test($field);
}

if (count($errors) or $required_error) {
    if ($ajax){
        echo 'FAIL';
        return;
    }

    $template_dict = array(
        'errors' => ''
    );

    if ($required_error == true) {
        $template_dict['errors'] .= "
Please fill out all required fields
";
    }

    foreach ($errors as $error) {
        $template_dict['errors'] .= "
$error
";
    }

    // show errors
    echo $template_dict['errors'];
} else {
    $template_dict = $fields;

    // send mail
    $formcontent = "Full Name: $fullname \nEmail Address: $emailaddress \nActivites: $activites \nCountry: $country \nContact preference: $contactpreference \nMessage: $message \n";
    $formcontent = wordwrap($formcontent, 70, "\n", true);

    $recipient = 'email@address.com'; $subject = "Form Name"; $mailheader = "From: $emailaddress \r\n";

    mail($recipient, $subject, $formcontent, $mailheader);

    if ($ajax) {
        echo 'PASS';
        return;
    }

    echo 'Success';
}
?>

I've tried to address as many things as I could think of, though I'm not sure how relevent a lot of them will be :).

Anyway, on a lot of things I just glossed over the subject for brevity, however, if you have any questions or want me to defend any statements, I gladly will if prompted.

error_reporting

I tend to leave error_reporting and display_errors out of PHP scripts as much as possible.  Those should ideally be server-wide settings more so than application specific.  A development server should always have error_reporting(E_ALL) and display_errors = On and a production server should always have error_reporting(E_ALL) and display_errors = Off.  If error reporting ever has to be lowered to accomdate variable doesn't exist or undefined index type notices, that's a sign of bad code.

Magic Quotes

If magic quotes is on, either turn it off, or if not possible, move servers.  Magic quotes is an ancient, horrible mistake PHP made, and no server admin should ever have it enabled.

get_string

You're doing too much in this function.  The name and the function signature gives the impression that it gets a string if it exists in an array, and otherwise returns the default.

What is actually happening though is much more than this.

Also, the logic in it is a bit misleading.  What if you call it with a default that is not the empty string, but then an empty string is in the array?  Because of the strlen check, the non empty default is going to be picked.  Also, I would not expect this function to be calling trim.

In my opinion, this function needs either better documentation, a better name, or both.  (Or pull the multiple behaviors into multiple functions.)

I would probably implement this function as follows:

function array_get(array $array, $key, $default = null)
{
    return (array_key_exists($key, $array)) ? $array[$key] : $default;
}

I might then build on top of this:

function array_get_string(array $array, $key, $default = '')
{
    $val = array_get($array, $key);
    return (is_string($val)) ? $val : $default;
}

Or, if you wanted to bake in the trim functionality:

function array_get_string(array $array, $key, $default = '', $trim = true)
{
    $val = array_get($array, $key);
    if (is_string($val)) {
        return ($trim) ? trim($val) : $val;
    }
    return $default;
}

Another example of how you could build on it:

function array_get_digits(array $array, $key, $default = 0)
{
    $val = array_get($array, $key);
    if (is_int($val) || ctype_digit($val)) {
        return (int) $val;
    } else {
        return $default;
    }
}

test

Globals are evil.  I'm too lazy to back it up in this post, but the key points:
* ties your code to them (you must always use variables named $errors and $required_errors.  What if you want to call test and have it populate a different set of variables?)
* your code no longer gets along with code you didn't write.  What if a different piece of code happens to use $errors and your code silently clobbers it?
* much harder to maintain -- it's difficult to follow the mutation of a variable when it's done in a far off place with no easily traceable execution flow

What you should do instead is to return an array containing the errors.

Also, test is a bad name.  What are you testing?  Perhaps validateField or testInputField or something along those lines.

When sanitizing should happen

$post = filter_input_array( INPUT_POST, FILTER_SANITIZE_SPECIAL_CHARS );

This is done too early.  Your validations should see text as it actually is, not as valid HTML.  SANITIZE is a bit of a misnomer here.  It should actually be something like FILTER_ESCAPE_HTML or something.  HTML escaping only makes sense in the context of HTML.  Your validations are seeing text, not HTML.

This becomes a lot more important if you're storing information in a database.  Once you store text transformed, it becomes harder to search for it, and it becomes harder to predict the length of it.

The length could actually apply to your situation as well.  a & b is 5 characters, however, a &#38; b is 9.  What if you had a field that had a max of 5 characters?  Suddenly the user is very confused as to why his input is failing.  (Also, a &#38; b is meaningless outside of HTML.)  Since your email is plaintext, you would actually see &#38; in your email.

In short, escape at the last possible moment.

Random design input

I might approach your forms a bit more abstractly.  For example, I might take your array idea ($fields) and consider that a "form".  Then a validateForm function could take all of the elements in the form and check them for errors.  This function would return an array of any errors (or perhaps: array('errors' => [errors], 'valid' => [valid field's values]).  This would still use most of the same code you have now, just structed a bit differently.

Also, I would consider making 'validation' be an array.  What if at some point in the future you want to apply multiple validations to a field?  ("If it's wrong in way X, show one message; if it's wrong in way Y, show a different message").

Another design point: I would make 'required' be handled like any other error.  There's not really a reason to only validate until you come across the first empty required field, and this requires complicating the error handling code.

Type strictness

PHP's implicit type system is a bit odd at times.  For example, 0 == '' is true.  That means that in situations like $field['value'] == '' you should likely use $field['value'] === ''.

Or perhaps:

$field['value'] === '' || $field['value'] === null

I highly advise learning the type system and related materials (isset/empty/array_key_exists/etc).  PHP has lots of little quirks that can bite you (and have bitten me many times before I became paranoid :p).

Optimizing

The script looks fine in terms of performance.  The main time-taker in it will be the mail() call, and assuming your mail server is quick to respond, that shouldn't be significant.

mail

I like to use packages like Zend_Mail or PHPMailer.  For a simple email with a simple server setup, though, mail should be fine.  (For example, mail and SSL would be an unpleasant combination.)

I might worry about whether or not mail succedes though.  Might be nice to warn users that mailing failed.

Style

This section is 100% opinion

array_push is pretty rare to see in modern code.  There's borderline cases where it's actually better, but for visual reasons, most people prefer the $array[] syntax.

The following are equivalent:

$arr[] = 5;
array_push($arr, 5);

if (count($errors) or $required_error)

Would usually be written as:

if (count($errors) || $required_error)

(Though or and || are not interchangeable, they are in this situation.  In situations that do not explicitly call for or, || is typically used.)

if ($required_error == true)

Is a bit redundant.  It should just be:

if ($required_error)

The only time to use == for true is when you're strictly checking:

if ($required_error === true) //true iff $required_error is boolean true.  For example, $required_error = 1 would be false because 1 !== true.

I wouldn't mix # and // comments.  Just a being-picky thing, but it looks a bit odd to have both in one file (or really both in one codebase).  // is pretty much the standard in PHP, though both are just as well supported.

I would consider using heredoc style comments.  They can be parsed by IDEs like Netbeans and can be used to generate HTML documentation.  For example:

/**
 * Takes an array, an index for the array and an optional $default and returns $array[$index]
 * if it exists, or otherwise $default.
 *
 * @param array $array The array from which to select the data
 * @param string|int $index The index of the array
 * @param mixed $default The value to return if $index is not in $array
 * @return string $array[$index] if it exists, otherwise, $default
*/
function get_string(array $array, $index, $default = '')

Depending on how clear names are, documentation is often not needed for small code bases, but it's very helpful when you come back a year later and have to figure out what's going on in a function.

Note also that I made it so that $array must be an array.  This is only possible in PHP for certain types (array and classes).


Two approaches to count occurrences of one char in a String in Java I have two approaches:

First Approach:

int count =0;
for(int i = 0; i < text.length(); i++) {
    if(text.charAt(i) == charToCheck){
        count++;
    }
}
System.out.println(count);

Second Approach:

count = text.length() - text.replaceAll(
                             String.valueOf(charToCheck),"")
                            .length();
System.out.println(count);

Which one is fast w.r.t Time & Space Complexity? Why?

Are there any more approaches (which I am not aware of)? If yes, Please let me know.

As I stated in the comments, both of your approaches have a time complexity of O(n), but your first approach has a O(1) space complexity whilst your second one requires O(n) space in the worst case, because replaceAll creates a copy with all the matches removed.

From complexity point of view, your first approach is basically ideal. In order to replace all occurrences of a char, you at least have to iterate every char in your input once, which corresponds to O(n) time complexity. Also, space complexity is, as stated, O(1) which is, obviously, the best case.

[Note: Java makes no guarantees about the time and space complexity of methods. Thus, replaceAll could, in theory, also have a much worse time and space complexity if not implemented well. I assumed that it runs in O(n) (which is the best case) when matching on a single char only. This fact, of course, makes the second approach even worse.]


dict get with condition in Python foo = {}
print 'yes' if foo.get('bar', None) else None

Is there any way if I can improve this code?

Logic: if bar key is there, print 'yes' else print None

You can generalize what you want and create a simple function which returns yes if the key is present in your dict, otherwise returns None:

def key_exists(foo, key):
    """
    foo -> dictionary
    key -> lookup key
    Return foo if key exists in foo. Otherwise, return None
    """
    return 'yes' if key in foo else None

print(key_exists({'a': 'b'}, 'a'))
print(key_exists({'a': 'b'}, 'c'))
print(key_exists({}, 'c'))

The output for the above:

yes
None
None

If you, however, don't need a special message if the key is being found in the dict, and want to return a default value if it isn't found, you can do:

def key_exists(foo, key):
    """
    foo -> dictionary
    key -> lookup key
    Return foo if key exists in foo. Otherwise, return None
    """
    return foo.get(key, None)

The output for the above:

b
None
None

PS: Note that above (as Graipher mentioned) it's enough to directly return foo.get(key) as None is the default value anyway.

You usually want to use dict.get(key[, default]) when you need to assign default values. When get() is called, Python checks if the specified key exists in the dict. If it does, then get() returns the value of that key (which is not exactly what you want). If the key does not exist, then get() returns the value specified in the second argument to get(). That being said, you should go with the first solution

Apart from that, you should also know that doing 'key' in dict is faster than any other method. The time to find a key using this method is \$O(1)\$ whereas calling keys would generate a list and be \$O(n)\$


Single function to read one, read all, or set I have the below function, it is designed so I can call:

$object->temp('one', 1);    //add the key `one` to the temp array with value 1
$object->temp('two', 2);    //add the key `two` to the temp array with value 2
$object->temp('three', 3);  //add the key `three` to the temp array with value 3

$object->temp('three');       //returns 3
$object->temp('three', null); //removes the `three` key
$object->temp();              //returns array('one' => 1, 'two' => 2)

and it works well, my only gripe is that I don't like using the $value = 'read_temp in the definition, it feels like a hack to me.

So, is there anything wrong with that, and how can this function be optimized?

public function temp($key = null, $value = 'read_temp') {
    if($key === null)
        return $this->temp;

    if ($value === null){
        unset($this->temp[$key]);
    }

    elseif ($value == 'read_temp' && isset($this->temp[$key])) {
        $value = $this->temp[$key];
    } else {
        $this->temp[$key] = $value;
    }

    return $value;
}

I could split this into two setTemp getTemp (or three clearTemp to be pedantic) functions, but I would like to be able to have just one function call to do everything.

Here's one way to do it

public function temp($key = null , $value = null) {
    $args = func_get_args();
    if( count($args) === 2 ) {
        $this->temp[$key] = $value;
    } elseif( count($args) === 1 ) {
        return @$this->temp[$key];
    } else {
        return $this->temp;
    }
}

func_get_args() will return an array of the arguments that were actually passed - i.e. excluding default values. So if no arguments were passed, return the array; if there's 1 argument (the key), return the corresponding value; and if there are 2 arguments, set the key/value pair.

I'm using the warning-suppression @ instead of explicitly checking isset(). Again, the end result is the same; if you check with isset() and don't (explicitly) return anything for missing keys, the function still returns NULL. If you simply return $this->temp[$key]with no checking, but suppress the "undefined index" warning that might occur, you also get NULL.

I've also skipped the unset() call, since calling $obj->tmp("key", null) does pretty much the same thing. If you request a value that hasn't been set, you get NULL. If you set that value to NULL, you get NULL. If you were to unset the value, you'd also get NULL.
But if you want to keep the unset, you can just check $value === null in the first branch:

if( count($args) === 2 ) {
    if( $value === null ) {
        unset($this->temp[$key]);
    } else {
        $this->temp[$key] = $value;
    }
} // ...

If you skip the unset() part, it'll obviously keep the array keys around, so you won't know for sure if a key whose value is NULL was "deleted", or if it was explicitly set to NULL on purpose.
On the other hand, if you unset() when the passed value is NULL, you won't be able to set a value to NULL on purpose.

The simplest solution is to not use unset in the temp() function, but instead add an extra unset_temp_value($key) function that does the unset. Then you can use that to explicitly remove all traces of a key/value pair, while still being able to store NULLs "on purpose" using temp($key, null).

As mseancole points out below, a switch statement could be used too

public function temp($key = null, $value = null) {
  $args = func_get_args();
  switch( count($args) ) {
    case 2:
      return $this->temp[$key] = $value;
    case 1:
      return @$this->temp[$key];
    default:
      return $this->temp;
  }
}

Only difference is that this returns the value that's set in the first case (just because it's more compact then breaking)

And just for fun, here's the same thing again in Unreadable Mode™

public function temp($key = null, $value = null) {
  $args = count(func_get_args());
  return ($args==2?($this->temp[$key]=$value):($args==1?@$this->temp[$key]:$this->tmp));
}


Where to define constants (Bundle Keys) used between 2 Activities? public class FirstActivity extends Activity {

    private String userName;
    private Uri userPhoto;

    private void onOkButtonPressed() {
        Intent intent = new Intent(this, SecondActivity.class);
        intent.putExtra(SecondActivity.Key.USER_NAME, userName);
        intent.putExtra(SecondActivity.Key.USER_PHOTO, userPhoto);
        startActivity(intent);
    }

}

public class SecondActivity extends Activity {

    public static class Key {
        public static final String USER_NAME = "USER_NAME";
        public static final String USER_PHOTO = "USER_PHOTO";
    }

    private String userName;
    private Uri userPhoto;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.container);
        init();
    }

    private void init() {
        Intent intent = getIntent();
        userName = intent.getStringExtra(Key.USER_NAME);
        userPhoto = intent.getParcelableExtra(Key.USER_PHOTO);
    }

}

Or maybe it would be better to make the keys private inside SecondActivity and do something like this?

public class SecondActivity extends Activity {

    private static class Key {
        public static final String USER_NAME = "USER_NAME";
        public static final String USER_PHOTO = "USER_PHOTO";
    }

    public static void start(Context context, String userName, Uri userPhoto) {
        Intent intent = prepareIntent(context, userName, userPhoto);
        context.startActivity(intent);
    }

    public static Intent prepareIntent(Context context, String userName, Uri userPhoto) {
        Intent intent = new Intent(context, SecondActivity.class);
        intent.putExtra(Key.USER_NAME, userName);
        intent.putExtra(Key.USER_PHOTO, userPhoto);
        return intent;
    }

    // skipped

}

It slightly improves readability:

SecondActivity.start(this, userName, userPhoto);

But on the other hand, it becomes complicated to make changes. Every time when I want add new fields or change existing fields, I should also change the arguments list in the method start(). And this way is also not suitable in the case when number of the input parameters will increase significantly.

One thing that I find is that it is not necessary to wrap your keys inside their own inner class. Putting them inside SecondActivity directly is enough.

public class SecondActivity extends Activity {
    public static class Key {
        public static final String USER_NAME = "USER_NAME";
        public static final String USER_PHOTO = "USER_PHOTO";
    }
    ...

Becomes:

public static final String USER_NAME = "USER_NAME";
public static final String USER_PHOTO = "USER_PHOTO";

  Every time when I want add new fields or change existing fields, I should also change the arguments list in the method start(). And this way is also not suitable in the case when number of the input parameters will increase significantly.

You are correct about that.

Even though I have to say that I do like your idea of a prepareIntent method.

I do feel that it is more common to do your first version: (but without using the Key inner class).

private void onOkButtonPressed() {
    Intent intent = new Intent(this, SecondActivity.class);
    intent.putExtra(SecondActivity.USER_NAME, userName);
    intent.putExtra(SecondActivity.USER_PHOTO, userPhoto);
    startActivity(intent);
}

One thing I really have to point out is that this method is a bit overkill:

public static void start(Context context, String userName, Uri userPhoto) {
    Intent intent = prepareIntent(context, userName, userPhoto);
    context.startActivity(intent);
}

I would prefer to have the parent activity call prepareIntent, which gives you the possibility of changing or adding some of the extras passed to the intent. I also find it better because you can in a parent activity call startActivity directly without the need for context.startActivity

startActivity(SecondActivity.prepareIntent(userName, userPhoto));

To summarize: (My opinions at least)

Drop the start static method entirely
Drop the Key inner class and put the constants directly into SecondActivity
It seems to be more common to create the intent inside the parent activity

In the end, which way you choose is up to you.


Simple streaming parser to extract columns In reply to previous question I rewrote ColumnReader and would like more suggestions

using System;

namespace R2D.IO
{
    public sealed class LineReader
    {
        public string NewLine { get; set; }

        string _buffer = "";

        public void Parse(string text)
        {
            _buffer += text;

            var lastline = _buffer.LastIndexOf(NewLine);
            if (lastline == -1)
                return;

            var lines = _buffer.Substring(0, lastline).Split(NewLine.ToCharArray(), StringSplitOptions.RemoveEmptyEntries);
            _buffer = _buffer.Substring(lastline);

            foreach (var line in lines)
                Receive(line);
        }

        public event Action<string> Receive = (line) => { };
    }

    public sealed class ColumnReader
    {
        public string Delimiter { get; set; }
        public string NewLine
        {
            get { return reader.NewLine; }
            set { reader.NewLine = value; }
        }

        private LineReader reader = new LineReader();

        public ColumnReader()
        {
            reader.Receive += (line) => Receive(line.Split(Delimiter.ToCharArray()));
        }

        public event Action<string[]> Receive = (columns) => { };
    }
}

My main use-case is reading lines from a SerialPort:

var parser = new IO.LineReader { NewLine = port.NewLine };
port.DataReceived += (o, e) => parser.Parse(port.ReadExisting());
parser.Receive += (line) => { };

You can use the Microsoft.VisualBasic.FileIO.TextFieldParser and attach the serial port to a StreamReader and use that as input for the TextFieldParser.


Determining the similarity between two documents I've made some code that reads in text files (which hold quite large vectors of word frequencies), which in turn stores each index of a vector within an ArrayList for their specified team (i.e. Arsenal vector to the Arsenal ArrayList, Chelsea vector the Chelsea ArrayList, etc.). It then uses a cosine similarity function to determine similarity between the two documents and writes it to a file.

What I would like is to make the code that reads in the text files (and storing them in their corresponding ArrayList more efficient), rather than me change the parameters of the while loop each time i need to use it.

public static Double cosineSimilarity(ArrayList<Integer> vectorOne, ArrayList<Integer> vectorTwo) {

    Double dotProduct = 0.0;
    Double normVecA = 0.0;
    Double normVecB = 0.0;
    for(int i = 0; i < vectorOne.size(); i++) {
        dotProduct += vectorOne.get(i) * vectorTwo.get(i);
        normVecA += Math.pow(vectorOne.get(i), 2);
        normVecB += Math.pow(vectorTwo.get(i), 2);
    }

    return dotProduct / (Math.sqrt(normVecA) * Math.sqrt(normVecB));

}

public static void main(String[] args) throws IOException {

    ArrayList<Integer> arsenal = new ArrayList<Integer>();
    ArrayList<Integer> chelsea = new ArrayList<Integer>();
    ArrayList<Integer> liverpool = new ArrayList<Integer>();
    ArrayList<Integer> manchesterCity = new ArrayList<Integer>();
    ArrayList<Integer> manchesterUnited = new ArrayList<Integer>();
    ArrayList<Integer> tottenham = new ArrayList<Integer>();

    Scanner textFile = new Scanner(new File("Enter textfile  here"));

    while (textFile.hasNext()) {
        arsenal.add(textFile.nextInt());
   }

    Double output = cosineSimilarity(arsenal, chelsea);

    File fileName;
    FileWriter fw;

    // Create a new textfile for listOfWords
    fileName = new File("arsenalCosineSimilarities.txt");
    fw = new FileWriter(fileName, true);

    fw.write(String.valueOf("Chelsea: " + output + "\n"));

    fw.close();
}

For flexibility, your cosineSimilarity() method should taken in List<Integer> arguments, instead of ArrayList<Integer> arguments.  This method doesn't care how the list is stored, only that it is a list which implements .size() and .get(i) methods.

For efficiency you should used double variables, not Double objects in the method:

public static double cosineSimilarity(List<Integer> vectorOne,
                                      List<Integer> vectorTwo)
    double dotProduct = 0.0;
    double normVecA = 0.0;
    double normVecB = 0.0;

When you open a Scanner, you should .close() it, to prevent resource leaks.  The "try-with-resources" construct will automatically close resources that it opens.  So instead of:

Scanner textFile = new Scanner(new File("Enter textfile  here"));

use

try(Scanner textFile = new Scanner(new File("Enter textfile  here")) {
    // use textFile inside this block.
}
// textFile is automatically closed when the block is exited.

Ditto with FileWriter.  Use "try-with-resources" to automatically close the writer.

try(FileWriter fw = new FileWriter(fileName, true)) {
    fw.write( ... );
}
// fw has been automatically closed at this point.

If you are using .nextInt(), you should loop on .hasNextInt(), not .hasNext().

while(textFile.hasNextInt()) {
     arsenal.add(textFile.nextInt());
}

It sounds like you want a Map<String,List<Integer>> to store an ArrayList for each team.

List<String> team_names = List.of("Arsenal", "Chelsea", "Liverpool",
        "ManchesterCity", "ManchesterUnited", "Tottenham");

Map<String,List<Integer>> stats = new HashMap<>();

// Read in all team stats from (for example) "<TeamName>_stats.txt" files

for(String team_name : team_names) {
   List<Integer> team_stats = new ArrayList<>();
   try (Scanner textFile = new Scanner(new File(team+"_stats.txt"))) {
      while(textFile.hasNextInt()) {
          team_stats.add(textFile.nextInt());
      }
   }
   stats.put(team_name, team_stats);
}

Then you can use stats.get(team_name) to get each team's stats for comparison / analysis

for(String team1_name : team_names) {
   List<Integer> team1_stats = stats.get(team1_name);

   for(String team2_name: team_names) {
      // Skip comparing a team against itself
      if (team1_name.equals(team2_name))
          continue;

      List<Integer> team2_stats = stats.get(team2_name);

      double output = cosineSimilarity(team1_stats, team2_stats);

      // ... display "output", or write to file, or ...
   }
}

Depending on how you want your output written (one file for all comparisons, or one file per team for a comparison with all other teams, you'll want to open the FileWriter before the outer loop, or before the inner loop.


Populating lists with links from arrays I have the following configuration:

$('document').ready(function () {
    var what0 = [
        ["http://validator.w3.org/unicorn/", "Unicorn"],
        ["http://validator.w3.org/", "W3C Validation Service"],
        ["http://jigsaw.w3.org/css-validator/", "W3c CSS Validator"],
        ["http://text-compare.com/", "Text Compare!"]
    ],
        what1 = [
            ["https://imgflip.com/", "Imgflip.com"],
            ["http://jpillora.com/base64-encoder/", "Base64 Encoder"],
            ["http://www.showmycode.com/", "Adobe Flash Decompiler"],
            ["http://jsbeautifier.org/", "Beautify Js"]
        ],
        what2 = [
            ["http://plnkr.co/edit/?p=catalogue", "Plunker"],
            ["http://www.css3maker.com/index.html", "CSS3 Generator"],
            ["http://daneden.github.io/animate.css/", "Animate.css"],
            ["http://site.youidraw.com/", "YouDraw"]
        ],
        what3 = [
            ["http://realfavicongenerator.net/", "Favicon Generator"],
            ["http://grabicon.com/", "Grabicon"],
            ["http://www.buildmypinnedsite.com/en", "Windows 8.1 tile"],
            ["https://kraken.io/web-interface", "Kraken.io"]
        ],
        what4 = [
            ["http://ideone.com/rran1v", "C++0x Compiler"],
            ["http://webcompiler.cloudapp.net/", "online C++ compiler"],
            ["http://www.color-hex.com/", "Color Hex"],
            ["https://icomoon.io/app/#/select", "IcoMoon App"]
        ],
        wh = [what0, what1, what2, what3, what4];

    function Toggle(elem) {
        var hiding = document.getElementById("hiding" + elem),
            buttonforhiding = document.getElementById("buttonforhiding" + elem);
        $(hiding).hide();
        $(buttonforhiding).click(function () {
            $(hiding).toggle();
        });
    }
    for (var a = 0; a <= 4; a++) {
        Toggle(a.toString());
    }

    function populate(one, two) {
        var where = document.getElementById("hiding" + one),
            what = two,
            length = what.length - 1;
        for (var b = 0; b <= length; b++) {
            var vector = what[b];
            $(where).append('
' + what[b][1] + '
');
        }
    }
    for (var c = 0; c <= 4; c++) {
        var d = wh[c];
        populate(c, d);
    }

});
ul {
    list-style-type:none;
}

Folder 1

Folder 2

Folder 3

Folder 4

Folder 5

Its role is to populate some lists with the links in the what0, what1, what2, etc. arrays. It is working fine, but I was interested if you can come up with a code that is more efficient.

The requirements are:

The input data is the arrays called "what*", from 0 to any given number of arrays.
The output is the folder-style type lists. Also, these lists are initially hidden and shown only onclick.
The script must be using only JavaScript and jQuery.

Your what variable would be more accessible if it was just an JavaScript Object (w3school javascript objects), instead of list of lists.

var what_object = {
    "what": [
        [
            {
                "link": "http://validator.w3.org/unicorn/",
                "text": "Unicorn"
            },
            {
                "link": "http://validator.w3.org",
                "text": "W3C Validation Service"
            },
            {
                "link": "http://jigsaw.w3.org/css-validator/",
                "text": "W3c CSS Validator"
            },
            {
                "link": "http://text-compare.com/",
                "text": "Text Compare!"
            }
        ],
        [
            {
                "link": "https://imgflip.com/",
                "text": "Imgflip.com"
            },
            {
                "link": "http://jpillora.com/base64-encoder/",
                "text": "Base64 Encoder"
            },
            {
                "link": "http://www.showmycode.com/",
                "text": "Adobe Flash Decompiler"
            },
            {
                "link": "http://jsbeautifier.org/",
                "text": "Beautify Js"
            }
        ]
    ]
};

I have only done indexes 0, and 1.

jQuery.fn.loadWhatList = function(list_location) {

    if (Number(list_location) >= 0) {
        var selected_list_section = what_object.what[list_location];
        if (typeof(selected_list_section) === 'undefined') {
            return false;
        };
        for (var y = 0; y < selected_list_section.length; y++) {
            var selected_link = selected_list_section[y]; // The selected link
            var compiled_link = jQuery("

");
            compiled_link.find('a').attr('href', selected_link.link);
            compiled_link.find('a').html(selected_link.text);
            // Can also do all of your Click Events in here too...
            compiled_link.click(function(e) {
                jQuery(this).toggle();
            });
            // Load the compiled_link to the ELEMENT
            jQuery(this).append(compiled_link);
        };
        return true;
    };

    return false;
};


Three variations on a Webpack build script I am new to "modern" Javascript, and I am reviewing some code from a frontend contractor. I'd like to make the sequential code more descriptive/procedural.

The following code is a from a file called build.js, which is used to start a new project build (via Webpack). I guess we could call that a "build script" of sorts:

import path from 'path';
import webpack from 'webpack';
import fse from 'fs-extra';
import config from './../webpack/webpack.config.production';
import paths from './../paths';

fse.emptyDirSync(paths.distPath, (err) => {
  if (err) console.error(err);
});

fse.copySync(
  paths.staticPath,
  path.join(paths.distPath, 'static'),
  { clobber: true },
  (err) => { if (err) console.error(err); }
);

webpack(config).run((err, stats) => {
  if (err) console.error(err);
  else console.log(stats.toString());
});

This glue code is sort of simple, if you know modern JS. Still, I find that the lack of high-level description or modularity will be a problem later on for junior JS people, or for bigger and more complicated scripts.

It looks like in the JavaScript world, people like to comment, and I often find this version:

Version 1

... imports ...

// Empty build folder
fse.emptyDirSync(paths.distPath, (err) => {
  if (err) console.error(err);
});

// Copy in static files
fse.copySync(
  paths.staticPath,
  path.join(paths.distPath, 'static'),
  { clobber: true },
  (err) => { if (err) console.error(err); }
);

// Build
webpack(config).run((err, stats) => {
  if (err) console.error(err);
  else console.log(stats.toString());
});

This seems to be the "most idiomatic JavaScript" code when looking at other open source codebases.

I am not used to writing scripts, but in my Ruby/Rails community, a lot of people prefer to write self-descriptive code instead of comments, at least for their business logic. I know this is a script and it differs from business logic, but the following makes me more comfortable:

Version 2

... imports ...

function main() {
  emptyBuildFolder();
  copyStaticFilesToBuildFolder();
  build();
}

function emptyBuildFolder() {
  fse.emptyDirSync(paths.distPath, (err) => {
    if (err) console.error(err);
  });
}

function copyStaticFilesToBuildFolder() {
  fse.copySync(
    paths.staticPath,
    path.join(paths.distPath, 'static'),
    { clobber: true },
    (err) => { if (err) console.error(err); }
  );
}

function build() {
  webpack(config).run((err, stats) => {
    if (err) console.error(err);
    else console.log(stats.toString());
  });
}

main();

Basic main() or init() technique. I like the modularity and it feels more maintainable.

Here's an OO version which is the same but with more syntactic sugar:

Version 3

... imports ...

new class {

  constructor() {
    this.emptyBuildFolder();
    this.copyStaticFilesToBuildFolder();
    this.build();
  }

  emptyBuildFolder() {
    fse.emptyDirSync(paths.distPath, (err) => {
      if (err) console.error(err);
    });
  }

  copyStaticFilesToBuildFolder() {
    fse.copySync(
      paths.staticPath,
      path.join(paths.distPath, 'static'),
      { clobber: true },
      (err) => { if (err) console.error(err); }
    );
  }

  build() {
    webpack(config).run((err, stats) => {
      if (err) console.error(err);
      else console.log(stats.toString());
    });
  }

};

(or we could explicitly define a Build class and then call new Build() right away).

I prefer version 2 or 3, but in the JavaScript ecosystem I see a huge amount of code like Version 1. At least that was my feeling after reviewing ~ 50 real world open source JS React+Webpack projects.

I don't want to be the one guy writing non-idiomatic JS, but on the other hand, I find Version 2 or 3 so much better. I would only use comments to describe actually complicated things, or to explain the whys (and let variable/function/class names describe the hows). Why am I the only one thinking like this? Which version should I use?

I would say a very big "no" to Version 3. Instantiating an object should not have a side-effect of emptying a directory and kicking off a build — that's very surprising behavior. In OOP, the methods should be the verbs. Here, your noun has gone rogue and is doing all kinds of activities.

Version 2 adds verbosity with questionable benefits. If you want to define functions, then the functions should accept parameters and return results. Here, the functions act on inputs that are essentially global variables, so you still have to read the code within each function to understand what it does.

Version 1 is good, and I would say that the added comments are probably beneficial.

All four of the scripts lack decent error handling. If an error occurs, I would expect the entire script to abort, rather than printing a message and trying to continue.


Find unique range numbers im using the following code which is working.
The code provide range of random values but unique one i.e. it doesn't return  the same values until the full range was provided ,what do you think and doest in ES6 there is shorter way to achieve this?

var range = [10, 20];
var not = [];

function randomRange(range, n) {
  if (not.length >= range[1] - range[0]) {
    not.length = 0;
  }
  var curr = [];
  var res = [];
  for (let i = range[0]; i < range[1]; i++) {
    if (!not.some(function(num) {
      return i == num
    }) && not.length < range[1] - range[0]) {
      curr.push(i)
    }
  }
  for (let i = 0; i < n; i++) {
    var j = curr.splice(Math.floor(Math.random() * curr.length), 1)[0];
    res[i] = not[not.length] = j;
  }

  return res.filter(Boolean)
}

function log(result) {
  document.body.innerHTML += JSON.stringify(result) + "<br>"
}

window.onload = function() {

log(randomRange(range, 3));
log(randomRange(range, 3));
log(randomRange(range, 3));
log(randomRange(range, 3));
log(randomRange(range, 3));
log(randomRange(range, 3));
log(randomRange(range, 3));
log(randomRange(range, 3));
log(randomRange(range, 3));
log(randomRange(range, 3));
}

If what you want is to call the same function with the same array and obtain random numbers from that array every time, but without them repeating, well, that's almost literally the definition of "shuffle".

Have a look at this answer on stackoverflow. The idea in your case is that you shuffle the array and pick the first (or last) n elements.

That means something like (almost entirely copied from linked answer):

function shuffle(array) {
  var currentIndex = array.length, temporaryValue, randomIndex;

  while (0 !== currentIndex) {
    randomIndex = Math.floor(Math.random() * currentIndex);
    currentIndex -= 1;
    temporaryValue = array[currentIndex];
    array[currentIndex] = array[randomIndex];
    array[randomIndex] = temporaryValue;
  }

  return array;
}

var arr = [];
for (var i = 10; i <= 20; i++) {
   arr.push(i);
}
shuffle(arr);
console.log(arr.slice(0,3));
console.log(arr.slice(3,6));

The advantage is that you need to randomize only once, no need to loop through the array and generate random numbers every time.

The disadvantage is that if you want different numbers, you'll have to keep track of the index by yourself, because requesting arr.slice(0,3) will always return the same numbers.
But considering that you'd have to keep track of which numbers have already been used anyway, this should not be a problem.

Also, this may actually not be a disadvantage depending on what you use the function for.
(Of course you could shuffle every time, but that would lose any speed benefit of shuffling).

Another approach would be to shuffle once and then having a function that returns the first n numbers but also removes them from the list.
The obvious disadvantage in that case would be that you have to keep a copy of your original array, and when the shuffled array is empty, copy it and shuffle it again.

If you still want to use your approach (which works anyway), at least make sure to use meaningful variable names. In my opinion not is probably the worst possible name for a variable.


Simple menu with sub-levels The following works as intended, however is there a more concise way to write this script without changing it too much? It's a simple menu with sub-levels which have a plus/minus icon when each li opens/closes.

jQuery:

$(document).ready(function() {
    $li = $('#main > li.expand');
    $li.click(function(){
        $ul = $(this).find('ul.sub-level').delay(200).slideDown(400);
        $(this).removeClass('expand').addClass('minus');
        $('#main > li > ul').not($ul).slideUp('slow');
        $('#main > li.minus').not($(this)).removeClass('minus').addClass('expand');

    })

    $li2 = $('.sub-level > li.expand');
    $li2.click(function(){
        $(this).find('ul.sub-level2').delay(200).slideToggle(400);
        $li2.toggleClass('minus').toggleClass('expand');
    })
});

HTML:

test-1

sub-test-1

sfasfasf

sfafasf

test-2

test-3

sub-test-2

test-4

Some suggestions:

Use var when declaring variables, unless you really want to make global variables (which you probably don't)
Combine var statements using ,

var a = 1, b = 2, c = 3;

Combine classes in toggleClass

$(this).toggleClass('minus expand')

Cache selectors that are used often (like $('#main'))
You don't need to use $li2 inside the click handler, you can just use $(this)
Turn repeated code into a function

$(function(){
    var $main = ('#main'),
    slide = function(ele, find){
        return $(ele).toggleClass('minus expand').find(find).delay(200).slideToggle(400);
    },
    $li = $('> li.expand', $main).click(function(){
        var $ul = slide(this, 'ul.sub-level');
        $('> li > ul', $main).not($ul).slideUp('slow');
        $('> li.minus', $main).not(this).toggleClass('minus expand')
    }),
    $li2 = $('.sub-level > li.expand').click(function(){
       slide(this, 'ul.sub-level2');
    });
});


Best way to retrieve music metadata from audio files? I am using the music metadata-browser npm package to retrieve audio metadata from files. This library is being used in an Electron and React desktop app.

To get the metadata of audio files, (and add it to the redux state), I use the following function to get metadata and add the file to redux state. This method works well with small amounts of data, but gets really slow (obviously) as more audio files are given to be processed. Is there a better way I can process these files? Not sure if javascript has any worker/job techniques I could use.

import * as mm from 'music-metadata-browser';

const addFile = (filePath, file, dispatch, ext) => {
  if (Buffer.isBuffer(file)) {
    mm.parseBuffer(file, ext)
      .then(metadata => {
        const libraryEntry = createLibraryEntry(filePath, metadata);

        dispatch({ type: constants.ADD_FILE, libraryEntry, totalFiles });
      });
  }
  else {
    mm.parseBlob(file, ext)
      .then((metadata) => {
        const libraryEntry = createLibraryEntry(filePath, metadata);

        dispatch({ type: constants.ADD_FILE, libraryEntry, totalFiles });
      });
  }
};

const processFiles = (files, dirPath, dispatch) => {
  files.map((file, index) => {
    const parsedFile = file.split('.');
    const format = parsedFile[parsedFile.length - 1];
    const filePath = `${dirPath}/${file}`;
    const isDirectory = fs.lstatSync(filePath).isDirectory();
    if (isDirectory) {
      fs.readdir(filePath, (err, files) => {processFiles(files, filePath, dispatch);});
    }
    else if (isValidFormat(format)) {
      fs.readFile(filePath, (err, file) => {
        addFile(filePath, Buffer(file), dispatch, format);
      });
    }
    return;
  });
};

Thanks for the help.

It appears music-metadata is the Node.js/desktop flavor of music-metadata-browser (intended for browsers).  The author doesn't explain the differences between the two, but maybe the desktop variant will be faster for you.

Synchronous anything is a mistake for an inherently parallel task like this.   Use fs.lstat instead of the fs.lstatSync.  If createLibraryEntry or dispatch touch the disk they should be replaced with async versions as well.

Worker threads are a thing but you probably don't need them, because disk access is what's slowing you down and async disk access doesn't require threads.

If you have disk I/O that can't be asynchronized (if that's a word) by conventional callback functions, or if you've made everything async and it's still too slow, that is the time to consider using workers.


Efficient byte sequence detection I seek the assessment of which code block is more efficient in Java at detecting the byte sequence of

00 00 00 01

and

00 00 01

in a byte array.

Version 1

for (int i = 0; i < frame.length - 4; i++) {
        if (frame[i] == 0) {
            if (frame[i + 1] == 0) {
                if (frame[i + 2] == 0) {
                    if (frame[i + 3] == 1) {
                        // found marker 00 00 00 01
                        // do work here
                    }
                } else {
                    if (frame[i + 2] == 1) {
                        // found marker 00 00 01
                        // do work here
                    }
                }
            }
        }
    }

Version 2

    for (int i = 0; i < frame.length - 4; i++) {
        if (frame[i] == 0 && frame[i + 1] == 0 && frame[i + 2] == 0 && frame[i + 3] == 1) {
            // found marker 00 00 00 01
            // do work here
        } else if (frame[i] == 0 && frame[i + 1] == 0 && frame[i + 2] == 1) {
            // found marker 00 00 01
            // do work here
        }
    }

Application repo

Version 3

Consider the case of a sequence of 00 00 00 02. This sequence will pass the first 3 if conditions and fail at the 4th one. But what does this tell you ? It tells you that instead of increasing i by 1 it should be increased by 4.

This idea converted to code could look like this

for (int i = 0; i < frame.length - 4; i++) {

    if (frame[i] != 0) {
        continue;
    }

    i++;
    if (frame[i] != 0) {
        continue;
    }

    i++;
    if (frame[i] > 1) {
        continue;
    }

    if (frame[i] == 1) {
        // found marker 00 00 01
        // do work here

        continue;
    }

    i++;
    if (frame[i] == 1) {
        // found marker 00 00 00 01
        // do work here
    }
}

which isn't easy to read/understand at first glance, so a big comment about why the increasing of i is done like this would be in order.


MonoAlphabetic and PolyAlphabetic ciphers in Haskell This code defines functions to cipher text with both mono-alphetic (Caesar) ciphers and poly-alphabetic ciphers. I also define a shortcut for rot13. I like this code and I think it is easy to read, but code is never perfect so I ask you for improvements.

import Data.Char (toLower)

alphabet :: String
alphabet = ['a'..'z']

generalShift :: String -> Int -> Char -> Char
generalShift alphabet 0 letter = letter
generalShift alphabet positions letter
  | letter == last alphabet = generalShift alphabet (positions - 1) (head alphabet)
  | otherwise = generalShift alphabet (positions - 1) (succ letter)

shift :: Int -> Char -> Char
shift = generalShift alphabet

clean :: String -> String
clean = filter (`elem` alphabet) . (map toLower)

monoAlphabeticCipher :: Int -> String -> String
monoAlphabeticCipher key = map (shift key) . clean

rot13 :: String -> String
rot13 = monoAlphabeticCipher 13

poliAlphabeticCipher :: [Int] -> String -> String
poliAlphabeticCipher keys text = zipWith shift keys (clean text)

main :: IO()
main = print $ poliAlphabeticCipher [3, 2, 1] "abc"

generalShift

Note the following about generalShift:

the alphabet argument never changes throughout the recursive calls
the only elements of alphabet accessed are the first and last characters
last is a potentially expensive function since it traverses the entire list to find the last element

To elaborate on point #2, note that the following calls are exactly the same:

generalShift "abcdefg...xyz" 10 'x'
generalShift "az" 10 'x'

Given these observations, why not just pass the first and last characters of the alphabet to generalShift, i.e.:

generalShift :: Char -> Char -> Int -> Char -> Char
generalShift firstChar lastChar positions letter = ...

elem

elem is another potential expensive call. I would detect alphabetic characters using <= comparisons with the end points of the upper- and lowercase character ranges:

isAlphabetic ch = ('a' <= ch && ch <= 'z') || ('A' <= ch && ch <= 'Z')

clean str = filter isAlphaBetic . map toLower
  -- or the other way around: map toLower . filter isAlphabetic

more generalShift

There is a much faster way to compute the shift of a letter using the fact that Char is an enumeration (i.e. an instance of the Enum class) and therefore the following functions are available:

fromEnum :: Char -> Int
toEnum :: Int -> Char

These work like the ord() and chr() functions in other languages.

Let's assume the first and last characters of the alphabet are the standard a and z. I've leave you to figure out exactly what the code should look like, but here are some hints:

To shift a letter _n_ positions in the range a..z:

1. Determine how far away from 'a' the letter is.
2. Add n to that distance.
3. If the distance computed in step 2 would place it beyond 'z',
   subtract the number of characters between 'a' and 'z' and
   try again.
4. Add the distance computed in step 3 to the letter 'a'
   and return it as the encoded letter.

There will be a bunch of calls to fromEnum and toEnum as well as perhaps the mod function might help here.

poliAlphabeticCipher

Nothing really wrong here, but I have a suggestion... it is customary to repeat the key if it isn't long enough for the entire message. I.e., if the key was [3,2,1] and the message was "Hello", you would repeat the key as many times as was needed to cover every letter of the message. The key used to encode the message in this case would be [3,2,1,3,2].

You can use the cycle function to repeat a list forever:

cycle :: [a] -> [a]
-- e.g. cycle [1,4,2] = [1,4,2,1,4,2,1,4,2,1,4,2...]

and then polyiAlphabeticCipher may be written as:

poliAlphabeticCipher keys text = zipWith shift (cycle keys) (clean text)

With that change monoAlphabeticCipher may be implemented using poliAlphabeticCipher:

monoAlphabeticCipher shift text = poliAlphabeticCipher [shift] text


Choose random card from a deck still new to java and i am trying to improve my understanding and use of OOP concepts.here i am supposed to print the name of a randomly selected card from a deck of 52 cards.

Program:

/*
 * RandomCard.java
 * -------------------
 * Displays the name of a card randomly chosen from a complete deck of 52
 * playing cards.
 */

 import java.util.Random;

 public class RandomCard {
    /* Instance Variables */
    private Random rgen = new Random(); /* random generator */

    /**
     * Display the name of a randomly selected card from a deck of 52 cards
     */
    public static void main(String [] args) {
        System.out.println("This program displays a randomly chosen card");
        Card randomCard = selectCardFromDeckRandomly();
        System.out.println(randomCard);
    }

    /*
     * Randomly select a card from a card deck
     * @ return The selected card
     */
    private Card selectCardFromDeckRandomly() {
        String rank = selectRanomCardRank();
        String suit = selectRandomCardSuit();
        return (new Card(rank, suit));
    }

    /*
     * Randomly select a card rank
     * @ return the selected card rank
     */
    private String selectRanomCardRank() {
        String cardRank;
        int r = 1 + rgen.nextInt(13);
        switch (r) {
        case 1:
            cardRank = "Ace";
            break;
        case 2:
            cardRank = "2";
            break;
        case 3:
            cardRank = "3";
            break;
        case 4:
            cardRank = "4";
            break;
        case 5:
            cardRank = "5";
            break;
        case 6:
            cardRank = "6";
            break;
        case 7:
            cardRank = "7";
            break;
        case 8:
            cardRank = "8";
            break;
        case 9:
            cardRank = "9";
            break;
        case 10:
            cardRank = "10";
            break;
        case 11:
            cardRank = "Jack";
            break;
        case 12:
            cardRank = "Queen";
            break;
        default:
            cardRank = "King";
            break;
        }
        return cardRank;
    }

    /*
     * Randomly select a card suit
     * @ return The selected card suit
     */
    private String selectRandomCardSuit() {
        String cardSuit;
        int s = 1 + rgen.nextInt(4);
        switch (s) {
        case 1:
            cardSuit = "Clubs";
            break;
        case 2:
            cardSuit = "Diamonds";
            break;
        case 3:
            cardSuit = "Hearts";
            break;
        default:
            cardSuit = "Spades";
            break;
        }
        return cardSuit;
    }
}

class Card:

/*
 * Card.java
 * -------------
 * Creates a playing card of certain Rank and Suit.
 * This class is part of exercises 1 solution.
 * This program is intended as a practice on programming methodologies.
 */

 public class Card {
    /* Instance variables */
    private String rank;   /* The card rank : Ace,2,3,4,5,6,7,8,9,10,Jack,Queen,King */
    private String suit;   /* The card suit : Clubs, Diamonds, Hearts, Spades */

    /* Constructor */
    /**
     * Creates an instance object of class Card with the specified rank and suit
     * @param rnk The rank of the card
     * @param sut The suit of the card
     */
    public Card(String rnk, String sut) {
        rank = rnk;
        suit = sut;
    }

    public String toString() {
        return (rank + " of " + suit);
    }
}

Note: still not in a position to use arrays.

Evaluation points:

Encapsulation
Top-Down design (refactoring)
Readability and complexity reduction
Object oriented
Documentation

You did many things right. Great! =)

Readability and complexity reduction

Your

private String rank;

is nice and readable, but what is a rnk in

public Card(String rnk, String sut)

?

I would not suggest to compromise on the readability of publicly visible variable names. (And I would neither suggest doing so on any other variable of any access level.)

If there's no good alternative name for parameter names, use this to differentiate the readable (and equal) variable and parameter names:

public Card(String rank, String suit) {
    this.rank = rank;
    this.suit = suit;
}

A common alternative solution is to add an underscore character to the variable name like so:

private String _rank;
private String _suit;

Speaking of those two variables, it might make sense to make them final. After all, the suit of a card should never be changed. This reduces complexity, because you can only ever set the value once.

Top-Down design (refactoring)

  a randomly selected card from a deck of 52 cards.

You are not doing that at all. You pick some random rank and some other random suit and construct a card from that. The concept of a "deck" doesn't exist at all in your code. You merely have it implicitely defined just by how your random number generation works.

From top-down design code, I'd expect to see some object representing the deck of cards and some mechanism to get a random card from it. E.g.

Deck cardDeck = new Deck();
Card randomCard = cardDeck.getRandomCard();

Documentation

The public documentation of the constructor parameters

* @param rnk The rank of the card
* @param sut The suit of the card

is not adding any new information, while the hidden comments of the variables do:

/* The card rank : Ace,2,3,4,5,6,7,8,9,10,Jack,Queen,King */
/* The card suit : Clubs, Diamonds, Hearts, Spades */

I wish I could see those when being clueless about how to invoke the constructor.

My personal opinion is that comments like

/* Instance variables */

and

/* Constructor */

do not add any value. If you want them or if you are forced to add them, you should be consistent though and also add

/* Public methods*/

to your classes.

Object oriented

rank and suit should be Enum Types. One part of object oriented philosophy is to create types in a way that they are helpful. A string is a string, but a Suit is not a Rank. Among many others, this has the benefit of preventing mistakes like the following from even compiling:

private Card selectCardFromDeckRandomly() {
    String rank = selectRandomCardSuit();
    String suit = selectRanomCardRank();
    return (new Card(rank, suit));
}

  Note: still not in a position to use arrays.

You actually are in exactly that position.
In you case, limiting your code to not use arrays is what reduces your ability to improve it the most.

The use of enums leads to the use of their values() method (which returns an array), which allows you to get rid of the switches, resulting in more generic code.

Oh, and btw. isn't String [] args an array?


Follow-up: Find number of times that difference of array values are equals to a number This question is follow-up to this question.

Sample Calculation

10-21=-11
10-34=-24
10-45=-35
10-56=-46
21-10=11     <
21-34=-13
21-45=-24
21-56=-35
34-10=24
34-21=13
34-45=-11
34-56=-22
45-10=35
45-21=24
45-34=11   <<
45-56=-11
56-10=46
56-21=35
56-34=22
56-45=11   <<<

Code

Set<Integer> tchars = new HashSet<>();
        tchars.add(10);
        tchars.add(21);
        tchars.add(34);
        tchars.add(45);
        tchars.add(56);
        int number = 11;
        int counter = 0;

        Iterator it = tchars.iterator();
        while (it.hasNext()) {
            Integer inte = (Integer) it.next();
            if (tchars.contains(inte + number)) {
                System.err.println(inte + " " + number);
                counter++;
            }
        }
        System.err.println(counter);

Using a Set is problematic.

First, as pointed out on the original question in @Vogel612's comment, it restricts duplicate values. Second, it also makes your code a lot messier, as you're now grabbing an Iterator, calling .hasNext(), and testing in a while loop instead of using more concise syntax.

I'd just use a List<Integer>, get rid of all the casts, and use a for-each style syntax instead of using the Iterator. Finally, number and counter aren't very meaningful as variable names in the context of this snippet.  If they are meaningful in the wider context of wherever this came from so-be-it, but I find something more along the lines of difference and result much clearer.

Something more like this:

List<Integer> tchars = Arrays.asList(10, 21, 34, 45, 56);
int difference = 11;
int result = 0;

for (int test : tchars) {
    if (tchars.contains(test + difference)) {
        System.err.println(test + " " + difference);
        result++;
    }
}

System.err.println(result);

Dead simple, and you can see immediately what it is doing.


Create a probability list which sums up to one I use the numpy.random.choice method to draw a sample from a list.

The entries in the list are short dicts and look like

{'name': 'SomeName', 'strength': 0.75},
{'name': 'SomeName', 'strength': 0.25},
...

I want to use the 'strength' value (between 0 and 1) as a probability indicator.

To use
drawResults = choice(data, drawCount, p=resultProb)

I have to create the resultProb list which has to sum up to 1

So I came up with a function:

def makeProbabilities(data) -> list:
    probResult = []

    totalScore = sum(item['strength'] for item in data) # calculate in sum of all 'strength' values

    for item in data:
        if item['strength'] > 0:
            value = (100.0/totalScore)*item['strength'] #how much is this strength in relation to the total
            value2 = (1.0/100)*value                    #how much is the result above in relation to 1

            probResult.append(value2)
        else:
            probResult.append(0.0)

    return probResult

That seems to work, it just have a very small rounding error on the result (the sum is like 1.0000000001) but the numby.choice method does accept it.

But I have the strong impression that this solution is a bit awkward, un-pythonique and perhaps not very efficient on large data sets.

I am just discovering python so I am a bit lost with all the information concerning this language.

Any feedback on that is very welcome.

Binary floating point numbers cannot represent most numbers exactly,
a small error cannot be avoided when computing the sum of probabilities:

>>> probs = [0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1]
>>> sum(probs)
0.9999999999999999

For more information, see

Is floating point math broken? on Stack Overflow,
What Every Computer Scientist Should Know About Floating-Point Arithmetic.

Actually one can see in the source code of mtrand.pyx that this rounding error is cared for in the choice() function:

        atol = np.sqrt(np.finfo(np.float64).eps)
        # ...
        p = <ndarray>PyArray_ContiguousFromObject(p, NPY_DOUBLE, 1, 1)
        pix = <double*>PyArray_DATA(p)
        # ...
        if abs(kahan_sum(pix, d) - 1.) > atol:
            raise ValueError("probabilities do not sum to 1")

An error is only raised if the sum differs from 1.0 by more than
some “machine epsilon.”

We also  see  that NumPy uses the
Kahan summation
to add the probabilities, an algorithm which significantly reduces the
numerical error. I don't know if NumPy publicly exposes the Kahan summation
(I could not find it). But it is not difficult to implement, here is a
version from Rosetta Code:

def kahansum(input):
    summ = c = 0
    for num in input:
        y = num - c
        t = summ + y
        c = (t - summ) - y
        summ = t
    return summ

which would then be used as

totalScore = kahansum(item['strength'] for item in data)

Example:

>>> probs = [0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1]
>>> sum(probs)
0.9999999999999999
>>> kahansum(probs)
1.0

The computation of the probResult array can be simplified. First note that

value = (100.0/totalScore)*item['strength']
value2 = (1.0/100)*value

is equivalent to

value2 = item['strength'] / totalScore

and that would work for the case item['strength'] == 0 as well:

for item in data:
    value2 = item['strength'] / totalScore
    probResult.append(value2)

And now you can replace the loop with a list comprehension:

def makeProbabilities(data) -> list:
    totalScore = kahansum(item['strength'] for item in data)
    return [ item['strength'] / totalScore for item in data ]

Finally note that according to the Python naming conventions, function
and variable names should be “snake case”: make_probabilities, total_score.


Find nth Fibonacci Number, using iteration and recursion I'm a beginner programmer and I came upon this problem which is to find the nth number in the Fibonacci series.

I used to solve the problem using a for loop; today I learned about recursion but there is a problem: when I pass 40 or 41 to the recursive function, it takes a bit of time to calculate it, while in the iterative method it would instantly give me the answers.

I have these questions:

Why do most people (on the Internet) recommend using recursion?   Because it's simpler and easier to write the program?  (Logically I thought that we should write it in a way that is fast and simple)
Here are the 2 methods that a beginner like me can handle writing at the moment.  Is there a better way than these two methods?  And are these methods complex?

Here is the recursive method:

    #include <iostream>
    using namespace std;

    unsigned long long fibonacci(unsigned long long n);

    unsigned long long fibonacci(unsigned long long n){
        if(n<=1)
            return n;                                //base case
        return fibonacci(n-1) + fibonacci(n-2);      //recursive case
    }

    int main(){
        unsigned int a {};
        cout << "Enter number: ";
        cin >> a;
        cout << fibonacci(a) << endl;
        return 0;
    }

And here is the iterative (looping) method:

#include <iostream>
using namespace std;
int main() {
    int n{};
    unsigned long long t1{0}, t2{1};
    unsigned long long sum{};
    cout << "Enter the number of terms: ";
    cin >> n;
    cout << "Fibonacci Series: ";
    for (int i{2}; i < n; ++i) {
        sum = t1 + t2;
        t1 = t2;
        t2 = sum;
    }
    cout << t2;
    return 0;
}

Note: I know that using namespace std is a bad idea but I have also tried the same thing without the namespace and still I get the delay, so I did it here because it's easy to understand.

Edit1: First of all I would like to thank everyone who commented and answered my question...To be honest I didn't think that this question would bring a lot of attention to the community so I appreciate the time you put on this and it means a lot to me.

Edit2: Let me demonstrate some of the things that might have been a little odd to you.

Q: What do you mean by better when you say if there is a better way than these two methods?
A: By better, I meant that the code shall be simple and also takes less time to execute or perform the calculations

Q: What do you mean when you say most people (on the internet) use recursion?
A: I've seen code out there that uses recursion to solve problems.  The two common problems I've seen are Fibonacci and Factorial of a number. For factorial, I have also tried both methods (iteration and recursion) but I didn't get a delay for both of them when I type large numbers like 40 or 50. The common problem I saw with recursion is that eventually, you may run out of memory space because it is calling itself multiple times which results in a stack overflow (because stack as a part of memory will be filled). Although this problem doesn't occur in this example. So this raises another question here:

When should we use recursion?

Why do most people (on the internet) recommend using recursion because it's simpler and easier to write the program? Logically I thought that we should write it in a way that is fast and simple.

This is a perceptive question.  I wrote an article about exactly this topic in 2004, which you can read here:

https://docs.microsoft.com/en-us/archive/blogs/ericlippert/how-not-to-teach-recursion

Summing up: there are two good reasons to teach people to use recursion to solve Fib:

First, because it clearly illustrates what you have learned today. A naive translation of a recursive definition into a recursive function can often lead to poor performance.  That's an important lesson for beginners to take away. (EXERCISE: How many additions does your naive recursive program execute for a given n? The answer may surprise you.)
Second, because the first lesson then gives us an opportunity to lead the beginner to learn how to write a recursive algorithm so that it performs well.

Unfortunately, as you have discovered, a great many people on the internet have not internalized that teaching recursion via fib is solely useful as an illustration of bad uses of recursion and how to fix them, and not in itself an example of a good use of recursion.

It would be much better if people attempting to teach recursion did so by providing a mix of good and bad recursive algorithms, and taught how to spot and avoid the bad ones.

  Is there a better way than these two methods?

Another lesson you'll quickly learn is that asking "which is better?" is a sure way to get back the reply: "can you describe a clear metric for betterness?"

So: can you describe a clear metric for betterness?

If the goal is to print out the nth fib number, you can do way "better" than either of your solutions:

unsigned long long fibs[] = { 1, 1, 2, 3, 5, 8, ... }
if (0 <= n && n < sizeof(fibs... blah blah blah))
   cout << fibs[n];

Done. There are only so many fib numbers that fit into a long. You can look them up on the internet, copy them into your program, and you've got a short, fast fib program with no loops at all.  That's "better" to me.

But remember, the point of this exercise is to teach you something about recursion, so by that metric my program is certainly not "better".

  are these methods complex?

By "these methods" I think you mean "methods of writing recursively-stated algorithms into code other than naive recursion and unrolling the recursion into a loop".

That's a matter of opinion. Let me put it this way.

I work on a compiler team, and I interview a lot of people. My standard coding question involves writing a simple recursive algorithm on binary trees that is inefficient when written the naive way, but can be made efficient by making a few simple refactorings.  If the candidate is unable to write that clear, straightforward, efficient code, that's an easy no-hire.

Where things get interesting is when I ask "suppose you had to remove the left-hand recursion from this tree traversal; how might you do it?"

There are standard techniques for removing recursions.  Dynamic programming reduces recursions. You could make an explicit stack and use a loop.   You could make the whole algorithm tail recursive and use a language that supports tailcalls. You could use a language with cocalls. You could use continuation passing style and build a "trampoline" execution loop.

Some of these techniques are, from the perspective of the novice, terribly complicated.  I ask the question because I want to know what is in the developer's toolbox.


Top k closest pairs in a set of million 128-dimensional points I have a set of 1 million points in 128-dimensional space. Among all trillion pairs of the points in the set, I need to get a subset of 100 million pairs whose cosine distances are less than that of every pair outside the subset.

I tried measuring cosine distances of all trillion pairs, sort them and get first 100 million of sorted pairs. But this program is estimated to take few years in single thread.

from scipy.spatial.distance import cosine

points = load_points_list()         # List of points, where each point is tuple of 128 floats
M = len(points)                     # 1 million points

cosine_distances, pairs = [], []
for i in range(M):
    for j in range(M):
        cosine_distances.append(cosine(points[i], points[j]))
        pairs.append((i, j))

# Sort pairs based on cosine distances
cosine_distances, pairs = (list(s) for s in zip(*sorted(zip(cosine_similarities, pairs_indices))))

top_100_million_closest_pairs = pairs[:100000000]

Is there a more efficient algorithm or GPU-enabled acceleration for a problem at this scale?

First, cut your work load in half.  You only need to consider pairs where i < j, so:

for i in range(M-1):
    for j in range(i+1, M):
        # ...

If you actually need both (i, j) and (j, i), then you only need the 50 million closest pairs, and can duplicate & swap every pair in the list as the last step.   More savings!

Record & sort the first 100 million (50 million?) pairs.  Then, as new pairs are generated, if closer than the worst stored pair, use bisect.bisect() to find insertion spot, add the new pair, and kick out the worst pair.

Time: \$O(n^2 log k)\$,
Space: \$O(k)\$, n is number of points, k is desired number of pairs.


Could random unique selection using while cause an infinite loop? I recently came across code that looked something like this (generates 4 random numbers in an array, this is not the actual code, I just wrote this up now, so it's untested):

var uniqueNumbers = new Array();
var count = 4;
var max = 10;

function main() {
    for (var i = 0; i < count; i++) {

        do {
            var tempNum = Math.floor(Math.random() * max); //number 0-9
        } while (arrayContains(uniqueNumbers, tempNum))

        uniqueNumbers.push(tempNum);
    }
}

function arrayContains(a, val) {
    for (var i = 0; i < a.length; i++) {
        if (a[i] == val) return true;
    }
    return false;
}

Is it possible that the while loop could possibly be infinite (or at least take a lot of time)? In the way that it just so happens that we continuously randomly choose a number that happens to be in the array? I imagine the probability of this happening would go up as count increases. It's kind of reminding me of bogosort.

I would think that this would be improved by selecting from a diminishing 'pool' of numbers, instead of selecting randomly, something like:

var uniqueNumbers = new Array();
var count = 4;
var max = 10;
var pool = new Array();

function main() {
    createPool();
    for (var i = 0; i < count; i++) {
        if (var tempNum = selectFromPool()) {
            uniqueNumbers.push(tempNum);
        }
    }
}

function createPool() {
    for (var i = 0; i < max; i++) {
        pool.push(i);
    }
}

function selectFromPool() {
    if (pool.length <= 0) return false;

    var selectedIndex = Math.floor(Math.random() * pool.length);
    var selectedValue = pool[selectedIndex];

    pool = pool.slice(selectedIndex, selectedIndex+1);

    return selectedValue;
}

Is this a better, less fragile way of doing it? Or is there some proof that the while loop will never be infinite because of the pseudo-random nature of computer generated random numbers?

Your first attempt is indeed in danger of getting into an infinite loop if a certain value never gets returned from rand (but any half decent PRNG will return all of them eventually).

Your second attempt is very close to the (most optimal) Fisher-Yates shuffle, with the difference that you use a separate array to store the shuffled result while shrinking the original pool.


Recursive hashing function This method is part of my login system. Could I improve it further?

protected function _hashPassword($password = NULL, $rounds = 1000, $i = 1)
{
    if (! isset($password)) throw new Exception('No password set!');

    $salt   = 'K^^%/m>(|{z= $1^>%>W[=4U5*p|,E';
    $pepper = '08[)^,&%^^771^=>&,E[XP::4})h*I';

    $dinner = $salt.$password.$pepper;

    if ($i >= $rounds) return sha1($dinner);
    return sha1($this->_hashPassword($dinner, $rounds, $i+1).$password);
}

I would use a for loop instead of recursive calls:

protected function _hashPassword($password = NULL, $rounds = 1000) {
    if (!isset($password)) throw new Exception('No password set!');

    $salt   = 'K^^%/m>(|{z= $1^>%>W[=4U5*p|,E';
    $pepper = '08[)^,&%^^771^=>&,E[XP::4})h*I';

    $dinner = $salt . $password . $pepper;

    for ($i = 0; $i < $rounds; $i++) {
        $dinner = sha1($dinner . $password);
    }
    return $dinner;
}

From http://php.net/manual/en/functions.user-defined.php:

  It is possible to call recursive functions in PHP. However avoid
  recursive function/method calls with over 100-200 recursion levels as
  it can smash the stack and cause a termination of the current script.

Furthermore, I would use the username as 'pepper'. It would make your hash more unpredictable.