Nicer way to check if string contains all words from an array Currently I check whether a string contains all words from an array like this:

function contains($string, array $array) {
    $count = 0;
    foreach($array as $value) {
        if (false !== stripos($string,$value)) {
            ++$count;
        };
    }
    return $count == count($array);
}

Example call:

$string = 'Hello, people at Stackoverflow';
$array = array('Hello', 'People');

if (contains($string, $array)) {
    print 'This is true for the given example';
}

I wonder whether a more elegant, faster or even built-in way exists to improve this?

You could use something like this:

$string = 'Hello, people at Stackoverflow';
$array = array('Hello', 'People');
$i = count(array_intersect($array, explode(" ", preg_replace("/[^A-Za-z0-9' -]/", "", $string))));
echo ($i) ? "found ($i)" : "not found";

Outputs:

  found (1)

(array_intersect is case sensitive)

Or if you'd like to make it a function, then you could do something like this:

function contains($needles, $haystack) {
    return count(array_intersect($needles, explode(" ", preg_replace("/[^A-Za-z0-9' -]/", "", $haystack))));
}

$string = 'Hello, people at Stackoverflow';
$array = array('Hello', 'People');
$i = contains($array, $string);
echo ($i) ? "found ($i)" : "not found";


Throwing exceptions in a database wrapper I hope the extension methods ThrowArgumentNullExceptionIfNullOrEmpty and ThrowNullReferenceExceptionIfNullOrEmpty are straightforward.

I have a property in my class called ConnectionString. I can set this property through a method called SetConnectionString.

public static void SetConnectionString(string connectionString)
{
        _connectionString = connectionString.ThrowArgumentNullExceptionIfNullOrEmpty("connectionString", Localizer.GetString("0x000000004"));
}

My reason why I don't have

public static string ConnectionString { get; set; }

but

public static string ConnectionString { get { return _connectionString; } }

is because I want the property to be read-only and throw an ArgumentNullException if the argument in SetConnectionString is null or empty.

Since the class is static, it's possible for ConnectionString to be null (or an empty string if I do this: return _connectionString ?? "").

Part 1

In the code below I check the arguments for null or an empty string but to prevent an exception occurring when calling OpenConnection (see Part 2 for more on this method) I also check if the _connectionString (private variable of the ConnectionString property) is not null or empty (empty is for the internal connection string of the OleDbConnection object). Here I decided the exception should be a NullReferenceException because it's not directly an argument of the method.

Is this the correct approach to handle this situation? Consider that the code in between the checks might and might not have an impact on the application if the using fails.

public static int GetHighestID(string table, string column)
{
    _connectionString.ThrowNullReferenceExceptionIfNullOrEmpty(Localizer.GetString("0x000000000"));

    table.ThrowArgumentNullExceptionIfNullOrEmpty("table", Localizer.GetString("0x000000001"));

    column.ThrowArgumentNullExceptionIfNullOrEmpty("column", Localizer.GetString("0x000000002"));

    // Some other things are happening here ...

    using (var connection = new OleDbConnection(_connectionString))
    using (var command = new OleDbCommand(string.Format("SELECT MAX({0}) FROM {1}", column, table), connection))
    {
        OpenConnection(connection);

        return (int)command.ExecuteScalar();
    }
}

Part 2

The OpenConnection method as mentioned before is included below.

public static void OpenConnection(OleDbConnection connection)
{
    connection.ThrowArgumentNullExceptionIfNull("connection", Localizer.GetString("0x00000000C"));
    connection.ConnectionString.ThrowArgumentNullExceptionIfNullOrEmpty("connection.ConnectionString", Localizer.GetString("0x00000000F"));

    if (connection.State != ConnectionState.Open)
        connection.Open();
}

I also here check if the OleDbConnection object is not null and that the internal connection string is not null or an empty string. My intention is to always use this method when opening the connection with the database.

Again, is this the correct approach to handle this situation?

Are you sure you want your database wrapper to be static? What if you want to mock it in your unit tests?

I would make it a plain old class, pass the connection string through the constructor and do any kind of checks that I want right there, because that means your database wrapper won't be allowed to even exist without a valid connection string. With your current design, you're forced to check the connection string in each method, therefore duplicating code, and duplication is generally a no-no.

Static classes do have their use cases, but this is really not one of them. You usually want to write static classes to group methods that don't deal with any specific object, but here you are indeed dealing with something that, being a database, is about as specific as it gets.

Also, what's the GetHighestID method for?
I'm asking because I had to deal with codebases that, instead of letting the database do the auto-increment, would exactly do a SELECT MAX(Id) and add 1 before doing INSERTs.

In case that is what you're doing, then stop, because it defeats the purpose of having an auto-increment column at all. Also, the fact that the column is set to auto-increment is something your code shouldn't need to know about. As far as your code is concerned, all it needs to know is the name of the field that is the designated key for the table.
Plus, a database can handle auto-increments much better than you ever will, even more so when it comes to concurrent writes.

Last but not least, exceptions. Don't throw NullReferenceExceptions. That's the job of CLR, not yours. Now, with my design the need to throw it just goes away, but I just thought I would mention this. Throwing a NullReferenceException manually stinks.

Edit

  Well, the reason why I made it static is because the class has to be
  used in several classes. I have for ex. classes like Item, Person,
  Factory, ... which all have an interaction to the database. Each class
  has an Insert, Update, ... method. In my POV it looks easier to set
  the connection once e.g. while starting the application and then be
  able to use the class everywhere without having to initialize it every
  time.

You're right that it looks easier, but it really is not.

Let's examine the options we have here:

a static database wrapper, this would be your design
a non-static class
a non-static class implementing an interface

Option 2 would look like this:

class ClientClass
{
    private readonly Database _database;

    public ClientClass()
    {
        _database = new Database(ConnectionString);
    }
}

Now, the problem with options 1 and 2 is that, should you ever want to write unit tests for one of the classes that uses your database wrapper without actually issuing queries to the database, you would be out of luck.

And since you should be writing unit tests, you are indeed out of luck.

Why is that, though? Because you can't create a class that looks like your database wrapper but, for instance, returns the data you decide upon calling a hypothetical Select method.

Option 1 doesn't allow that because, even if you wrote such a class, you would have no way of substituting your FakeDatabase class without physically replacing FakeDatabase with Database in all the classes that use it. Static classes are initialized only once, so how are you going to swap the real implementation with a fake one?

Option 2 doesn't allow that either, because the name of your database wrapper is wired in the constructor of the class. There is still no way you can swap Database with FakeDatabasewithout touching the code.

Plus, option 2 is just bad because now your client classes need to know that a connection string exists at all, when they really shouldn't.

Option 3 is described down here:

  From what you say I understand that I should initialize the
  class each time and pass the connection string before calling the
  method. If not, what approach would you suggest?

That's correct. As @craftworkgames said, classes that need to communicate with the database get an instance of the database wrapper passed in, generally through the constructor.

To be even more precise, once your database wrapper is ready, you will want to extract an interface, let's call it IDatabase, that exposes all the wrapper's public method, and have your wrapper implement it.

Client classes, then, will have a constructor such as this:

class ClientClass
{
    private readonly IDatabase _database;

    public ClientClass(IDatabase database)
    {
         _database = database;
    }
}

Now you're golden, because if FakeDatabase implements IDatabase too, when writing a unit test you can pass in an instance of FakeDatabase with your client class none the wiser.

Client classes only need to know that the class you pass in exposes a certain set of methods. It does and should not know anything about how and from where that class pulls the data it returns, or where it writes to. Is it a RDBMS? Is it an XML file? Something else altogether? Client classes don't have to care about that, which is good, because it gives you the option to swap data stores without changing anything else.

Option 3 is called dependency injection. All the information you need is just a google away.

You could also read about:

programming to the interface, not the implementation
unit testing
mock objects

Lastly, this concept of making classes as unaware of the workings of other classes as possible, applies to every level of your architecture. A database layer just happens to be one of the prime examples.

Gosh, I hope all of that made sense.


Web API and Stored Procedures I am building a web service that gets data via Stored Procedures from a db and provides the result as JSON. The solution is built as a MVC 4 Web API project. I have to retrieve the data via Stored Procedures for several reasons (security, SQLAnywhere db, etc).

It is a relatively small scale project so I have opted for not defining a Model in the Web Service layer to save work (and possibly performance). There will likely be models in the front end application (which will be a MVC 4 Web Application).

The reasons why I am including an extra layer in the form of a Web Service is to a) add extra security, and b) that there will likely be a iPhone app for part of the application which then can utilize the same web service as the site.

It is working as I want it to, so what I want feedback on is

What do you think of the general approach to build a MVC 4 Web API solution without explicit models?
Do you have any comments on how to improve the details of the implementation?

One potential thought I have had is to skip the dataset and make a direct call to the Stored Procedure and loop over a datareader, which could potentially be faster. The other potential change is to loop over the datatable and build up the JSON manually instead of using the serializer.

What I like about the current solution is that it can be done on very few lines of code and that I do not have to manually name each JSON object since it is inherited from the datatable within the dataset.

I define a dataset called dsProducts that is loaded with a TableAdapter.

    // GET api/products
    public string Get()
    {

        string result = "";

        dsProductTableAdapters.GetProductsTableAdapter taTemp = new dsProductsTableAdapters.GetProductsTableAdapter();
        dsProducts dsTemp = new dsProducts();

        try {
            taTemp.Fill(dsTemp.GetProducts);
            result = jsonHelper.convertJSONString(dsTemp.GetProducts);
        } catch (Exception ex) {
            result = "{\"error\":\"1\"}";
        }

        return result;

    }

The convertJSONString and its help functions are defined as:

    public static string convertJSONString(DataTable table)
    {
        JavaScriptSerializer serializer = new JavaScriptSerializer();
        return serializer.Serialize(table.ToDictionary());
    }

    static Dictionary<string, object> ToDictionary(this DataTable dt)
    {
        return new Dictionary<string, object> {
            { dt.TableName, dt.convertTableToDictionary() }};
    }

    static object convertTableToDictionary(this DataTable dt)
    {
        var columns = dt.Columns.Cast<DataColumn>().ToArray();
        return dt.Rows.Cast<DataRow>().Select(r => columns.ToDictionary(c => c.ColumnName, c => r[c]));
    }

General approach to build Web API without models, doesn't give the full web API capabilities.

E.g:

Help: You cannot have Web API help generated automatically.
You cannot return error handling scenarios: status, message

If you don't want to create the DTOs (Models), you can use the dynamic types. JSON formatter works well with the dynamic types.


