HackerRank Angry Professor challenge I've written an answer to HackerRank's Angry Professor problem. Please let me know if there's a quicker/better way of solving this as I think I wrote quite a bit of code for this problem (answer has to be in 1 class btw). Also, any improvements on my code in Java 8 are welcome too.

Problem statement (as written on HackerRank):

  The professor is conducting a course on Discrete Mathematics to a
  class of N students. He is angry at the lack of their discipline, and
  he decides to cancel the class if there are less than K students
  present after the class starts.

  Given the arrival time of each student, your task is to find out if
  the class gets cancelled or not.

Answer:

import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;

public class AngryProfessor {
    public static void main(String[] args) {
        List<List<List<Integer>>> testCases = readInput();

        for(List<List<Integer>> testCase : testCases){
            int numStudents = testCase.get(0).get(0);
            int minStudents = testCase.get(0).get(1);
            // Gotta STDOUT in hacker rank for it to be graded.
            System.out.println(isLectureCancelled(calcNumAttending(testCase.get(1), numStudents), minStudents));
        }
    }

    private static void addData(List<Integer> arrayList, String[] numbersToAdd){
        for(String num : numbersToAdd){
            arrayList.add(Integer.valueOf(num));
        }
    }

    /**
     * Calculates the number of students that make the lecture on time.
     *
     * @param studentTimes    the relative time at which the student arrives (student is on time if time is <= 0)
     * @param numStudents     number of students that make the lecture on time
     * @return numStudents
     */
    private static int calcNumAttending(List<Integer> studentTimes, int numStudents){
        int numAttending = 0;
        for(int i = 0; i < numStudents; i++){
            if(studentTimes.get(i) <= 0){
                numAttending++;
            }
        }
        return numAttending;
    }

    /**
     * Determines whether the lecture is cancelled or not.
     *
     * @param numAttending    Number of students who attend the lecture
     * @param minStudents     Minimum number of students for the lecture to go ahead
     * @return whether the lecture is cancelled or not
     */
    private static String isLectureCancelled(int numAttending, int minStudents){
        return numAttending < minStudents ? "YES" : "NO";
    }

    /**
     * Reads and stores all the data given. Each index is a test case. At that index is a list containing two lists. One
     * for each line of input that's read. The first list contains numStudents and minStudents, and the second list
     * contains the student times.
     *
     * @return a List containing all the data.
     */
    private static List<List<List<Integer>>> readInput(){
        Scanner k = new Scanner(System.in);
        int numTestCases = Integer.valueOf(k.nextLine());
        List<List<List<Integer>>> testCases = new ArrayList<>(numTestCases);

        // fills up testCases with ArrayLists before filling up with numbers
        for(int i = 0; i < numTestCases; i++){
            List<List<Integer>> list = new ArrayList<>(2);
            for(int j = 0; j < 2; j++){
                list.add(new ArrayList<>());
            }
            testCases.add(list);
        }

        // Inputs data into the ArrayLists
        for(int i = 0; i < numTestCases; i++) {
            String line1 = k.nextLine();
            String line2 = k.nextLine();
            addData(testCases.get(i).get(0), line1.split(" "));
            addData(testCases.get(i).get(1), line2.split(" "));
        }
        k.close();
        return testCases;
    }
}

you have a nice, correct program here!
...please see what I made of it with little scope on "fewer lines" and little "performance" and of course some Java8 tweaks:

import java.util.LinkedList;//why always Array-Array?(especially for 2 - 20 entries)
import java.util.List;
import java.util.Scanner;

public class Solution {//adjusted class name for code upload

  public static void main(String[] args) {
    readInput()// don't need the variable, but same call as in original program
    .stream().forEach(//just as a showcase for streams and lambda but no real improvement
      /*TestCase*/ t -> {System.out.println(t.outcome);}
    );
  }

  private static List<TestCase> readInput() {//no changes, except the return (list) type
    final List<TestCase> testCases = new LinkedList<>();//"final" becuase it is..., LinkedList: no(0) initial capacity
    try (Scanner k = new Scanner(System.in)) {//try with ressources {@link: https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html}
        int numTestCases = Integer.valueOf(k.nextLine());//original code
        for (int i = 0; i < numTestCases; i++) {//...
            testCases.add(//...
               new TestCase(//new "TestCase" with ...
                   k.nextLine(),//the next line <- blocks
                   k.nextLine() //..and the next line <-blocks
               )
            );
        }
    }
    return testCases;
  }

  private static class TestCase {//This is "nicer" to read/understand than nested lists
    //we don't really need these (they could also be local constructor variables):
    private final int students;
    private final int minAttendees;
    private final int[] arrivals;//(I) prefer arrays>list, when knowing the size in advance and size < 65536 (2^16)
    //we only need this:
    final String outcome;

    private TestCase(String courseData, String studsData) {
         /*This is the "power" but also "the weakness" of this approach:
         It does everything (possible) in one loop/line/routine -
         it saves lines of code (and maybe little exec. time)!
         But it is less flexible, and for big routines
          (many students, and maybe not "keyboard scanner")
         this would definitely be a bottleneck.*/
        String[] tmp = courseData.split(" ");//according to original code, reuse of String[] variable
        students = Integer.valueOf(tmp[0]);//according to original code/input format
        minAttendees = Integer.valueOf(tmp[1]);//...
        tmp = studsData.split(" ");//...
        arrivals = new int[students];//..initialize...
        int attendees = 0;//...
        for (int i = 0; i < students; i++) {//only one loop...
            arrivals[i] = Integer.valueOf(tmp[i]);//...copying...
            if (arrivals[i] <= 0) {//...evaluating...
                attendees++;
            }
        }
        outcome = attendees < minAttendees ? "YES" : "NO";//..and finally aggregating
    }
  }
}


Find minutes with no data in database I'm trying to show a table, for each ID, to see if there is data available. In this case, its measurements. If there's no data, that means the logger isn't doing its job properly.

I currently have two tables: data and times. data contains id, datetime, sensor_id, and value. times contains id and value.

Times is filled with 00:00, 00:01, 00:02, etc., all the way through to 23:59.

I have this query:

SELECT t.`value`, d.`sensor_id`, COUNT(t.`value`) as `numrows`
FROM `data` d
RIGHT JOIN `times` t
ON d.`datetime` LIKE CONCAT('% ', t.`value`, '%')
WHERE d.`datetime` LIKE '%$2014-11-05%'
AND d.`sensor_id` IN(1,2,3,4,5,999)
GROUP BY d.`sensor_id`, t.`value`
ORDER BY d.`sensor_id` ASC, t.`id` ASC

while($s = $select->fetch_assoc()) {
    $checkArray[$s['sensor_id']][$s['value']] = $s['numrows'];
}

foreach($checkArray as $key => $arr) {
    echo 'Sensor: ' . $key;
    for($i = 0; $i <= 23; $i++) {
        for($j = 0; $j <= 59; $j++) {
            $time = strlen($i) == 1 ? '0' . $i : '' . $i;
            $time .= ':';
            $time .= strlen($j) == 1 ? '0' .$j : '' . $j;

            if(isset($arr[$time]) && $arr[$time] >= 1) { //See if has at least one row
                echo 'YES DATA FOR ' . $time . '<br>';
            }
        }
    }
}

Of course, I sort this in a table, and the result is this:

Just for sensors 1, 2, 3, 4, and 5, the load time is over 5.5 seconds. I don't know how to optimize this further. I've put indexes on the queried columns, but I can't think of anything else.

My SHOW CREATE TABLE for data:

CREATE TABLE `data` (
 `id` int(13) NOT NULL AUTO_INCREMENT,
 `sensor_id` int(13) NOT NULL,
 `datetime` datetime NOT NULL,
 `value` float NOT NULL,
 PRIMARY KEY (`id`),
 UNIQUE KEY `sensor_id_2` (`sensor_id`,`datetime`,`value`),
 KEY `sensor_id` (`sensor_id`),
 KEY `value` (`value`),
 KEY `datetime` (`datetime`),
 KEY `sensor_id_3` (`sensor_id`),
 KEY `datetime_2` (`datetime`)
) ENGINE=InnoDB AUTO_INCREMENT=103921 DEFAULT CHARSET=utf8

And my SHOW CREATE TABLE for times:

CREATE TABLE `minutes` (
 `id` int(13) NOT NULL AUTO_INCREMENT,
 `value` varchar(16) NOT NULL,
 PRIMARY KEY (`id`),
 UNIQUE KEY `id_3` (`id`),
 KEY `value` (`value`),
 KEY `id` (`id`),
 KEY `value_2` (`value`),
 KEY `id_2` (`id`)
) ENGINE=InnoDB AUTO_INCREMENT=1441 DEFAULT CHARSET=utf8

Is this the only query on data that matters?  You have too many simple indexes.  For example, there's no reason to have an index on just sensor_id if you already have one with sensor_id in the front.

CREATE TABLE `data` (
 `id` int(13) NOT NULL AUTO_INCREMENT,
 `sensor_id` int(13) NOT NULL,
 `datetime` datetime NOT NULL,
 `value` float NOT NULL,
 PRIMARY KEY (`id`),
 UNIQUE KEY `sensor_id_2` (`sensor_id`,`datetime`,`value`)
)

The minutes table is even worse.  There's only two columns but you have six indexes (the PRIMARY KEY is an index on id).  Three would be serious overkill.

CREATE TABLE `minutes` (
 `id` int(13) NOT NULL AUTO_INCREMENT,
 `value` varchar(16) NOT NULL,
 PRIMARY KEY (`id`),
 UNIQUE KEY `value_id` (`value`, `id`),
 UNIQUE KEY `id_value` (`id`, `value`)
)

Your query looks weird to me.  You're doing string operations (LIKE) on a datetime field.  This probably means that you are skipping the index and almost certainly that you aren't using the index to limit the possible rows.  A more common form would be

SELECT t.`value`, d.`sensor_id`, COUNT(t.`value`) as `numrows`
FROM `data` d
RIGHT JOIN `times` t
  ON TIME_FORMAT(d.`datetime`, '%H:%i') = t.`value`
WHERE d.`datetime` >= '2014-11-05 00:00:00'
  AND d.`datetime` <= '2014-11-05 23:59:00'
  AND d.`sensor_id` IN (1,2,3,4,5,999)
GROUP BY d.`sensor_id`, t.`value`
ORDER BY d.`sensor_id` ASC, t.`value` ASC

And you should be aware that when you specify IN, you are repeating the query once for each value.  So this query would run six times.  You may find it easier to run the six queries on the PHP side, as they'll be smaller.  This can also help your programming logic.

You might want to look up EXPLAIN plans, as they should help hint at what your problems are.  You might consider posting your SQL and explain plan to Stack Overflow (hint, just add EXPLAIN before the SELECT and look at the results in an admin tool like phpMySQL).

