Partial view for frequently used drop down lists I have one input that is needed in many forms, so I created a partial view for that. In the partial view I query the database and generate a drop down list.

I just want to make sure, this is the best way to achieve this.

The partial view looks like this:

@model long?

@{
    var data = Utility.GetData();
}

@Html.DropDownListFor(m => m, new SelectList((System.Collections.IEnumerable)data, "Id", "Text"),
    new { @class = "form-control input-sm" })

And I'll use it like this:

@Html.EditorFor(x => x.LiIdPeriod, "Years")

I should note that this is a SPA and intranet application.

I'm new to MVC (relatively), but I don't think I'll ever plan on doing too much in a view. In my understanding, it's merely a template for an object (view model) you throw at it. There shouldn't be much server-side functionality in there at all. Getting data from a database and selecting a SelectList from this collection is too much. There isn't enough separation between concerns of presentation and functionality.

Get that database call into a controller (or better still, a business layer) and keep your views simple. Then you can decide whether you want to call this new action asynchronously or not.


Counting the out-of-order elements of an array Is there any way to write is C function better so the procedure could spend less time to calculate the results?

Assume that the array size is 1.000.000 and  all the numbers are greater than 0.

The function read the array backwards, saves the first number to maximum, then check the next number, a, and if it's greater than the maximum, it adds +1 to total. Then the maxinum takes the value of a and procide to next one until the end of array.

static int total = 1;

int chck_high( int *my_array, int *endp) {
    // this function should only be called if there is at least one value in the array
    int maximum = *(--endp);

    while ( endp > my_array ) {
        int a = *(--endp);

        if ( a > maximum ) {
            total++;
            maximum = a;
        }
    }

    return maximum;
}

I tried to use unsigned ints, but I don't know if it's worth it. Can someone please tell me if there is a better way to write that code?

Previous version of the code:

int process( int *my_array, int *endp) {
    int a, b;
    //static int total = 1;
    if ( my_array == 0 ) return 0;
    if ( my_array == endp ) return INT_MIN;
    else a = *my_array++;
    if ( (b= process( my_array, endp )) == INT_MIN ) return a;
    if ( a > b )
    {total++; return printf( "%d > %d and total now is %d\n", a, b, total ), a; }
    return b;
}

The two functions called with the following code:

chck_high(my_array, my_array + count);

where my_array is my array with nums and count is the number that says the size of the array.

Avoid static variables as much as possible.
Instead of static total, you could pass in a pointer to total and make the function modify the value it's pointing to.

The names are not great:

Instead of my_array, start would be better
Instead of endp, end would be better
Instead of chck_high, ... I don't know what would be better, because I don't really see the general logic this function represents. It counts the number of times a new local maximum is found going backwards from the end of the range, and sets the value of total. I'm wondering if this logic is really necessary in this form, or perhaps the overall logic of your program could be redesigned to simpler elements.

Instead of a comment like this:

// this function should only be called if there is at least one value in the array

It would be better to use an assertion:

assert(start < end);

Note that this requires to #include <assert.h>

This maybe a matter of taste, but I would find a for loop would be more natural for this instead of while. Using a for loop, in C99 and above, you could declare the loop variable inside the for, which would have extra benefits:

It would help you limit variables in the smallest scope necessary (inside the loop)
It would force you to use a new local variable for looping, instead of reusing the function parameter, which is a good thing

Putting it together, the function would become:

int chck_high(int *start, int *end, int *total) {
    assert(start < end);

    int maximum = *--end;

    for (int * pos = end; pos > start; --pos) {
        int a = *pos;
        if ( a > maximum ) {
            ++*total;
            maximum = a;
        }
    }

    return maximum;
}

To enable C99 mode when compiling with gcc, use the -std=c99 flag.

You can use the function like this, for example:

int main() {
    int arr[] = {1, 2, 51, 41, 4, 5};
    int total = 1;
    int maximum = chck_high(&arr[0], &arr[0] + 6, &total);
    printf("total=%d max=%d\n", total, maximum);
}


Prime Factors (32 bits, without Sieve) I wrote this prime factors program as a programming exercise.

Any comments or constructive criticisms are welcome.

One question: prime[] is an array which stores the number's prime factors and associated exponents. How many elements, at most, does this array require? That is, what is the greatest number of prime factors needed to express any number between 1 and 4294967295? As an example 750 would need 3 elements since 750 = 2 * 3 * 5 ^ 3. I could do an exhaustive test using a static variable to keep track of the maximum value of pIndex but the 2.4 billion numbers would take at least 12 hours on my single core machine. Is there a quicker way to determine the answer?

/* primeexponents.c

   display a number's prime factors using exponents and also its number of
   divisors

   note: unsigned 32 bit numbers (1 - 4294967295)
*/

#include <stdio.h>
#include <stdbool.h>

void primeExponents (unsigned long int number, unsigned int list[])
{
    printf ("\nnumber = %lu\n", number);

    struct
    {
        unsigned long int factor;
        int exponent;
    } prime[20] = { { 0, 0 } };

    unsigned long int num = number;
    int pIndex = 0, lIndex = 0;

    prime[pIndex].factor = (unsigned long int) list[lIndex];

    while ( prime[pIndex].factor <= num / prime[pIndex].factor ) {
        if ( num % prime[pIndex].factor == 0 ) {
            ++prime[pIndex].exponent;
            num /= prime[pIndex].factor;
        }
        else {
            if ( prime[pIndex].exponent > 0 )
                ++pIndex;
            ++lIndex;
            prime[pIndex].factor = (unsigned long int) list[lIndex];
        }
    }

    // num = greatest prime factor

    if ( num == prime[pIndex].factor )
        ++prime[pIndex].exponent;
    else {
        if ( prime[pIndex].exponent > 0 )
            ++pIndex;
        prime[pIndex].factor = num;
        prime[pIndex].exponent = 1;
    }

    printf ("prime factors =");

    unsigned long int product = 1;
    int divisors = 1;

    for ( int i = 0; i <= pIndex; ++i ) {
        printf (" %lu", prime[i].factor);
        if ( prime[i].exponent > 1 )
            printf (" ^ %i", prime[i].exponent);
        if ( i < pIndex )
            printf (" *");
        // calculate product of prime factors to verify result
        for ( int j = 1; j <= prime[i].exponent; ++j )
            product *= prime[i].factor;
        // calculate number of divisors
        divisors *= prime[i].exponent + 1;
    }
    printf ("\n");

    printf ("product of prime factors = %lu", product);
    if ( product != number )
        printf (" <<< error >>>");
    printf ("\n");

    printf ("divisors = %i\n\n", divisors);
}

void generateList (unsigned int list[])
{
    // generate a list of prime numbers to use as trial factors, i.e.
    // list[0] = 2, [1] = 3, [2] = 5, [3] = 7, [4] = 11, ... [6541] = 65521

    bool isPrime;
    list[0] = 2;
    list[1] = 3;
    int index = 2;

    for ( unsigned int i = 5; i <= 65535; i += 2 ) {
        isPrime = true;

        for ( int j = 1; (i / list[j] >= list[j]) && isPrime; ++j ) {
            if ( i % list[j] == 0 )
                isPrime = false;
        }

        if ( isPrime ) {
            list[index] = i;
            ++index;
        }
    }
}

int main (void)
{
    printf ("\nPrime Factors with Exponents Program\n\n");

    unsigned int list[6542];

    generateList (list);

    unsigned long int number;

    do {
        printf ("enter a number (from 1 to 4294967295, 0 to exit): ");
        scanf ("%lu", &number);

        if ( number != 0 )
            primeExponents (number, list);
    }
    while ( number != 0 );

    return 0;
}

You don't need an exhaustive search: \$2*3*5*7*11*13*17*19*23*29 = 6469693230 > 2^{32}\$, so it is safe to allocate 9 slots.

primeExponents does too much and shall be split into three functions, one to factorize a number, another to validate factorization, and yet another to print it.

The factorization logic is extremely hard to follow. Consider instead

void factorize(int num, factors_t * factors, int * primes)
{
  for (; num > 1; ++primes) {
    factors->prime = *primes;
    factors.exponent = 0;

    while (num % *primes == 0) {
      factors->exponent++;
      num /= **primes; // Edit: shame on me.
    }

    if (factors->exponent > 0) {
      factors++;
    }
  }
}


Format A TimeSpan With Years I have a class with 2 date properties: FirstDay and LastDay.  LastDay is nullable.  I would like to generate a string in the format of "x year(s) y day(s)".  If the total years are less than 1, I would like to omit the year section.  If the total days are less than 1, I would like to omit the day section.  If either years or days are 0, they should say "day/year", rather than "days/years" respectively.

Examples:
2.2 years:             "2 years 73 days"
1.002738 years:   "1 year 1 day"
0.2 years:             "73 days"
2 years:                "2 years"

What I have works, but it is long:

private const decimal DaysInAYear = 365.242M;

public string LengthInYearsAndDays
{
    get
    {
        var lastDay = this.LastDay ?? DateTime.Today;
        var lengthValue = lastDay - this.FirstDay;

        var builder = new StringBuilder();

        var totalDays = (decimal)lengthValue.TotalDays;
        var totalYears = totalDays / DaysInAYear;
        var years = (int)Math.Floor(totalYears);

        totalDays -= (years * DaysInAYear);
        var days = (int)Math.Floor(totalDays);

        Func<int, string> sIfPlural = value =>
            value > 1 ? "s" : string.Empty;

        if (years > 0)
        {
            builder.AppendFormat(
                CultureInfo.InvariantCulture,
                "{0} year{1}",
                years,
                sIfPlural(years));

            if (days > 0)
            {
                builder.Append(" ");
            }
        }

        if (days > 0)
        {
            builder.AppendFormat(
                CultureInfo.InvariantCulture,
                "{0} day{1}",
                days,
                sIfPlural(days));
        }

        var length = builder.ToString();
        return length;
    }
}

Is there a more concise way of doing this (but still readable)?

Overall, your code doesn't look bad.  You have good use of white space and indentation.  I found your variable names to be a little confusing (what's the difference between totalDays and days?  Without actually digging into the code, it's not obvious).

I like that you are using StringBuilder for concatenating strings, that is a good habit to get into.

I think you are doing too much manipulation on the years and days.  I was able to take your 5 lines of code to calculate the years and days, and make it 3

var totalDays = Math.Floor(lengthValue.TotalDays);
var years = totalDays / DaysInAYear;
var days = totalDays % DaysInAYear;

Years will be totalDays divided by the number of days in a year.  Days will be the remainder of the same division.

I would move the pluralize out into its own method.  There is no need to use an anonymous method in this instance.

You are repeating yourself when you are creating the string.  If you look closely, the code is almost identical.  The differences are easily passed in as variables.  The way I corrected this is to create a method called CreateWords.  This method returns the formatted string from variables passed in:

private string CreateWords(int value, string measure)
{
    if (value == 0) return string.Empty;

    return string.Format("{0} {1}{2}",
                value,
                measure,
                PluralSuffix(value));
}

private string PluralSuffix(int value)
{
    return value > 1 ? "s" : string.Empty
}

Your main method would then call:

builder.Append(CreateWords(years, "year"));
builder.Append(CalculateSpaceCharacter(days));
builder.Append(CreateWords(days, "day"));

where CalculateSpaceCharacter would look like:

private static string CalculateSpaceCharacter(int value)
{
    return value > 0 ? " " : string.Empty;
}

And finally, there is no need to assign the length variable at the end.  Just return builder.ToString()


A minimalistic kind of read-copy-update class This is a very simple read-copy-update (RCU)-inspired synchronization class:

#include <atomic>

template<typename T>
class RCU
{
private:

    std::atomic<T*>  curr;
    std::atomic<T*>  prev;
    std::atomic<int> readers;

public:

    RCU()
    : curr   (new T),
      prev   (nullptr),
      readers(0)
    {

    }

    ~RCU()
    {
        delete curr;
        delete prev;
    }

    void lock()
    {
        readers++;
    }

    void unlock()
    {
        readers--;
    }

    T* get()
    {
        return curr.load();
    }

    T* clone()
    {
        return new T(*curr.load());
    }

    bool swap(T* t)
    {
        /* If prev is full, a swap is still pending. Clean up memory and
        abort the operation. */

        if (prev.load() != nullptr)
        {
            delete t;
            return false;
        }

        /* Store the current pointer into prev. Readers might still be reading
        through the pointer. */

        prev.store(curr);

        /* Curr contains the new proposed value. No one is reading from this
        right now. */

        curr.store(t);

        /* Spin until there are no more readers to *prev. When so, free the
        memory it points to and say goodbye. */

        while (readers.load() > 0);

        T* old = prev.load();
        prev.store(nullptr);
        delete old;

        return true;
    }
};

The idea is to have many reader threads that repeatedly read from some shared data, while a writer thread that rarely updates it. The writer thread takes care of cleaning up the memory when no readers are reading. Readers are "tracked" through the lock() and unlock() methods. The RCU-like class should synchronize the threads in a lock-free way.

Usage:

struct Data
{
    int x;
    int y;
    int z;
};

RCU<Data> rcu;

void writer_thread()  // the writer rarely runs
{
    while (true)
    {
        Data* data = rcu.clone();
        // modify data as needed
        data->x++;
        data->y++;
        data->z++;
        rcu.swap(data); // Checking the return value might be useful
    }
}

void reader_thread()
{
    while (true)
    {
        rcu.lock();
        std::cout << rcu.get()->x << "\n"; // read from data
        rcu.unlock();
    }
}

Questions: does it make sense? Is it really thread-safe? Can a thread slip through the cracks somewhere, somehow?

Serious issues with this:

Memory Management.
Not using RAII to lock/unlock

Memory Management
Don't pass pointers it does not indicate owners. Always wrap pointers in a smart pointer. Have a look at std::unique_ptr.
But for types like Data there is no need to use pointers. Simply use Data as the object type (not Data*).
RAII
Look up the concept of RAII.
A good example is std::lock_guard https://en.cppreference.com/w/cpp/thread/lock_guard This shows you how to use RAII to automate the processes of lock/unlock so it is exception safe.


Getting a connection to the database and executing statements I've got two classes for getting a connection to the database and executing things, Class 'A' was made by me and Class 'B' was made by a colleague, we were wondering which one is better for use (Security, efficiency etc), that's why we are asking it here. Which one is better, and why is it? And how can we improve that one?

Class A:

class databaseHandler {

private $db;
private static $instance;

/*

This is the constructor of the PHP class, we create the object for databaseHandler here.

*/
function __construct() {
    $this->createConnection();
}

/*

We return an instance, if the object isn't created yet, we make it a new object and then return it.

*/
public static function getInstance() {
    if(!self::$instance) {
        self::$instance = new databaseHandler();
    }
    return self::$instance;
}

    /* returns the connection to the database, so we can use it to create statments */
public function getConnection() {
    return $this->db;
}

/*
Create a connection to the database, using constants to set the correct data for the connection.
*/
private function createConnection() {
    require 'config.php';
    try {
    $this->db = new PDO("mysql:host=" . HOST . ";dbname=" . DATABASE . ";charset=utf8", USERNAME, PASSWORD);

    $this->db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_WARNING);
    } catch(PDOException $e) {
        echo $e->getMessage();
    }

    }

}
?>

Class B:

class DatabaseConnection
{
 public $dbc, $stmt, $result;

  public function __construct()
  {
    global $dbc;
    $dbc = new PDO("mysql:host=<removed host from post>;dbname=REDACTED", "REDACTED", "REDACTED");
  $dbc->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
  }

public function closeConn ()
{
  global $dbc;
  $dbc = NULL;
}

public function selectAll ($tableName)
{
  global $dbc, $stmt, $result;

  $stmt = $dbc->prepare("SELECT * FROM $tableName");
  $stmt->execute();
  $result = $stmt->fetchAll();
  $this->closeConn();
  return $result;
}

public function getConnection() {
  global $dbc;
  return $dbc;
}

public function updateStock ($artnr, $amountToDecline)
{
  global $dbc, $stmt, $result;

  $stmt = $dbc->prepare("SELECT `stock` FROM `artikelen` WHERE `artnr` = $artnr");
  $stmt->execute();
  $result = $stmt->fetchAll();
  $updatedStock = $result[0]['stock'] - $amountToDecline;
  if ($updatedStock < 0) {
  return false;
  $this->closeConn();
  } else {
    $stmt2 = $dbc->prepare("UPDATE `artikelen` SET  `stock` = '$updatedStock' WHERE  `artikelen`.`artnr` = $artnr");
    $stmt2->execute();
    return true;
    $this->closeConn();
  }
}
}

We are asking this, as we are planning to use this on our first project with an actual customer.

I think both classes have problems. Class A looks like an improperly implemented singleton and Class B has so many issues (detailed below) that it should probably just be thrown away as it is not really close to being suitable for inclusion in any code base.

In class A, why store both an instance of the class and an instance of PDO?  If you really wanted to stick with singleton I would simplify this down to something like:

class PdoSingleton
{
    public static $pdo;

    private function __construct() {}

    public static function getInstance() {
        if(self::$pdo instanceof PDO === false) {
            self::$pdo = new PDO(...);
        }
        return self::$pdo;
    }
}

I would however suggest you guys consider using dependency injection rather than (or at least in conjunction with) singleton.  Taking a dependency injection approach allows you to remove code in classes that need a dependency (such as a PDO object) and enforce strict contracts in your code.

So rather than this sort of code:

class ClassThatNeedsPDO
{
    protected $pdo;

    public function __construct() {
        try {
            $this->pdo = PdoSingleton::getInstance();
        } catch (PDOException $e) {
            // do something
        }
        // do other stuff
    }
}

You take this approach:

class ClassThatNeedsPDO
{
    protected $pdo;

    public function __construct(PDO $pdo) {
        $this->pdo = $pdo;
        // do other stuff
    }
}

With you either passing PDO object defined in you bootstrap sequence upon instantiation, or possibly using singleton provider like this:

$obj = new ClassThatNeedsPdo(PdoSingleton::getInstance());

This makes it to where the class that needs that dependency can never even be instantiated if the appropriate dependency, which is type-hinted in parameter declaration, is not passed (an InvalidArgumentException would be thrown).   Your code fails loudly, which is what you want.

In class A, why require your config? It should be considered very unusual to have a require inside a class like this.  The require should be somewhere in your bootstrap sequence prior to where this singleton is first used.  Then you just use the constants as you are currently doing since constants are global in scope.

In class A, while I appreciate that you are trying to work with PDO in exception mode (a good thing) and using try-catch here around the instantiation.  Think about what impact this has on your code.

Here if PDO instantiation fails with exception, you basically just swallow it up with only some output to standard out to indicate there was a problem. Subsequent code could call this class, expecting an instance without knowing that the class was instantiated in a bad state.  You should be failing loudly here and letting the caller to the class make the decision on how to handle the situation.

I don't think that there is really a case here to even use try-catch, as I would think it reasonable that a PDOException bubble up from a PDO singleton class.  Maybe the only reason to use try-catch here is in case you wanted to log some information about the internal state of the class before re-throwing or wrapping the exception.  Either way, an exception should be thrown externally when you can't instantiate in an expected state.

A class such as this should never output anything by the way. It should log errors and/or throw exceptions, but not output directly to the user.  Leave this to code up the call stack that is better positioned to provide end-user messaging.

In class B, I don't understand global declarations throughout.  Using global is generally a PHP coding anti-pattern that should be removed from the code.  It seems as if the intent here is to access the class properties, which should be done using $this->*.

Class B looks like it wants to grow into a database wrapper, which to me is a bit of an anti-pattern unless that wrapper adds some specific value like:

managing connections (like using singleton or factory patterns)
abstracting user from specific database implementation details (PDO itself is already such an abstraction)
providing abstract ORM/model behaviors to be implemented in object-specific inheriting classes (it seems like you might be trying to achieve this in this class though I don't know why specific class/table logic like you have in updateStock() would be in a generalized database class)
providing natural language query-building functionality

My suggestion would be to abandon such an approach in favor of writing individual classes for your database entities such as artikelen, or better yet  using a proper PHP ORM.

Never hard code database or other security credentials into your code.  These should be derived from configuration that lies outside your code repository (ideally derived from environmental configuration).

You should change your DB password NOW as you just posted all your credentials for the whole world to see.

You have unreachable lines of code in updateStock(). When return is called, any lines of code after that call in that code path will never be reached.

You should never have unreachable lines in your code.

In class B, you should be using parametrized prepared statements vs. concatenating string values into your query strings.  You are totally open to SQL injection as you are doing nothing to sanitize your parameters for use in the queries.

The public methods in class B that accept arguments have no guarding code to validate those arguments.  You should, at a minimum make sure you have reasonable values (non-zero length strings, integer values, etc.) before getting to the point of making the (relatively) expensive query against the database.

Ideally you would throw InvalidArgumentException if parameters don't meet basic validation criteria.  This lets calling code know it did something wrong.

There is no reason to ever perform two queries in updateStock().  The first select is just a wasteful query against the database and it seems your code just assumes the record will exist anyway. You can go directly to trying the update (after validating and sanitizing input of course), setting the new values based on the old value.

That query might look like the following (of course you should parametrize this):

UPDATE `artikelen`
SET  `stock` = IF(`stock` > $amountToDecline, `stock` - $amountToDecline, 0)
WHERE  `artikelen`.`artnr` = $artnr

You can look at the number of affected rows to determine if the record was found or not (should should get 0 if nothing was done and 1 if an update was performed), alleviating the need to perform an upfront select at all.

It seems clear that you two are not working using a common coding standard or styleguide.  If you are going to collaborate, you should agree on this so that you don't end up with poorly written code that varies from script to script and class to class.  I would recommend looking at the PSR standards, particularly PSR-1 and PSR-2 as a starting point.

http://www.php-fig.org/psr/

You should enforce whatever style you decide upon using a style checker before committing code to your repository.

I would also suggest that you both familiarize yourself with http://www.phptherightway.com/ as a reference you can use to help guide your development.


Do you Connect with my Groove? I'm new at groovy, and this is my first time writing a database connection script. It may seem like example code, but it is more like test code. It works as posted, and I'm planning to use this as a model for a larger business application, so I want to make sure I do it right.

Are there any groovy features that it could benefit from? Did I miss any edge cases, or anything else I could improve?

import groovy.sql.Sql

def dbUrl      = "jdbc:postgresql://localhost/GroovyTest"
def dbUser     = "Phrancis"
def dbPassword = "test"
def dbDriver   = "org.postgresql.Driver"

def sql = Sql.newInstance(dbUrl, dbUser, dbPassword, dbDriver)

println "Sql Instance: " + sql

sql.execute """SET SEARCH_PATH TO groovy_test;"""

sql.execute """
        START TRANSACTION;
        DROP TABLE IF EXISTS test;
        CREATE TABLE test (
            id SERIAL,
            string TEXT,
            number INTEGER,
            decimal NUMERIC,
            datetime TIMESTAMP
            );
        COMMIT;"""

def params1 = ['''');DROP TABLE test;--''', 42, 3.14159, 'NOW()']
def params2 = ['Hello, World!', 99999999, 0.1209823098234, '2015-06-25']

sql.execute """
        START TRANSACTION;
        INSERT INTO test (string, number, decimal, datetime)
            VALUES (?, ?, ?, CAST(? AS TIMESTAMP));
        COMMIT;""", params1

sql.execute """
        START TRANSACTION;
        INSERT INTO test (string, number, decimal, datetime)
            VALUES (?, ?, ?, CAST(? AS TIMESTAMP));
        COMMIT;""", params2

sql.eachRow("SELECT * FROM test;") { row ->
    println """
The row Id is: ${row.id}
The string is: ${row.string}
The number is: ${row.number}
The decimal is: ${row.decimal}
The date-time is: ${row.datetime}"""
}
sql.close()

Here is the stdout after this runs:

Sql Instance: groovy.sql.Sql@385c9627

The row Id is: 1
The string is: ');DROP TABLE test;--
The number is: 42
The decimal is: 3.14159
The date-time is: 2015-06-25 19:49:25.603991

The row Id is: 2
The string is: Hello, World!
The number is: 99999999
The decimal is: 0.1209823098234
The date-time is: 2015-06-25 00:00:00.0

Process finished with exit code 0

Defining database connection parameters

It is not recommended to hard-code your connection parameters within the script.

You could for example parse the database properties from an external Json source. There are other methods available in Groovy as well, such as reading properties from XML, or from a database.properties file, or from a *.groovy file, but I think Json might be the one you would feel most comfortable with.

def dbProperties = new JsonSlurper().parseFile(new File('db.json'))

Where db.json is something like this:

{
    "url": "jdbc:postgresql://localhost/GroovyTest",
    "user": "Phrancis",
    "password": "test",
    "driver": "org.postgresql.Driver"
}

And then instantiating the Sql object:

def sql = Sql.newInstance(dbProperties.url, dbProperties.user, dbProperties.password, dbProperties.driver)

Strings

""" are used for multi-line strings, for single-line strings it is overkill.

sql.execute """SET SEARCH_PATH TO groovy_test;"""

Can become:

sql.execute "SET SEARCH_PATH TO groovy_test;"

Additionally, " is used for GString, that will work just fine here but is not required, a normal String would be enough. So we can use ' insteand so that it becomes:

sql.execute 'SET SEARCH_PATH TO groovy_test;'

Here you have a multi-line GString, but as you're not using any GString-specific features (variable interpolation for example), you can replace the """ with '''

sql.execute """
        START TRANSACTION;

        DROP TABLE IF EXISTS test;
        (...)

Duplicated SQL query string

This code is essentially repeated twice:

sql.execute """
        START TRANSACTION;

        INSERT INTO test (string, number, decimal, datetime)

            VALUES (?, ?, ?, CAST(? AS TIMESTAMP));

        COMMIT;""", params1

The only difference is that the second time params1 is exchanged for params2.

There are three ways to fix this:

Extract the query string to a variable
Extract a method, which takes the params as input and performs the query
Extract a method closure. (Very similar to extracting a method)

First approach would look like this:

def query = """
        START TRANSACTION;

        INSERT INTO test (string, number, decimal, datetime)

            VALUES (?, ?, ?, CAST(? AS TIMESTAMP));

        COMMIT;"""

sql.execute query, params1
sql.execute query, params2

Second approach:

void sqlInsert(List<Object> params) {
    sql.execute """
        START TRANSACTION;

        INSERT INTO test (string, number, decimal, datetime)

            VALUES (?, ?, ?, CAST(? AS TIMESTAMP));

        COMMIT;""", params
}

sqlInsert query, params1
sqlInsert query, params2

Note that sqlInsert query, params1 is the same as sqlInsert(query, params1)

Third approach:

def sqlInsert = { List<Object> params ->
    sql.execute """
        START TRANSACTION;

        INSERT INTO test (string, number, decimal, datetime)

            VALUES (?, ?, ?, CAST(? AS TIMESTAMP));

        COMMIT;""", params
}

sqlInsert.call(params1)
sqlInsert.call(params2)

// or alternatively
sqlInsert(params1)
sqlInsert(params2)


Matching BigInts in Scala I'm currently in the process of learning scala, and I'm looking for some best practices/proper idioms for the use of pattern matching with BigInts.  I'm writing some algorithmic code (for Project Euler) as a way to learn the language.

The problem I am running into is what is the best idiomatic way to match a BigInt, since there's a type conversion problem if I try to write case 1 => and BigInt isn't a case class.  Is there a canonical way to do this, or should I just give up and use if statements?

The following is one possible pattern:

def pollardRho(n: BigInt): BigInt = {
  val one = BigInt(1)

  n match {
    case `one` => n
    //additional cases here
  }
}

This one just feels wrong: A hack to get around the fact that I can't match it directly, especially with the backticks in there to keep scala from trying to assign the resulting value to one or when I have to do it for more than a single value or in a single case.

Another possibility:

def pollardRho(n: BigInt): BigInt = {

  n match {
    case _ if n == 1 => n
    //additional cases here
  }
}

This sort of pattern appeals to the Erlang programmer in me, but I have some concerns:

Given the warnings I've read not to treat Scala like any other language, I am not sure that my instinct here is not an anti-pattern (or at least considered less readable) in the scala world.
It feels like there might be cleaner way to use a custom PartialFunction instead of n match if what I am fundamentally going to do is just a series of tests around n.  I am not sure, however, exactly what that would look like (most of the examples of PartialFunctions I've seen don't look quite like this application).

My question is, out of the array of possibilities in scala, which one is the best/most readable/most idiomatic?

Write your own extractor:

object Big {
  def unapply(n:BigInt) = Some(n.toInt)
}

//usage
def f(b:BigInt) = b match {
   case Big(0) => "none"
   case Big(1) => "one"
   case _ => "many"
}

Obviously, this isn't the fastest solution, but I think, it's quite idiomatic...


Asserting CPUID and enabling A20 At this point, the user will probably want to know if this system is Long Mode capable, which is indicated by the word CPUID in green. This is determined by attempting to toggle bit 21 in EFLAGS.

5BD  C6460004          mov      [bp], RED       Assume CPUID incapable
5C1  55                push     bp
5C2  89E5              mov      bp, sp
5C4  669C              pushfd                   Preserve original state
5C6  669C              pushfd
5C8  804EFA20          or       [bp-6], 20H
5CC  669D              popfd                    Set flag
5CE  669C              pushfd                   Re-read it
5D0  58                pop      ax
5D1  58                pop      ax              AX = High order bits
5D2  669D              popfd                    Restore original flags
5D4  5D                pop      bp
5D5  0FBAE005          bt       ax, 5           Is bit 21 on
5D9  7328              jnc      603             No if CY=0

Now we'll test if CPU has Extended Function support.

5DB  C6460006          mov      [bp], BROWN
5DF  66B801000080      mov      eax, 0x80000000 Ext Func supported?
5E5  6650              push     eax
5E7  0FA2              cpuid
5E9  6659              pop      ecx
5EB  6639C8            cmp      eax, ecx
5EE  7613              jna      603         AX >= 80000001H

5F0  C646000E          mov      [bp], YELLOW
5F4  B001              mov      al, 1       AX = Read feature bits
5F6  0FA2              cpuid
5F8  660FBAE21D        bt       edx, 29     Long Mode?
5FD  7304              jnc      603

5FF  C6460002          mov      [bp], GREEN

Display CPUID with selected color

603  31C9              xor      cx, cx
605  89CB              mov      bx, cx
607  8A5E00            mov      bl, [bp]        Selected color
60A  BA0706            mov      dx, 607H        Line 7 column 8
60D  E818FF            call     528

Finally, determine state of A20 line and for now only because it works, Fast A20 Gate has been implemented.

610  56                push     si
611  1E                push     ds
612  06                push     es
613  C6460002          mov      [bp], GREEN
617  B90080            mov      cx, 8000H      Retry count

This sets up DS:SI =    560H    Is volatile as not needed anymore
             ES:DI = 100560H

61A  8CCE              mov       si, cs
61C  C1E604            shl       si, 4
61F  81C66000          add       si, 60H       Offset from CS
623  89F7              mov       di, si
625  83C710            add       di, 16
628  31C0              xor       ax, ax
62A  8ED8              mov       ds, ax
62C  48                dec       ax
62D  8EC0              mov       es, ax
62F  57                push      di
630  56                push      si
631  AD                lodsw
632  F7D8              neg       ax
634  AB                stosw
635  3944FE            cmp       [si-2], ax   If equal A20 is off
638  5E                pop       si
639  5F                pop       di
63A  0F858E00          jnz       6cc          Bounce A20 already on

Do Fast A20

63E  C646000E          mov       [bp], YELLOW
642  E492              in        al, 92H
644  0C02              or        al, 2
646  E692              out       92H, al
648  49                dec       cx
649  75E4              jnz       62f        Execute test again

NOTE: There is 125 bytes reserved here for additional methods of
      enabling A20.

6C8  C6460004          mov       [bp], RED   A20 enabling failed

Display "A20" in one of the three colors

6CC  07                pop       es
6CD  1F                pop       ds
6CE  5E                pop       si
6CF  8A5E00            mov       bl, [bp]
6D2  31C9              xor       cx, cx
6D4  B21C              mov       dl, 0x1c
6D6  E84FFE            call      528
6D9  EB25              jmp      700

From here to end of sector is where I2H (6DB) lives and can be viewed at the bottom of this post

These observations are for the most in the order of the code

  At this point, the user will probably want to know if this system is Long Mode capable, which is indicated by the word CPUID in green. This is determined by attempting to toggle bit 21 in EFLAGS.

Seeing "This is determined by..." makes this paragraph somewhat mis-leading because it would seem that Long Mode capability is determined merely by looking at EFLAGS[21]. Your code correctly does it in a 3-step process, but that's not something that the reader sees right away. People with less time on their hands might have given up reading already!

push     bp
mov      bp, sp
...
or       [bp-6], 20H
...
pop      bp

You've used the BP register to set a bit in a stacked number. This can be optimized by writing or [esp+2], 20H. Although this instruction requires 2 extra bytes (ASP prefix and SIB byte), you gain 4 bytes from dropping the push, mov, and pop instructions.
There's a trade off here. If in a similar situation, you were to manipulate several stacked values in a row then using BP would be shorter.

bt       ax, 5           Is bit 21 on
jnc      603             No if CY=0

Bit 5 lives in the AL register. Testing to see if it is on or off can be shortened by not using the double opcode instruction BT. Best choice here is the Modr/m byte less instruction test al, imm8.

test    al, 20h
jz      603          ;Bit 21 is off

5DF  66B801000080      mov      eax, 0x80000000 Ext Func supported?

Noticed the discrepancy between the dump and the instruction's immediate?
If you copy/pasted this code then there's something amiss with the disassembler, else it's a typo.

push     eax
cpuid
pop      ecx
cmp      eax, ecx
jna      603         AX >= 80000001H
mov      [bp], YELLOW
mov      al, 1       AX = Read feature bits
cpuid
bt       edx, 29     Long Mode?
jnc      603

With the instruction mov al, 1 you're avoiding to have to write the much longer mov eax, 80000001H. Very understandable but the 1 byte inc ax would still be shorter.
There's also some danger lurking in this code snippet. Who is to say that Intel/AMD in future iterations of the processor will not be returning non-zero values in EAX[8,31] ? Already today some leaf numbers aren't used.

push    eax           ;(1)
cpuid
xchg    ecx, eax      ;Result from CPUID
pop     eax           ;(1)
cmp     ecx, eax
jna     603
mov     [bp], YELLOW
inc     ax
cpuid
bt      edx, 29     Long Mode?
jnc     603

By popping exactly 80000000h, you guarantee that the inc ax will produce the desired 80000001h.
I've used xchg ecx, eax because it's 1 byte shorter than mov ecx, eax.

mov      bx, cx
mov      bl, [bp]        Selected color

Since the first instruction just wants to zero the BH register, you could shave off 1 byte by writing:

movzx    bx, [bp]        Selected color

push      di
push      si
lodsw
neg       ax
stosw
cmp       [si-2], ax   If equal A20 is off
pop       si
pop       di

When I first saw this use of string primitives I expected to see a range of addresses to be compared, but you only process a single word. Next code then uses less instructions and is a few bytes shorter. Note the use of the Segment Override Prefix.

mov     ax, [si]
neg     ax
mov     [es:di], ax
cmp     [si], ax

Using the previous code you can also stop using DI and shave off another 4 bytes since you know that DI=SI+16:

mov     ax, [si]
neg     ax
mov     [es:si+16], ax
cmp     [si], ax

One final point about this code is about the use of neg. The negation of zero is also zero. That's why this kind of testing often uses not which guarantees the value gets modified.  This doesn't impact your code however since you've chosen an address that certainly does not contain zero. But choosing a fixed address like eg. 21h*4 (DOS isn't loaded for sure) would make setting up SI much shorter and even allow you to not use SI at all.

mov     ax, [0084h]
not     ax
mov     [es:0084h+16], ax
cmp     [0084h], ax

61F  81C66000          add       si, 60H       Offset from CS

I don't know what happened here, but I think the compiler ought to give you the sign-extended version of this instruction automatically. Bytes 83h,C6h,60h.

648  49                dec       cx
649  75E4              jnz       62f        Execute test again

This of course is just loop. 1 byte shorter.

My last concern

6D4  B21C              mov       dl, 0x1c

You didn't initialize the DH register because you know it has the correct value from the code above. That's fine but given the separation between setting it up and using it, and considering the note you've included, ...

  NOTE: There is 125 bytes reserved here for additional methods of enabling A20.

... I wonder if you will remember to preserve DH the day you're ready to add these additional methods?


Word counter using a word list and some text files My program works with two types of files.
File 1 contains 500,000 distinct words.
File set 2 contains 173 text files, each containing 500 paragraphs, that I scraped from Wikipedia.
The program counts how many times each word from the first file appears in the second set of files.

The main problem I have is that it's taking around 4 seconds per word to process so it will take around 24 days to complete all 500k words in a core5 7th gen 8gb ram laptop. Is it possible to make it more process efficient?

I am still learning Java so my knowledge is not that vast.  I am using Java 8, with IntelliJ as my IDE.

import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.io.PrintWriter;
import java.util.*;

public class Main {

public static void main(String[] args) {

    //This is the map that will contain each word
    Map<String, Integer> map = new HashMap<>();
    //int that will count how manny times the word is in the File set 2
    int wordCounter = 0;
    //List that contain arround 500k unrepeted words
    List<String> list = new ArrayList<>();
    //List that contains the current file words
    List<String> list1 = new ArrayList<>();

    try {
        //scans the file that contains the 500k unrepeted words
        Scanner s = new Scanner(new File("C:\\Users\\filepath"));
        //while loop that add the words to a list so it can manipulate it latter on
        while (s.hasNext()) {
            list.add(s.next());
        }
        //random output to see the Set size
        System.out.println(list.size());

        //main loop that will cheek each word in the 500k file
        for (int i = 0; i < list.size(); i++) {
            //loop to se each file of words
            for (int j = 0; j < 100; j++) {
                try {
                    //read each file
                    Scanner d = new Scanner(new File("C:\\Users\\filepath" + j));
                    //add the information of each file
                    while (d.hasNext()) {
                        list1.add(d.next());
                    }
                    d.close();
                    //this code counts the number of words in all the files a
                    wordCounter = wordCounter + Collections.frequency(list1, list.get(i).toLowerCase());
                    //clears the list so it has more space and not run out of it
                    list1.clear();

                } catch (IOException k) {
                    k.printStackTrace();
                }
            }
            //adds the information to the map
            map.put(list.get(i), wordCounter);
            //this sorts the information and discard the words that only has 1 or less matches
            if (wordCounter > 1) {
                try {
                    FileWriter fw = new FileWriter("C:\\Users\\filePath", true);
                    PrintWriter pw = new PrintWriter(fw);
                    pw.append("\n");
                    pw.append(map.toString());
                    pw.close();

                } catch (IOException f) {
                    f.printStackTrace();
                }
            }
            //this clean the map so it doesnt run out of memory
                map.clear();
            //resets the counter to 0
                wordCounter = 0;
                //simple display so it seems nice
            System.out.println(i);

        }
    } catch (IOException f){
            f.printStackTrace();
        }

        }

    }

Somewhere I read that because of Java using a virtual machine it makes the processing of data much slower. Would that be something to consider?

You should try to switch inner and outer loop, because it will be much faster to read each wikipedia articles in and count the word frequencies for all 500k words (you anyway have the 500k word list in memory all the time). What you do now is reading 500k times all articles into memory which is time consuming.

To sum up all usages of a word, you can use the map which already exists. Just read out the current sum for a given word, add the occurrences in the current article and write it back to the map. Right now you are just writing one entry to the map, convert it to a string and clear it right afterwards. I assume, you had the idea in mind to do it how I described.

Don't worry about Java execution speed in general, because the code will get compiled (just-in-time-compilation) to machine code eventually.


Basic webpage structure I'm new to HTML coding and have created a basic webpage structure for a basic website with 5 pages.

Can anyone suggest any improvements that could help with the structure, layout and design for the pages?

Code for one of the pages is below (all the same except for the names):

<!DOCTYPE html>

    <title>Fellows and Fullwood LTD</title>
    <meta Charset="utf-8"/>
    <link rel="stylesheet" type="text/css" href="styles.css" />

Fellows and Fullwood

            <u1>
Home

About Us

News

Careers

Contact Us

            </u1>

Contact us
<form  action="" method="POST" enctype="multipart/form-data">

                <input type="hidden" name="action" value="submit"/>

Your name:

              <input name="name" type="text" value="" size="30"/>

                Your email:

                <input name="email" type="text" value="" size="30"/>

 Your message:

                <textarea name="message" rows="7" cols="30"> </textarea>

<input type="submit" value="Send email"/>

          </form>

lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here lots and lots of text here

Sidebar

            <u1>

Home

About Us

News

Careers

Contact Us

          </u1>

&copy;Copyright Fellows and Fullwood 2014.

HTML 5 introduces the  element.

Don't use presentational attributes, like align, use CSS.

The alt attribute is mandatory. If the text that follows it duplicates the information in the image, then use alt="".

The longdesc attribute has been removed from HTML 5. In HTML 4 it should point to an HTML document that describes the image (for people who cannot see it), not the image itself.

          Fellows and Fullwood

The  element is obsolete and should not be used (and I'm pretty sure 12 isn't a valid size for it anyway). Use CSS.

HTML 5 introduces the  element.

          <u1>
Home

About Us

News

Careers

Contact Us

          </u1>

HTML 5 introduces

Contact us
<form  action=""

You can omit the action attribute entirely if you want to resolve to the current URI.

         method="POST" enctype="multipart/form-data">

              <input type="hidden" name="action" value="submit"/>

Most of the paragraphs in this form at dubious at best but this is just wrong. There is no content for the user at all, and certainly no paragraph.

Your name:

Please learn to love labels

            <input name="name" type="text" value="" size="30"/>

In general, prefer container elements and CSS margins/paddings to hard line breaks.
 is most useful when line breaks are a significant part of content which is otherwise continuous (such as street addresses or poetry).

&copy;Copyright Fellows and Fullwood 2014.

HTML 5 introduces the  element.


proxy pattern in Python This is my try at the proxy pattern.

What do you Pythoneers think of my attempt?

class Image:
    def __init__( self, filename ):
        self._filename = filename
    def load_image_from_disk( self ):
        print("loading " + self._filename )
    def display_image( self ):
        print("display " + self._filename)

class Proxy:
    def __init__( self, subject ):
        self._subject = subject
        self._proxystate = None

class ProxyImage( Proxy ):
    def display_image( self ):
        if self._proxystate == None:
            self._subject.load_image_from_disk()
            self._proxystate = 1
        print("display " + self._subject._filename )

proxy_image1 = ProxyImage ( Image("HiRes_10Mb_Photo1") )
proxy_image2 = ProxyImage ( Image("HiRes_10Mb_Photo2") )

proxy_image1.display_image() # loading necessary
proxy_image1.display_image() # loading unnecessary
proxy_image2.display_image() # loading necessary
proxy_image2.display_image() # loading unnecessary
proxy_image1.display_image() # loading unnecessary

Output:

loading HiRes_10Mb_Photo1
display HiRes_10Mb_Photo1
display HiRes_10Mb_Photo1
loading HiRes_10Mb_Photo2
display HiRes_10Mb_Photo2
display HiRes_10Mb_Photo2
display HiRes_10Mb_Photo1

It seems like overkill to implement a class to represent the proxy pattern. Design patterns are patterns, not classes. What do you gain from your implementation compared to a simple approach like the one shown below?

class Image(object):
    def __init__(self, filename):
        self._filename = filename
        self._loaded = False

    def load(self):
        print("loading {}".format(self._filename))
        self._loaded = True

    def display(self):
        if not self._loaded:
            self.load()
        print("displaying {}".format(self._filename))

Some other notes on your code:

It doesn't follow PEP8. In particular, "Avoid extraneous whitespace [...] immediately inside parentheses."
No docstrings.
It makes more sense to use True and False for a binary condition than to use None and 1.
It's not necessary to append the class name to all the methods. In a class called Image, the methods should just be called load and display, not load_image and display_image.
You should use new-style classes (inheriting from object) to make your code portable between Python 2 and 3.


In-Memory FileSystem in Java - Is there a better OOP approach? I have implemented an in-memory filesystem - this is a design question from LeetCode. Is there a better OOP approach to represent files and directories? The current code uses a boolean flag isDirectory to distinguish between files and directories and I'm not confident that it is the best approach. See the code below. I'll also be happy to get any other comments about the code - readability, variable and method naming, etc.
class FileSystem {

    Inode root;

    public FileSystem()
    {
        root = new Inode("/", true);
    }

    public List<String> ls(String path)
    {
        List<String> listing = new ArrayList<>();

        String[] queries = path.substring(1).split("/");

        Inode node = root;

        for(String query: queries)
        {
            if(query.isEmpty()) // happens when we search root
                break;

            node = node.children.get(query);

            if(node == null)
                return listing;
        }

        if(node.isDirectory)
        {
            node.children.forEach((name, inode) -> {
                listing.add(name);
            });
        }
        else
        {
            listing.add(node.Name);
        }

        Collections.sort(listing);

        return listing;
    }

    public void mkdir(String path)
    {
        String[] subdirs = path.substring(1).split("/");
        Inode node = root;

        for(String dir: subdirs)
        {
            if(dir.isEmpty()) // happens when we search root
                break;

            if(!node.children.containsKey(dir))
                node.children.put(dir, new Inode(dir, true));

            node = node.children.get(dir);
        }

    }

    public void addContentToFile(String filePath, String content)
    {
        String[] subdirs = filePath.substring(1).split("/");
        Inode node = root;

        for(int i=0; i<subdirs.length; i++)
        {
            if(!node.children.containsKey(subdirs[i]))
            {
                if(i < subdirs.length-1)
                    node.children.put(subdirs[i], new Inode(subdirs[i], true));
                else
                    node.children.put(subdirs[i], new Inode(subdirs[i], false));

            }

            node = node.children.get(subdirs[i]);
        }

        node.fileContent.append(content);

    }

    public String readContentFromFile(String filePath)
    {
        String[] subdirs = filePath.substring(1).split("/");
        Inode node = root;

        for(String dir: subdirs)
        {
            node = node.children.get(dir);
        }

        return node.fileContent.toString();
    }

}

// Trie data structure for files and directories

    class Inode
    {
        public String Name;
        public boolean isDirectory=true;
        public StringBuilder fileContent=null;
        public HashMap<String, Inode> children = null;

        public Inode(String name, boolean is_dir)
        {
            this.Name = name;
            this.isDirectory = is_dir;

            if(!is_dir)
                fileContent = new StringBuilder();

            if(is_dir && children == null)
                children = new HashMap<>();
        }

    }

create Subclasses / interfaces
if you think there is a difference in Behaviour then there is a need for a seperate class - if you think some classes have something in common, extract an interface!
so create a class Directory and a class File. Let both classes implement the Node Interface. (Note - in Java we don't use the hungarian notation )
inconsistency // oo-abuse
what's the purpose of Node if you don't use it? compare these two approaches

public List<String> ls(String path) vs.
public List<Node> ls(Node path)

in case 1 it is absolutly not clear how the String path should look like, especially since it is OS-dependant. (what would String[] subdirs = filePath.substring(1).split("/"); on other OS do?) It is also not clear what kind of output this Method delivers
in case 2 we have cleary declared what kind of input we expect, (even though i'm missing a parent attribut - that would ease up the searching) - we reduce the problem with different OSes to the single place where it belongs: at the constructor of the Node (Single responsibility). And we have a very accurate output (compared to the List<String> from case 1 - how would we know if a String is a Directory or a File? Node already provides all these information!)
File System vs. Editor
i'm not sure how addContentToFile() and readContentFromFile match for a FileSystem - in my opinion these two methods belong to an Editor/Viewer and not the the File System - create an Editor (an Editor class)!
think of the possibilities: you can then provide different Editors for different file Types?
Error Handling
i would expect a file system to be a bit more chatty about IllegalArgumentException (why are none thrown? how is such case treated?)


Is this timer efficient? I found myself writing code to run at a specified frequency more than once, so I decided to make this simple timer. My main concern is that this is not the most efficient way of doing this since with SDL I would just ask for the time in ms and sleep in ms.

I'm storing the current time in timer_update() to avoid the possibility of it becoming greater than the final time and generating a huge pause since it's unsigned.

Usage is pretty simple, timer_init() is called once before the loop and timer_update() is called once at the end of every iteration.

timer.h

#ifndef TIMER_H
#define TIMER_H

typedef struct {
    double step;
    double tfinal;
} Timer;

void timer_init(Timer *timer, double hertz);
void timer_update(Timer *timer);

#endif

timer.c

#include "timer.h"

#define _POSIX_C_SOURCE 199309L
#include <time.h>
#include <errno.h>
#include <stdio.h>

static unsigned long get_ms(void)
{
    struct timespec buffer;
    unsigned long temp;

    if(clock_gettime(CLOCK_MONOTONIC, &buffer) != 0){
        perror("clock_gettime");
        return 0;
    }

    temp = buffer.tv_sec * 1000;
    temp += buffer.tv_nsec / 1000000;
    return temp;
}

static struct timespec ms_to_timespec(unsigned long ms)
{
    struct timespec temp = {ms / 1000, (ms % 1000) * 1000000};
    return temp;
}

void timer_init(Timer *timer, double hertz)
{
    timer->step = 1000 / hertz;
    timer->tfinal = get_ms() + timer->step;
}

void timer_update(Timer *timer)
{
    unsigned long now = get_ms();
    struct timespec required_sleep, rem;

    if(now < timer->tfinal){
        required_sleep = ms_to_timespec(timer->tfinal - now);
        timer->tfinal += timer->step;

        try_to_sleep:
        if(nanosleep(&required_sleep, &rem) == -1 && errno == EINTR)
            if(nanosleep(&rem, &required_sleep) == -1 && errno == EINTR)
                goto try_to_sleep;
    }

    else
        timer->tfinal += timer->step;
}

Is this timer efficient?

Yes, but there are two optimizations that I can see.  Get rid of the goto
and use a do-while loop.  I have noticed a slight performance
increase when it has been written this way in other programs.

This increase will become non-existent however if you have compiler
optimization settings enabled.  If they are enabled, they compile
into the exact same assembly code.  However, I also find a loop
more readable than a goto, so that is another reason to switch
over.
There other optimization I could see is by cleaning up your if-else a tiny bit (untested).  This should speed up your code a tiny bit (again, if optimizations on your compiler are enabled, your results will vary).

if(now < timer->tfinal)
{
    required_sleep = ms_to_timespec(timer->tfinal - now);

    // loop implementation
}
timer->tfinal += timer->step;

You may wonder why this will speed up your code.  The more branches you have, the more susceptible you are to a branch prediction failure and those failures can be expensive when it comes to efficiency.

A few other notes:

Use a designated initializer to initialize a structure (C99).

struct timespec temp =
{
    .step = ms / 1000,
    .tfinal = (ms % 1000) * 1000000
};

Group your #includes and #defines together in a more organized fashion.

#include <time.h>  // group library headers
#include <errno.h>
#include <stdio.h>
#include "timer.h" // group user defined headers

#define _POSIX_C_SOURCE 199309L  // group defines

Your != 0 comparision check in your if conditional could be removed since it is redundant (the if will always execute if the conditional isn't equal to 0).  However, this may increase readability to you, so it would be okay to keep it.


C++ implementation of quicksort with iterators (unstable sorting) Please review the following quicksort implementation.
qsort below is intentionally unstable, as partition reoders equal elements.

#include <iostream>
#include <vector>
#include <algorithm>

using namespace std;

// Ensures pivot is at position end.
template<typename Iter, typename T>
Iter part_pivot(Iter begin, Iter end, T pivot) {
  while (begin != end) {
    if (*begin < pivot)
      ++begin;
    else if (*--end < pivot)
      iter_swap(begin, end);
  }
  return begin;
}

template<typename Iter>
Iter part(Iter begin, Iter end) {
  typename iterator_traits<Iter>::value_type pivot = *begin;
  Iter mid = part_pivot(begin, end, pivot);
  // Ensure pivot is at position mid.
  iter_swap(--end, mid);
  return mid;
}

template<class Iter>
void quicksort(Iter begin, Iter end) {
  if (std::distance(begin, end) < 2)
    return;
  Iter mid = part(begin, end, *begin);
  quicksort(begin, mid);
  quicksort(++mid, end);
}

template<class T>
void quicksort(vector<T>& v) {
  quicksort(v.begin(), v.end());
}

template <class T>
void print(const vector<T>& v) {
  for (const T& t : v)
    cout << t << " ";
  cout << endl;
}

int main() {
  vector<int> v = {1,4,8,34,2,3,45,6,87,4,3};
  print(v);
  quicksort(v);
  print(v);
}

Two problems with the code that I see immediately:

if (std::distance(begin, end) <= 2) is wrong since it leaves an array like {2,1} untouched. This means that the code can screw up every time it recurses to an array of size 2.
Iter partition(Iter begin, Iter end, const T& pivot): Passing pivot as const T& is a bad idea. When passed as const reference, when you perform the iter_swap, the value pointed to by begin changes and so does pivot. Hence running your code gives me a segfault but changing const T& to T takes care of the segfault.

Even if these are fixed, there is something more subtle. Suppose you try to sort an already sorted array like {1,1,1}. Partitioning this will return mid = begin. So unless you make an explicit check for a sorted array you will recurse infinitely on qsort(mid,end).


Branching with multiple Try I have a series of four method calls that all return Try[Something], with the last returning Try[Unit]. Something like this:

def getFields(): Try[List[A]] = { ... }
def getValues(): Try[List[B]] = { ... }
def createTmpFile(as: List[A], bs: List[B]): Try[C] { ... }
def runProcessWithFile(c: C): Try[Unit] { ... }

def run(): Try[Unit] = {
  for (
    fields <- getFields();
    values <- getValues();
    file <- createTmpFile(fields, values);
    _ <- runProcessWithFile(file)
  ) yield ()
}

This seems OK to me. I guess my first question is about Try[Unit]. That seems a little awkward. Option[Exception] seems equally awkward. Is there a "best practice" return type in Scala for a possible Exception?

Also, things got complicated when I wanted to add branching based on whether getValues() returns an empty list. This is what I have right now:

def run(): Try[Unit] = {
  for (
    fields <- getFields();
    values <- getValues();
    _ <- if (values.nonEmpty) {
      for (
        file <- createTmpFile(fields, values);
        _ <- runProcessWithFile(file)
      ) yield ()
    } else {
      Success(())
    }
  ) yield ()
}

That seems pretty hairy. Is there a better way to do this? I suppose I could abandon Try altogether and just let everything throw Exceptions, but that seems like throwing the baby out with the bathwater.

Having a Try[Unit] is completely ok - it models the fact that there may be exceptions on a side effecting operation in a type. That your code looks ugly is not because your monad is wrong but because you used it wrong. You already started in modeling your application with monads, why just stopping in the middle?

You can easily restructure your monad a little bit:

def run(): Try[Unit] = {
  val res = for {
    fields <- getFields()
    values <- getValues()
  } yield {
    if (values.isEmpty)
      Success(())
    else for (file <- createTmpFile(fields, values))
      yield runProcessWithFile(file)
  }
  res.flatten
}

What we see here is a monad.map(f).flatten which is the same as monad.flatMap(f) which is the same as for (m <- monad) yield f(m). Applying this fact to your code snippet results in:

def processTmpFile(as: List[A], bs: List[B]): Try[Unit] =
  if (values.isEmpty)
    Success(())
  else for (file <- createTmpFile(fields, values))
    yield runProcessWithFile(file)

def run(): Try[Unit] = {
  for {
    fields <- getFields()
    values <- getValues()
  } yield processTmpFile(fields, values)
}

Operating on a deeper level of the monad (what you did with your condition) means that you can either nest the monad and unpack it later at the end or that you can unpack it immediately after the nesting happened (that is what a for comprehension does).


lodash get method equivalent in javascript I wanted to build similar to lodash get method. Where you put the object and key name to get value if exist. I have done this but just wanted to have more better approach. Here is what has been done so bar.

var x = {
  d: 'new vale',
  a: {
    b: {
      c: {
        e: 'value'
      }
    }
  }

}

function getVal(obj, key) {
  var keys = obj[key];
  if (!keys) {
    for (var i in obj) {
      if (Object.prototype.toString.call(obj[i]) === '[object Object]') {
        return getVal(obj[i], key)
      }

    }
  } else {
    return obj[key]
  }

}

console.log(getVal(x, 'c'))

You can try the following script as alternate approch which:

Get a nested property of a complex object at any level of depth without using recursion.

This script uses replacer optional function for JSON.stringify(), basically it takes a JavaScript value (your initial object) and convert to a JSON string, and it uses that string to search within your array. The script works without any recursions, with any number of nested properties for your objects. The order of your property is important in this script, as the conversion to string take it in consideration.

Cavet: Date object is returned stringified.

let data = {
  d: 'new vale',
  a: {
    b: {
      c: {
        e: 'value'
      }
    }
  }
};

let getPropertyRecursive = function(data, name) {
  let result = {
    isFound: false,
    value: null
  };
  JSON.stringify(data, function(k, v) {
    if (k === name) {
      result.isFound = true;
      result.value = v;
      return undefined;
    } else {
      return v;
    }
  });
  return result;
};

let result1 = getPropertyRecursive(data, 'e');
let result2 = getPropertyRecursive(data, 'a');
console.log(result1);
console.log('-----');
console.log(result2 );


Python webscrape Amazon Music, generate tracklist So I've been working on a tracklist generator that scrapes from Amazon Music using a url using Python for albums with one artist. I've made uhhh this, I'm really new to this requests and beautifulsoup4 thing. I wonder if I can improve it to make it more efficient.

import requests
from bs4 import BeautifulSoup

Amazon=str(input("Please enter an Amazon music url:"))
r=requests.get(Amazon)
soup = BeautifulSoup(r.text,'html.parser')
name=soup.find_all('a', attrs={'class':'a-link-normal a-color-base TitleLink a-text-bold'}) #find out the names of the track title
time=soup.find_all('td',attrs={'class':'a-text-right a-align-center'}) #find the duration of the track
artist= soup.find('a', attrs={'id':'ProductInfoArtistLink'}) #find the creator of the track, which for now can only take one

for i in range(1,len(name),2):
    print(str(int(i/2+1))+'. '+name[int(i)].text+' - '+ artist.text + ' (' + time[int((i-1)/2)].text[12:16] + ')')

#first int produces a placeholder number for the track e.g 1., 2.
#second int produces track name, which len shows twice of number of tracks
#artist text gives artist name
#time gives time and puts it in brackets

Whenever you find yourself writing long (or sometimes even short) comments explaining a single line/a block of lines, you should ask yourself if this was not better placed in a function. Functions can be given a meaningful name and you can add a docstring to them (which can be considerably longer than a comment practically can). It also give you one obvious place to change if, for example, the Amazon Music website is changed at some point.

IMO, here the function names should already be self-explanatory enough, so I did not add any docstrings.

import requests
from bs4 import BeautifulSoup
from itertools import count

def get_soup(url):
    r = requests.get(url)
    r.raise_for_status()
    return BeautifulSoup(r.text, 'lxml')

def track_titles(soup):
    attrs = {'class': 'a-link-normal a-color-base TitleLink a-text-bold'}
    return [a.text for a in soup.find_all('a', attrs=attrs)[::2]]

def track_durations(soup):
    attrs = {'class': 'a-text-right a-align-center'}
    return [td.text.strip() for td in soup.find_all('td', attrs=attrs)]

def track_artist(soup):
    return soup.find('a', attrs={'id':'ProductInfoArtistLink'}).text

if __name__ == "__main__":
    url = input("Please enter an Amazon music url:")
    soup = get_soup(url)

    titles = track_titles(soup)
    durations = track_durations(soup)
    artist = track_artist(soup)

    for i, title, duration in zip(count(1), titles, durations):
        print(f"{i}. {title} - {artist} ({duration})")

Other things I changed:

Added a if __name__ == "__main__": guard to ensure you can import this module from another script.
Used the fact that input already returns a string (in Python 3).
Added r.raise_for_status() so that the code raises an exception if the request does not succeed.
Consistently followed Python's official style-guide, PEP8.
Used the usually faster lxml parser.
Iterate over the elements instead of the indices.
Used the recently introduced f-string to make the formatting easier.
Used str.strip instead of hardcoding indices for the duration.
Made the functions do all the cleanup.


Improving Efficiency of Random Walker I'm seeing a web course of computational statistics, the first project consists of a random walker, the implementation I have done works as intended:
import numpy as np
import numpy.random as rd
import matplotlib.pyplot as plt

WALKER_STEPS: int = 5_000

# random orientation converted from degrees to radians
directions: np.array = rd.uniform(low=0, high=360, size=WALKER_STEPS) * np.pi / 180

x: list = [0]
y: list = [0]
idx: int = 0
for rad in directions:
    # updates the positions of the walker
    x.append(x[idx] + np.cos(rad))
    y.append(y[idx] + np.sin(rad))
    idx += 1

plt.plot(x, y)
plt.show()

I don't like the way I'm adding new elements to the lists x, y and I think that there's a more elegant way to do it.
Here a image of the result:

Note: I didn't do an abstraction because the exercise in small.

Use np.cumsum()
import numpy as np
import numpy.random as rd
import matplotlib.pyplot as plt
WALKER_STEPS: int = 5_000

# random orientation converted from degrees to radians
directions: np.array = rd.uniform(low=0, high=360, size=WALKER_STEPS) * np.pi / 180

x = np.cos(directions).cumsum()
y = np.sin(directions).cumsum()

plt.plot(x, y)
plt.show()


Stack Exchange Chat Caret Pathfinder In The Nineteenth Byte, we have an excessive amount of carets (carrots, if you will) that direct one's attention to another message. For example:

Amidst the confusion, I made a userscript that highlights in yellow the pointed-at message when the message in question is clicked.

(function(){
    var elements = {}, messages;

    function findCarrotMessage(){
        messages = Array.prototype.slice.call(document.querySelectorAll("[id^='message-']"));
        messages.forEach(function(e){
            if(!elements[e.id] && e.className==="message"){
                elements[e.id] = true;
                e.addEventListener("click", function(s){
                    // count the number of ^'s
                    var num = this.textContent.match(/\^+/g), self = this;
                    if(num){
                        var maxHash = -1;
                        num.forEach(function(k){
                            var destinationMessage = messages[Array.from(messages).indexOf(self)-k.length];
                            destinationMessage.style.background = "yellow";
                            setTimeout(function(e){e.style.background = "";}, 3500, destinationMessage);
                            maxHash = Math.max(destinationMessage.id.slice(destinationMessage.id.indexOf("-")+1), maxHash);
                            elements[self.id] = false;
                        });
                        window.location.hash = "#message-" + maxHash;
                    }
                    this.removeEventListener("click", arguments.callee, false);
                }, false);
            }
        });
    }

    setInterval(findCarrotMessage,50);
})();

How can I improve my code? Thanks!

There is still code-golf

Here you have some golfed code:

function(e){e.style.background = "";}

ungolf it:

function(e) {
    e.style.background = ""
}

Run away from arguments.callee

  The 5th edition of ECMAScript (ES5) forbids use of arguments.callee() in strict mode. Avoid using arguments.callee() by either giving function expressions a name or use a function declaration where a function must call itself.

 - arguments.callee on MDN

here you have:

e.addEventListener("click", function(s){

instead do...

var MyFunction = function(s) {

and then...

e.addEventListener("click", MyFunction);

this allows you to then do:

this.removeEventListener("click", MyFunction, false);

Don't forget whitespace:

e.className==="message"

add whitespace around the ===. There a lot more places where spaces could be added

Conflicting things

In one part you're using:

 Array.prototype.slice.call

in another you are using:

Array.from

you should probably decide on one

Don't to a ton of things in one line

maxHash = Math.max(destinationMessage.id.slice(destinationMessage.id.indexOf("-")+1), maxHash)

that is waaaay to much in one line for a code reviewer like me ;) to be supporting. This:

destinationMessage.id.slice(destinationMessage.id.indexOf("-")+1)

looks like it can become a regex:

( /-(\d+)/.exec(destinationMessage.id) || [])[1]

or even better:

destinationMessage.id.split("-")[1]

use .getAttribute

rather than something like:

e.id

prefer:

e.getAttribute("id")

Remove all code-golf

var destinationMessage = messages[Array.from(messages).indexOf(self)-k.length]

you should split this into multiple variables. What if indexOf returns -1, you should handle that because otherwise your code will error and completely breakdown

use .bind rather than self = this

You're having to introduce self because the forEach's function creates a new scope. Instead use .bind and keep your old scope

num.forEach(function(k) {
    // code
}.bind(this))

Even better, use e.target to get the element that's been clicked. I highly recommending avoiding using this in lambdas to store data.


Rust recursion pattern in simple cat program I'm trying to get my head around recursion in Rust. I've put together a basic implementation of cat here to play with the basics.

Here's my recursive version:

use std::env;
use std::fs::File;
use std::fmt::Display;
use std::io;
use std::io::BufReader;
use std::io::prelude::*;
use std::path::Path;

const STDIN: &'static str = "-";
const BUFFER_SIZE: usize = 4096;

enum Method {
    File,
    Line,
}

fn main() {
    // skip the first element as it is the program name
    let args = env::args();
    if args.len() == 1 {
        cat(io::stdin(), "stdin", Method::Line);
    } else {
        for arg in args.skip(1) {
            if arg.eq(STDIN) {
                cat(io::stdin(), "stdin", Method::Line);
            } else {
                let path = Path::new(&arg);
                let display = path.display();
                let file = File::open(path)
                    .expect(format!("cat: failed to open: {}", display).as_str());
                cat(file, display, Method::File);
            }
        }
    }
    io::stdout().flush().unwrap();
}

// Read from `input` and print to stdout.
// `name` is used in error messages.
// `m` selects whether to read line-by-line or to read the entire input.
fn cat<R, N>(input: R, name: N, m: Method) where R: Read, N: Display {
    let mut reader = BufReader::with_capacity(BUFFER_SIZE, input);
    cat_recurse(&mut reader, name, m);
}

fn cat_recurse<R, N>(reader: &mut BufReader<R>, name: N, m: Method) where R: Read, N: Display {
    let mut s = String::new();
    let n = match m {
        Method::Line => reader.read_line(&mut s),
        Method::File => reader.read_to_string(&mut s),
    }.expect(format!("cat: failed to open {}", name).as_str());
    if n > 0 {
        print!("{}", s);
        // recurse
        cat_recurse(reader, name, m);
    }
}

I know passing around the Method enum is a bit ugly but it was the best way I could come up with to get the app to work with stdin properly. I'm interested in feedback on the design of the recursion pattern specifically. Does it seem about right or is there a simpler way to represent this that I haven't thought of yet? I'd also appreciate any more general feedback you can offer, but the recursion bit is where I'm trying to learn right now.

Where clauses go on separate lines; easier to see them as they change the functionality so much.
Move the actual argument skipping near to the comment.
There's no need to call eq; use == instead.
String::as_str can often be avoided, just take a reference via &.
The code always builds the failure string, even when it doesn't fail! To avoid this, use unwrap_or_else, which allows access to the specific error as a closure argument.
Similarly, the code always gets Path::display, even when there is no failure.
There's no need to get the display anyway, the code already has the user-provided arg.
There's no need for Path at all; strings are convertable to OsStr, which is what File::open needs.
The name argument and the Method enum are always correlated one-to-one and they should be combined.
The error text in cat_recursive is incorrect; the file / stdin are already open, this error denotes failure to read from it.
Remove useless comments ("recurse"). As an aside, it was pointed out to me many years ago that the verb form is "recur", and an algorithm is "recursive"; "recurse" isn't really a word.
If you are going to document a function, use Rustdoc formatting.
If you are going to document an argument that is a standalone type, consider documenting the type instead. This is one reason to have types at all - to has a package of related meaning.

use std::{env, io};
use std::fs::File;
use std::io::BufReader;
use std::io::prelude::*;

const STDIN: &'static str = "-";
const BUFFER_SIZE: usize = 4096;

/// Read line-by-line or to read the entire input
enum Method<'a> {
    File(&'a str),
    Line,
}

impl<'a> Method<'a> {
    fn name(&self) -> &str {
        match *self {
            Method::File(name) => name,
            Method::Line => "stdin",
        }
    }
}

fn main() {
    // skip the first element as it is the program name
    let args = env::args().skip(1);
    if args.len() == 0 {
        cat(io::stdin(), Method::Line);
    } else {
        for arg in args {
            if arg == STDIN {
                cat(io::stdin(), Method::Line);
            } else {
                let file = File::open(&arg)
                    .unwrap_or_else(|e| panic!("cat: failed to open {}: {}", &arg, e));
                cat(file, Method::File(&arg));
            }
        }
    }
    io::stdout().flush().unwrap();
}

/// Read from `input` and print to stdout.
fn cat<R>(input: R, m: Method)
    where R: Read,
{
    let mut reader = BufReader::with_capacity(BUFFER_SIZE, input);
    cat_recursive(&mut reader, m);
}

fn cat_recursive<R>(reader: &mut BufReader<R>, m: Method)
    where R: Read,
{
    let mut s = String::new();
    let n = match m {
        Method::Line => reader.read_line(&mut s),
        Method::File(..) => reader.read_to_string(&mut s),
    };
    let n = n.unwrap_or_else(|e| panic!("cat: failed to read {}: {}", m.name(), e));
    if n > 0 {
        print!("{}", s);
        cat_recursive(reader, m);
    }
}

The big points

The algorithm isn't really recursive for files. The entire file is read in via read_to_string. This means that memory is allocated to hold the entire file. Then we recur once to read nothing and end.
When reading line-by-line, the String is reallocated for every call. This will increase memory usage with each call, up to the last call, at which point we will have allocated the entire input size again.
Which leads to the important point: Rust doesn't have tail recursion! Writing an unbounded recursive function is eventually going to end in tears for someone.
Additionally, this cat doesn't support binary files, as it crams everything into a String.
I'm not sure why there are two methods of reading, one for files and one for stdin. Why not always read line-by-line or as one big blob?


"options" binding with an enum as data source I need to bind a dropdown to an enum. For this I've made a key_value_pair class and I manually go through each enum option to add it. Is there a more elegant way to achieve this?

HTML:

<select data-bind="options: regions, value: region, optionsValue: 'key', optionsText: 'value'"></select>

TypeScript:

class key_value_pair<key_type, value_type>
{
    key: key_type;

    value: value_type;

    constructor(key: key_type, value: value_type)
    {
        this.key = key;

        this.value = value;
    }
}

class calculator
{
    regions: KnockoutObservableArray<key_value_pair<number, string>>;

    region: KnockoutObservable<API.region>;

    constructor()
    {
        this.regions = ko.observableArray();
        this.regions.push(new key_value_pair(API.region.US, API.region[API.region.US]));
        this.regions.push(new key_value_pair(API.region.Europe, API.region[API.region.Europe]));
        this.regions.push(new key_value_pair(API.region.Korea, API.region[API.region.Korea]));
        this.regions.push(new key_value_pair(API.region.Taiwan, API.region[API.region.Taiwan]));
        this.regions.push(new key_value_pair(API.region.China, API.region[API.region.China]));

        this.region = ko.observable();
    }
}

Your solution works fine and I didn't find a better solution. I've just upgraded it little bit, by automatic fill the options, so it is usable to any Enum class.

Here I found how to enumerate Enum type

export class SelectEditorObject
{
    private options: KnockoutObservableArray<keyValuePair<number, string>>;
    private selection: KnockoutObservable<number>;

    constructor(e: any, value: number)
    {
        var options = SelectEditorObject.getNamesAndValues(e);
        this.selection = ko.observable<number>(value);
        this.options = ko.observableArray<keyValuePair<number, string>>(options);
    }

    public toString()
    {
        var selected = undefined;
        this.options().forEach((pair) =>
        {
            if (pair.key === this.selection())
            {
                selected = pair.value;
            }
        }, this);
        return selected;
    }

    public getValue()
    {
        return this.selection();
    }

    private static getNames(e: any)
    {
        return Object.keys(e).filter(v => isNaN(parseInt(v, 10)));
    }

    private static getValues(e: any)
    {
        return Object.keys(e).map(v => parseInt(v, 10)).filter(v => !isNaN(v));
    }

    public static getNamesAndValues(e: any): Array<keyValuePair<number, string>>
    {
        return SelectEditorObject.getValues(e).map(v => { return new keyValuePair(v, e[v]) });
    }
}

export class keyValuePair<key_type, value_type>
{
    public key: key_type;
    public value: value_type;
    constructor(key: key_type, value: value_type)
    {
        this.key = key;
        this.value = value;
    }
}


Parser for json-like format in PHP At work they use a format similar to JSON but without quotes that looks like this:

{foo:{qux:1,quux:0}, bar:{}}

The reason they don't just use json is because the C# package Newtonsoft.Json can deserialize this like if it was regular json and it works. I need to use it in Php but json_decode is not as forgiving.

So here's my attempt at a simple parser:

<?php

namespace Foo\Bar;

class NotJsonParser
{
    const STEP_NAME = 0;
    const STEP_VALUE = 1;

    /**
     * @param $string
     * @return array
     */
    public static function parseNotJSON($string)
    {
        $generator = self::stringIterator($string);
        $data = self::parser($generator);
        return $data[''];
    }

    /**
     * @param \Generator $generator
     * @return array
     */
    private static function parser(\Generator $generator)
    {
        $data = [];
        $step = self::STEP_NAME;
        $name = '';
        $value = '';

        while ($generator->valid()) {
            $i = $generator->current();
            switch ($i) {
                case ' ':
                case "\n":
                    continue;
                case '{':
                    $generator->next();
                    $value = self::parser($generator);
                    $data[$name] = $value;
                    $step = self::STEP_NAME;
                    $name = '';
                    $value = '';
                    break;
                case '}':
                    if ($name) {
                        $data[$name] = $value;
                    }
                    return $data;
                case ',':
                    if ($name) {
                        $data[$name] = $value;
                    }
                    $step = self::STEP_NAME;
                    $name = '';
                    $value = '';
                    break;
                case ':':
                    $step = self::STEP_VALUE;
                    break;
                default:
                    if ($step === self::STEP_NAME) {
                        $name .= $i;
                    } else {
                        $value .= $i;
                    }
            }
            $generator->next();
        }
        return $data;
    }

    /**
     * @param string $str
     * @return \Generator
     */
    private static function stringIterator($str)
    {
        for ($i = 0; $i < strlen($str); $i++) {
            yield $str[$i];
        }
    }
}

And here's the usage:

>>> $result = \Foo\Bar\NotJsonParser::parseNotJSON("{foo:{qux:1,quux:0}, bar:{}}");
=> [
     "foo" => [
       "qux" => "1",
       "quux" => "0",
     ],
     "bar" => [],
   ]

How could I improve this? I know it really lacks error handling. I don't mind that the numbers stay as strings. Also the format never goes more than 2 levels deep. Whitespace between tokens is non important but there shouldn't be whitespace inside the keys (e.g.: {foo bar: baz} should be an error).

Also how should I encode it back from an array to a string? I was thinking just using json_encode and then removing the quote characters.

The following workaround will wrap your keys and values with double quotes.  Such hacks will always be vulnerable to edge cases.  To avoid sprinting down a rabbit hole of possibilities (I can think of a few cases off the top of my head -- 1. keys/values already containing quotes 2. declared empty/null keys ...there will be more), I'll just provide a solution for your sample input.

For now, I'm using \w to ensure that the space before bar is not included.  There are several ways to do this action, but I would need to have intimate knowledge of your project data to develop the expression that I feel is "best / most robust".

If you discover any fringe cases that break this simple regex pattern, please update your question and I can create a patch.

Code: (Demo)

$unquoted_json = <<<NOTJSON
{foo:{qux:1,quux:0}, bar:{}}
NOTJSON;

$quoted_json = preg_replace('~\w[^:{},]*~', '"$0"', $unquoted_json);
$array = json_decode($quoted_json, true);
var_export($array);
echo "\n---\n";
echo json_encode($array);

Output:

array (
  'foo' =>
  array (
    'qux' => '1',
    'quux' => '0',
  ),
  'bar' =>
  array (
  ),
)
---
{"foo":{"qux":"1","quux":"0"},"bar":[]}


Three identical AJAX button handlers in three views In my Rails 4 app I have this event listener:

$('.remove-tag-button').on('click', function () {
    $.ajax({
      type: "POST",
      data: {id: <%= @item.id %>, tag: $(this).data("tag-name")},
      url: "/remove_tag"
    });
  });

in three places and I'm wondering if there's a way to refactor.

I have an items controller with some actions including show, add_tag, and remove_tag.

def show
  @item= Item.find_by_id(params[:id])
end

def add_tag
  (omitted for brevity)
  respond_to do |format|
    format.js
end

def remove_tag
  (omitted for brevity)
  respond_to do |format|
    format.js
end

My show.html.erb has a form with a button to remove tags (class="remove-tag-button") that the event listener is referring to.  I have the listener inside script tags at the bottom of my show.html.erb (inside a $(document).ready()) and also inside my add_tag.js.erb and remove_tag.js.erb.

show.html.erb

  <%= render partial: 'items/tag_list', locals: {item: @item} %>

 <form action="<%= add_tag_path %>" method="post" id="custom-tag" class="input-group" autocomplete="off" data-remote="true">
   <%= text_field_tag :tag, nil, placeholder: 'Add tag', class: "form-control typeahead", id: 'add-tag' %>

     <%= submit_tag 'Add', id: "add-button", class: "btn btn-default" %>

 </form>

add_tag.js.erb

$("#taglist").html('<%= escape_javascript(render partial: "items/tag_list", locals: {item: @item}) %>');

$('.remove-tag-button').on('click', function () {
  $.ajax({
    type: "POST",
    data: {id: <%= @item.id %>, tag: $(this).data("tag-name")},
    url: "/remove_tag"
  });
});

remove_tag.js.erb

$("#taglist").html('<%= escape_javascript(render partial: "items/tag_list", locals: {item: @item}) %>');

$('.remove-tag-button').on('click', function () {
  $.ajax({
    type: "POST",
    data: {id: <%= @item.id %>, tag: $(this).data("tag-name")},
    url: "/remove_tag"
  });
});

items/_tag_list.html.erb

<% item.all_tags_list.each do |t| %>

    <%= link_to t, tag_path(t), class: "item-tag" %>
    <% if item.tags_from(current_user).include?(t) %>

        <%= link_to "×", '', class: "remove-tag-button", :'data-tag-name' => t, remote: true %>

    <% end %>

<% end %>

I have 3 instances of this code because a user needs to be able to add and remove tags through AJAX requests while on the page. So it listens when the page loads, then I need it to still be listening after a user has added and/or removed a tag (both which are done through AJAX).

This seems sloppy.  I'm wondering if theres a better way to do it.

I was able to get rid of the duplicate code by using event delegation.  I changed my event listener to:

$('#taglist').on('click', '.remove-tag-button', function () {
    $.ajax({
      type: "POST",
      data: {id: <%= @item.id %>, tag: $(this).data("tag-name")},
      url: "/remove_tag"
    });
  });


Script to check for failed logins and then trawl users' .bash_history for keywords I have created a script to check for failed logins and then trawl User .bash_history for keywords. There is also processing on the .bash_history files to convert epoch to human-readable timestamps.
I've run my script through shellcheck.net and the only real thing that it's complaining about are my For loops. It's saying they should be while loops instead, and I'm struggling to convert them.

Here's the script in its entirety:

#! /bin/bash
#

printf "\n"
echo -e "\e[93m*** Failed Login Checker ***\e[0m"
sleep 2
grep -E 'Failed password|invalid' /var/log/secure* | grep "sshd\[" | uniq
read -r -p "Press [Enter] key to continue..."

printf "\n"
echo -e "\e[93m*** BASH History Checker - script initiated ***\e[0m"
sleep 2

HOLDINGDIR=/root/check_history

if [ ! -d $HOLDINGDIR ];
then
        mkdir $HOLDINGDIR
fi

for i in $( find /home -name .bash_history -type f );
do
        cp "$i" $HOLDINGDIR/$( ls -la "$i" | awk '{print$3}' )_history
done

echo -e "\e[93mAll BASH History files copied - now processing date format...\e[0m"

for k in $( find $HOLDINGDIR -name '*_history' -type f );
do
        echo "Processing file: " "$k"
        sed -i -E 's/^#([0-9]+).*$/date -d @\1/e' "$k"
        chmod 755 "$k"
done

printf "\n"
echo -e "\e[93mAll BASH History files timestamps converted\e[0m"

printf "\n"
read -r -e -p "Enter search term: " -i "sudo su" search
echo "You typed: " "$search"
sleep 3
printf "\n"

for m in $( find $HOLDINGDIR -name '*_history' -type f );
do
        printf "\n"
        FILENAME=$m
        echo "-----------------------------------------------------------"
        echo "History file for: " "$FILENAME" | awk -F/ '{print$4}'
        echo "-----------------------------------------------------------"
        grep -F -B1 -i "$search" "$m"
        read -r -p "Press [Enter] key for next file..."
done

Don't embed terminal escapes in your strings

You don't know what terminal will be displaying your output - or even whether output will be going to a terminal.  The standard tool to get the appropriate strings (if they exist) is tput.  You'll want something like:

sgr0="$(tput sgr0)"
sgr93="$(tput sgr 93)"

which you can then use in your strings

echo -e isn't portable

Since you're using printf, you can conveniently use that when you don't want a newline:

printf "\n%s" "$sgr93*** Failed Login Checker ***$sgr0"

Pointless sleeping

sleep 2
read -r -p "Press [Enter] key to continue..."

These server only two purposes - annoy the user, and make it harder to use in a pipeline.

Unquoted variable expansion

Although the value you've given $HOLDINGDIR is currently safe to expand without quotes, I recommend you quote its expansion anyway, to give you the freedom to change it later (for example, you might want its value to incorporate $HOME instead of hard-coding /root).

No check that mkdir succeeded

If $HOLDINGDIR already exists as a regular file, or its parent directory doesn't exist or can't be written, then it won't be created, and we don't check that it succeeded.  The easy way to deal with this is to make any failed command abort the script:

set -x

Unbounded find

Do you really mean to search recursively for .bash_history files?  It probably makes more sense to look in each user's home directory only.  And not all home directories are necessarily under /home.  I'd be more inclined to iterate over awk -F: '{ print $6 "/.bash_history" }' /etc/passwd instead:

awk -F: '{ print $1, $6 "/.bash_history" }' /etc/passwd \
 | while read u f
   do if test -e "$f"
      then cp -v "$f" "$HOLDINGDIR/${u}_history"
      fi
   done

Replace copy+edit with a pipeline

You copy files and then process them with sed.  It's much better to simply have sed take its input from the original file and send its output to the new file, so instead of the cp above we can just:

sed -e 's/^#([0-9]+).*$/date -d @\1/e' "$f" >"$HOLDINGDIR/${u}_history"

and remove the following for loop.

And drop the chmod 755: you don't really want to make the transformed history files executable and readable by everybody.

Interactive reading of search term

read -r -e -p "Enter search term: " -i "sudo su" search

Again, this makes it hard to run from anything other than a terminal.  And if you mistype it, you have to start right from the beginning, seeking out all the history files again.  Instead, you could provide the terms as positional arguments, then you can

for search in "$@"
do grep -FiH -B1 "$search" "$HOLDINGDIR"/*_history
done

Even better, you could search for all the search terms together, remembering that grep -F accepts a newline-separated list of search terms:

grep -FiH -B1 "$(printf '%s\n' "$@")" "$HOLDINGDIR"/*_history

Modified program

#!/bin/bash

set -e

if [ "$#" = 0 ]
then
    echo "No search terms specified!" >&2
    exit 1
fi

HOLDINGDIR=/root/check_history
test -d "$HOLDINGDIR" || mkdir "$HOLDINGDIR"

awk -F: '{ print $1, $6 "/.bash_history" }' /etc/passwd \
 | while read u f
   do if test -e "$f"
      then sed -e 's/^#([0-9]+).*$/date -d @\1 "+#%c"/e' "$f" >"$HOLDINGDIR/${u}_history"
      fi
   done

grep -FiH -B1 "$(printf '%s\n' "$@")" "$HOLDINGDIR"/*_history