Symfony rest endpoint for get a number products of one user I want to improve quality of this symfony rest endpoint (solid principle, Kiss, best practice...) Can you review my code please?

Symfony controllController function that return json list of product

/**
 * @Route("/api/products/my_list/{number}", methods={"GET"})
 * @param Security $security
 * @param BeamyAPI $beamyAPI
 * @param Request $request
 * @param ProductService $productService
 * @return string
 */
public function myList(
    Security $security,
    BeamyAPI $beamyAPI,
    Request $request,
    ProductService $productService,
    int $number = 20
)
{
    $productListUser = $this->em->getRepository(ProductAdmin::class)
                             ->findProductsUser(
                                 $security->getUser(),
                                 $number
                        );
    return new JsonResponse(
        $productListUser,
        Response::HTTP_OK
    );
}

Trait to format array response

Trait ArrayFormat
{
    /**
     * format array for user product endpoint
     *
     * @param array $data
     * @return array
     */
    public function formatUserProduct($product) : array
    {
       return [
            'id' => $product['id'],
            'name' => $product['name'],
            'notifications' =>  '',
            'logo' => [
                'contentUrl' => $product['logo']['contentUrl']
            ]
        ];
    }
}

Repository permit to get array of products list for one user

/**
     * get product list of user
     *
     * @param UserInterface $user
     * @param integer $number
     * @return array|null
     */
    public function findProductsUser(UserInterface $user, int $number) :?array
    {

        $listProductUser = $this->em->getRepository(ProductAdmin::class)->findBy(
            ['user' => $user],
            ['product' => 'ASC'],
            $number
        );

        $res = [];
        array_walk($listProductUser, function(&$productUser){
            $product = $this->productService->getProductInfo($productUser->getProduct());
            $productUser =  ArrayFormat::formatUserProduct($product);
        });

        return $listProductUser;
    }

Thanks