It's possible that the thing to do is to replace your DATETIME column with a DATE column and a minutes_id column.  Then your join would be on a fast integer join and you could do a straightforward date check.

Rather than doing a RIGHT JOIN, you might be better off with a regular inner join.  Then look for missing times in PHP.  Outer joins (left and right) are much slower than properly indexed inner joins.  You may find it easier to look for the missing times in PHP than to have your database do it.

I find it questionable to read all the data in and then use it.  It's often better to read the data and then use it immediately.

$times = array();
for ( $hour = 0; $hour < 24; $hour++ ) {
    for ( $minute = 0; $minute < 60; $minute++ ) {
        $times[] = sprintf('%02u:%02u', $hour, $minute);
    }
}

reset($times);
$last_sensor = 0; // assumes that there is no sensor 0
while ( $s = $select->fetch_assoc() ) {
    if ( $s['sensor_id'] != $last_sensor ) {
        echo 'Sensor: ' . $s['sensor_id'] . PHP_EOL;
        $last_sensor = $s['sensor_id'];

        // clear any remaining times from the last sensor
        while ( list(, $time) = each($times) ) {
            echo "$time 0" . PHP_EOL;
        }
        reset($times);
    }

    // iterate through until we find the current time
    list(, $time) = each($times);
    while ( $time != $s['value'] ) {
        // mark the missing times
        echo "$time 0" . PHP_EOL;
        list(, $time) = each($times);
    }

    echo "$time {$s['numrows']}" . PHP_EOL;
}


Accurate sum of a range object in O(1) I recently created a function calculating the sum of an arbitrary range object in \$O(1)\$:

def sum_of_range(range_obj):
    """Calculates the sum of a range object (Python 3) in O(1).

    This function always returns a python integer representing the sum
    without loss of precision (as would happen if it uses intermediate
    floating point values).
    """
    start, stop, step = range_obj.start, range_obj.stop, range_obj.step
    len_ = len(range_obj)
    if step > 0:
        if start >= stop:
            return 0
        # Find the last "really" included number. Subtract one from stop
        # because the stop is not included in ranges.
        upper = (stop - 1 - start) // step * step + start
    else:
        if stop >= start:
            return 0
        # In case the step is negative we need to add one to the difference
        # instead of subtracting one.
        upper = (stop - start + 1) // step * step + start
    sum_first_and_last = upper + start
    return (sum_first_and_last * len_) // 2

I used "random" tests to verify that it works:

import random

for i in range(100000):  # make a lot of random tests
    up = random.randint(-10000, 10000)
    lo = random.randint(-10000, 10000)
    step = random.randint(-25, 25)
    if step == 0:  # steps of zero are not possible
        continue
    rg = range(lo, up, step)
    assert sum_of_range(rg) == sum(rg)

Is there any room for improvement? Especially (but not only):

Code readability
Code performance
Should I replace the random tests with explicit tests or can I neglect explicit tests? Or should I include both?

Let's get a closer look at the sum you're trying to calculate:

$$start + (start+step) + (start+2 \times{}step) + \ldots + (start + (len-1)\times{} step)$$

Which can be reordered as:

$$start + \ldots + start + step + 2\times{}step + \ldots + (len-1)\times{}step$$

Which gives you a nice formula regardless of the sign of start or step or even the value of stop:

$$start \times{} len + step \times\frac{len(len-1)}{2}$$

All the complexity being hidden in the len(range_obj) call. So you can just write:

def sum_of_range(range_obj):
    """Calculates the sum of a range object (Python 3) in O(1).

    This function always returns a python integer representing the sum
    without loss of precision (as would happen if it uses intermediate
    floating point values).
    """

    length = len(range_obj)
    step_counts = length * (length - 1) // 2
    return range_obj.start * length + range_obj.step * step_counts


Not feeling 100% about my Controller design. Basically, I'm uploading an excel file and parsing the information then displaying what was parsed in a view.

using System.Data;
using System.Data.OleDb;
using System.Web;
using System.Web.Mvc;
using QuimizaReportes.Models;
using System.Collections.Generic;
using System;

namespace QuimizaReportes.Controllers
{
    public class UploadController : Controller
    {
        public ActionResult Index()
        {
            return View();
        }

        [HttpPost]
        public ActionResult Index(HttpPostedFileBase excelFile)
        {
            if (excelFile != null)
            {
                //Save the uploaded file to the disc.
                string savedFileName = "~/UploadedExcelDocuments/" + excelFile.FileName;
                excelFile.SaveAs(Server.MapPath(savedFileName));

                //Create a connection string to access the Excel file using the ACE provider.
                //This is for Excel 2007. 2003 uses an older driver.
                var connectionString = string.Format("Provider=Microsoft.ACE.OLEDB.12.0;Data Source={0};Extended Properties=Excel 12.0;", Server.MapPath(savedFileName));

                //Fill the dataset with information from the Hoja1 worksheet.
                var adapter = new OleDbDataAdapter("SELECT * FROM [Hoja1$]", connectionString);
                var ds = new DataSet();
                adapter.Fill(ds, "results");
                DataTable data = ds.Tables["results"];

                var people = new List<Person>();

                for (int i = 0; i < data.Rows.Count - 1; i++)
                {
                    Person newPerson = new Person();
                    newPerson.Id = data.Rows[i].Field<double?>("Id");
                    newPerson.Name = data.Rows[i].Field<string>("Name");
                    newPerson.LastName = data.Rows[i].Field<string>("LastName");
                    newPerson.DateOfBirth = data.Rows[i].Field<DateTime?>("DateOfBirth");

                    people.Add(newPerson);
                }

                return View("UploadComplete", people);
            }

            return RedirectToAction("Error", "Upload");
        }

        public ActionResult Error()
        {
            return View();
        }
    }
}

Not feeling so confident this is the best approach. Any suggestion any of you MVC3 vets have for this aspiring senior programmer? :)

The logic for reading the Excel file (i.e. everything from var connectionString until before the return) belongs in a model method, not the controller.

You might also want to handle the case when the uploaded file isn't an Excel file or it doesn't have the columns you expect it to. Depending on how you use this or who uses it, this might not be necessary, but if normal users are allowed to upload files, they might very well upload the wrong file even if you tell them not to. And in that case a meaningful error message leads to a better user experience.

Regarding the question in your comment: As I said, I don't know how this is going to be used, but I can't think of a situation where it would be helpful for the user to be able to re-upload the file by hitting the refresh button. So yes, redirecting to an UploadComplete action makes sense.


Importing an Excel document into a project I use the following code to verify that columns have data and display the appropriate error message.

My code is working but it just doesn't look tidy. Is there any way to refactor all the if statements into a loop to display the error message, possibly another method which checks all? checkIfColumnIsEmpty is a bool method which returns true if the column is empty.

        //if either column 0 and 1 are empty && column 2,3,4 and 5 are not
    if (!checkIfColumnisEmpty(r.ItemArray[0]) || !checkIfColumnisEmpty(r.ItemArray[1])
    && checkIfColumnisEmpty(r.ItemArray[2]) && checkIfColumnisEmpty(r.ItemArray[3])
    && checkIfColumnisEmpty(r.ItemArray[4]) && checkIfColumnisEmpty(r.ItemArray[5]))
    {
        if (checkIfColumnisEmpty(r.ItemArray[0]) && !checkIfColumnisEmpty(r.ItemArray[1]))
        {
            throw new ImportBOQException("Error importing document: First column is empty");
        }
        else if (!checkIfColumnisEmpty(r.ItemArray[0]) && checkIfColumnisEmpty(r.ItemArray[1]))
        {
            throw new ImportBOQException("Error importing document: Second column is empty");
        }
        else if (!checkIfColumnisEmpty(r.ItemArray[0]) && !checkIfColumnisEmpty(r.ItemArray[1]))
        {
            //all columns are valid so...
            Column0inSpreadsheet = r.ItemArray[0] as string;
            Column1inSpreadsheet = r.ItemArray[1] as string;

            //Other code which performs other operations, once the level as reached this far
        }
    }

    //if column 0 and 1 are NOT empty && Either column 2,3,4 or 5 is empty
    else if (checkIfColumnisEmpty(r.ItemArray[0]) && checkIfColumnisEmpty(r.ItemArray[1])
    || !checkIfColumnisEmpty(r.ItemArray[2]) || !checkIfColumnisEmpty(r.ItemArray[3])
    || !checkIfColumnisEmpty(r.ItemArray[4]) || !checkIfColumnisEmpty(r.ItemArray[5]))
    {
        if (checkIfColumnisEmpty(r.ItemArray[2]))
        {
            throw new ImportBOQException("Error importing document: Third column is empty");
        }
        else if (checkIfColumnisEmpty(r.ItemArray[3]))
        {
            throw new ImportBOQException("Error importing document: Fourth column is empty");
        }
        else if (checkIfColumnisEmpty(r.ItemArray[4]))
        {
            throw new ImportBOQException("Error importing document: Fifth column is empty");
        }
        else if (checkIfColumnisEmpty(r.ItemArray[5]))
        {
            throw new ImportBOQException("Error importing document: Sixth column is empty");
        }
        else
    //all columns are valid so...
        {
            Column2inSpreadsheet = (r.ItemArray[2]) as string;
            Column3inSpreadsheet = (r.ItemArray[3]) as string;
            Column4inSpreadsheet = (r.ItemArray[4]) as string;
            Column5inSpreadsheet = (r.ItemArray[5]) as string;

    //Other code which performs other operations, once the level as reached this far
        }
    }
    else
    //other errors ot related to empty colums
    {
        throw new Exception("Error Uploading");
    }
}

First of all, better names than columnX would be nice. I guess the indentation is a copying-issue, otherwise this would be the next optimization. Furthermore I have the feeling that mixing the && and the || in your condition without parenthesis is not what you want to do?

col0empty=checkIfColumnisEmpty(r.ItemArray[0])
col1empty=checkIfColumnisEmpty(r.ItemArray[1])
col2empty=checkIfColumnisEmpty(r.ItemArray[2])
col3empty=checkIfColumnisEmpty(r.ItemArray[3])
col4empty=checkIfColumnisEmpty(r.ItemArray[4])
col5empty=checkIfColumnisEmpty(r.ItemArray[5])

if ((!col0empty || !col1empty) && col2empty && col3empty && col4empty && col5empty)
{
    if (col0empty && !col1empty) errorWithEmptyColumn("First");
    else if (!col0empty && col1empty) errorWithEmptyColumn("Second");
    else if (!col0empty && !col1empty)
    {
        //Code after validation
    }
}
else if ((col0empty && col1empty) || !col2empty || !col3empty || !col4empty || !col5empty)
{
    if (col2empty) errorWithEmptyColumn("Third");
    else if (col3empty) errorWithEmptyColumn("Fourth");
    else if (col4empty) errorWithEmptyColumn("Fifth");
    else if (col5empty) errorWithEmptyColumn("Sixth");
    else
    {
        //Code after validation
    }
}
else
{
    throw new ImportBOQException("Error Uploading"); //don't throw raw exceptions
}
// } one to much

private void errorWithEmptyColumn(String columnName) throws ImportBOQException
{
    throw new ImportBOQException("Error importing document: "+columnName+" column is empty");
}

For further optimization it is import to know how similar the code in the two remaining branches is?

You could also extract the condition in an variable and give them a name: i.e. rowTypeX and rowTypeY. Maybe the result is more intuitive if you move the handling of the row types in a method each:

empty=col0empty && col1empty && col2empty && col3empty && col4empty && col5empty
rowTypeX=col2empty && col3empty && col4empty && col5empty;
rowTypeY=col0empty && col1empty;
if ( empty) throw new ImportBOQException("Empty row");
else if (rowTypeX) handleRowTypeX(r);
else if (rowTypeY) handleRowTypeY(r);
else throw new ImportBOQException("Error Uploading");


Algorithm to find the Array Balance Point Task: Find the balance point of an array; return the index that if you break the array into left and right at that point will create the same sum on both sides.
e.g: given the array [2, 4, 5, 1, -2, 7, 2, 1, 4]
returned value should be: 3
left: 2, 4, 5, 1 = 12
right: -2, 7, 2, 1, 4 = 12

public static arrayBalance(input?: number[]): any {
    let sum = 0;
    for(let i=0; i<input.length; i++) {
        let cur = input[i];
        sum += cur;
    }

    let mid = sum / 2;
    let sum2 = 0;
    let ret = 0;
    while(sum2 < mid && ret < input.length) {
        sum2 += input[ret];
        ret++;
    }

    return sum2 == mid ? (ret - 1) : -1;
}

  Time complexity: \$O(2n)\$,
  Space complexity: \$O(1)\$

Am I right regarding the complexities?
Do you have different implementation suggestions?

First, a suggestion on the interface: use null to represent no match rather than -1, since that makes it easier to force the caller to check for a no-solution case (i.e. if they try to use the unchecked return of this function as an index, a -1 will only fail at runtime, whereas null will fail at transpile time).