$productListUser = $this->em->getRepository(ProductAdmin::class)
                             ->findProductsUser(...

Than

public function findProductsUser(UserInterface $user, int $number) :?array
    {

        $listProductUser = $this->em->getRepository(ProductAdmin::class)->findBy(...

seems to me that findProductsUser is method of whatever is returned by $em->getRepository(ProductAdmin::class) why is that method retrieving itself from entity manager? Shouldn't it be just $this->findBy(...?

Further, the responsibility of formatting the entity should not be done by the repository. You need to either wrap the repository with the formatter into another class, or do it in the controller. Also there is no reason why the formatter must be a trait. Make it a service and inject it to the entire controller through contructor if all methods of the controller need it, or have it injected in an action method, like you do with Security, BeamAPI, etc...


Random Number Generator Class I've written an abstract class in C# for the purpose of random number generation from an array of bytes. The .NET class RNGCryptoServiceProvider can be used to generate this array of random bytes, for example.

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

namespace MyLibrary
{
    /// <summary>
    /// Represents the abstract base class for a random number generator.
    /// </summary>
    public abstract class Rng
    {
        /// <summary>
        /// Initializes a new instance of the <see cref="Rng"/> class.
        /// </summary>
        public Rng()
        {
            //
        }

        public Int16 GetInt16(Int16 min, Int16 max)
        {
            return (Int16)(min + (Int16)(GetDouble() * (max - min)));
        }

        public Int32 GetInt32(Int32 min, Int32 max)
        {
            return (Int32)(min + (Int32)(GetDouble() * (max - min)));
        }

        public Int64 GetInt64(Int64 min, Int64 max)
        {
            return (Int64)(min + (Int64)(GetDouble() * (max - min)));
        }

        public UInt16 GetUInt16(UInt16 min, UInt16 max)
        {
            return (UInt16)(min + (UInt16)(GetDouble() * (max - min)));
        }

        public UInt32 GetUInt32(UInt32 min, UInt32 max)
        {
            return (UInt32)(min + (UInt32)(GetDouble() * (max - min)));
        }

        public UInt64 GetUInt64(UInt64 min, UInt64 max)
        {
            return (UInt64)(min + (UInt64)(GetDouble() * (max - min)));
        }

        public Single GetSingle()
        {
            return (Single)GetUInt64() / UInt64.MaxValue;
        }

        public Double GetDouble()
        {
            return (Double)GetUInt64() / UInt64.MaxValue;
        }

        public Int16 GetInt16()
        {
            return BitConverter.ToInt16(GetBytes(2), 0);
        }

        public Int32 GetInt32()
        {
            return BitConverter.ToInt32(GetBytes(4), 0);
        }

        public Int64 GetInt64()
        {
            return BitConverter.ToInt64(GetBytes(8), 0);
        }

        public UInt16 GetUInt16()
        {
            return BitConverter.ToUInt16(GetBytes(2), 0);
        }

        public UInt32 GetUInt32()
        {
            return BitConverter.ToUInt32(GetBytes(4), 0);
        }

        public UInt64 GetUInt64()
        {
            return BitConverter.ToUInt64(GetBytes(8), 0);
        }

        /// <summary>
        /// Generates random bytes of the specified length.
        /// </summary>
        /// <param name="count">The number of bytes to generate.</param>
        /// <returns>The randomly generated bytes.</returns>
        public abstract byte[] GetBytes(int count);
    }
}

Any suggestions for improvements would be welcome.

I think the design is pretty good. A few comments:

I'd rename the class to something a bit more descriptive, say RandomGenerator. Then when you implement the class you can declare it with CspGenerator: RandomGenerator or MersenneGenerator: RandomGenerator and it's obvious what the class does.
Comment the get() methods. IMO all public elements should be documented. Get/set could be left out, but that is a matter of preference. In particular I'd like to know what kind of range min anf max is and is used for.
Is getBytes() needed externally? If not, I would consider making it class-level rather than public.

The formatting is good - even in Visual Studio I've seen it get messed up as code is refactored and changed.


Koch snowflake in Python with Turtle After finding a piece of code on StackOverflow that drew the Koch snowflake fractal, I made a ton of modifications to it and used it to produce an animation divided in two parts:

Constant size, recursion depth increasing.
Constant recursion depth, size increasing.

You can see the animation here.

I am interested mainly in:

Have I got too many constants? I like them because they make the program easy to customize, but 12 constants looks like a lot.
Any idea to avoid repetition in these blocks of code?

write_as_title(\
    "Constant size, recursion depth increasing.", font=FONT)
time.sleep(WRITING_SLEEPING_INTERVAL)

for deepness in range(MAX_DEPTH):
    costum_reset()
    snowflake(deepness, SIZE_WHEN_CONSTANT_DEPTH)
    time.sleep(DRAWING_SLEEPING_INTERVAL)

costum_reset()
write_as_title(\
    "Constant recursion depth, size increasing.", font=FONT)
time.sleep(WRITING_SLEEPING_INTERVAL)
for size in RANGE_OF_SIZES:
    costum_reset()
    snowflake(DEPTH_WHEN_CONSTANT_SIZE, size)

They are similar but I cannot quite abstract the similarity out.

from __future__ import division

import turtle
import time

WIDTH, HEIGHT = 800, 600
FONT = ("Arial", 25, "normal")
DRAWING_SLEEPING_INTERVAL = 0.5
WRITING_SLEEPING_INTERVAL = 2.5
MAX_DEPTH = 5
SIZE_WHEN_CONSTANT_DEPTH = 300
DEPTH_WHEN_CONSTANT_SIZE = 4
RANGE_OF_SIZES = range(0, 5000, 5)
SIDES_OF_AN_EQUILATERAL_TRIANGLE = 3
EQUILATERAL_TRIANGLE_INTERNAL_ANGLE = 120
DRAWING_START_X = - 200
DRAWING_START_Y = 150

def snowflake(n, size=300):

    for _ in range(SIDES_OF_AN_EQUILATERAL_TRIANGLE):
        snowflake_edge(n, size)
        turtle.right(EQUILATERAL_TRIANGLE_INTERNAL_ANGLE)

    turtle.update()

def snowflake_edge(n, size=300):
    if n == 0:
        turtle.forward(size)
        return

    for movement in (lambda: turtle.left(EQUILATERAL_TRIANGLE_INTERNAL_ANGLE // 2),
                   lambda: turtle.right(EQUILATERAL_TRIANGLE_INTERNAL_ANGLE),
                   lambda: turtle.left(EQUILATERAL_TRIANGLE_INTERNAL_ANGLE // 2)):
        snowflake_edge(n - 1, size / SIDES_OF_AN_EQUILATERAL_TRIANGLE)
        movement()

    snowflake_edge(n - 1, size / SIDES_OF_AN_EQUILATERAL_TRIANGLE)

def costum_reset():
    turtle.reset()
    turtle.penup()
    turtle.setx( DRAWING_START_X )
    turtle.sety( DRAWING_START_Y )
    turtle.pendown()

def write_as_title(title, font=FONT):
    turtle.setx(- (WIDTH // 2.5) )
    turtle.sety(HEIGHT // 3)
    turtle.write(title, font=font)

if __name__ == "__main__":
    turtle.setup(width=WIDTH, height=HEIGHT)
    turtle.hideturtle()
    turtle.tracer(0, 0)

    write_as_title(\
        "Constant size, recursion depth increasing.", font=FONT)
    time.sleep(WRITING_SLEEPING_INTERVAL)

    for deepness in range(MAX_DEPTH):
        costum_reset()
        snowflake(deepness, SIZE_WHEN_CONSTANT_DEPTH)
        time.sleep(DRAWING_SLEEPING_INTERVAL)

    costum_reset()
    write_as_title(\
        "Constant recursion depth, size increasing.", font=FONT)
    time.sleep(WRITING_SLEEPING_INTERVAL)
    for size in RANGE_OF_SIZES:
        costum_reset()
        snowflake(DEPTH_WHEN_CONSTANT_SIZE, size)

Your code reads well and is easily understandable. On top of also finding that you might have too much constant (more on that in a little bit), I just have a few nitpicks:

whitespace in expressions is weird sometimes;
a docstring would help understanding the recursion involved in snowflake_edge;
you don't need the newline continuation (\) inside parenthesis, they already act as an implicit one;
you allow to parametrize write_as_title with a custom font but nothing similar is done with other kind of parameters.

On reducing the similarities between the two drawings
I would have put a call to costum_reset (costum? really?) at the beginning of both write_as_title and snowflake since both drawings need a clear state to begin with.
I would also put time.sleep(WRITING_SLEEPING_INTERVAL) at the end of write_as_title. Considering its value, it won't hurt anyone in an interactive session and is still needed in a program for people to be able to read the text.
This way, the main part would look like:
if __name__ == "__main__":
    turtle.setup(width=WIDTH, height=HEIGHT)
    turtle.hideturtle()
    turtle.tracer(0, 0)

    write_as_title(
        "Constant size, recursion depth increasing.", font=FONT)
    for deepness in range(MAX_DEPTH):
        snowflake(deepness, SIZE_WHEN_CONSTANT_DEPTH)
        time.sleep(DRAWING_SLEEPING_INTERVAL)

    write_as_title(
        "Constant recursion depth, size increasing.", font=FONT)
    for size in RANGE_OF_SIZES:
        snowflake(DEPTH_WHEN_CONSTANT_SIZE, size)

You can even make the two sensible parts two functions if you want to separate the concerns better.
On reducing the amount of constants
Of your 12 constants, a few are really constant, some are arbitrary fixed for the purpose of your application, and some could be tweaked to achieve a better (prettier?) result.
Maybe that allowing to change the latter group using the command line could be more intuitive to tweak the parameters of the display. I personally would keep the following constants:

WIDTH, HEIGHT = 800, 600
FONT = ("Arial", 25, "normal")
WRITING_SLEEPING_INTERVAL = 2.5
SIDES_OF_AN_EQUILATERAL_TRIANGLE = 3
EQUILATERAL_TRIANGLE_INTERNAL_ANGLE = 120
DRAWING_START_X = - 200
DRAWING_START_Y = 150

And use argparse to provide default values or custom ones from the command line for:

DRAWING_SLEEPING_INTERVAL
MAX_DEPTH
SIZE_WHEN_CONSTANT_DEPTH
DEPTH_WHEN_CONSTANT_SIZE
SIZES_LIMIT

Note the use of SIZES_LIMIT which will then create the range object RANGE_OF_SIZES.
All of these last "constants" are only used within the if __name__ == '__main__' so it's pretty easy to adapt.


A WinAPI C++ program for printing the master boot record of the hard drive I have rolled this program for reading the MBR:

#define WIN32_LEAN_AND_MEAN

#include <Windows.h>
#include <cctype>
#include <iostream>
#include <string>
#include <utility>

using std::isprint;
using std::cout;
using std::cin;
using std::move;
using std::size_t;
using std::string;

constexpr size_t MBR_SIZE = 512;
constexpr size_t BYTES_PER_LINE = 4;
constexpr size_t NUMBER_OF_LINES = MBR_SIZE / BYTES_PER_LINE;

// Converts 4 least significant bits of 'c' to corresponding
// hexadecimal string.
static string fourBitsToString(char c)
{
    string s = " ";

    if (c >= 0 && c <= 9) {
        s[0] = '0' + c;
    } else {
        c -= 10;
        s[0] = 'A' + c;
    }

    return move(s);
}

// Converts a character to its hexadecimal representation.
static string charToHex(char c)
{
    char lo = c & 0xf;
    char hi = (c >> 4) & 0xf;
    string s;
    char chars[] = {hi, lo};

    for (char ch : chars) {
        s += fourBitsToString(ch);
    }

    return std::move(s);
}

// Prints the MBR to console.
static void PrintMBR(char buffer[MBR_SIZE])
{
    size_t byteIndex = 0;
    string lineSeparator;
    string columnSeparator;

    for (size_t i = 0; i < NUMBER_OF_LINES; i++) {
        cout << lineSeparator;
        lineSeparator = '\n';
        columnSeparator = "";

        for (size_t j = 0; j < BYTES_PER_LINE; j++) {
            string ch = charToHex(buffer[byteIndex++]);
            cout << columnSeparator << ch;
            columnSeparator = " ";
        }

        cout << ' ';

        for (size_t j = 0; j < BYTES_PER_LINE; j++) {
            char c = buffer[byteIndex - BYTES_PER_LINE + j];
            cout << (isprint((unsigned int) c) ? c : '.');
        }
    }
}

// Reads the entire master boot record (MBR) into lpBuffer.
// Expects lpBuffer to point to a memory point that may
// hold at leasst 512 bytes.
static DWORD ReadMBR(PCHAR lpBuffer)
{

    HANDLE diskHandle = CreateFile(
        TEXT("\\\\.\\PhysicalDrive0"),
        GENERIC_READ,
        FILE_SHARE_READ | FILE_SHARE_WRITE,
        NULL,
        OPEN_EXISTING,
        0,
        NULL);

    char* buffer = new char[MBR_SIZE];

    ReadFile(
        diskHandle,
        lpBuffer,
        MBR_SIZE,
        NULL,
        NULL);

    return GetLastError();
}

int main() {
    char buffer[MBR_SIZE];
    ReadMBR(buffer);
    PrintMBR(buffer);
    cin.get();
    return 3;
}

(Compile as Release instead of Debug if it throws a nasty runtime breakpoint.)

I would love to hear any comments possible, but my main question is whether it is  idiomatic C/C++/WinAPI.

Note: To see the actual MBR, run this as administrator.

Idiomatic is relative. I guess it's a coding style you could find in older code-bases (and many winapi based applications are rather old), but you wouldn't write C++ like this today. Consider for instance:

#include <iostream>
#include <iomanip>
#include <iterator>
#include <algorithm>
#include <numeric>
#include <array>

constexpr std::size_t MBR_SIZE = 255;
constexpr std::size_t BYTES_PER_COLUMN = 4;
constexpr char LINE_DELIMITER = '\n';
constexpr const char* COL_DELIMITER = " ";

int main() {
    std::array<unsigned char, MBR_SIZE> mbr;
    std::iota(std::begin(mbr), std::end(mbr), 0); // filling it for testing purposes

    std::cout << std::hex << std::setfill('0');
    std::size_t index = 0;
    std::transform(std::begin(mbr), std::end(mbr), std::ostream_iterator<int>(std::cout, COL_DELIMITER), [index](auto c) mutable {
        if (index++ % BYTES_PER_COLUMN == 0) std::cout << LINE_DELIMITER;
        std::cout << std::setw(2); // setw must be applied for each output
        return c;
    });
    std::cout << std::endl;
}

In a dozen of lines you get the same result as printMBR and its two auxiliary functions (at least I believe so, I haven't had the chance to run your code), which are longer and more complicated.

To be more specific about your code:

don't declare a variable before you're ready to define it, and don't define it before you're ready to use it.
don't std::move your return value, it prevents copy elision
don't declare free functions static, it's a java thing
inside ReadMBR there's a char* buffer = new char[MBR_SIZE]; you don't use afterwards
I'm not a fan of that many using directives; prefix the names with std:: and be done with it, unless you prefer to set up aliases: then using string = std::string is clearer
use standard algorithms, such as std::copy, std::accumulate, etc (in headers <algorithm> and <numeric>) or range-base for loops (for auto item : sequence) instead of "raw" loops. For instance, charToHex could be written

like this:

std::string s;
for (auto nibble : { (c >> 4) & 0xf, c & 0xf })
    s += fourBitsToString(nibble);
return s;

or:

char lo = c & 0xf;
char hi = (c >> 4) & 0xf;
char chars[] = {hi, lo};

return std::accumulate(std::begin(chars), std::end(chars), std::string(), [](auto init, auto elem) {
    return init += (fourBitsToString(elem));
});

And, on a more aesthetic note, a four-bits aggregate can be called a half-byte or a nibble.


Clearing duplicates from an Excel 2007 sheet This is the original code I had for removing duplicates from a sheet:

Sub Remove_Duplicates()
'
Application.ScreenUpdating = False

    ActiveSheet.Range("$A$1:$Z$2000").RemoveDuplicates Columns:=Array(5, 10, 11, 12, 13, 14, 15, 16), Header:=xlYes

Range("C" & Rows.Count).End(xlUp).Offset(1).Select

Application.ScreenUpdating = True

Call Hide_Columns_Swivel

End Sub

I added a section to this code (see below) that clears contents of fake empty cells. But now the code takes up to 15 seconds to run. It works as it is suppose to, just really slowly.

Sub Remove_Duplicates()
'
Application.ScreenUpdating = False

Dim usedrng As Range

    ActiveSheet.Range("$A$1:$Z$2000").RemoveDuplicates Columns:=Array(5, 10, 11, 12, 13, 14, 15, 16), Header:=xlYes

    For Each usedrng In ActiveSheet.UsedRange
        If usedrng.Value = "" Then
            usedrng.ClearContents
        End If
    Next

Range("C" & Rows.Count).End(xlUp).Offset(1).Select

Application.ScreenUpdating = True

Call Hide_Columns_Swivel

End Sub

I need help making this more efficient as I do not know enough about VBA to do that. All assistance is greatly appreciated.

If you are trying to get zero-length strings left after reverting formulas (that may have resulted in "" ) to their values, the quickest way is to spin through the columns and apply a quick Text-to-Columns, Fixed Width, Finish to each.

Dim c As Long
With worksheets("Sheet1")
    With .Range("A1").CurrentRegion  `<~~ set to the desired range of one or more columns
        For c = 1 To .Columns.Count
            .Columns(c).TextToColumns Destination:=.Columns(c), _
                DataType:=xlFixedWidth, FieldInfo:=Array(0, 1)
        Next c
    End With
End With

This process will take zero-length strings and make then truly blank cells. This is an important step in preppinga worksheet for direct non-CSV import to MS-SQL.


Reading and Converting XML File to Array I'm using WordPress ( PHP Based Platform ) and I'm trying to read an external XML File, convert it into a usable array, then compare two arrays - one from the xml file and another from an options database. I've coded it so that it does work but it just looks messy. I'm not sure if it's my naming conventions or how I'm formatting it but to me it looks lengthy and messy. I'm open to any / all suggestions on how to clean this up or make this function better.

The following function checks an XML file of private plugins then cross references it with the list of active plugins to make sure it's there.

/**
 * Get a list of Private Plugins from XML File
 * @return bool|array $plugins( 'name', 'version', 'path', 'slug', 'updated' )
 */
function getPrivate_plugins() {
    $plugins             = false;
    $active_plugins      = get_option( 'active_plugins' );
    $private_remote_path = 'http://domain.com/plugins.xml';
    $ci                  = curl_init( $private_remote_path );
    curl_setopt( $ci, CURLOPT_RETURNTRANSFER, 1 );
    $plugin_list         = curl_exec( $ci );
    curl_close( $ci );

    if( ! empty( $plugin_list ) ) {
        $plugin_obj   = simplexml_load_string( $plugin_list );
        $plugin_json  = json_encode( $plugin_obj );
        $plugin_array = json_decode( $plugin_json, TRUE );
        $plugin_array = ( ! empty( $plugin_array ) && isset( $plugin_array['plugin'] ) ) ? $plugin_array['plugin'] : '';

        if( ! empty( $plugin_array) ) {
            $plugin_array = array_map(
                function( $arr ) {
                    return ( isset( $arr['path'] ) ) ? $arr['path'] : '';
                },
                $plugin_array
            );
            $plugins = array_intersect( array_filter( $plugin_array ), $active_plugins );
            $plugins = ( ! empty( $plugins ) ) ? $plugins : false;
        }
    }

    return $plugins;
}

The only WordPress based function is get_option() which return to $active_plugins an array of paths [0] => 'plugin-directory/plugin-file.php'

The XML File it's reading is simple:

<?xml version="1.0" encoding="UTF-8"?>
<plugins>
    <plugin>
        <name>Private Plugin A</name>
        <slug>private-plugin-a</slug>
        <path>private-plugin-a/private-plugin-a.php</path>
        <version>1.1</version>
        <updated>Feb 23, 2015 03:26pm</updated>
    </plugin>
    <plugin>
        <name>Private Plugin B</name>
        <slug>private-plugin-b</slug>
        <path>private-plugin-b/private-plugin-b.php</path>
        <version>1.2</version>
        <updated>Feb 24, 2015 12:26pm</updated>
    </plugin>
</plugins>

Types

Your code would be easier to read if one variable could only be one type. $plugins is initiated as a boolean, but can also be an array. But what would a boolean value even mean? $plugins = false; doesn't really make sense.

If there are no plugins, your code should just return an empty array. If there are errors loading the plugins, you could return false (but don't use the $plugin variable for that), or ideally throw an exception if you are using PHP5.

Don't reuse variables

You shouldn't reuse variables for completely different things. For example $plugin_array seems to be holding a lot of different data, which is quite confusing.

This for example:

    $plugin_array = json_decode( $plugin_json, TRUE );
    $plugin_array = ( ! empty( $plugin_array ) && isset( $plugin_array['plugin'] ) ) ? $plugin_array['plugin'] : '';

    if( ! empty( $plugin_array) ) {
        // do stuff
    }

Would be better as:

    $plugin_array = json_decode( $plugin_json, TRUE );
    $is_valid_data = ! empty( $plugin_array ) && isset( $plugin_array['plugin'] );
    $plugin = $plugin_array['plugin'];

    if( $is_valid_data ) {
        // do stuff
    }

Or you could just save the $is_valid_data variable by putting the expression inside the if directly.

Return early

Generally, your code will get a lot more readable if you handle simple checking at the top of your function, and return early. This will also reduce the level of nesting. eg:

if( empty( $plugin_list ) ) {
    return array();
}

And the same with $plugins = ( ! empty( $plugins ) ) ? $plugins : false;, which could just be:

if ( empty( $plugins ) ) {
    return array();
}

Or just ignore it, as there doesn't seem to be an error to handle (it's already empty, and returning an empty array if there are no plugins seems appropriate). Same goes for if( ! empty( $plugin_array) ) {.


Find Todos by Line Number (or any other reference) in Python - Revised as functions A new function based version of the code from this question as suggested by @Hlib Babii

I write a lot of TODO's but I never keep track of where they are.

This program will search through files (and recursively through folders if needed) and find any TODO comments and their line number.

The details will be printed to the screen and a file will be generated in the folder which is a csv of the files and TODO details.  This means there is always a local list of TODOs in the folder and this can be stroed in git..

You can choose different filetypes, comments and TODO phrases to suit , so it is not just for python.

GitHub

from os.path import exists
import sys
from glob import glob

def _get_files(path, ext, recursive=False):
    # return a list of files
    return glob(path + f"/**/*{ext}", recursive=recursive)

def _find_todos_in_file(fn, todo_token, comment_start):
    # return a list of todos in the file
    temp_todos = []
    with open(fn, "r") as input_:
        for line_no, line in enumerate(input_):
            if comment_start in line and todo_token in line:
                # check to make sure that it is a true comment and not a variable name.
                # Avoid false positives like :: `TODOs.append(todo) # there are no todos in this line`
                comment_index = line.find(comment_start)
                todo_index = line.find(todo_token)
                if todo_index > comment_index:
                    temp_todos.append([fn, f"Line - {line_no+1}  ::  {line.strip()}"])
    return temp_todos

def find_todos(path, ext, todo_token = 'TODO', comment_start = '#', recursive=False):
    # returns a dictionary of todos
    todos = {}
    files = _get_files(path, ext,recursive=recursive)
    if exists(path):
        for x in files:
            try:
                print(f"Searching  ::  {x}")
                result = _find_todos_in_file(x, todo_token, comment_start)
                if result:
                    todos[x] = result
            except PermissionError:
                pass # not a ext file (possible a folder)
    else:
        raise OSError("Path does not exist.")
    return todos

def show_todos(todos: dict):
    # show todos
    line = "-" * 100
    for k, v in todos.items():
        print(f"\n{line}\n\n{k}")
        for x in v:
            print(f">>>{x[1]}")

def save_csv(todos, ext):
    # save todos to a csv file
    import csv
    for k, v in todos.items():
        k = k.split(ext)[0][:-1]
        with open(f"{k}-TODOS.csv", "w", newline="") as csvfile:
            w = csv.writer(csvfile, delimiter=",", quoting=csv.QUOTE_MINIMAL)
            for row in v:
                w.writerow(row)

if __name__ == "__main__":
    todos = find_todos('.', ext='py', comment_start='#', recursive=True)
    show_todos(todos)
    save_csv(todos, 'py')

Overall, pretty good job.  Here are some observations:

_get_files

path + f"/**/*{ext}"

Might cause trouble if path ends in a / and may be unsafe.  If a user inputs an empty path, this could start at the root of the file system, and try to scan every file.  Consider checking for such things and maybe using os.path.normpath or path.resolve() in pathlib.  pathlib also provides exists() and glob().

find_todos()

if exists(path) should probably come before it is used in the call to _get_files():

if exists(path):
    files = _get_files(path, ext,recursive=recursive)
    ...

A PermissionError might occur when a filed is opened, so I would tend to put the try:...except PermissionError: block in _find_todos_in_file()

_find_todos_in_file()

Each line is scanned at least twice, and maybe four times:

if comment_start in line and todo_token in line: # two scans
        ....
    comment_index = line.find(comment_start)     # one scan
    todo_index = line.find(todo_token)           # one scan

line.find(comment_start) returns -1 if the comment_start isn't in the line.  Also, find() takes a parameter that says where to start searching.  So the above code can be simplified and made more efficient like so:

comment_index = line.find(comment_start)
if comment_index >= 0:
    todo_index = line.find(todo_token, comment_index)

    if todo_index >= 0:
        temp_todos.append((line_no+1, line.strip))

I changed what gets appended.  The file name is redundant, it gets passed in as an argument, so the caller already knows what it is -- find_todos() uses it as a dict key.  And I put line_no and the text in a tuple rather than format them into a string.  Putting them in a string seems more like a job for an output function.  And keeping them as a tuple is more more flexible (e.g. you want to put them in a database, or see if TODOs show up more near the end of a file, etc).

other stuff

find_todos() and find_todos_in_file() basically iterate over all the lines in a bunch of files.  This is what the fileinput library does.  These two function could be combined and rewritten something like this:

def find_todos(path, ext, todo_token, comment_start, recursive):
    todos = defaultdict(list)

    files = glob("{path}/**/*{ext}", recursive=recursive)

    with fileinput.input(files=files) as f:
        for line in f:
            if f.isfirstline():
                filename = f.filename()
                print(f"Searching: {filename}")

            comment_index = line.find(comment_start)
            if comment_index >= 0:
                todo_index = line.find(todo_token, comment_index)

                if todo_index >= 0:
                    todos[filename].append((f.filelineno(), line.strip))

For a simple command line tool, the argparse library can help with parsing command parameters and options.  For more complicated or enhanced tools try a command line tool library like Click or Python Prompt.

def make_arg_parser():
    import argparse

    parser = argparse.ArgumentParser(
                        description='TODO finder/lister.')

    parser.add_argument('path',
                        default='.',
                        help="where to search for TODO's")
    parser.add_argument('ext',
                        default='py',
                        help="kind of files to search")
    parser.add_argument('comment',
                        default='#',
                        help="line comment marker")
    parser.add_argument('todo',
                        default='TODO',
                        help="text to mark TODO items")
    parser.add_argument('--norecurse',
                        action='store_false',
                        dest='recursive',
                        help="don't recurse into subdirs")
    parser.add_argument('--csv',
                        action='store_true',
                        help="save to a csv file")

    return parser

if __name__ == "__main__":
    cli_parser = make_arg_parser()
    args = cli_parser.parse_args()

    todos = find_todos(args.path,
                       ext=args.ext,
                       todo_token=args.todo,
                       comment_start=args.comment,
                       recursive=args.recursive)
show_todos(todos)

if args.csv:
    save_csv(todos, 'py')

The above is just an example.  It mirrors the call to find_todos() and adds an argument for todo_token and a flag to save to a csv file.


Finding the 10001st prime I'm solving Project Euler problem 7, which says:

  By listing the first six prime numbers: 2, 3, 5, 7, 11, and 13, we can see that the 6th prime is 13. What is the 10001st prime number?

Here's the code I've written:

def primes(n):
    primes = []
    attempt = 3
    while len(primes) < (n-1):
        for i in range(len(primes)):
            if attempt % primes[i] == 0:
                attempt += 2
                break
        else:
            primes.append(attempt)
            print (primes)
    return (primes)

While testing a number, if it finds that the number is divisible by one of the primes in the list, the for loop breaks, and starts again with a larger number. If it isn't divisible, it's added to the list of primes. This keeps going until the list is as large as desired (in this case 10000, and I've omitted 2 so the last member of the list is the 10001st prime)

The problem is that this is extremely slow. I've seen other solutions that can apparently solve this in seconds, if not less. What are some ways I could make this run faster?

First off, don't print every single loop. It's wasteful and surprisingly intensive. Also, you don't need to return all primes, your brief only requires one. You should make it more clear what the function is for. Return the last found prime, probably also rename your function to prime.

Don't omit 2, it's only a strange and confusing work around that you haven't even explained in the code. Just include it in the initialisation of primes.

primes = [2]

Also you don't need to do

for i in range(len(primes)):
    if attempt % primes[i] == 0:

Python has a simpler for var in iterable syntax that lets you get the values directly, which is more efficient.

for i in primes:
    if attempt % i == 0:

But rather than using a for loop at all you can use all. It employs short circuiting and will immediately stop iterating over the loop when it finds any value that has an invalid factor. This is faster than breaking when a prime is found, as primes are much rarer than non primes especially when you get into the high values.

def primes(n):
    primes = [2]
    attempt = 3
    while len(primes) < n:
        if all(attempt % prime != 0 for prime in primes):
            primes.append(attempt)
        attempt += 2
    return primes[-1]

Note, you also forgot to increment attempt when a prime was found. That means that every time you found a prime, it then had to be checked yet again in a very costly way since only the last element in the list would invalidate it.


Basic C++ tone generator I'm trying to learn C++ (with experience in C and Java, inter alia).
I wrote a program to output a waveform of multiple (superimposed) tones, each varying in pitch.

To execute:

Compilation: g++ -W -Wall -pedantic -std=c++11 main.cpp Tone.cpp
Running: ./a.out
Playing: aplay out -r 44100 -f S16_LE (or S16_BE if you're on a big-endian system)

What can I improve in terms of C++ style and idioms?
I tried to use RAII for my streams and collections, and I tried to avoid news and pointers.

A couple specific questions:

In write_sample, are the cast to (char *) and the sizeof divisions okay?
If not, what's the standard C++ way to write binary data to a file?
Is sizeof really even used in C++, or is this a discouraged C artifact?
What should be const vs. constexpr?
I used a C-style array for octaves in main, instead of std::array<double, 7>, because I like the automatic initializer counting and the iteration is easier.
Is this okay? What would be better?
Does C++ use size_t, int16_t, and the like?

Tone.hpp

#ifndef TONE_H
#define TONE_H

class Tone
{
    private:
        const double sampleRate;
        double s; /* current position along the unit sine curve */
        double lastSample;
    public:
        double getSample() const;
        double nextSample(double frequency, double amplitude);
        Tone(double sampleRate);
};

#endif

Tone.cpp

#include <cmath>
#include "Tone.hpp"

Tone::Tone(double sampleRate) : sampleRate(sampleRate), s(0), lastSample(0)
{
}

double Tone::getSample() const
{
    return this->lastSample;
}

double Tone::nextSample(double frequency, double amplitude)
{
    this->s += frequency / this->sampleRate;
    this->lastSample = amplitude * sin(2*M_PI * this->s);
    return this->lastSample;
}

main.cpp

#include <cmath>
#include <vector>
#include <iostream>
#include <fstream>
#include "Tone.hpp"

void write_sample(std::ostream &stream, double sample);

int main()
{
    constexpr double SAMPLE_RATE = 44100;
    std::vector<Tone> tones;

    // Each of these represents a final octave difference from A4
    const double octaves[] = { -2, 0, 4./12, 7./12, 1, 16./12, 19./12 };
    const size_t count = sizeof(octaves) / sizeof(*octaves);

    std::ofstream outfile("out", std::ofstream::binary);

    // Create a tone generator for each
    for (size_t i = 0; i < count; i++)
    {
        tones.push_back(Tone(SAMPLE_RATE));
    }

    // Set parameters for the output sound
    double duration = 4.0;
    double attack = 1.0;
    double decay = 2.5;
    double amplitude = 0.8;

    // Generate the required samples
    for (double t = 0; t < duration; t += 1 / SAMPLE_RATE)
    {
        // Interpolate with sinusoidal easing up to the attack time, then constant
        double interp = t >= attack ? 1 : sin(M_PI/2 * t);

        // Constant until the decay time, then interpolate linearly out
        double strength = t < (duration - decay) ? 1
            : 1 - (t - (duration - decay)) / decay;

        // Superimpose the samples from each tone generator
        double totalSample = 0;
        for (size_t i = 0; i < count; i++)
        {
            double octave = interp * octaves[i];
            double frequency = 440 * pow(2, octave);
            double sample = tones[i].nextSample(frequency, strength * amplitude);
            totalSample += sample;
        }
        totalSample /= count;

        if (outfile.good())
        {
            write_sample(outfile, totalSample);
        }
        else
        {
            return 1;
        }
    }

    return 0;
}

// Writes a sample to the given output stream as a 16-bit signed integer.
void write_sample(std::ostream &stream, double sample)
{
    int16_t output = (int16_t) (0x7FFF * sample);
    stream.write((char *) &output, sizeof(int16_t) / sizeof(char));
}

Overall it looks fine for a newcomer, you've made proper use of const for variables and methods, which is something beginners tend to overlook.

A few points you can still work on:

For such a tiny class like Tone, I would not bother separating declaration from implementation and would have declared it inline in the header file only. That would be less boilerplate and easier to maintain. A word of caution though: Inline code can be better optimized by the compiler and is recommend for small methods (like get/set accessors), but don't overuse it. Too much inlining will increase compile-times and generate bloated executables. I don't recommend it as an optimization either, the gain is in reducing redundant/boilerplate code implicit in the prototype/implementation setup.
Prefixing in-class member access with this-> is very rare in C++. You should only be doing that when, for instance, you need to disambiguate some shadowed member access (but even that is discouraged because shadowing members is a bit of a design flaw; better to uniquely identify your variables).
This is a personal preference, I find it better to declare public members of a class first, followed by protected and then private. My rationale is that the public section should have more emphasis, since it is what readers and users of my code will be interested on most of the time. Private/protected details are of interest for the implementer/maintainer, so they don't need to be visible right at the top of the file.
When using <cmath>, the correct way of referencing its functions, like sin/pow, is thru the std:: namespace. They happen to visible at the global level because on most compilers math.h (the C header) and cmath are implemented as wrappers to the same file. There is no requirement from the C++ Standard that this be implemented is such way, so a C++ compiler is not required to provide sin() outside the std namespace when including cmath.
Avoid using C-style casts. To cast numerical values, C++ provides static_cast. To cast pointers, reinterpret_cast is usually the operator to use. A quick overview on C++ cast operators.
Yes, size_t and int16_t are part of the Standard and are declared in <cstddef> and <cstdint> respectively. For ultimate correctness, they should be accessed from the std namespace, (e.g.: std::size_t, std:int16_t) for the same reasons cited on §4.
Your main() function is too long and doing too much. You should probably have another class, Sample perhaps, that holds the array of Tones. That class should provide methods for processing and have write_sample() as a member.
There is a much simpler way of initializing the tones vector. You can fill it with defaults on declaration using a parameterized constructor and eliminate the loop:

std::vector<Tone> tones(count, Tone(SAMPLE_RATE));

If you use an std::array you gain the size() method that knows the array length, which would remove the need for the sizeof() calculation, which is not the clearest thing you can do. Another way I tend to use for inferring the length of statically allocated arrays, without compromising readability, is by declaring a helper template function:

// Length in elements of type 'T' of statically allocated C++ arrays.
template<class T, std::size_t N>
constexpr std::size_t arrayLength(const T (&)[N])
{
    return N;
}

...

const double octaves[] = { -2, 0, 4./12, 7./12, 1, 16./12, 19./12 };
const auto count = arrayLength(octaves);

constexpr will force the function to be evaluated at compile time, thus making it suitable for use in places a compile-time constant would be required.


Softmax function implementation I'm implementing some mathematical functions right now for future use in more exciting things and wanted to know if I'm on the right track in my approach, as I don't have much C++ experience.

Here I'm offering my softmax function implementation.

#include <cmath>
#include <iterator>
#include <functional>
#include <numeric>
#include <type_traits>

template <typename It>
void softmax (It beg, It end)
{
  using VType = typename std::iterator_traits<It>::value_type;

  static_assert(std::is_floating_point<VType>::value,
                "Softmax function only applicable for floating types");

  auto max_ele { *std::max_element(beg, end) };

  std::transform(
      beg,
      end,
      beg,
      [&](VType x){ return std::exp(x - max_ele); });

  VType exptot = std::accumulate(beg, end, 0.0);

  std::transform(
      beg,
      end,
      beg,
      std::bind2nd(std::divides<VType>(), exptot));
}

Info on softmax

The softmax function is defined as

$$\text{softmax}(\mathbf{x})_{i} = \frac{\exp(x_{i})}{\sum_{j=1}^{n} \exp(x_{j})}$$

As mentioned in a comment, my reasoning for evaluating the softmax after subtracting the maximum of my vector is numerical stability. Consider the case where each element of my iterator is equal to some constant \$\alpha\$. If \$\alpha\$ is large and positive we might overflow or if large and negative we might underflow. Subtracting \$\max _{i} x_{i}\$, we ensure that the largest argument taken by \$\exp\$ is \$0\$ (so no overflow) and that at least one denominator term protects us from underflow leading to division by zero.

Thoughts while writing this

Is the static_assert a reasonable thing to check? The other thought I had was to use SFINAE which I don't know too much about. Like

template <typename Condition>
using EnableIf =
    std::enable_if_t<Condition::value>;

template <
  typename It,
  EnableIf<std::is_floating_point<
             typename std::iterator_traits<It>::value_type
             >
           >...
  >
void softmax(...)

What I liked about this approach that if I try to pass an iterator with a non-floating point type somewhere I get a nice immediate error recognition in my Emacs session for no function overload.. but I thought the static_assert might be better because there really is no other overload I have in mind for softmax - passing a non-floating iterator is really just an error.
Is there any possible issues / considerations when using the value type of the iterator like I am doing here?

Algorithm

You could actually skip looking up the maximum element in the range. This is because it gets cancelled anyways:
 $$y = {{e^{x - x_{max}}} \over {\Sigma e^{x - x_{max}}}} ={ {e^x \over e^{x_{max}}} \over {{{1} \over {e^{x_{max}}}}\Sigma e^x}} = {{e^x} \over {\Sigma e^x}} $$

Also, currently the results are stored in place, i.e. the original input data will be lost. This might not always be wanted, so maybe accept an iterator to write the results to?

static_assert vs. SFINAE

I personally like the SFINAE approach more in this case, because it's easier to introduce another overload if needed (e.g. for iterators over associative containers) and you get immediate error reporting. That said, if the decision is final that you won't ever need another overload, static_assert works fine.

iterator value_type

Well, if the container is nicely conforming to standard library guidelines, you'll be fine with using std::iterator_traits<It>::value_type. For custom containers, this might not be the case, though - for those cases you could use decltype(*beg) instead.


Construct binary tree from inorder and preorder traversal console.clear();

function cons(inorder, preorder) {
  if (isEmpty(inorder) || isEmpty(preorder)) return null

  if (size(inorder) === 1 && size(preorder) === 1)
    return {val: inorder[0], l: null, r: null};

  var leftSize = inorder.indexOf(head(preorder));

  return {
    val: preorder[0],
    l: cons(take(inorder, leftSize), take(tail(preorder), leftSize)),
    r: cons(drop(inorder, leftSize + 1), drop(tail(preorder), leftSize))
  };
}

function head(xs) {
  return xs[0];
}

function size(xs) {
  return xs.length;
}

function isEmpty(xs) {
  return xs.length === 0;
}

function take(xs, i) {
  return xs.slice(0, i);
}

function drop(xs, i) {
  return xs.slice(i);
}

function tail(xs) {
  return xs.slice(1);
}

var inorder  = [1, 2, 3, 4, 5, 6];
var preorder = [4, 2, 1, 3, 5, 6];

var tree = cons(inorder, preorder);
console.log(tree.r.val === 5);
console.log(tree.l.val === 2);
console.log(tree.r.r.val === 6);
console.log(tree.l.l.val === 1);
console.log(tree.l.r.val === 3);

Algorithm

When inorder and preorder both have a single element,
it must be the same value,
and the left and right parts in inorder are empty.
Therefore the special treatment for size(inorder) === 1 && size(preorder) === 1 is unnecessary,
you can simply drop that condition and the algorithm remains correct.

Performance

Although the implementation is elegant,
using building blocks common in functional programming,
slicing and dicing arrays is not very efficient in JavaScript.
The performance could be improved by using indexes of sub-array ranges.
It will be ugly, but fast.

Naming

l is the worst variable name ever.
Depending on your font,
it may be difficult to discern from 1 or |.
In this example, I would spell out left and right for the tree branches, naturally.


The fastest way to import and edit data in R So I want to import and edit a huge amount of data. I have a file with multiple data sets from time series that are chronologically ordered. Now I want to open them and edit to have a final dataset where the files are edited and ordered.

The code looks like this but it is taking some time to load the data:

setwd <-("C:/Users/D60378/Desktop/DATA")
path_data <- 'test'
files_data <- list.files(path_data)
length(files_data)
for (i in 1:length(files_data)){
 # use intermediary path if nested dir
tempPath <- file.path(getwd(), path_data, files_data[i])
  # name of the dataser
   name <- gsub('-', '_', substr(files_data[i], start = 0, stop =     (nchar(files_data[i])-4)))
   print(name)
     df <- read.csv(tempPath, skip=5, header=FALSE, sep = ';', dec = ',') #sheet 1 is the
   # column names
   df_names <- read.csv(tempPath, skip=4, nrows=1, header=FALSE, sep = ';', dec=',')
   dfnames_test_MS7 <- c()
   for (i in 1:length(df_names)){
     print(as.character(df_names[[i]]))
    dfnames_test_MS7[i] <- as.character(df_names[[i]])
   }
   dfnames_test_MS7[1] <- "DateTime"
   for (i in 1:length(dfnames_test_MS7)){
     dfnames_test_MS7[i] <- gsub(' ', '_', dfnames_test_MS7[i])
   }
  dfnames_test_MS7

   names(df) <- dfnames_test_MS7
   assign(name, df)
}

1) Firstly you could use fread from data.table to speed up reading of .csv files.

2) It looks like that you do not need the two inner loops, you could do that with vectorization.

If you could provide some example data of df and df_names(using dput) then I could write the necessary code, and test the timings...

Update:

It looks like for smaller files (200 cols and 2000 rows) read_csv2 from readr is faster than data.table's fread:

Unit: milliseconds
                                                               expr      min       lq     mean   median       uq      max neval cld
 read.csv(tempPath, skip = 5, header = FALSE, sep = ";", dec = ",") 475.7547 480.2411 488.7772 484.3644 487.7255 515.8005     5   c
                                      read_csv2(tempPath, skip = 4) 179.2461 181.9832 182.2904 182.3569 182.4955 185.3702     5 a
                               fread(tempPath, skip = 4, dec = ",") 463.4811 468.1232 470.4556 468.3920 469.5664 482.7155     5  b

You should test it yourself and see if there is significant difference.

The end code could look something like this:

files_data <- list.files(path_data, full.names = T)
files_data

files_data <- grep(".csv", files_data, value = T, fixed = T)
length(files_data)

files_data # paths to files

myAproach <- function(i) {
  require(readr)
  tempPath <- files_data[i]
  name <- basename(tempPath)
  name <- gsub('-', '_', substr(name, start = 0,
                                stop = (nchar(name) - 4)),
               fixed = T)
  print(name)

  df <- read_csv2(tempPath, skip = 4)

  dfnames_test_MS7 <- colnames(df)
  dfnames_test_MS7[1] <- "DateTime"
  dfnames_test_MS7 <- gsub('V', 'x', dfnames_test_MS7, fixed = T)
  #fixed for speed
  colnames(df) <- dfnames_test_MS7
  df
}
resDflist <- lapply(1:length(files_data), myAproach)

resDflist is list of data.frames. In my opinion it is easier to work with lists than assign the data.frames to global environment.


WAI-ARIA and Pagination I would like to get some advice on WAI-ARIA markup I have added to my paginated post navigation.

Does this look correct, am I missing anything?

Should anything be added/removed to the current page link (#2)?

Also, curious on my "Page Count" and "View All" sections what if anything can be added to make it more ARIA-friendly.

        Page 2 of 4

                «

            1

            2

            3

            4

                »

            View All

If you’re using HTML5 <nav> element, it’s not necessary to apply a role of ‘navigation’ – certain HTML elements inherently pass their relevant roles to the accessibility API. If you were wrapping the navigation menu in a <div>, for example, then you’d need to apply a role of ‘navigation’.

Role="menubar" has been correctly applied to the <ul> element, as has role="menuitem" to the <a> tags.

I would suggest applying a role="none" to each <li> element; otherwise this could confuse some users of assistive technologies since listitems and menuitems would be interspersed with each other.

Might be an idea to provide an aria-label to the <ul> with role="menubar"; used to help people understand the menu purpose and uniquely identify it from other menubars on the page.

Can also apply a tabindex="0" to first and last <a> elements – gives keyboard focus to link and makes a part of the tab order of the page. Alternatively, give a tabindex="-1" to make <a>s focusable but not part of the tab order of the page.


Write a function to print numbers I would like to write a python or c++ function to print numbers like the following examples (with n as input parameter):

When n = 3, print:

1 2 3
8 0 4
7 6 5

When n = 4, print:

4  5  6  7
15 0  1  8
14 3  2  9
13 12 11 10

I have an answer in the following code block, but it is not quite elegant. Does anyone have a better solution?

def print_matrix(n):
  l = [i for i in range(0,n*n)]

  start = (n-1)/2
  end = n-1-start
  count = 0
  if start ==end:
    l [start+ n*end] = count
    start -= 1
    count +=1

  while start >=0:
        end = n - start
        for i in range(start, end-1):
                x= i
                y= start
                print y*n +x
                l[y*n +x] = count
                count +=1
        for i in range(start, end-1):
                x = end-1
                y = i
                print y*n +x
                l[y*n +x] = count
                count +=1
        for i in range(end-1,start,-1):
                x= i
                y= end-1
                print y*n +x
                l[y*n +x] = count
                count +=1
        for i in range(end-1, start,-1):
                x = start
                y = i
                print y*n +x
                l[y*n +x] = count
                count +=1
        start -=1

  print l

Using a 2D array to store the matrix, we can simplify the function greatly. This uses Python 2 syntax.

def print_matrix(n):
    mat = [[0]*n for _ in xrange(n)]

    k = 0 # value to write
    for level in reversed(xrange(n, 0, -2)):
        start = n/2 - level/2
        pr = pc = start # current row, column indices
        if level == 1: # special case: no perimeter crawl
            mat[pr][pc] = k
            k += 1
            continue
        # walk along each edge of the perimeter
        for dr, dc in [(0,1), (1,0), (0,-1), (-1,0)]:
            for i in xrange(level - 1):
                mat[pr][pc] = k
                k, pr, pc = k+1, pr+dr, pc+dc

    width = len(str(n*n-1))
    for i in mat:
        for j in i:
            print '{:<{width}}'.format(j, width=width),
        print

Samples:

>>> print_matrix(4)
4  5  6  7
15 0  1  8
14 3  2  9
13 12 11 10
>>> print_matrix(3)
1 2 3
8 0 4
7 6 5
>>> print_matrix(2)
0 1
3 2
>>> print_matrix(1)
0
>>> print_matrix(9)
49 50 51 52 53 54 55 56 57
80 25 26 27 28 29 30 31 58
79 48 9  10 11 12 13 32 59
78 47 24 1  2  3  14 33 60
77 46 23 8  0  4  15 34 61
76 45 22 7  6  5  16 35 62
75 44 21 20 19 18 17 36 63
74 43 42 41 40 39 38 37 64
73 72 71 70 69 68 67 66 65


LeetCode: Burst Balloons C# https://leetcode.com/problems/burst-balloons/

  Given n balloons, indexed from 0 to n-1, each balloon is painted with
  a number on it represented by array nums. You are asked to burst all
  the balloons. If you burst balloon i, the number of coins you will get is calculated as:
  $$ nums[left] * nums[i] * nums[right] $$Here, left and right are adjacent indices of i. After the burst, left and right then becomes adjacent.

  Find the maximum coins you can collect by bursting the balloons
  wisely.

  Note:

  You may imagine $$ nums[-1] = nums[n] = 1 $$They are not real therefore
  you can not burst them. 0 ≤ n ≤ 500, 0 ≤ nums[i] ≤ 100

Example:

Input: [3,1,5,8]
Output: 167
Explanation: nums = [3,1,5,8] --> [3,5,8] -->   [3,8]   -->  [8]  --> []
             coins =  3*1*5      +  3*5*8    +  1*3*8      + 1*8*1   = 167

using System;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace RecurssionQuestions
{
    /// <summary>
    /// https://leetcode.com/problems/burst-balloons/
    /// </summary>
    [TestClass]
    public class BurstBalloonsTest
    {
        [TestMethod]
        public void TestExample()
        {
            int[] nums = {3, 1, 5, 8};
            BurstBalloonsClass burst = new BurstBalloonsClass();
            Assert.AreEqual(167, burst.MaxCoins(nums));
        }
    }

    public class BurstBalloonsClass
    {

        public int MaxCoins(int[] nums)
        {
            int[] numbers = new int[nums.Length +2];  // we add 2 because of the question
            // nums[-1] = nums[n] = 1
            int n = 1;
            foreach (var x in nums)
            {
                if (x > 0) // we care only about positive values for profit
                {
                    numbers[n++] = x;
                }
            }
            numbers[0] = numbers[n++] = 1;
            int[][] memo = new int[n][];
            for (var index = 0; index < memo.Length; index++)
            {
                memo[index]= new int[n];
            }
            // we allocate NxN matrix for memoization

            return Burst(memo, numbers, 0, n - 1);
        }

        private int Burst(int[][] memo, int[] numbers, int left, int right)
        {
            if (left + 1 == right)
            {
                return 0;
            }

            if (memo[left][right] > 0)
            {
                return memo[left][right];
            }

            int ans = 0;
            // we try all the options between left and right
            // we compare the answers of all of the options and the maxmial one
            // if poped all of the ballons from left to u and from i to right we have only an option to pop
            // numbers[left] * numbers[i] * numbers[right]
            for (int i = left + 1; i < right; ++i)
            {
                ans = Math.Max(ans, numbers[left] * numbers[i] * numbers[right]
                                    + Burst(memo, numbers, left, i)
                                    + Burst(memo, numbers, i, right));
            }

            memo[left][right] = ans;
            return ans;
        }
    }
}

Some things come to mind when I read your code.

There is a nice feature on arrays in C# called CopyTo(). It gives you the possibility to copy an array without using a loop. Like so:

int[] newNums = new int[n];
nums.CopyTo(newNums, 1);
newNums[0] = newNums[n-1] = 1; //This line I really like.

There is also something called a multidimensional Array which basically does the same thing as int[][]. This gives you the possibility to do this:

int[,] memo = new int[n,n];

Use them together and you can remove all your loops from int maxcoins.

Regarding your actual work method Burst I would say that you make things rather confusing. When debugging, the first thing that happens is that you cache a number series which is not allowed until 3 balloons are popped. It adds [0][1][5] to the cache. A more reasonable approach would have been [0][1][2] then [0][1][3] up to [0][1][5]. After that switch to [0][2][3] etc.  When you reach [0][4][5] you start over at [1][2][3].

To do that you would have to create nested loops in your Burst method:

        for (int i = left+1; i < right; ++i)
        {
            for(int j = i+1; j<= right; ++j)
                ans = Math.Max(ans, numbers[left] * numbers[i] * numbers[j]
                                + Burst(memo, numbers, left, i)
                                + Burst(memo, numbers, i, j));
        }

However, there is an even more simple approach to this problem and that is to actually pop the balloons in the array. This can be done easiest with a list instead of an array. But both are possible. So what you would do is to create a new set of balloons after each pop. So here caching is out of the question because you never really know what range you processed before. And to be honest I guess it would most likely slow down the process and make the code harder to read. I use the same variable names as you but in reality I would name nums and numbers as balloons and newNumbers would be named remainingBalloons. I did however change the variable i to baloonToPop to make it easier to understand what it represents. I left the adding of the ones in the code because it simplifies the Burst method.

    public void MaxCoins(List<int> nums)
    {
        nums.Insert(0, 1); //Add the ones to the array
        nums.Add(1);
        int result = Burst(nums);
    }

    private int Burst(List<int> numbers)
    {
        int result = 0;
        for (int baloonToPop = 1; baloonToPop < numbers.Count-1; baloonToPop++)
        {
            List<int> newNumbers = new List<int>();
            newNumbers.AddRange(numbers);
            newNumbers.RemoveAt(baloonToPop);
            int sumFromBaloonPop = numbers[baloonToPop - 1] * numbers[baloonToPop] * numbers[baloonToPop + 1];
            result = Math.Max(result, Burst(newNumbers) + sumFromBaloonPop);
        }
        return result;
    }

To do it with an array (in case there is a library restriction) you would have to create a new Array and then copy Subranges from the initial one instead of creating a copy of the List using AddRange + RemoveAt as above:

int[] newNumbers = new int[numbers.Length-1];
Array.ConstrainedCopy(numbers, 0, newNumbers, 0, baloonToPop);
Array.ConstrainedCopy(numbers, baloonToPop + 1, newNumbers, baloonToPop, numbers.Length - (baloonToPop + 1));


3D vector CUDA kernel I designed this CUDA kernel to compute a function on a 3D domain:

p and Ap are 3D vectors that are actually implemented as a single long array:

__global__ void update(P_REAL* data, P_REAL* tmp, P_REAL* f, P_REAL* reduced,
       const unsigned int sizeX,const unsigned int sizeY,const unsigned int sizeZ,
       const P_REAL aX, const P_REAL aY, const P_REAL aZ, const P_REAL aF,
       const P_REAL cX, const P_REAL cY, const P_REAL cZ){

extern __shared__ P_REAL s_pAp[];

// Get grid size
unsigned int gY((sizeY-2)/blockDim.y),gZ((sizeZ-2)/blockDim.z);

unsigned int idX(blockDim.x*blockIdx.x+threadIdx.x+1);
unsigned int idY(blockDim.y*blockIdx.y+threadIdx.y+1);
unsigned int idZ(blockDim.z*blockIdx.z+threadIdx.z+1);

unsigned int sX(sizeY*sizeZ);
unsigned int i((idX*sizeY+idY)*sizeZ+idZ);
unsigned int bi((threadIdx.x*blockDim.y+threadIdx.y)*blockDim.z+threadIdx.z);

Ap[i]=cX*(p[i-sX]+p[i+sX]-2.0f*p[i])
     +cY*(p[i-sizeZ]+p[i+sizeZ]-2.0f*p[i])
     +cZ*(p[i-1]+p[i+1]-2.0f*p[i]);
s_pAp[bi] = Ap[i]*p[i];

// Wait for all threads
__syncthreads();

Reduction of s_pAp (to be summed):

// Take advantage of the geometry of the block
// At each iteration, there are 7 points to consider
// Use byte-shift to ease division by 2

for(int offset = blockDim.x/2;offset>0;offset>>=1){
    if(threadIdx.x < offset && threadIdx.y < offset && threadIdx.z < offset){
        s_pAp[bi]+=s_pAp[bi+offset]+s_pAp[bi+offset*blockDim.z]+s_pAp[bi+offset*blockDim.z*blockDim.y]
                +s_pAp[bi+offset*(1+blockDim.z)]
                +s_pAp[bi+offset*(1+blockDim.z*blockDim.y)]
                +s_pAp[bi+offset*blockDim.z*(1+blockDim.y)]
                +s_pAp[bi+offset*(1+blockDim.z*(1+blockDim.y))];
    }

    // Wait for all threads
    __syncthreads();
}

// Write final result with (0,0,0) thread
if(threadIdx.x==0 && threadIdx.y==0 && threadIdx.z == 0){
    reduced[(blockIdx.x*gY+blockIdx.y)*gZ+blockIdx.z] = s_pAp[0];

}
}

This works like a charm, but when profiling I get awful performance. Could I get some insight on how to improve this code?

The indentation within update() is misleading.  Most of all, everything inside of the function should be indented.  Otherwise, it'll be hard to tell what exactly is inside of the function.

I do, however, like that you've indented the (lengthy) parameter list by eight spaces instead of the standard four.  It should be easier to tell them apart from the code inside of the function.
More whitespace within statements can be added for more readability.

This line, for instance, is hard to read:

Ap[i]=cX*(p[i-sX]+p[i+sX]-2.0f*p[i])

You can add more whitespace as such:

Ap[i] = cX * (p[i-sX] + p[i+sX] - 2.0f * p[i])

Now you can really tell what everything is at a glance.

You can also add a space before the opening curly braces for functions.  It may not be a big deal with functions with smaller signatures, but yours are pretty lengthy, and it's harder to see the opening curly brace with them.

You also have inconsistent whitespace, such as in here:

for(int offset = blockDim.x/2;offset>0;offset>>=1){

There is spacing between the =, but nowhere else.  This should look clearer:

for (int offset = blockDim.x / 2; offset > 0; offset >>= 1) {

Considering that CUDA's blockIdx and blockDim are primarily signed, you should make your thread ID variables of the same type.  If they won't be too large, make them int.  If they can be larger than an int, make them either long or long long and cast blockIdx to the same.


A localization helper in codeigniter I created a project to better understand CI3, while developing it I tried to introduce localization elements.
When I tried to localize the view everything messed up. I made a helper to identify if the user choose a language different than default language then load the right language.

function lang_load($lang_file)
{
    if($this->session->get_userdata('language'))
    {
        $this->lang->load($lang_file,$this->session->get_userdata('language'));
    }else
    {
        $this->lang->load($lang_file);
    }
}

so everytime instead use

$this->lang->load($lang_file);

I use

lang_load($lang_file)

I use it on Controllers and in the Views, but I guess it is breaking the MVC principle when used on view side.

This is pretty trivial example for review, but you are right to question use of this function from within a view.

Personally I think this is information that needs to be determined upstream of this decision point, making the function unnecessary.

If you always used this format for loading language:

$this->lang->load($lang_file, $lang);

where $lang here could be a variable, constant, call to static class method, or some other way to get to the current language preference (with this preference determined somewhere outside the logic for actually loading the localization).


Reduce a grid array [][] into a string I have a class with a property of grid[][] and each value is either true or false. I need to convert this to a string with letters o and b in place of the boolean.

First way, simple for loops over the array and return the string:

 get rle() {
     let result = "";
     for(let y = 0; y < this.grid.length; y++) {
         for(let x =0; x < this.grid[y].length; x++) {
             result += ( (this.grid[y][x]) ?  "o" : "b" );
         }
     }
     return result;
}

Or a more JS style solution?

get rle() {
    return this.grid.reduce( (total, currentValue, currentIndex, arr) => {
        return total + arr[currentIndex].reduce( (total, currentValue, currentIndex, arr) => {
            return total + ( (arr[currentIndex]) ?  "o" : "b" );
        }, "");
    }, "");
 }

Is this a good JS style solution, and what in your opinion is better? I prefer the first because anyone can instantly understand it. The JS solution makes me frown with 3 nested returns, looks odd.

First, a few points of the code you wrote. If you have an array of values, you can join the values together to efficiently create a string. This would eliminate the need for at least one of the reduce calls. Second, in a reduce call, the value of the second callback parameter (currentValue in your code) is the value of the array parameter at the index parameter (arr[currentIndex] in your code). Combining that with Javascript's capability to ignore excess function parameters, your reduce calls should take only two parameters, and use the currentValue in place of the arr[currentIndex].

You should also avoid using the same variable names in the same scope. Having two sets of total, currentValue, currentIndex, and arr could get confusing quickly, and lead to strange bugs.

Now, for the one-liner:

return this.grid.flat().map((el) => el ? "o" : "b").join("");

See Array#flat, Array#map, and the aforementioned Array#join. Of these, Array#flat is the newest and possibly unsupported method. It can be easily polyfilled or replaced. The MDN page shows some clever replacements like arr.reduce((all, row) => all.concat(row), []) and [].concat(...arr).


basic jQuery plugin template I would like to know if I'm writing a plugin for jquery correctly. I followed the official guide and also added some tweaks that I've found from various sources over time.

While this works perfectly, I don't know if I'm doing something wrong.

This is the sample:

(function($)
{
    var plugin = "my_plugin_name";
    var methods = {
        init : function(user_settings)
        {
            return this.each(function(index)
            {
                if(!$(this).hasClass("plugin_class"))
                {
                    $(this).addClass("plugin_class");

                    var $this = $(this);
                    var data = $this.data(plugin);

                    if(!data)
                    {
                        var default_settings = {
                            optionA: "abc",
                            optionB: 123,
                            optionB: true
                        };

                        if(user_settings)
                        {
                            $.extend(true, default_settings, user_settings);
                        }

                        $this.data(plugin,
                        {
                            "settings": default_settings
                        });
                    }

                    privateMethod($this);
                }
           });
        },
        exposedMethodX : function(value) //$(selector).my_plugin_name("exposedMethodX", true)
        {
            console.log(value)
        }
    };

    $.fn[plugin] = function(method)
    {
        if(methods[method])
        {
            return methods[method].apply(this, Array.prototype.slice.call(arguments, 1));
        }
        else if(typeof method === "object" || !method)
        {
            return methods.init.apply(this, arguments);
        }
        else
        {
            alert("Method " +  method + " does not exist");
        }
    };

    function privateMethod(obj)
    {
        console.log(obj.data(plugin).settings);
    }
})(jQuery);

In general the template looks like a good start. I noticed that the code appears to resemble the format advised in PHP-Fig's PSR-2, namely "The opening brace MUST go on its own line"1 for functions and methods plus control structures like if and else blocks. Most of the Javascript style guides I have seen call for curly braces to exist on the same line of the block they are opening. Personally I don't even agree with this in PHP, let alone Javascript, however if that is your personal preference then keep it consistent.

Inside the init method, there is an iterator:

return this.each(function(index)
{
    if(!$(this).hasClass("plugin_class"))
    {
        $(this).addClass("plugin_class");

       var $this = $(this);

The reference var $this = $(this); could be stored before the check of the class name and/or class name addition in order to reduce DOM queries.

Also, the name $this could be more descriptive - at least something like $element or $elem (as shown in the example under Provide Public Access to Secondary Functions as Applicable from the Advanced Plugin Concepts).

Addressing Your Additional question

You asked in a comment:

  what about the $.fn[plugin] part? I'm not sure about the first if statement. could that function be refactored and improved?

It is possible that ecmascript-6 features like the spread syntax could potentially be used to simplify the call, though given that current jQuery browser support includes IE 9+ it may not be wise to utilize ES-6 features.


Write a wrapper to REST API with asyncio I want understand how to correctly structure a functional asyncio-based program.

The code below wraps two external APIs to provide the client a simple send_weather() function. Its structure reflects my current understanding of ansynchronous programming and the very little I've grasped of functional programming.

import asyncio
from functools import partial

import aiohttp

SUCCESSFUL_PUSH = {'deleted': True}  # Value specific to fake push service

async def _post(headers, url, data):
    async with aiohttp.ClientSession(headers=headers) as session:
        async with session.post(url, json=data, verify_ssl=False) as response:
            return await response.json()

async def _get(headers, url):
    async with aiohttp.ClientSession(headers=headers) as session:
        async with session.get(url, verify_ssl=False) as response:
            return await response.json()

def make_comms(headers):
    get = partial(_get, headers)
    post = partial(_post, headers)
    return get, post

async def _notify(post, message, user):
    url = f"https://api.keen.io/dev/null?user={user}"  # Fake push service
    return await post(url, message)

async def _find_city(get, city):
    url = f"https://www.metaweather.com/api/location/search?query={city}"
    obj_list = await get(url)
    if obj_list:
        return obj_list[0]
    return None

async def _city_weather(get, city_id):
    url = f"https://www.metaweather.com/api/location/{city_id}"
    return await get(url)

async def _send_weather(get, post, city, user):
    city_data = await _find_city(get, city)
    if city_data is None:
        print("Could not find city")
        return
    forecast_data = await _city_weather(get, city_data["woeid"])
    weather = forecast_data["consolidated_weather"][0]["weather_state_name"]
    message = f"Current weather in {city}: {weather}"
    reply = await _notify(post, {"message": message}, user)
    if reply == SUCCESSFUL_PUSH:
        print(f"Current weather in {city}: {weather}")
    else:
        print(f"Push notification to {user} failed: {reply}")

def init(get, post):
    return partial(_send_weather, get, post)

if __name__ == "__main__":
    HEADERS = {"Content-Type": "application/json"}
    get, post = make_comms(HEADERS)
    send_weather = init(get, post)
    asyncio.run(send_weather("Glasgow", "Bob"))

How to structure an asynchronous functional program?

Calling aiohttp.ClientSession in _post and _get is really bad. Don't do this.
There is no main function, this just makes the code harder to convert into a setuptools application.
All these functools.partial only hinder readability rather than enhance it.

Just make a class.
The code isn't very functional. And I can only imagine what a purist would have to say about _send_weather. There are so many side effects in one little, hard to read, function.
There is no separation of concerns, _find_city and _city_weather are solely around getting a forecast, whilst _notify is sending a notification. You've mangled them into one _send_weather function. Not great.
Since the code isn't functional, and this is possibly the worst project to learn functional programming on. Let's just make it so that the code is easy to read.

Make a WeatherForecast class.

At instantiation, it only takes a session.
This is so we can remove all those partials with self.get.
Include get into this class, however without the session creation part.
Include find_city as find_cities, and don't mutate the output.
Include city_weather.
Change send_weather to get_weather and delegate calls to find_city and city_weather.

It is not allowed to print here. All it does is get the weather for the wanted city.

This is a helper function and so mutates the values to get the two other functions to work and to get the output as we desire. Nothing else.

Remove the session creation part from post.
Make a function notify_user that notifies the user of city's weather.

This is as simple as using two f-strings, to build the URL and the message.
Change _send_weather so that it takes a session, city, and user.

To make the function easy to read, we are limited to three things in the function.

Calling another function.
Performing a simple if else to control data flow.
Formatting output to the user - basic f-strings.

Here's how I performed the above. Which is as functional as your code. And you could argue that my functions are more pure than yours, as get_weather only gets weather. notify_user notifies the user. And send_weather only prints and delegates to other impure functions. None of them are pure and can't be because they all have side effects. Which pretty much makes implementing this in 'pure FP' a joke.

import asyncio
from functools import partial

import aiohttp

SUCCESSFUL_PUSH = {'deleted': True}  # Value specific to fake push service

class WeatherForecast:
    def __init__(self, session):
        self.session = session

    async def get(self, headers, url):
        async with self.session.get(url, verify_ssl=False) as response:
            return await response.json()

    async def find_cities(self, city):
        return await self.get(f"https://www.metaweather.com/api/location/search?query={city}")

    async def city_weather(self, city_id):
        return await self.get(f"https://www.metaweather.com/api/location/{city_id}")

    async def get_weather(self, city_name):
        cities = await self.find_city(city_name)
        if not cities:
            return None
        forecast = await self.city_weather(cities[0]["woeid"])
        return forecast["consolidated_weather"][0]["weather_state_name"]

async def post(session, url, data):
    async with session.post(url, json=data, verify_ssl=False) as response:
        return await response.json()

def notify_user(session, city, weather, user):
    return await post(
        session,
        f"https://api.keen.io/dev/null?user={user}",
        {"message": f"Current weather in {city}: {weather}"},
    )

def send_weather(session, city, user):
    weather = await WeatherForecast(session).get_weather(city)
    if weather is None:
        print("Could not get forecast")
    else:
        reply = notify_user(session, city, weather, user)
        if reply == SUCCESSFUL_PUSH:
            print(f"Current weather in {city}: {weather}")
        else:
            print(f"Push notification to {user} failed: {reply}")

async def main():
    headers = {"Content-Type": "application/json"}
    async with aiohttp.ClientSession(headers=headers) as session:
        send_weather(session, "Glasgow", "Bob")

if __name__ == "__main__":
    asyncio.run(main())


Better way to add fields to superclass in Java I'm making my first steps in OOP and I'm currently doing some exercises. I've done a simple program with class structure like this (I'll provide only essential parts):

Super class:

public abstract class Employee {
    private String lastName;
    private float salary;

    public Employee(String lastName, float salary) {
        this.lastName = lastName;
        this.salary= salary;
    }
}

And several child classes, all similar to this:

public class Worker extends Employee {
    private float hourRate;

    public Worker (String lastName, float salary, float hourRate) {
        super(lastName, salary);
        this.hourRate = hourRate;
    }
}

My concern is, when I'll decide to add some field to the Employee class that have to be set in constructor, let's say firstName, than I have to add it there and in constructor's arguments of all child classes and where I call super() inside them. It doesn't seem to be the best solution... Is there a better way to do such a thing? Maybe there is some pattern I don't know yet?

Short answer: no, there's no better way.

There's also nothing wrong with this way of working. You're adding a field that every instance of this class needs to have for your program to work correctly. That's a good reason to enforce initialising it in the constructor. Your compiler (or even better, your IDE) will tell you where else you need to update your code.

In case you're developing a package that other people use where they need to have some time to adjust to the new version you need to be more careful when making API changes (like adding parameters to a public constructor).

In that case you make the current constructor @deprecated and provide a new one next to it. You can also provide a default value for the new field so that the original constructor still "works". For example:

public class Person {
    private String lastName;
    private String firstName;

    @deprecated
    public Person(String lastName) {
        this("", lastName);
    }

    public Person(String firstName, String lastName){
        this.firstName = firstName;
        this.lastName = lastName;
    }

    ...

This way, their IDE will still tell them they're using an outdated constructor, but at least they'll still be able to run their current code without changing all the calls from the start.

Alternatively you could look into other options besides adding the fields to that class. For example, you could have a lookup class that, given a certain employee, looks up his salary in a table for example. This greatly depends on what your program is supposed to do. With the little information you've given us here it's hard to propose concrete solutions like this.