function arrayBalance(input: number[]): number | null {

In reading over the code I got confused by the variable names -- mid isn't actually a midpoint, it's the expected sum of each half.  Between all of the variables being declared with let rather than const and the uninformative names (why is there sum and sum2?), it's hard to even tell at a glance which values are being recalculated and which are constants.

Instead of taking six lines of code to find the "half-sum" that we want each side of the "split" to have, let's just do it in one (brevity is the soul of wit, and this kind of thing is exactly what the reduce function is for), and declare it as const since it's not going to change for the rest of the function once we've computed it:

const halfSum = input.reduce((a, b) => a + b) / 2;

From here, all we need to do is find out how many array elements we need to total up to equal halfSum.

Here's how I might write the rest, signifying the running total of the "left" side of the array as leftSum and doing a simple for loop over the array indices:

let leftSum = 0;
for (let i = 0; i < input.length; i++)
{
    leftSum += input[i];
    if (leftSum == halfSum)
        return i;
}
return null;

As we iterate through i we build a sum of everything to the left of i (leftSum).  Our goal is to make leftSum equal halfSum.  If no solution is found within the loop we return null.

It's tempting to use inequality comparisons to try to optimize the no-solution case, but consider cases where the input array has lots of negative numbers distributed randomly!  halfSum could be negative or zero, and leftSum could go up and then down and then up and then down again as you increment i, so unless you've gone through the entire array you can't ever be certain that a solution does not exist.

Note also that it's possible for there to be multiple valid solutions; this implementation will always return the lowest one in that instance, but you could write a version of this function that always goes through the entire array and returns another array of all the solutions it found.


Parsing oscilloscope data, follow up Follow-up up on Parsing oscilloscope binary data.
I wrote a simple parser for Tektronix's internal file format .isf.
I revised my code to the following:

import numpy as np
import os.path

def parse_curve(isf_file):
    """
    Reads one tektronix .isf file and returns a dictionary containing
    all tags as keys. The actual data is stored in the key "data".
    """
    extensions = set([".isf"])
    if os.path.splitext(isf_file)[-1].lower() not in extensions:
        raise ValueError("File type unkown.")

    with open(isf_file, 'rb') as ifile:
        # read header
        header = {}
        while True:
            name = _read_chunk(ifile, " ")
            if name != ":CURVE":
                value = _read_chunk(ifile, ";")

                assert name not in header
                header[name] = value
            else:
                # ":CURVE " is the last tag of the header, followed by
                # '#XYYY' with X being the number of bytes of YYY.
                # YYY is the length of the datastream following in bytes.
                value = ifile.read(2)
                y_str = ifile.read(int(value[-1]))
                value += y_str

                # the number of bytes might be present with or without the
                # preceding header ":WFMPRE:"
                nobytes = header.get("BYT_NR",
                                     header.get(":WFMPRE:BYT_NR", "0")
                                     )
                assert int(y_str) == int(header["NR_PT"]) * int(nobytes)
                header[name] = value
                currentposition = ifile.tell()
                break

        assert header["ENCDG"] == "BINARY"

        # read data as numpy array
        header["data"] = _read_data(ifile, currentposition, header)

    return header

def _read_chunk(headerfile, delimiter):
    """
    Reads one chunk of header data. Based on delimiter, this may be a tag
    (ended by " ") or the value of a tag (ended by ";").
    """
    prior_delimiter = None
    chunk = []
    while True:
        c = headerfile.read(1)
        if c != delimiter:
            chunk.append(c)
            if c == '"':
                # switch delimiter to make sure to parse the whole string
                # enclosed in '"'.
                delimiter, prior_delimiter = c, delimiter
        elif prior_delimiter:
            # switch back delimiter
            chunk.append(c)
            delimiter, prior_delimiter = prior_delimiter, None
        else:
            return "".join(chunk)

def _read_data(bfile, position, header):
    """
    Reads in the binary data as numpy array.
    Apparently, there are only 1d-signals stored in .isf files, so a 1d-array
    is read.
    """
    # determine the datatype from header tags
    datatype = ">" if header["BYT_OR"] == "MSB" else "<"
    if header["BN_FMT"] == "RI":
        datatype += "i"
    else:
        datatype += "u"
    # BYT_NR might be present with preceding header ":WFMPRE:BYT_NR"
    nobytes = header.get("BYT_NR",
                         header.get(":WFMPRE:BYT_NR", "")
                         )
    datatype += nobytes
    assert len(datatype) >= 3

    bfile.seek(position)
    data = np.fromfile(bfile, datatype)
    assert data.size == int(header["NR_PT"])

    # calculate true values
    data = data * float(header["YMULT"]) + float(header["YZERO"])

    return data

In the original question, _read_data did a little more, this part was now moved to another module.

One concern I still have, is whether the assert part when reading the ":CURVE" tag is the best solution:

nobytes = header.get("BYT_NR",
                     header.get(":WFMPRE:BYT_NR", "0")
                     )
assert int(y_str) == int(header["NR_PT"]) * int(nobytes)

In general it looks good to me; only small nits:

You want to move from asserts to throwing proper, informative exceptions - for instance, showing both expected and actual values.

At the end of if name != ":CURVE":, you can just issue a continue instead of an else block - helps with the indentation.

Instead of making chunk a list and then doing a join at the end, you can make it a string and do string appending.


AngularFire Tic Tac Toe Game I am building a Tic Tac Toe game with AngularJS and making it online playable with AngularFire.  The Tic Tac Toe logic is all there so this question concerns Angularfire a little bit more. I want to be able to match a user with an opponent as well as delete inactive games.

I have done my homework and at this point I want to get the smell out of my code.  Since I am handling asynchronous requests, I built a factory that returns a promise.

app.factory("GameCreator", ["$q","$firebase",function($q, $firebase){
    var deferred = $q.defer();
    var ref = new Firebase("https://myfirebase.firebaseio.com/");
    $firebase(ref).$on("loaded",function(value){
        var games = $firebase(ref);
        var IDs = games.$getIndex();
        var initGame = function(n){
            games.$add({
                //the board's state and turns, shared by all users
                board:[['','',''],['','',''],['','','']],
                xTurn:true,
                turns:0,
                playerone:{piece:'x', ready:false,won:false},
                playertwo:{piece:'o',ready:false,won:false},
                //helpers to sync and assign player
                xIsAvailable:false,
                oIsAvailable:true
            });
            //my opponent did something.  His/her actions need to appear on my screen.
            games.$on("change",function(){
                deferred.resolve( games.$child(games.$getIndex()[n]) );
            });
        };
        var n = IDs.length;
        //if no games, then build a game
        if(n == 0){
            initGame(n);
        }
        //else, determine placement in new vs existing game
        else{
            //if last - 1 has a spot, put me in that game, else create me a new game
            var checkThisGame = games.$child(IDs[n-1]);
            checkThisGame.$on('loaded',function(){
                if(checkThisGame.oIsAvailable == true){
                    checkThisGame.oIsAvailable = false;
                    checkThisGame.$save();
                    deferred.resolve(checkThisGame);
                }
                else{
                    initGame(n);
                }
            });
        }
    return deferred.promise;
    });
}])

In my controller, I then use the resolved promise to fulfill my objectives:

app.controller ('BoardCtrl', function($scope,$timeout,GameCreator,$window) {
    GameCreator.then(function(returnedData){
        returnedData.$on('loaded',function(){
            $scope.game = returnedData;
            var piece;
            if(returnedData.oIsAvailable == true) {
                piece = 'x';
            }
            else {
                piece = 'o';
            }
            $scope.myPiece = {
                val:piece
            };
            $window.onbeforeunload = function (event) {
                //delete this game from the firebase I/O
                returnedData.$remove();
                return null;
            }
            //the rest of the tic tac toe game logic follows here,
            //with all proper calls to $scope.game.$save()
        });
    });
);

Since I do not completely know what I am doing, I will ask: did I do this right?  Is there some refactoring that could be done?  It works as expected but I thought I would ask for a review.

I am not an angular expert, but this

var piece;
if(returnedData.oIsAvailable == true) {
    piece = 'x';
}
else {
    piece = 'o';
}
$scope.myPiece = {
    val:piece
};

could be

$scope.myPiece = {
    val: returnedData.oIsAvailable ? 'x' : 'o'
};

Also

I know the firebase API calls the object returned by new Firebase a (root) reference. It still irks me since my brain keeps thinking what is ref?? I would probably go against the grain and call it db, but that could be because I am simply not familiar enough with the API
if(checkThisGame.oIsAvailable == true){ should be if(checkThisGame.oIsAvailable){ or if(checkThisGame.oIsAvailable === true){
the game is well documented
JsHint.com finds a number of small things, you should use the site


dynarr - yet another simple C dynamic array library I wrote my own dynamic array library for C as I was not happy with the others I found. It is very crude but the basic logic should be correct, it mainly revolves making realloc easier to use and avoiding repetitive code.
Please offer your suggestions and criticisms!
#include <stdlib.h>
#include <string.h>

#define DYNARRAY_H_INCREMENT 50

typedef struct dynarr dynarr;

struct dynarr {
    void *actual;
    size_t size;
    size_t e_size;
    size_t occupied;
    size_t increment;
    void (*mem_callback)(dynarr *);
};

void *dynarr_nomen(dynarr *array) {
    if (array->mem_callback) array->mem_callback(array);
    array->actual = NULL;
    array->occupied = 0;
    return NULL;
}

dynarr dynarr_init_full(size_t e_size, size_t increment, void (*mem_callback)(dynarr *)) {
    return (dynarr){
        .actual = NULL,
        .size = 0,
        .e_size = e_size,
        .occupied = 0,
        .increment = increment,
        .mem_callback = mem_callback,
    };
}

dynarr dynarr_init(size_t element_size) {
    return dynarr_init_full(element_size, DYNARRAY_H_INCREMENT, NULL);
}

void *dynarr_push(dynarr *array, void *data) {
    if (array->occupied == array->size) {
        // Array is full
        // Increase size
        size_t new_real_size = (array->size + array->increment) * array->e_size;
        array->actual = realloc(array->actual, new_real_size);
        if (!array->actual) return dynarr_nomen(array);
        array->size += array->increment;
    }

    // Copy data
    char *charr = (char *) array->actual;
    memcpy(charr + array->occupied++ * array->e_size, data, array->e_size);

    return array->actual;
}

void *dynarr_get(dynarr *array, size_t *size) {
    if (size) *size = array->occupied;
    return array->actual;
}

void *dynarr_compact(dynarr *array) {
    if (array->occupied == array->size) goto end;
    array->actual = realloc(array->actual, array->occupied * array->e_size);
    if (!array->actual) return dynarr_nomen(array);
    array->size = array->occupied;
    end: return array->actual;
}

void dynarr_free(dynarr *array) {
    free(array->actual);
    array->actual = NULL;
    array->occupied = 0;
}

I intend to split declarations into a separate header in the future, I have kept it as a simple single source file for now.
I also made a very crude example program which could definitely use some major improvements... or maybe it can even be replaced by something else.
#include <signal.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include "dynarr.c"

volatile sig_atomic_t signaled = false;

void trap_interrupt(int sig) {
    signaled = true;
}

int main(void) {
    signal(SIGINT, trap_interrupt);

    int c;
    char data;
    dynarr input = dynarr_init(sizeof data);

    for (;;) {
        if (signaled) break;
        c = getc(stdin);
        if (c == EOF) break;
        data = c;
        if (!dynarr_push(&input, &data)) {
            puts("Ran out of memory!!!");
            break;
        };
    }

    size_t count;
    dynarr_get(&input, &count);
    printf("Stored %zu bytes in dynamic array\n", count);

    return EXIT_SUCCESS;
}

Regarding incrementing by a fixed amount
The problem with resizing the storage by a fixed amount each time you reach capacity is that it effectively makes pushing a new element to the array an \$\mathcal{O}(N)\$ operation. If you instead double the amount of storage each time, pushing will be amortized \$\mathcal{O}(1)\$.
The drawback of doubling the capacity each time is that you might waste more memory than if you only increase it by fixed amounts. However, unless you are constrained by the amount of memory you have, I would recommend the doubling approach.
Memory leaks
Your code leaks memory when running out of memory. If you call realloc(), but it returns NULL, the original memory is not freed and still valid. So instead of unconditionally overwriting array->actual, you should write something like:
void *new_actual = realloc(array->actual, new_real_size);

if (!new_actual) {
    free(array->actual);
    return dynarray_nomem(array->actual);
}

array->actual = new_actual;
array->size = new_real_size;

Alternatively, and perhaps even better, is to just keep the original data and return NULL, and let the caller worry about whether they want to destroy the array or want to keep using it:
void *new_actual = realloc(array->actual, new_real_size);

if (!new_actual) {
    return NULL;
}
...

(Re)move the callback
I don't see the point of having a callback function that is called when memory allocation failed, if the caller already has to check the return value of dynarray_push() and dynarray_compact(). I would just remove this unnecessary complexity.

This is an useful feature which can be used to avoid error checking for each push, instead of the caller can specify a callback function which would use something like longjmp to initiate a no memory exit sequence.

If you want this, I recommend you move the mem_callback pointer out of struct dynarr, and make it a global variable named dynarr_mem_callback.
Consider returning a pointer to the pushed element
It would be much more useful if dynarray_push() would return a pointer to the element that was just pushed, instead of a pointer to the start of the array. There is already dynarray_get() to get a pointer to the start, but see below.
Add a function to get a pointer to a specific element
Since it is likely that a user might want to access a specific element in the dynamic array, it would be helpful to add a function that does that in one go. In fact, I would name that function dynarray_get():
void *dynarray_get(dynarray *array, size_t offset) {
    // Bounds checking, has a performance impact though:
    if (offset >= array->occupied)
        return NULL;

    return (char *)array->actual + offset * array->e_size;
}

And add separate functions dynarray_data() and dynarray_size() to get a pointer to the underlying storage and the number of elements respectively.
Avoid using goto
It is quite easy to avoid the goto statement in dynarray_compact(), just replace it with return array->actual. Even better would be to just unconditionally call realloc(), since it is unlikely that size and occupied will be the same, and this is not a function that would be called very often anyway, so it is not performance critical:
void *dynarr_compact(dynarr *array) {
    void *new_actual = realloc(array->actual, array->occupied * array->e_size);

    if (new_actual) {
        array->actual = new_actual;
        array->size = array->occupied;
    }

    return array->actual;
}


Task.Finally extension, good, bad, or ugly? I wrote this, and it has helped to avoid the 'Some exception weren't handled' problem. Is there something glaringly wrong with this that I might have missed?

    /// <summary>
    /// Handles any exceptions on this task, and executes action on specified scheduler.
    /// </summary>
    /// <param name="task">The task.</param>
    /// <param name="exceptionHandler">The exception handler.</param>
    /// <param name="finalAction">The final action.</param>
    /// <param name="scheduler">The scheduler.</param>
    public static void Finally(this Task task, Action<Exception> exceptionHandler,
                                               Action finalAction, TaskScheduler scheduler)
    {
        task.ContinueWith(t =>
        {
            if(finalAction != null) finalAction();

            if(t.IsCanceled || !t.IsFaulted || exceptionHandler == null) return;
            var innerException = t.Exception.Flatten().InnerExceptions.FirstOrDefault();
            exceptionHandler(innerException ?? t.Exception);
        }, scheduler);
    }

I think what you've really got here is two extension methods hiding inside one.  Your method is doing the work of both a "finally" and a "catch", and I recommend refactoring it into two methods.  The catch method may look something like this:

Edit: clearly I missed checking the exception type before executing the action, but I'm away from a computer and trying to edit code with my iPhone isn't working too well.

public static void Catch<TException>(this Task task, Action<TException> exceptionHandler,
                                     TaskScheduler scheduler = null) where TException : Exception
{
    if (exceptionHandler == null)
        throw new ArgumentNullException("exceptionHandler cannot be null");

    task.ContinueWith(t =>
    {
        if(t.IsCanceled || !t.IsFaulted || t.Exception == null) return;
        var innerException = t.Exception.Flatten().InnerExceptions.FirstOrDefault();
        exceptionHandler(innerException ?? t.Exception);
    }, scheduler ?? TaskScheduler.Default);
}

Edit: Finally got time to get back to this.  Here's an example of a complete solution:

using System;
using System.Threading.Tasks;

namespace ConsoleApplication1
{
class Program
{
    static void Main(string[] args)
    {
        Task.Run(() => {
                Console.WriteLine("In the Task");
                throw new ArgumentException("Thrown from task");
                //throw new Exception("Thrown from task");
            })
            .Catch<Exception>((e) => Console.WriteLine("Caught Exception {0}", e))
            .Catch<ArgumentException>((ae) => Console.WriteLine("Caught Argument Excetion {0}", ae))
            .Finally(() => Console.WriteLine("in the Finally"));

        Console.ReadKey();
        Console.ReadKey();
    }
}

static class TaskExtensions
{

    public static void Finally(this Task task, Action finalAction,
                               TaskScheduler scheduler = null)
    {
        if (finalAction == null)
            throw new ArgumentNullException("finalAction cannot be null");

        task.ContinueWith(t => finalAction(), scheduler ?? TaskScheduler.Default);
    }

    public static Task Catch<TException>(this Task task, Action<TException> exceptionHandler,
                                         TaskScheduler scheduler = null) where TException : Exception
    {
        if (exceptionHandler == null)
            throw new ArgumentNullException("exceptionHandler cannot be null");

        task.ContinueWith(t =>
        {
            if (t.IsCanceled || !t.IsFaulted || t.Exception == null)
                return;

            var exception =
                t.Exception.Flatten().InnerExceptions.FirstOrDefault() ?? t.Exception;

            if (exception is TException)
            {
                exceptionHandler((TException) exception);
            }
        }, scheduler ?? TaskScheduler.Default);

        return task;
    }
}
}


Divide List into groups I have a set of Service, and I want to partition them into ServiceClass, and have a reference from a Service to the Service class its belong.
I am storing the reference in Dictionary<Service, ServiceClass>.

In the code below, I want to split into five service classes. I feel the code below is not well written, such as the naming of variable, but I am not sure how to improve it, can anyone help?

int group = 5;
List<Service> service = FileIO.ReadFeatureFile(@"D:\C Drive\Desktop\input.txt");
int numPerClass = service.Count / group;
Dictionary<Service, ServiceClass> ServiceToServiceClass = new Dictionary<Service, ServiceClass>();

for (int i = 0; i < group - 1;i++ )
{
    List<Service> serviceGrp = service.GetRange(0, numPerClass);
    ServiceClass sc=new ServiceClass(serviceGrp);
    foreach(Service service1 in serviceGrp)
    {
        ServiceToServiceClass.Add(service1, sc);
    }
    service.RemoveRange(0, numPerClass);
}

ServiceClass sc1 = new ServiceClass(service);
foreach (Service service1 in service)
{
    ServiceToServiceClass.Add(service1, sc1);
}

First off, as dreza said, work on your variable names.  Secondly, you should really think about using more than one method for solving these types of problems.

All of my suggestions are assuming this is inside a class meant to hold this information.

Now to your code.

The line int group = 5; should be a constant.  I would also rename it to something like numberOfServiceGroups.  So the code line looks like:

const int numberOfServiceGroups = 5;

I would move the dictionary to a class variable and instantiate it in the constructor.  I would also rename it to something like _serviceLookup.  The _ signifying it is a class variable

Instead of using GetRange and RemoveRange, I would change it to use the linq statements Skip([int items]).Take([int items]) and because you are using a for loop, the Skip count can be calculated:

for (var i = 0; i < numberOfServiceGroups; i++)
{
    var servicesToProcess = service.Skip(i * servicesPerGroup).Take(servicesPerGroup);
    // More on this in a minute
}

The nice thing about Take() is that it will not blow-up if you go over bounds, it will just return what is left.  Doing this will eliminate the need to have the catch loop at the end of your method.

I would move the processing of the services into the group to another method.  This reduces nesting.

private void ProcessGroup(IList<Service> services)
{
    var serviceClass = new ServiceClass(services);
    foreach (var currentService in services)
    {
        _serviceLookup.Add(currentService, serviceClass);
    }
}

So your method now looks like:

private void Group()
{
    const int numberOfServiceGroups = 5;

    List<Service> services = FileIO.ReadFeatureFile(@"D:\C Drive\Desktop\input.txt");
    var servicesPerGroup = service.Count/numberOfServiceGroups;
    var serviceToServiceClass = new Dictionary<Service, ServiceClass>();

    for (var i = 0; i < numberOfServiceGroups; i++)
    {
        var servicesToProcess = services.Skip(i * servicesPerGroup).Take(servicesPerGroup);
        ProcessGroup(servicesToProcess.ToList());
    }
}

And your class:

public class ClassName
{
    public IDictionary<Service, ServiceClass> _serviceLookup;

    public ClassName()
    {
        _serviceLookup = new Dictionary<Service, ServiceClass>();
    }

    private void Group()
    {
        const int numberOfServiceGroups = 5;

        List<Service> services = FileIO.ReadFeatureFile(@"D:\C Drive\Desktop\input.txt");
        var servicesPerGroup = service.Count/numberOfServiceGroups;

        for (var i = 0; i < numberOfServiceGroups; i++)
        {
            var servicesToProcess = services.Skip(i * servicesPerGroup).Take(servicesPerGroup);
            ProcessGroup(servicesToProcess.ToList());
        }
    }

    private void ProcessGroup(IList<Service> services)
    {
        var serviceClass = new ServiceClass(services);
        foreach (var currentService in services)
        {
            _serviceLookup.Add(currentService, serviceClass);
        }
    }
}

This should be a good start.  I see a few more things I'd like to do, but they are beyond this scope.

Also note, I have not tested this due to missing classes / information.


Understanding interface with animal classes I made this small program to demonstrate to myself an example of interface.  I wanted to confirm whether it is correct and if there is anything more I should know about interfaces other than the importance of implementing polymorphism and multiple inheritance.

//program.cs
class Program
{
    static void Main(string[] args)
    {
        Dog oDog = new Dog();
        Console.WriteLine(oDog.Cry());

        Cat oCat = new Cat();
        Console.WriteLine(oCat.Cry());

        Console.ReadKey();
    }

//IAnimal.cs
interface IAnimal
{
    string Cry();
}

//Dog.cs
class Dog : IAnimal
{
    public string Cry()
    {
        return "Woof!";
    }
}

//Cat.cs
class Cat : IAnimal
{
    public string Cry()
    {
        return "Meow!";
    }
}

Your example accesses the cat through a variable of type Cat and the dog through Dog. This would work, even if both classes did not implement a common interface. Inheritance is not involved.

In order to really demonstrate the usefulness of polymorphism I suggest the following code:

interface IAnimal
{
    string Name { get; set; }
    string Cry();
}

class Dog : IAnimal
{
    public string Name { get; set; }
    public string Cry()
    {
        return "Woof!";
    }
}

class Cat : IAnimal
{
    public string Name { get; set; }
    public string Cry()
    {
        return "Meow!";
    }
}

And here is a possible test:

static void Main(string[] args)
{
    var animals = new List<IAnimal>();

    animals.Add(new Dog { Name = "Robby" });
    animals.Add(new Dog { Name = "Fify" });
    animals.Add(new Cat { Name = "Mimy" });

    PrintAnimals(animals);

    Console.ReadKey();
}

private static void PrintAnimals(IEnumerable<IAnimal> animals)
{
    foreach (IAnimal animal in animals) {
        Console.WriteLine("Here is {0}: {1}", animal.Name, animal.Cry());
    }
}

It demonstrates that different types of animals can be treated the same way, but behave differently. This is what polymorphism is about. The word means many (poly) shapes or forms (morph). You can add different types of animals to a collection.

The PrintAnimals method has a parameter of type IEnumerable<IAnimal> allowing you to pass it different types of collections (arrays, lists, linked lists, stacks and many more, as yet another example of polymorphism). It uses a unique Console.WriteLine statement for all types of animals, without even knowing of which type an animal really is. The only thing it knows is, that it implements IAnimal. You could even add new types of animals later, without having to change the PrintAnimals method and it would still work as expected.


Algorithm that spans orthogonal vectors: Python The code consists of three functions:

Spans arbitrary vector (takes dimension as an argument)
Spans vector orthogonal to the one passed in the argument
Finds cross product between two vectors.

The code is following:

def span_vector(n):
    '''n represents dimension of the vector.'''
    return [random.randrange(-1000,1000) for x in range(n)]

def span_orthogonal(vec):
    '''vec represents n-dimensional vector'''
    '''Function spans an arbitrary vector that is orthogonal to vec'''

    dimension = len(vec)
    orthogonal_vec = []
    for k in range(dimension-1):
        orthogonal_vec.append(random.randrange(-1000,1000))
    last_k = ((-1)*sum([vec[x]*orthogonal_vec[x] for x in range(dimension-1)]))/vec[-1]
    orthogonal_vec.append(last_k)
    return orthogonal_vec

def cross_product(v1,v2):
    return sum(v1[x]*v2[x] for x in range(len(v1)))

What can be improved?

EDIT The last function must read dot_product, but not cross_product. I made a mistake.

You should probably be using Numpy, although I don't know enough about your situation to comment any further.

Assuming that you need to retain "pure Python", the following improvements can be made:

Negation

Replace (-1)* with -

Generators

Replace your for k in range(dimension-1): loop with

orthogonal_vec = [
    random.randrange(-1000,1000)
    for _ in range(dimension-1)
]

Type hints

n: int, vec: typing.Sequence[float] (probably) . And the first two functions return -> typing.List[float]. cross_product both accepts and returns float.

Inner list

sum([ ... ])

shouldn't use an inner list. Just pass the generator directly to sum.


Plotting with interpolation using Matplotlib Program description:

You are given a set of two functions: $$f=x^3-6x^2+x+5; g=(x-2)^2-6$$
Plot them using Matprolib on a user input segment [a; b].

My solution:
import matplotlib.pyplot as plt
import numpy as np
from scipy.interpolate import interpolate # for smoothing

def func(x):
    return [pow(x, 3) - 6 * pow(x, 2) + x + 5, pow((x-2), 2) - 6]
# plt.plot([1,5, -3, 0.5], [1, 25, 9, 0.5])
# plt.plot(1, 7, "r+")
# plt.plot(-1, 7, "bo")
f = []
f_1 = []
x = []
interval = [int(x) for x in input("Define segment (i.e. a b): ").split(' ')]
for val in range(interval[0], interval[1] + 1):
    x.append(val)
    f.append(func(val)[0])
    f_1.append(func(val)[1])

linear_space = np.linspace(interval[0], interval[1], 300)
a_BSpline = interpolate.make_interp_spline(x, f)
b_BSpline = interpolate.make_interp_spline(x, f_1)

f_new = a_BSpline(linear_space)
f_1_new = b_BSpline(linear_space)

plt.plot(linear_space, f_new, color="#47c984",
linestyle="solid", linewidth=1)

plt.plot(linear_space, f_1_new, color="#fc6703",
linestyle="solid", linewidth=1)

plt.gca().spines["left"].set_position("zero")
plt.gca().spines["bottom"].set_position("zero")
plt.show()

Input: -10 10
Output:

Question:
Is there any way to make this code more concise?
Thank you in advance.

As I pointed out in the comments, I do not think smoothing is needed when plotting functions that are already smooth, such as polynominals. In those cases, the graphs would look rough only if the points plotted are not dense enough (on the x axis).

Since you have already imported and used numpy, it would be better to use numpy.polyval for vectorized evaluation of polynominals.

Drawing spines at zero would not work well if the input range does not include zero. In that case, the "center" position might be used instead of "zero". (I will not implement this logic in my code below)

The top and right spines should not be shown.

Consider using plt.style.use for setting common style information. See here for alternative options for style setting.

Avoid constants like "#47c984". Name them with appropriate constant names for better readability.

It is a good practice to comply to the PEP 8 style when writing Python code. Use an IDE (such as PyCharm) or an online checker (such as this) for automatic PEP 8 violation checking.

When running code outside a method / class, it is a good practice to put the code inside a main guard. See here for more explanation.

Here is an improved version:
import numpy as np
import matplotlib.pyplot as plt

if __name__ == "__main__":
    # Define constants (color names come from https://colornamer.robertcooper.me/)
    INPUT_MESSAGE = "Lower and upper bounds of x, separated by a space: "
    NUM_POINTS = 300
    PARIS_GREEN = "#47c984"
    BLAZE_ORANGE = "#fc6703"

    # Read input
    lb, ub = map(float, input(INPUT_MESSAGE).split())

    # Compute coordinates of points to be plotted
    x = np.linspace(lb, ub, NUM_POINTS)
    f_x = np.polyval([1, -6, 1, 5], x)
    g_x = np.square(x - 2) - 6

    # Plotting
    plt.style.use({"lines.linestyle": "solid", "lines.linewidth": 1})
    plt.plot(x, f_x, color=PARIS_GREEN)
    plt.plot(x, g_x, color=BLAZE_ORANGE)

    # Adjust spines
    spines = plt.gca().spines
    spines["left"].set_position("zero")
    spines["bottom"].set_position("zero")
    spines["right"].set_visible(False)
    spines["top"].set_visible(False)

    # Display plot
    plt.show()


How to reduce code in hash created using Database and Params in rails I have wrote following solution for one answer but I want to refactor to make it more simple and readable.
I am receiving params
sample_params = [{ user_id: 1, email: 'example1@example.com' },
{ user_id: 5, email: 'example5@example.com' },
{ user_id: 13, email: 'example13@example.com'}]

I want to create following hash to replace emails received from params
[
{department_id: 1, users: [{user_id: 1, email: 'example1@example.com'},
 {user_id: 5, email: 'example5@example.com'}]},
{department_id: 2, users: [{ user_id: 13, email: 'example13@example.com']

I am retrieving data from db using following codes
data =
  User
  .where(id: sample_params.pluck(:user_id))
  .pluck(:department_id, :id)
  .group_by(&:first).map do |department_id, users|
    { department_id: department_id,
      users: users.map { |_, id| id } }
  end

and then I am creating hash using following
result = []
data.each do |department|
  department_users = []
   department[:users].each do |user|
     emails = sample_params.select { |user| user[:user_id] == 1 }[0][:email];
     department_users << { id: user, emails: emails }
   end; result << {department_id: department[:department_id], users: department_users}
end

How can I refactor to get following benefits

Easy to read
less number of lines
Performance It has one query of sql so not an issue but on memory I am saving all in array.

Not sure if it's what you're looking for, in terms of readability:
data = User.where(id: sample_params.pluck(:user_id))
           .pluck(:department_id, :id).group_by {|u| u.shift }

results = data.map do |department_id, user_ids|
  users = user_ids.flatten.map do |id|
    email = sample_params.find { |user| user[:user_id] == id }[:email]
    {user_id: id, email: email}
  end
  { department_id: department_id, users: users }
end

About saving memory because of the array, there will be always the queried data to the db in memory, besides this new array you're generating. The garbage collector won't take care of all these until your http request has been finished, so I'm not sure if there's something here you can do to improve the use of space.
If this is the case where there are thousands of users mapped to this json, maybe you should consider to use .find_each, maybe caching data or another approach, but only you know what's the nature of the data you're querying.


Thread Safe Objects in CSharp - ConcurrentDictionary I'm using ConcurrentDictionary to hold the records.. I'm reading files from the local system in parallel, to speed up the process.

Sample 1:

static ConcurrentDictionary<Int64, Byte[]> concurrentDictionary = new ConcurrentDictionary<Int64, Byte[]>();

private void ReadFilesFromDirectory()
{
  string[] docNames = Directory.GetFiles(CompleteLettersDirectory);
  Parallel.ForEach(docNames, new ParallelOptions { MaxDegreeOfParallelism = 8 }, file =>
    {
      try
      {
          int64 letterId = Convert.ToInt64(file.Split('_')[1]);
          Byte[] CompleteLetterContent;
          CompleteLetterContent = ConvertToPdfWithAspose(System.IO.File.ReadAllBytes(file), letterId);
          concurrentDictionary.GetOrAdd(letterId, CompleteLetterContent);
      }
      catch (Exception ex)
      {
          //Logger.WriteFile(letterId.ToString() + Environment.NewLine, ex);
      }
    }
}

Sample 2: In this sample I want to keep multiple values in dictionary as such I created a letter class which has multiple properties. As Letter class is not threadsafe, will it create any issues ?

static ConcurrentDictionary<Int64, Letter> concurrentDictionary = new ConcurrentDictionary<Int64, Letter>();

private void ReadFilesFromDirectory()
{
  string[] docNames = Directory.GetFiles(CompleteLettersDirectory);
  Parallel.ForEach(docNames, new ParallelOptions { MaxDegreeOfParallelism = 8 }, file =>
    {
      try
      {
          int64 letterId = Convert.ToInt64(file.Split('_')[1]);
          Byte[] LetterContent;
          Byte[] CompleteLetter;

          Letter letter = new Letter();
          letter.LetterId = letterId;

      if(file.Contains("CL"))
      {
    letter.CompleteLetter = ConvertToPdfWithAspose(System.IO.File.ReadAllBytes(file), letterId);
      }
      else
      {
    letter.LetterContent = ConvertToHTML(System.IO.File.ReadAllBytes(file), letterId);
      }
    concurrentDictionary.GetOrAdd(letterId, letter);
          }
      catch (Exception ex)
  {
          //Logger.WriteFile(letterId.ToString() + Environment.NewLine, ex);
      }
    }
}

public class Letter
  {
      public Int64 LetterId { get; set; }
      public Byte[] LetterContent { get; set; }
      public Byte[] CompleteLetter { get; set; }
  }

Can anyone advise if code samples are threadsafe.
Also, should string docNames which holds documents from the directory and is passed to Parallel.ForEach be also threadsafe or not? Or any other recommendation.

I'm reading files from the local system in parallel, to speed up the process.

Are you sure it's actually beneficial? Hard disks work best when you read them in sequence, not when they have to go back and forth to satisfy concurrent requests. (And even if you have a SSD, you're not going to gain anything.)

You seem to be in the habit of using framework names for types (e.g. Byte, Int64) instead of the C# names (byte, long). I believe that using the C# names is more common.

Byte[] CompleteLetterContent;
CompleteLetterContent = ConvertToPdfWithAspose(System.IO.File.ReadAllBytes(file), letterId);

There is no reason to separate the declaration and assignment here. Also, local variables are generally written in camelCase:

Byte[] completeLetterContent = ConvertToPdfWithAspose(System.IO.File.ReadAllBytes(file), letterId);

System.IO.File.ReadAllBytes(file)

You're already using System.IO, so the namespace should be redundant here. And if it isn't because you have something else called File, then you should probably fix that conflict.

concurrentDictionary.GetOrAdd(letterId, CompleteLetterContent);

You're calling GetOrAdd(), but you always only want to add, you never want to get. For that, you can either use TryAdd() or the indexer setter:

concurrentDictionary.TryAdd(letterId, CompleteLetterContent);
concurrentDictionary[letterId] = CompleteLetterContent;

In all three cases, the behavior is the same if there are no values with the same key, but I think the latter two are clearer.

      Byte[] LetterContent;
      Byte[] CompleteLetter;

These variables are never used, get rid of them.

Letter letter = new Letter();
letter.LetterId = letterId;

You can use var (since the type is clear from the assignment) and object initializer here:

var letter = new Letter { LetterId = letterId };

if(file.Contains("CL"))
{
    letter.CompleteLetter = ConvertToPdfWithAspose(System.IO.File.ReadAllBytes(file), letterId);
}
else
{
    letter.LetterContent = ConvertToHTML(System.IO.File.ReadAllBytes(file), letterId);
}

In both branches, you're repeating the System.IO.File.ReadAllBytes(file) part. Consider extracting it into a local variable before the condition.

The code you have shown appears to be thread-safe, assuming that the methods that you're calling from it (ConvertToPdfWithAspose, ConvertToHTML) are thread-safe.


Image loader/writer design (no API, pure C++ on Windows) I have an image loader, now only for bitmaps. I'm a little confused because I want to split my code to different classes, one for writing a BMP data to a file (WRITER), one for loading BMP data from file, or from pre-generated dataset (1) and storing it in its private members.( (1) like a noise function saved to an array).

Currently I have the 2 class for this. The header:

namespace PND
{

    inline void out(const ::std::string); //standard output log
    inline void err(const ::std::string); //error log

    bool file_exists(::std::string m_filename) //this could be used in both classes
    {
        ::std::ifstream infile(m_filename+".bmp");
        return infile.good();
    }

    class BMP
    {
    public:
        BMP();
        BMP(const BMP&);//copy csrt
        ~BMP();
        BMP& operator= (BMP other); //ass. operator

        bool loadData(::std::string filename);//from file, filename is the file's filename
        bool loadData(::std::string filename,const int width, const int height, const unsigned char* data, const unsigned int length);//from array, filename is user defined
        //they are simple pixel data accessors with out of bounds handling
        unsigned char getData_B(const unsigned int x, const unsigned int y);
        unsigned char getData_G(const unsigned int x, const unsigned int y);
        unsigned char getData_R(const unsigned int x, const unsigned int y);

        unsigned int* getSize() { return m_size; }; //some getter
        ::std::string getName() { return m_filename; };

    private:

        void swap(BMP& other);
        void freeData();
            //the bitmap data:
        BITMAPINFOHEADER m_infoheader;
        BITMAPFILEHEADER m_fileheader;
        ::std::string m_filename;
        unsigned char* m_data;
        unsigned int m_size[2];
        long __m_size; //it is the length of the m_data array (num of pixels * 3)
    };

    class WRITER
    {
    public:
        bool writeData(::std::string filename, const int width, const int height, const unsigned char* data, const unsigned int length);
        bool writeData(::std::string filename, const BMP& bmp);//now this is the problem, more info below the code

    private:
        ::std::ofstream outstream;

        bool openFile(::std::string filename);
        bool writeHeader(const int width,const int height);
        bool writeFile(const unsigned char* data, const unsigned int length);
        bool closeStream();
    };
}

I'm in trouble because I would like to access (read only) the bitmap data (headers, pixel data) from the image - that was saved to a BMP object - in the WRITER class. But because they are private members I can not access them in the writeheader() and writefile() functions. It would be nice to access them without making them public members, and I also try to avoid using inheritance in this case since a WRITER is not really a BMP. Lots of gettersare also not the best way for accessing arrays I think.

So the BMP could only load and save the image data. And with a WRITER I can write this data to a .bmp file. I want to make a pretty clean interface with public functions, and do not mix classes if not neccessary.

The only standard ways to access private data from another class are:

Getters
Friend class

If you want to make a friend class, just do something like this:

class Writer ;

class BMP
{
    //...
private:
friend class Writer ;
}

Now the Writer class can access all of BMP's data fields.

I would personally suggest using getters though. You could have getters for the header information and use operator [] for accessing any arrays.

unsigned char& BMP::operator [] (const size_t i) {
    return m_data [i] ;
}

const unsigned char& BMP::operator [] (const size_t i) const {
    return m_data [i] ;
}


What is the idiomatic Rust way to write this naive JavaScript token scanner code fragment? I am learning Rust and finding it quite a difficult but rewarding and fun challenge to learn how to write idiomatic and succinct, clean code in it.

I have written the following simple naive JavaScript scanner code as an exercise to learn some basics of Rust.  While it works, it feels like it is far from ideal Rust code.

Are there better ways to write these simple functions?  A few things that stand out to me are:

The loops and pattern matching feel more verbose than they could be
The use of the Chars iterator feels almost right but somehow off

In particular, the scan_identifier function is very clumsy since the iterator can't rewind already consumed characters in the string

Note I am a compiler dev and know that this isn't great scanner code.  That is not the point of this exercise.  I am looking to learn how to do Rust basics through practice.

fn scan_identifier(first: char, chars: &mut std::str::Chars) {
    let mut identifier = String::new();
    identifier.push(first);

    let mut id_chars = chars.clone();
    while let Some(c) = id_chars.next() {
        match c {
            'a'...'z'|'A'...'Z'|'0'...'9' =>
                identifier.push(c),
            _ => break,
        }
    }
    println!("{}", identifier);
    chars.nth(identifier.chars().count() - 2);
}

fn scan_string(chars: &mut std::str::Chars) {
    let mut string = String::new();
    while let Some(c) = chars.next() {
        match c {
            '\'' => { println!("'{}'", string); return; },
            _ => string.push(c),
        }
    }
    println!("unterminated string! '{}", string);
}

fn scan(src: &str) {
    let mut chars = src.chars();
    while let Some(c) = chars.next() {
        match c {
            '(' => println!("("),
            ')' => println!(")"),
            ';' => println!(";"),
            '\'' => scan_string(&mut chars),
            _ => scan_identifier(c, &mut chars),
        }
    }
}

fn main() {
    let input = "print('Hello, world!');";
    println!("Input:\n\n{}\n\nTokens:\n", input);
    scan(input);
}

You should become very familiar with all the methods on Iterator; in this case take_while would be highly relevant.
Other parts of the ecosystem that deal with iterators are invaluable, like String::extend.
Extract the logic about what is an identifier character, then you can write that code much simpler: identifier.extend(chars.clone().take_while(is_id_char)).
In most cases, you can use a for loop instead of while let. scan_string is a good example of this. scan is not, because you wish to pass the iterator into further methods inside the loop body, which is not possible with the for syntax.
In addition to the standard library Iterator methods, you should internalize what Itertools provides.
For example, take_while_ref avoids the need to clone the input iterator and then drive the original with nth.
You could chose to accept a generic iterator instead of Chars specifically.

extern crate itertools;

use itertools::Itertools;

fn is_id_char(c: &char) -> bool {
    match *c {
        'a'...'z' | 'A'...'Z' | '0'...'9' => true,
        _ => false,
    }
}

fn scan_identifier<I>(first: char, chars: &mut I)
    where I: Iterator<Item = char> + Clone
{
    let mut identifier = String::new();
    identifier.push(first);
    identifier.extend(chars.take_while_ref(is_id_char));

    println!("{}", identifier);
}

fn scan_string<I>(chars: &mut I)
    where I: Iterator<Item = char> + Clone
{
    let string: String = chars.take_while_ref(|&c| c != '\'').collect();

    if Some('\'') != chars.next() {
        println!("unterminated string! '{}", string);
    } else {
        println!("'{}'", string);
    }
}

fn scan(src: &str) {
    let mut chars = src.chars();
    while let Some(c) = chars.next() {
        match c {
            '(' => println!("("),
            ')' => println!(")"),
            ';' => println!(";"),
            '\'' => scan_string(&mut chars),
            _ => scan_identifier(c, &mut chars),
        }
    }
}

fn main() {
    let input = "print('Hello, world!');";
    println!("Input:\n\n{}\n\nTokens:\n", input);
    scan(input);
}

I wouldn't write any parsing / tokenizing / scanning that operates on characters, however. Especially since you are converting from a &str to a String, when you could just be returning string slices and avoiding extra allocation.


Hex string to Base64 The base64 decode algorithm is taken directly from Base64 wiki page. This exercise is meant to help me learn Rust, so any pointers in that general direction are very welcome. :)

fn main() {
    println!("{}",
         hex_to_base64("49276d206b696c6c696e6720796f757220627261696e206c696b65206120706f69736f6e6f7573206d757368726f6f6d"));
}

fn hex_to_base64(hex_string: &str) -> String {
    println!("Converting {} from hex to base64", hex_string);
    let alphabet: Vec<char> =
        "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/=".chars().collect();

    // Decode the input string from hex into individual bytes
    let input_chars: Vec<char> = hex_string.chars().collect();
    let mut decoded_bytes: Vec<u8> = Vec::new();

    for i in 0..hex_string.len() {
        if i % 2 != 0 { continue }
        let first_byte = input_chars[i].to_digit(16).unwrap() as u8;
        let second_byte = input_chars[i + 1].to_digit(16).unwrap() as u8;
        println!("Decoding 0x{}{} to bytes:", input_chars[i], input_chars[i + 1]);
        let byte = (first_byte << 4) | second_byte;
        println!("    {}", byte);
        decoded_bytes.push(byte)
    }

    // Encode the decoded bytes into Base64
    let mut output = String::from("");

    let mut b: u8;
    for i in 0..decoded_bytes.len() {
        if i % 3 != 0 { continue }
        b = (decoded_bytes[i] & 0xFC) >> 2;
        output.push(alphabet[b as usize]);
        b = (decoded_bytes[i] & 0x03) << 4;
        if i + 1 < decoded_bytes.len() {
            b |= (decoded_bytes[i + 1] & 0xF0) >> 4;
            output.push(alphabet[b as usize]);
            b = (decoded_bytes[i + 1] & 0x0F) << 2;
            if i + 2 < decoded_bytes.len() {
                b |= (decoded_bytes[i + 2] & 0xC0) >> 6;
                output.push(alphabet[b as usize]);
                b = decoded_bytes[i + 2] & 0x3F;
                output.push(alphabet[b as usize]);
            } else {
                output.push(alphabet[b as usize]);
                output.push('=');
            }
        } else {
            output.push(alphabet[b as usize]);
            output.push_str("==");
        }
    }
    return output
}

#[test]
fn test_hex_to_base64() {
    assert!(hex_to_base64("49276d206b696c6c696e6720796f757220627261696e206c696b65206120706f69736f6e6f7573206d757368726f6f6d") == String::from("SSdtIGtpbGxpbmcgeW91ciBicmFpbiBsaWtlIGEgcG9pc29ub3VzIG11c2hyb29t"));
}

I am glad to see your method takes a &str instead of a &String or String!
Shouldn't be printing output inside function.
The one function is doing too much. Instead, create two functions, one to convert hex -> bytes, andone to convert bytes -> base64. Note that your comments already declare where functions should be! You could promote those to doc comments, or the function name may be enough.
Don't specify exact types when declaring variables, let type inference handle it. decoded_bytes: Vec<u8> = Vec::new(); should be decoded_bytes = Vec::new();. Note that changed code has very few foo: type lines, mostly for collect where the return type is up to the caller.
Can collect into a Vec<_>, no need to specify item type.
Use chunks instead of reimplementing it. Note that it returns a smaller chunk when not a multiple of the chunk size. hex_to_bytes will crash with odd count of values, I think the previous did as well.
Once operating on an iterator, switch to map and add collect. Avoids need for mutation.
Prefer expect over unwrap. When it crashes you will be thankful.
String::new instead of String::from("").
Traditionally, the Base64 conversion table is a hard-coded constant, not re-created for each call.
The length of the bytes never changes, get it once before the loop and reuse it.
Declare variables as near to usage as they can be. b shouldn't be outside the loop.
Consider rebinding b when it's reset to a brand new value.
Consider getting the result of indexing and storing it into a variable, since it shouldnt change
Instead of checking the length and then indexing, use if let combined with get.
Use chunks again.
No explicit return at end of method.
Use assert_eq!.
Can compare string literal against String, no need to allocate

  Is there a better way to write debug statements?

You may be interested in the log crate, although I'd argue that these printing statements only had any value when the function was being originally developed, and probably don't deserve to stay in as logging either.

  I'm not sure what this one means?

Instead of

let mut b = 1;
b += 1;
b = 5;

You may want to try

let mut b = 1;
b += 1;
let b = 5;

This may help indicate that you deliberately reset the value.

fn main() {
    let b64 = hex_to_base64("49276d206b696c6c696e6720796f757220627261696e206c696b65206120706f69736f6e6f7573206d757368726f6f6d");
    println!("{}", b64);
}

/// Decode the input string from hex into individual bytes
fn hex_to_bytes(hex_string: &str) -> Vec<u8> {
    let input_chars: Vec<_> = hex_string.chars().collect();

    input_chars.chunks(2).map(|chunk| {
        let first_byte = chunk[0].to_digit(16).unwrap();
        let second_byte = chunk[1].to_digit(16).unwrap();
        ((first_byte << 4) | second_byte) as u8
    }).collect()
}

/// Encode the decoded bytes into Base64
fn bytes_to_base64(decoded_bytes: &[u8]) -> String {
    let mut output = String::new();
    let alphabet: Vec<_> =
        "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/=".chars().collect();

    for chunk in decoded_bytes.chunks(3) {
        let c0 = chunk[0];
        let b = (c0 & 0xFC) >> 2;
        output.push(alphabet[b as usize]);

        let mut b = (c0 & 0x03) << 4;

        if let Some(c1) = chunk.get(1) {
            b |= (c1 & 0xF0) >> 4;
            output.push(alphabet[b as usize]);

            let mut b = (c1 & 0x0F) << 2;

            if let Some(c2) = chunk.get(2) {
                b |= (c2 & 0xC0) >> 6;
                output.push(alphabet[b as usize]);

                let b = c2 & 0x3F;
                output.push(alphabet[b as usize]);
            } else {
                output.push(alphabet[b as usize]);
                output.push('=');
            }
        } else {
            output.push(alphabet[b as usize]);
            output.push_str("==");
        }
    }

    output
}

fn hex_to_base64(hex_string: &str) -> String {
    bytes_to_base64(&hex_to_bytes(hex_string))
}

#[test]
fn test_hex_to_base64() {
    assert_eq!(
        hex_to_base64("49276d206b696c6c696e6720796f757220627261696e206c696b65206120706f69736f6e6f7573206d757368726f6f6d"),
        "SSdtIGtpbGxpbmcgeW91ciBicmFpbiBsaWtlIGEgcG9pc29ub3VzIG11c2hyb29t"
    );
}


Jquery Testimonial Slider Controls I built a testimonial slider that automatically plays on load and stops auto playing when the slider controls are used to change the active slide.

I feel like it could be optimized easily, especially the adding and removing active class seems like it could be made better but I'm not sure how.

$(document).ready(function(){
        // Check var to see if slider controls has been clicked
        var clickedSlider = false;
        // Automatically change the active Testimonial Slide
        function autoSlide(){
            // Get active Slide en corresponding Control item
            var activeSlide = $('.testimonial-slider-slides a.active');
            var activeControl = $('.testimonial-slider-controls a.active');
            // If last testimonial is active  start back from the beginning
            if(activeSlide.is(':last-child')){
                activeSlide.removeClass('active');
                activeControl.removeClass('active');
                $('.testimonial-slider-slides a').first().addClass('active');
                $('.testimonial-slider-controls a').first().addClass('active');
            } else {
                // Else go to next slide
                activeSlide.removeClass('active');
                activeControl.removeClass('active');
                activeSlide.next().addClass('active');
                activeControl.next().addClass('active');
            }
        }
        // If controls haven't been clicked, autoplay the slider
        window.setInterval(function(){
            if (clickedSlider === false) {
                autoSlide();
            }
        }, 2000);
        // Show corresponding slide when clicking on slider control
        $('.testimonial-slider-controls a').click(function(e){
            e.preventDefault();
            clickedSlider = true;
            $('.testimonial-slider-controls a.active').removeClass('active');
            $(this).addClass('active');
            var slide = ($(this).data('for'));
            $('.testimonial-slider-slides a').removeClass('active');
            $('[data-id="' + slide + '"]').addClass('active');
        });
    });

Any thoughts on this?

Update:
This is the HTML I use:

    Slide 1
    Slide 2
    Slide 3

This was updated after i first posted here but the only real change in the javascript / jquery is the slides are now selected by .testimonial-slider-slides-slide instead of .testimonial-slider-slides a

Thanks to Mike Brant's Answer the data-for and data-id attributes can be left out

Some thoughts:

Don't continually re-query the DOM. Cache your control and slide elements in a variable so you don't need to spend time re-querying for them.
You don't need to maintain state around clickedSlider condition.  Consider using clearInterval() instead as more "proper" way to manage interval functionality.
Consider breaking apart your autoSlide() function into it's key parts - one function for managing the current slide index and another for actually performing the action of making slide at given index "active".  This gives you more code re-use in giving the capability to jump directly to a slide at a given index as is needed in your control click handler.
Do you really need data properties for your controls and slides?  If there is a 1-to-1 relationship between the slides and the controls and their ordering in DOM is the same, you should be able to know the control at index X relates to the slide at index X.

This might give you code more like:

$(document).ready(function(){
    var $testimonialSlides = $('.testimonial-slider-slides a');
    var $testimonialControls = $('.testimonial-slider-controls a');
    var slideCount = $testimonialSlides.length;
    var slideIndex = 0;

    function activateSlide(idx) {
        $testimonialSlides
            .removeClass('active')
            .eq(idx).addClass('active');
        $testimonialControls
            .removeClass('active')
            .eq(idx).addClass('active');
    }

    function autoSlide(){
        slideIndex++;
        if (slideIndex === slideCount) {
            slideIndex = 0;
        }
        activateSlide(slideIndex);
    }

    var autoSlideInterval = window.setInterval(autoSlide, 2000);

    $testimonialControls.click(function(e){
        e.preventDefault();
        window.clearInterval(autoSlideInterval);
        slideIndex = $testimonialControls.index(this);
        activateSlide(slideIndex);
    });
});

You may also consider building this as a proper jQuery plug-in or class (though not shown in my example above).  This would allow you to better encapsulate state of the slider functionality, such that state would not need to be held in main $(document).ready() scope. This also potentially makes this simple functionality re-usable in the future - just pass in config for DOM selectors, class name(s) to apply (i.e. active), interval time, etc.  I would rather see a usage pattern like:

$(document).ready(function() {
    // Perhaps not all of the configurations shown below need to be passed
    // if there are default settings.
    // This assumes autoslider() code is already included in document
    $('.testimonial-slider-slides a').autoslider(
        {
            controls: '.testimonial-slider-controls a',
            activeSlideClass: 'active',
            interval: 2000,
            startIndex: 0
        }
    );

    // other application code that needs to execute on ready()
    // perhaps even another autoslider with it's own encapsulated state
});

Why have superfluous comments?  If your code is well-written with meaningful function and variable names (I think you do a good job of this), then you should not need such comments.


Counting the UpperCase, LowerCase, Numbers and Special Characters in a String I'm making a Java program that will analyze the given String by counting the lowercase, uppercase, numbers and special characters. Is there a way by making it functional?

import java.io.BufferedReader;
import java.io.InputStreamReader;
import java.io.IOException;

public class PassTest {

    public static String password = "";
    public static int upperCase = 0;
    public static int lowerCase = 0;
    public static int numberCount = 0;
    public static final String FINAL_CHAR_REGEX = "[!@#$%^&*()[\\\\]|;',./{}\\\\\\\\:\\\"<>?]";

public static void main(String[] args) throws IOException {
    BufferedReader dataIn = new BufferedReader(new InputStreamReader(System.in));
        System.out.print("Enter your password: ");
             password = dataIn.readLine();
            for ( int i = 0; i < password.length() ; i++ ) {
                if (Character.isUpperCase(password.charAt(i))){ upperCase++; }
                if (Character.isLowerCase(password.charAt(i))){ lowerCase++; }
                if (Character.isDigit(password.charAt(i)))    { numberCount++;}
            }
        int specialCharCount = password.split(FINAL_CHAR_REGEX, -1).length - 1;
            System.out.printf("Your password contains %d uppercases, %d lowercases, %d digits and %d special characters.\n\n", upperCase, lowerCase, numberCount, specialCharCount);
        ;
       }
}

public static non-final variables are destroying your Zen, dude!

(No offense meant, it's a serious message in a funny wrapping meant to be easy to remember.)

Data hiding

Java is a language which provides so many big and small things which help us with encapsulation and data hiding. We want to encapsulate and hide data because that minimizes the risks of wrong coupling and bogus dependencies. There are 4 big design smells in the world, and almost all problems which software development projects face can be tracked down to these 4 design smells: Fragility, Rigidity, Immobility and Viscosity. More than 3.0 of these 4 design smells are primarily caused by coupling and dependency.

public static non-final variables are globally accessible mutable variables, or short global variables, and we don't want global variables.

Consider making your variables local.

Data modeling / reentrance

Think of the reusability of your function. Imagine, some big Internet company like Google, Amazon, Alibaba, Flipkart or whatsoever likes your password check algorithm and wants to reuse it.

static fields exist exactly once. (Strictly speaking they exist once per Class instance, and different ClassLoader could create separate independent such instances, but if you're not familiar with that see this as esoteric stuff for now.)
non-static fields exist once per object.
local variables exist once per function call.

This means that from a runtime reusability perspective, static fields are dangerous. What if two users would check their passwords at the same time? Their data would get messed up.

If you create non-static fields, each password check would need its own PassTest object.

If you have local variables, each password check automatically has its own copy of the relevant data.

So, I hope I've given you two good reasons for making your variables local.

Name: Test vs. Validator vs. Analyzer

You call your class PassTest. Actually, the word Test already is reserved with some meaning that's different from how you use the word Test.

A Test is an automated (ya James Bach beat me to it, as if I care :P) reproducible verification of a program's behavior for specified test cases. A test has a result, and that's either "PASS" or "FAIL".

If you want to tell the user whether the password is good or not, you might actually call it Validator instead, or if it's purely about giving some info about the password, Analyzer might be a good name.

So, I'd rename the class to PasswordAnalyzer.

Security: Sniffing / Spying the password

You might want to go for Console.readPassword() instead of BufferedReader(...System.in...).readLine(). The drawback is that Console.readPassword() bypasses stdin and directly reads from the terminal. The upside is that Console.readPassword() disables the terminal echo for the password characters and returns a char[] which is more secure.

Why not show the password on the screen?

Someone might look over your shoulder and see the password.

Why is char[] more secure than String?

Because after use of the char[], you can clear the char[] and fill it with zero - and you should! You cannot do that with a String, because Strings are immutable.
The time between end of password use and garbage collection might be infinitely long, and that's where someone might spy the password in memory.

Chinese Characters

You might want to know that Chinese characters like '噸' are neither uppercase nor lowercase. A proper Java program would allow Chinese to use it as well and produce meaningful results for them, too.

Closing Resources

You might want to dataIn.close() because that's good habit.
In other programs, forgetting to close() or even to flush() actually can lead to bugs which can be very tough to find.
I made it a habit to always finish what I start, which for streams and stuff means I always close what I open as soon as it's unused, even if it seems unnecessary.

The following code snippet shows how to dataIn.close() using try-with-resources in Java7 onwards.

try (final BufferedReader dataIn = new BufferedReader(new InputStreamReader(System.in)) {
    // ...
}

The closing brace implicitly calls dataIn.close(), that's how try-with-resources is defined and works. Of course this is only required if, for some reason, you decide to stick to System.in instead of using Console.readPassword().

if-if vs. if-else if

In case things are mutually exclusive, if-else if should be preferred over if - if, for two reasons.

Performance - the else if checks are skipped if the if matched.
Expressing the intent (more important than performance unless requirements + performance analysis report says otherwise and a profile proves that this part of the code is the bottleneck)

The question now is, are isUpperCase(), isLowerCase() and isDigit() mutually exclusive? AFAIK they are, so go for else if instead of if.

Special characters

Are you sure that you have listed them completely? In the year 2015, do you want to limit this to the small bunch of ASCII non-digit non-control non-letters, when the Unicode offers so much more? Why not say: "everything else is a special character"?

Sanitize your input (NullPointerException on dataIn.readLine().length())

What if the user doesn't enter a password but closes the stream (using Ctrl+D on UNIX resp. Ctrl+Z on cmd.exe)?
In that case, dataIn.readLine() returns null, and password.X will throw new NullPointerException no matter what X is.

Therefore you might want to explicitly check that password != null.

Looping over characters of a String

The relevant part of your loop is:

for (int i = 0; i < password.length(); i++) {
    ... password.charAt(i) ...
    ... password.charAt(i) ... // again
    ... password.charAt(i) ... // and again and again...
}

The return value of password.charAt(i) would always be the same, so you can speed up the code and express the intent better at the same time by introducing a local variable and change the relevant part of the code to be like this:

for (int i = 0; i < password.length(); i++) {
    final char currentChar = password.charAt(i);
    ... currentChar ...
    ... currentChar ...
    ... currentChar ...
}

You can even go one step further because you're accessing each and every char of the entire String password, not just a few. Therefore, creating a char[] copy of the String comes with no penalty, in fact it makes the code faster - and again more maintainable:

for (final char currentChar : password.toCharArray()) {
    ... currentChar ...
    ... currentChar ...
    ... currentChar ...
}

However, in order to prevent password sniffing, you might want to clear each character as soon as it is analyzed, and thus go for:

final char[] password = Console.readPassword();
for (int i = 0; i < password.length; i++) {
    final char currentChar = password[i];
    password[i] = 0;
    ... currentChar ...
    ... currentChar ...
    ... currentChar ...
}

Platform-independent newline

In printf(), you're using \n to create a newline. You might want to use %n instead. \n is always LF (0x0A). But depending on the platform, the program should actually print \n (LF, 0x0A, UNIX), \r\n (CRLF, 0x0D 0x0A, DOS / Windows), \r (CR, 0x0D, old Mac) or even something else (weird IBM mainframes with EBCDIC and stuff).
With %n, printf will generate a newline for you with whatever the underlying platform would like to have.

Characters, char vs int, code points

When Java was invented, Unicode looked like 16 Bit.
Today, the Unicode is bigger than that.
A char in Java does not represent a Unicode character in general. It only represents a Unicode character if that character has a code point <65536. Thus analyzing single char values can be insufficient for rare alphabets.

@AJMansfield provided a solution which is functional and uses code points instead of char. Have a look at it. It also takes security into account.