Seamlessly migrating  to From this Stack Overflow answer, I learned that C++17 will have __has_include, which can

  [make] migrating from experimental to std almost seamless

This still leaves the question of how to do it. Doing something like namespace std { using namespace std::experimental; } is undefined behaviour, so I came up with this method:

optional.h demonstration on coliru

#pragma once

#if __has_include(<optional>)
#   include <optional>
#   define HAS_STD_OPTIONAL
#elif __has_include(<experimental/optional>)
#   include <experimental/optional>
#   define HAS_STD_EXPERIMENTAL_OPTIONAL
#else
#   error Must have an optional type, either from <optional> or if not supported from <experimental/optional>.
#endif

#if defined HAS_STD_OPTIONAL

namespace opt {
    template<class T>
    using optional = std::optional<T>;
    using bad_optional_access = std::bad_optional_access;
    using nullopt_t = std::nullopt_t;
    using in_place_t = std::in_place_t;

    constexpr auto nullopt = std::nullopt;
    constexpr auto in_place = std::in_place;

    template<class T>
    constexpr auto make_optional(T && value)
    {
        return std::make_optional(std::forward<T>(value));
    }
}

#elif defined HAS_STD_EXPERIMENTAL_OPTIONAL

namespace opt {
    template<class T>
    using optional = std::experimental::optional<T>;
    using bad_optional_access = std::experimental::bad_optional_access;
    using nullopt_t = std::experimental::nullopt_t;
    using in_place_t = std::experimental::in_place_t;

    constexpr auto nullopt = std::experimental::nullopt;
    constexpr auto in_place = std::experimental::in_place;

    template<class T>
    constexpr auto make_optional(T && value)
    {
        return std::experimental::make_optional(std::forward<T>(value));
    }
}

#endif

A modification of @Jerry's answer:

#pragma once

#if __has_include(<optional>)

#   include <optional>
namespace stdx {
  using namespace ::std;
}
#elif __has_include(<experimental/optional>)
#   include <experimental/optional>
namespace stdx {
  using namespace ::std;
  using namespace ::std::experimental;
}

#else
#   error <experimental/optional> and <optional> not found
#endif

this has the advantage that the same namespace can be used for all "experimental" features (stdx).

So stdx::optional and stdx::variant both work.

We have to only using namespace ::std::experimental; after at least one <experimental/ header is included, as if it does not exist that is ill formed, and introducing one ourselves also makes the program ill formed.


Temperature conversion program with a lot of repetition My son (9) is learning Python and wrote this temperature conversion program. He can see that there is a lot of repetition in it and would like to get feedback on ways to make it shorter.

def get_temperature():
    print "What is your temperature?"
    while True:
        temperature = raw_input()
        try:
            return float(temperature)
        except ValueError:
            print "Not a valid temperature. Could you please do this again?"

print "This program tells you the different temperatures."
temperature = get_temperature()

print "This is what it would be if it was Fahrenheit:"
Celsius = (temperature - 32)/1.8
print Celsius, "degrees Celsius"
Kelvin = Celsius + 273.15
print Kelvin, "Kelvin"

print "This is what it would be if it was Celsius."
Fahrenheit = temperature * 9 / 5 + 32
print Fahrenheit, "degrees Fahrenheit"
Kelvin = temperature + 273.15
print Kelvin, "Kelvin"

print "This is what it would be if it was Kelvin"
Celsius = temperature - 273.15
print Celsius, "degrees Celsius"
Fahrenheit = Celsius * 9 / 5 + 32
print Fahrenheit, "degrees Fahrenheit"

The easiest way to remove repetition here is with nested for loops, going through all the combinations of units. In the code below I also use the fact that you can convert between any two scales in the following way:

subtract something then divide by something to get back to Celsius, then
multiply by something then add something to get to the desired unit

The "something"s can be stored in a single tuple (list of values), making the conversions simpler.

scales = ('Celsius', 'degrees ', 1, 0), ('Farenheit', 'degrees ', 1.8, 32), ('Kelvin', '', 1, 273.15)
value = get_temperature()
for from_unit, _, slope1, intercept1 in scales:
    print "This is what it would be if it was", from_unit
    celsius = (value - intercept1) / slope1
    for to_unit, prefix, slope2, intercept2 in scales:
        if to_unit != from_unit:
            print '{0} {1}{2}'.format(intercept2 + slope2 * celsius, degrees, to_unit)

Dividing code into functions is of course a good idea in general, but not that useful in this case. If he's interested in performing other operations on temperatures it might be interesting to go a step further and make a simple class like the following.

scales = {
            'Celsius': ('degrees ', 1, 0),
            'Farenheit': ('degrees ', 1.8, 32),
            'Kelvin': ('', 1, 273.15)
        }
class Temperature:
    def __init__(self, value, unit = 'Celsius'):
        self.original_unit = unit
        _, slope, intercept = scales[unit]
        self.celsius = (value - intercept) / slope
    def to_unit(self, unit):
        _, slope, intercept = scales[unit]
        return slope * self.celsius + intercept
    def print_hypothetical(self):
        print "This is what it would be if it was", self.original_unit
        for unit, (prefix, _, _) in scales.iteritems():
            if unit != self.original_unit:
                print "{0} {1}{2}".format(self.to_unit(unit), prefix, unit)
value = get_temperature()
for unit in scales:
    m = Temperature(value, unit)
    m.print_hypothetical()


Partial sums of two dimensional array I recieve a number n and then recieve n*n integers, representing a matrix, or an array of [n][n]. I want to create an array [n][n], where every cell contains the sum of all cells "left and above" from the original array, while not creating the array, just using the values.
For example, the cell [5][7], will contain the sum of the block [0][0] to [5][7] out of the original array.

I seriously need to simplify or speed up my code.

void read() {
    size = scan.nextInt();
    arrayLoad();
}

void arrayLoad() {
    area_values = new int [size][size];
    for (int i = 0; i < size; i++) {
        for (int j = 0; j < size; j++) {
            add(scan.nextInt(),i,j);
        }
    }
}

void add(int num, int x, int y) {
    for (int i = x; i < size; i++) {
        for (int j = y; j < size; j++) {
            area_values[i][j] += num;
        }
    }
}

This is O(n^4), which, as you may be able to tell, is not exactly easy for n 4+ digits.

This is an interesting problem. I think your solution will be possible and much, much faster if you use memoization. This is the process where you remember your previous calculations and reuse them in your current calculations.

So, for example, suppose you process your array row-by-row.... if your input 2D array is:

1 1 1 1 1 1 1
1 1 1 1 1 1 1
1 1 1 1 1 1 1
1 1 1 1 1 1 1
1 1 1 1 1 1 1
1 1 1 1 1 1 1
1 1 1 1 1 1 1

And we were now processing the second row, and fourth column, our current result would look like (our current position is marked with a ?):

1 2 3 4 5 6 7
2 4 6 ? 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0

Now, at that position, we know that the value will be 8 because we are human, and can see that. The best algorithm on the computer though, to get that 8, is that we can split the data we know in to 4 areas....

Consider the input data again (I have marked our spot with the parenthesis):

1 1 1 1 1 1 1
1 1 1(1)1 1 1
1 1 1 1 1 1 1
1 1 1 1 1 1 1
1 1 1 1 1 1 1
1 1 1 1 1 1 1
1 1 1 1 1 1 1

The 4 areas that we can think of as special are:

the value we are processing right now (the value 1)
the column above the us (contains the values [1])
the row we have processed so far (contains the values [1, 1, 1])
the rectangle above-left of where we are so far (contains the value [1, 1, 1] as well)

Now, what is the sum of the values in the rectangle? We know that, because we have our grid right now which looks like:

1 2(3)4 5 6 7
2 4 6 ? 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0

So, the rectangle from (0,0) to (0,2) has the sum 3 which has already been calculated (and which I have marked with parenthesis).

We have also calculated the sum of the data in the row to the left of us... look at the grid again:

1 2(3)4 5 6 7
2 4(6)? 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0
0 0 0 0 0 0 0

The sum of the values in the row to the left is the difference between the rectangle above left, and the rectangle plain-left, or 6 - 3 == 3.

Similarly, the sum of the values in the column above us, is the sum of the rectangle above us, less the rectangle above-left of us.

So, for any position in the solution matrix, the solution-sum of the values is:

the value at this point
plus the sum of all values above-left of us
plus the sum of all values above us
plus the sum of all values to the left.

Using a more convenient input format than the scanner, this can all be done in a single loop....

int[][] sumAboveLeft(int[] datasource) {
    if (datasource == null || datasource.length == 0 || datasource.length != datasource[0] * datasource[0] + 1) {
        throw new IllegalArgumentException();
    }
    // first value in the source data is the matriz size
    final int size = datasource[0];
    // because the first value is the size, we expect the loop limit to be unusual.
    final int limit = datasource.length - 1;

    int[][] result = new int[size][size];

    for (int i = 0; i < limit; i++) {
        // convert the linnear/flat address to a row/column
        int row = i / size;
        int col = i % size;

        // rectangle-sum above us
        int abovesum = row > 0 ? result[row - 1][col] : 0;
        // rectangle-sum to left of us
        int leftsum = col > 0 ? result[row][col - 1] : 0;
        // rectangle-sum above-left of us.
        int aboveleftsum = (col > 0 && row > 0) ? result[row - 1][col - 1] : 0;
        // our value at this point (note the index+1 offset because the first value is the size)
        int val = datasource[i + 1];
        // the sum here is
        // the value here
        // plus above-left-rectangle-sum
        // plus above-column-sum
        // plus left-row-sum
        result[row][col] = val
                         + aboveleftsum
                         + (leftsum - aboveleftsum)
                         + (abovesum - aboveleftsum);
    }
    return result;

}

I have put a lot of comments in there for you...

I have also tested it with:

public static void main(String[] args) {
    System.out.println(Arrays.deepToString(sumAboveLeft(new int[]{
         7,
         1, 1, 1, 1, 1, 1, 1,
         1, 1, 1, 1, 1, 1, 1,
         1, 1, 1, 1, 1, 1, 1,
         1, 1, 1, 1, 1, 1, 1,
         1, 1, 1, 1, 1, 1, 1,
         1, 1, 1, 1, 1, 1, 1,
         1, 1, 1, 1, 1, 1, 1,
    })));
}

So, the process can be done in O(n), where n is the number of members in the matrix.


Generating a dynamically expandable form with multiple similar categories Overview:

Here's the look of my form.

In this example, I have a form that allows dynamically adding fields in each category. Like, I can add multiple attractions like 5 attractions, same goes for Featured artists, so basically I can add an infinite set of data per category in this form.

So after submitting, all those fields (dynamic fields) are sent in array format. It checks if the category have at least one file uploaded. The function here is already correct, it gives the output.

if (count(array_filter($_FILES['attraction']['name'])) > 0) {

    $attraction_status = true;

    $attraction = $_FILES['attraction'];
    $attraction['tmp_name']['image'] = true;
    $attraction_field = Field::incrementFieldComparison($attraction['tmp_name'], $request->post('attraction_name'), $request->post('attraction_description'));

    $attraction_name        = implode('|', $attraction_field->field_1);
    $attraction_description = implode('|', $attraction_field->field_2);
    $attraction_image       = implode('|', $attraction_field->reference);
}

The problem is, I have like 7 categories in this form. So basically, I have this same chunk of code for seven times with different type of category.

if (count(array_filter($_FILES['featured_artist']['name'])) > 0) {

    $featured_artist_status= true;

    $featured_artist= $_FILES['featured_artist'];
    $featured_artist['tmp_name']['image'] = true;
    $featured_artist_field= Field::incrementFieldComparison($attraction['tmp_name'], $request->post('featured_artist_name'), $request->post('featured_artist_description'));

    $featured_artist_name = implode('|', $featured_artist_field->field_1);
    $featured_artist_description = implode('|', $featured_artist_field->field_2);
    $featured_artist_image = implode('|', $featured_artist_field->reference);
}

Is this fine, as long as I'm getting the right output? I'm concerned about the repetition of code. Is there any way to improve this code if this is a bad practice of coding?

Answering your questions:

  Is this fine, as long as I'm getting the right output?

Not really, by your own admission, the possibility of more fields means you'll have blocks and blocks and blocks of the same code. See Structure below.

  I'm concerned about the repetition of code. Is there any way to improve this code if this is a bad practice of coding?

See The code below.

Structure:

Although the code given could be greatly simplified, I think to a degree your methodology is at fault.

Currently, you would have seven variables for each dynamic field, as the number (currently five) increases, the amount of localised variables would be out of control!

Rather than having such a large number of these, consider using a dictionary.

For example:

$content = [];
$content['featured_artist'] = $variable;

Additionally, considering the DRY (don't repeat yourself) principles, the count(array_filter($_FILES[ statements should be turned into a function, and returned to a variable, which can be inserted into the theoretical $content described above.

The code:

Using the following code as a base, with some re-structuring, and maybe some good old fashioned wizardry, the code can be reborn!

if (count(array_filter($_FILES['featured_artist']['name'])) > 0) {

    $featured_artist_status= true;

    $featured_artist= $_FILES['featured_artist'];
    $featured_artist['tmp_name']['image'] = true;
    $featured_artist_field= Field::incrementFieldComparison($attraction['tmp_name'], $request->post('featured_artist_name'), $request->post('featured_artist_description'));

    $featured_artist_name = implode('|', $featured_artist_field->field_1);
    $featured_artist_description = implode('|', $featured_artist_field->field_2);
    $featured_artist_image = implode('|', $featured_artist_field->reference);
}

Such wizardry may look like this:

$completeFieldDict = [];
foreach ($_FILES as $key => $value){
    if (count(array_filter($value["name"])) > 0){
        $completeFieldDict[$key] = buildField($key);
    }
}
function buildField($name){

    $fieldDictionary = [];
    $fieldDictionary['status'] = true;

    $fieldDictionary['content'] = $_FILES[$name];
    $fieldDictionary['content']['tmp_name']['image'] = true;
    $fieldDictionary['field'] = Field::incrementFieldComparison($fieldDictionary['content']['tmp_name'], $request->post($key . '_name'), $request->post($key . '_description'));

    $fieldDictionary['name']        = implode('|', $fieldDictionary['field']->field_1);
    $fieldDictionary['description'] = implode('|', $fieldDictionary['field']->field_2);
    $fieldDictionary['image']       = implode('|', $fieldDictionary['field']->reference);

    return $fieldDictionary;
}

Although, the limitations of this are notable, in form of such a large dictionary.
But, as previously mentioned this could be greatly improved upon refactoring the structure.

Personally, I'd suggest having a database transaction embedded within buildField(), and not saving variables locally.


PyTorch Unit-testing in Python I'm new to PyTorch and I'm writing a unit test for an activation function I'm making.

I plan to test against a reference implementation for this function. I want to approach this in a test-driven way, so I learned to write a test using a known-good function: the ReLU implementation "MyReLU" from  this beginner tutorial.

The tests passed, but is there any way I can improve the code below? I worry I might not be taking full advantage of PyTorch's libraries and capabilities.

import unittest
import numpy as np
import torch
from torch.autograd import gradcheck
from my_activation_functions import MyReLU

class ReluTest(unittest.TestCase):
    def setUp(self):
        self.relu = MyReLU.apply

    def test_relu_values_x_leqz(self):
        tin_leqz = torch.tensor(np.linspace(-10,0,300))
        tout_leqz = list(self.relu(tin_leqz))
        for x in tout_leqz:
            self.assertEqual(x,0)

    def test_relu_values_x0(self):
        tin_eqz = torch.tensor([0,0,0,0,0])
        tout_eqz = list(self.relu(tin_eqz))
        for x in tout_eqz:
            self.assertEqual(x,0)

    def test_relu_values_x_geqz(self):
        tin_geqz = torch.tensor(np.linspace(0.001,10,300))
        tout_geqz = list(self.relu(tin_geqz))
        test_geqz = list(tin_geqz)
        for ii in range(len(tout_geqz)):
            self.assertEqual(tout_geqz[ii], test_geqz[ii])

    def test_drelu_values(self):
        tin = (torch.randn(20,20,dtype=torch.double,requires_grad=True))
        self.assertTrue(gradcheck(self.relu, tin, eps=1e-6, atol=1e-4))

if __name__ == '__main__':
    unittest.main(verbosity=2)

Since you are asking about PyTorch's capabilities you are not taking advantage of, you might want to use:

torch.linspace(-10,0,300) instead of torch.tensor(np.linspace(-10,0,300))
torch.zeros(5, dtype=torch.long) instead of torch.tensor([0,0,0,0,0])
tensor operations instead of iterating over each element of the tensor in a loop. This might not matter much in unit-tests but is important if you want to get GPU acceleration:

self.assertTrue(torch.equal(tout_leqz, torch.zeros_like(tin_leqz)))


C-ifying a capturing lambda What do you think of this code?

#include <utility>

namespace
{

template <typename F, int I, typename L, typename R, typename ...A>
inline F cify(L&& l, R (*)(A...) noexcept(noexcept(
  ::std::declval<F>()(::std::declval<A>()...))))
{
  static thread_local L l_(::std::forward<L>(l));
  static thread_local bool full;

  if (full)
  {
    l_.~L();

    new (static_cast<void*>(&l_)) L(::std::forward<L>(l));
  }
  else
  {
    full = true;
  }

  struct S
  {
    static R f(A... args) noexcept(noexcept(
      ::std::declval<F>()(::std::forward<A>(args)...)))
    {
      return l_(::std::forward<A>(args)...);
    }
  };

  return &S::f;
}

}

template <typename F, int I = 0, typename L>
F cify(L&& l)
{
  return cify<F, I>(::std::forward<L>(l), F());
}

It can be used to supply a capturing lambda as a C callback:

int main()
{
  int a;

  auto const f(cify<void(*)()>([a]{::std::cout << "a: " << a << ::std::endl;}));

  f();

  return 0;
}

If you need the same callback across multiple threads, or when thread_local is unimplemented in your compiler, you can remove the thread_local keyword.

To fix the problem in the answer, one can make use of the __COUNTER__ macro. It is only one of numerous possible solutions.

std::vector<void(*)()> callbacks;

template <int I>
void add_callback (int x)
{
        callbacks.emplace_back(cify<void(*)(), I>([x] () { std::cout << x; }));
}

int main ()
{
        add_callback<__COUNTER__>(1);
        add_callback<__COUNTER__>(2);

        for (auto& callback : callbacks) {
                callback();
        }
        return 0;
}

There's an existing technique which is not dissimilar referred to as "exception vomiting". Observe:

void f(void(*p)()) {
    p();
}
template<typename F> void real_f(F func) {
    try {
        throw func;
    } catch(...) {
        f([] {
            try {
                throw;
            } catch(F func) {
                func();
            }
        });
    }
}

This abuses the fact that the compiler must provide a thread-local stack for complex objects for use as exception storage, regardless of their support for other thread local features, and therefore enjoys much broad compiler support. The most obvious drawbacks are a) it's horrible, and b) it's limited to stack semantics.

You could consider a less terrible version as follows assuming the relevant compiler support:

template<typename F> void real_f(F func) {
    thread_local std::vector<std::function<void()>> funcs;
    funcs.push_back(func);
    f([] { funcs.back()(); });
    funcs.pop_back();
}

Your cify function is nothing more than real_f as posted here, but a bit more generic and a lot less reliable as you can't handle multiple instances properly even with the use of __COUNTER__.

The general case cannot be solved without the use of a JIT to create new functions at runtime.


Sending periodic heartbeats, wondering if there are any timer bugs What?

heartbeatjs is a small light weight library that helps you run periodic heartbeat functions and detects timeouts when they occur by launching events.

It was mainly designed for tcp/ip connections, but you can use it with any protocol you want as it designed to be generic.

Project

The project page, with examples and documentation can be  seen here:

https://fl4m3ph03n1x.github.io/heartbeatjs/index.html

Code

This library is mostly a collection of getters and setters mixed with some timers. I would like a second opinion regarding the code and any possible bugs one may find, as I am about to release the next version next week.

const isFunction = require("lodash.isfunction");

const DEFAULT_TIMEOUT = 5000;
const DEFAULT_INTERVAL = 3000;

const heartBeatFactory = () => {

    let interval = DEFAULT_INTERVAL,
        timeout = DEFAULT_TIMEOUT,
        ping,
        pong,
        timer,
        lastHeartbeatTime,
        timeoutTimer,
        hasStarted = false;

    const events = {
        timeout: () => {}
    };

    const hasTimedOut = () =>
        Date.now() - lastHeartbeatTime > timeout;

    const getBeatInterval = () => interval;

    const setBeatInterval = newInterval => {
        if(isNaN(newInterval))
            throw new TypeError(`${newInterval} must be a Number.`);

        interval = newInterval;
    };

    const getBeatTimeout = () => timeout;

    const setBeatTimeout = newTimeout => {
        if(isNaN(newTimeout))
            throw new TypeError(`${newTimeout} must be a Number.`);

        timeout = newTimeout;
        clearTimeout(timeoutTimer);
        timeoutTimer = setTimeout(events.timeout, getBeatTimeout());
    };

    const getPing = () => ping;

    const setPing = newPing => {
        ping = newPing;
    };

    const getPong = () => pong;

    const setPong = newPong => {
        pong = newPong;
    };

    const receivedPong = () => {
        lastHeartbeatTime = Date.now();
        clearTimeout(timeoutTimer);
        timeoutTimer = setTimeout(events.timeout, getBeatTimeout());
    };

    const stop = () => {
        lastHeartbeatTime = undefined;
        clearInterval(timer);
        timer = undefined;
        clearTimeout(timeoutTimer);
        timeoutTimer = undefined;
    };

    const start = fn => {
        if (!isFunction(fn))
            throw new TypeError(`${fn} must be a function.`);

        hasStarted = true;
        lastHeartbeatTime = Date.now();
        timer = setInterval(fn, getBeatInterval());
        timeoutTimer = setTimeout(events.timeout, getBeatTimeout());
    };

    const onTimeout = fn => {
        if (!isFunction(fn))
            throw new TypeError(`${fn} must be a function.`);

        events.timeout = fn;
    };

    const isBeating = () => timer !== undefined;

    const reset = () => {
        if (isBeating())
            stop();

        if( hasStarted ){
            setBeatInterval(DEFAULT_INTERVAL);
            setBeatTimeout(DEFAULT_TIMEOUT);
            ping = undefined;
            pong = undefined;
            onTimeout(() => {});
        }
    };

    return Object.freeze({
        getBeatInterval,
        setBeatInterval,
        getBeatTimeout,
        setBeatTimeout,
        hasTimedOut,
        getPing,
        setPing,
        getPong,
        receivedPong,
        setPong,
        stop,
        start,
        reset,
        isBeating,
        onTimeout
    });
};

module.exports = heartBeatFactory;

Just wondering why you don't set hasStarted = false either in the stop() or in the reset() method.

By doing it in the stop() you could omit the second if in the reset() method.

Seeing a reset() method at least screams for resetting all values to default.


Beginnings of a neuron simulation I have 2 classes that do extremely similar things. The only differences are that they draw using different colors, and that one draws in the back.

The classes NeuronOutput and NeuronReciever are very similar.

class NeuronOutput():
    """The place where a Neuron outputs its signal"""
    def __init__(self,neuron):
        self.neuron=neuron

        width=self.neuron.width*0.5
        offset=width/2
        distance=self.neuron.width*0.4
        self.pos=[self.neuron.x+math.sin(self.neuron.angle)*distance,self.neuron.y-math.cos(self.neuron.angle)*distance]
        self.drawRect=[self.pos[0]-offset,self.pos[1]-offset,width,width]
    def draw(self,surf):
        pygame.draw.ellipse(surf,[200,150,200],self.drawRect)
class NeuronReciever():
    """Where a Neuron recieves input"""
    def __init__(self,neuron):
        self.neuron=neuron

        width=self.neuron.width*0.5
        offset=width/2
        distance=self.neuron.width*0.4
        self.pos=[self.neuron.x-math.sin(self.neuron.angle)*distance,self.neuron.y+math.cos(self.neuron.angle)*distance]
        self.drawRect=[self.pos[0]-offset,self.pos[1]-offset,width,width]
    def draw(self,surf):
        pygame.draw.ellipse(surf,[200,255,200],self.drawRect)

There is also a class Neuron that uses instances of the two above classes as properties. It is also fairly similar to them, however it is a bit different in the way it initializes and draws.

class Neuron():
    """A single neuron with Synapses that connect to others"""
    def __init__(self,x,y):
        self.x=x
        self.y=y
        self.pos=[x,y]
        self.width=30
        self.drawOffset=self.width/2;
        self.connections=[]
        self.angle=math.radians(random.randint(0,360))
        self.drawRect=[self.x-self.drawOffset,self.y-self.drawOffset,self.width,self.width]
        self.output=NeuronOutput(self)
        self.input=NeuronReciever(self)

    def draw(self,surf):

        pygame.draw.ellipse(surf,[200,150,50],self.drawRect)
        self.output.draw(surf)
        self.input.draw(surf)
        self.syn.draw(surf)

(The code is the beginning of a neuron simulation.)

I want to keep all these as separate classes for the sake of readability and ease of use. How can I reduce the repetitiveness of this code, while still keeping it readable?

So you have two classes which are very similar and differ only in one thing? Then make a parent class that has all the behavior and let the others inherit from it or add a switch to flip between the two behaviors.

I would probably go with the first. To accomplish this, make self.drawRect a property (and rename it to self.rect, it does not actually draw anything, it just says where to draw):

class NeuronIO:
    """Draw input or output of a neuron.

    Abstract base class.
    """
    color = 0, 0, 0  # default color is black

    def __init__(self, neuron):
        self.neuron = neuron

        width = 0.5 * self.neuron.width
        offset = width / 2
        distance = 0.4 * self.neuron.width
        self._pos = None
        self._rect = None

    def draw(self, surf):
        pygame.draw.ellipse(surf, self.color, self.rect)

    @property
    def pos(self):
        """Needs to be implemented by the child classes"""
        raise NotImplementedError

    @property
    def rect(self):
        if self._rect is None:
            pos = self.pos
            self._rect = pos[0] - offset, pos[1] - offset, width, width
        return self._rect

class NeuronOutput(NeuronIO):
    """The place where a Neuron outputs its signal"""
    color = 200, 150, 200

    @property
    def pos(self):
        if self._pos is None:
            self._pos = (self.neuron.x + math.sin(self.neuron.angle) * distance,
                         self.neuron.y - math.cos(self.neuron.angle) * distance)
        return self._pos

class NeuronReciever(NeuronIO):
    """Where a Neuron receives input"""
    color = 200, 255, 200
    @property
    def pos(self):
        if self._pos is None:
            self._pos = (self.neuron.x - math.sin(self.neuron.angle) * distance,
                         self.neuron.y + math.cos(self.neuron.angle) * distance)
        return self._pos

Here both rect and pos are cached so that they are only calculated the first time they are needed.

In addition I added some whitespace around operators and after commas, as recommended by Python's official style-guide, PEP8.

At this point you should ask yourself: "But how often do I need to draw the receiver or output without also drawing a neuron?". If the answer to that question is "probably never!", then you should just fold all of that functionality into the Neuron class:

import math
import random
import pygame

class Neuron:
    """A single neuron with Synapses that connect to others"""

    def __init__(self, x, y):
        self.x = x
        self.y = y
        self.pos = x, y
        self.width = 30
        self.offset = self.width / 2
        self.connections = []
        self.angle = math.radians(random.randint(0, 360))
        self.rect = (self.x - self.offset, self.y - self.offset,
                     self.width, self.width)
        self.color = 200, 150, 50

    def draw(self, surf):
        pygame.draw.ellipse(surf, self.color, self.rect)
        pygame.draw.ellipse(surf, *self.input)
        pygame.draw.ellipse(surf, *self.output)

    @property
    def input(self):
        color = 200, 255, 200
        rect = self.calc_rect()
        return color, rect

    @property
    def output(self):
        color = 200, 255, 200
        rect = self.calc_rect(output=True)
        return color, rect

    def calc_rect(self, output=False):
        width = 0.5 * self.width
        distance = 0.4 * self.width
        offset = 0.25 * self.width
        if output:
            pos = (self.x + math.sin(self.angle) * distance,
                   self.y - math.cos(self.angle) * distance)
        else:
            pos = (self.x - math.sin(self.angle) * distance,
                   self.y + math.cos(self.angle) * distance)
        return pos[0] - offset, pos[1] - offset, width, width

Here I did not go through the effort of making sure that Neuron.input and Neuron.output are only calculated once, but you could do it in the same way as I did above.

Note that I made all of your lists into tuples (like Neuron.pos). This is just aesthetics atm, but if you want to be able to do e.g. neuron.pos[0] += 1, then you need to make them lists again.


Remove duplicates from an unsorted Linked List in Ruby Currently I'm going over the cracking the coding interview. I'm in the Linked List 2.1 question which is as follow:

  Remove Duplicates, write code to remove duplicates from an unsorted
  Linked List. How would you solve the problem if a temporary buffer is
  not allowed?

I used a Hash, which breaks the temporary buffer not allowed conditioned. Not sure how one can go about solving this without using an extra data structure. The above is the method I used.

# CTCI-2.1: Write code to remove duplicates from an unsorted LinkedList
  def remove_duplicates
    node = @head
    h = Hash.new(0)
    return false unless node.next
    h[@head.data] += 1
    while (node = node.next)
      h[node.data] += 1
      if h[node.data] > 1
        previous_node = find_previous(node.data)
        previous_node.next = previous_node.next.next
      end
    end
  end

This is a \$O(n)\$. How can this be improved? How one will go about solving this without using an additional data structure(Temporary buffer?) Here is the rest of the Linked List:
require 'pry'

class Node
  attr_accessor :next
  attr_reader :data

  def initialize(data)
    @data = data
    @next = nil
  end

  def to_s
    "Node with value: #{data}"
  end
end

class LinkedList
  def initialize
    @head = nil
  end

  def append(value)
    if @head
      find_tale.next = Node.new(value)
    else
      @head = Node.new(value)
    end
  end

  def find_tale
    node = @head

    return node if !node.next
    return node if !node.next while (node = node.next)
  end

  def find(value)
    node = @head

    return false if !node.next
    return node if node.data == value

    while (node = node.next)
      return node if node.data == value
    end
  end

  def append_after(target, value)
    node = find(target)
    return unless node

    old_next = node.next
    node.next = Node.new(value)
    node.next.next = old_next
  end

  def find_previous(value)
    node = @head

    return false if !node.next
    return node if node.next.data == value

    while(node = node.next)
      return node if node.next.data == value
    end
  end

  def delete(value)
    if @head.data == value
      @head = @head.next
    end

    node = find_previous(value)
    node.next = node.next.next
  end

  def display
    node = @head

    puts node
    while (node = node.next)
      puts node
    end
  end

  # CTCI-2.1: Write code to remove duplicates from an unsorted LinkedList
  def remove_duplicates
    node = @head
    h = Hash.new(0)
    return false unless node.next
    h[@head.data] += 1
    while (node = node.next)
      h[node.data] += 1
      if h[node.data] > 1
        previous_node = find_previous(node.data)
        previous_node.next = previous_node.next.next
      end
    end
  end
end

Here is the test I used:

# TEST
list = LinkedList.new

list.append('A')
list.append('B')
list.append('A')
list.append('A')
list.append('C')
list.append('D')

puts "Display the list"

list.display

list.remove_duplicates
puts 'Answer should be A B C D'
list.display

Alternatives

There are other alternatives (spoiler alert) with around the same time complexity, that adhere to the specification of in-place removal.

Review

  This is in \$O(n)\$.

I'm not sure it is. The outer iteration while (node = node.next) is \$O(n)\$.

while (node = node.next)
  h[node.data] += 1
  if h[node.data] > 1
    previous_node = find_previous(node.data)
    previous_node.next = previous_node.next.next
  end
end

And find_previous(data) is \$O(\log{n})\$.

 def find_previous(value)
    node = @head

    return false if !node.next
    return node if node.next.data == value

    while(node = node.next)
      return node if node.next.data == value
    end
 end

This makes remove_duplicates to be \$O(n\log{n})\$.

If you keep track of the previous node while iterating the nodes, you could optimize your algorithm to be \$O(n)\$, but as you are using a hash table, it fails to meet the requirements of the challenge.


pure Python Bézier curve implementation I came up with this recursive pure-Python implementation of De Casteljau's algorithm for computing points on a Bézier curve:

def bezier_curve(control_points, number_of_curve_points):
    return [
        bezier_point(control_points, t)
        for t in (
            i / (number_of_curve_points - 1) for i in range(number_of_curve_points)
        )
    ]

def bezier_point(control_points, t):
    if len(control_points) == 1:
        result, = control_points
        return result
    control_linestring = zip(control_points[:-1], control_points[1:])
    return bezier_point([(1 - t) * p1 + t * p2 for p1, p2 in control_linestring], t)

Assumptions about control_points

The elements of control_points represent the control points of the Bézier curve. They must be of the same type of mutually compatible types fulfilling the following rules:

points shall all be of the same dimension.
Multiplying a point by a scalar shall result in a point of the same
dimension and with a value according to vector-scalar multiplication
(i.e., multiply each of the point's Cartesian coordinates with the scalar)
Adding two points shall result in a point of the same dimension and
with a value according to vector addition (i.e., component-wise
addition of the points' Cartesian coordinates)

Some examples that work as control_points:

list of turtle.Vec2D
list of complex
list of numpy.array with shape (2,)
numpy.array with shape (n, 2), where n is the number of control points

(tuples instead of lists work, too. Probably any sequencey container will work.)

Why pure Python?

Because I want this to be usable in a QGIS plugin, but SciPy, NumPy, etc. (usually) aren't available to QGIS plugins. As I'm unsure which Python libraries are available in QGIS (and the answer to that seems to be platform-dependent), I'd like to avoid external libraries (those that would have to be installed with pip or one of its alternatives) completely.

Using standard library functions should be fine, so if any part of the implementation could benefit from them, please point that out.

What I'd like to know in this review

Could / should the readability and comprehensibility of this implementation be improved?
Did I go foul of any performance (computation speed, memory usage, etc.) no gos? (It doesn't need to be super-fast, but it shouldn't be unnecessarily slow if I can avoid it.)

Performance for low degrees (e.g., degree 2, i.e. cubic Bézier with three control points per curve) will probably be more relevant than performance at high degrees (many control points per curve)
Performance for large outputs (large number_of_curve_points) might be relevant

About the destructuring assignment result, = control_points to unpack the single point while at the same time making sure it really is exactly one point

Is this idiomatic in Python (i.e. "pythonic")?
Is this readable and comprehensible enough or too obscure?
Is there any good alternative that's an expression, i.e. that could be used directly in the return statement without going through an assignment? (control_points[0] is an expression but doesn't fail when there's more than one element in control_points.)

Unnecessary Generator

You've got an unnecessary generator expression here:

def bezier_curve(control_points, number_of_curve_points):
    return [
        bezier_point(control_points, t)
        for t in (
            i / (number_of_curve_points - 1) for i in range(number_of_curve_points)
        )
    ]

You don't need to generate i / (n-1); you could simply pass that argument to the bezier_point() function:

def bezier_curve(control_points, number_of_curve_points):
    return [ bezier_point(control_points, i / (number_of_curve_points - 1))
             for i in range(number_of_curve_points)
           ]

Slight optimization: instead of computing number_of_curve_points - 1 \$O(N)\$ times (pure Python will not cache the result), precompute it:

def bezier_curve(control_points, number_of_curve_points):
    last_point = number_of_curve_points - 1
    return [ bezier_point(control_points, i / last_point )
             for i in range(number_of_curve_points)
           ]

Tail Recursion

Python does not do Tail Call Optimization, so with M control points, you will recursively enter and exit M calls, for each of the N points along your curve.  That is M*N unnecessary stack frame entry/exits.  You should do the looping yourself:

def bezier_point(control_points, t):
    while len(control_points) > 1:
        control_linestring = zip(control_points[:-1], control_points[1:])
        control_points = [(1 - t) * p1 + t * p2 for p1, p2 in control_linestring]
    return control_points[0]

Since we loop while len(control_points) > 1, it should be guaranteed that control_points will only have one point when the loop exits, so return control_points[0] is safe.  The exception is if the function is called with zero control points, but then control_points[0] will properly fail with an IndexError.


Checking the status of a website by hitting a PHP script I currently have a system which loops through a bunch of my website domains and checks their status and sends me a notification letting me know if they are up or down. The stripped down version of the relay code is below, I've removed the email/DB update code etc.

It pings a transponder (a file on the client website server) which returns 'SUCCESS' or whatever the error it has encountered and returns that as a string.

function checkStatus($domain){
  $statusArray = array();

  $url = $domain . '/transponder.php';

  $statusArray['transponder_response'] = file_get_contents($url);

  if($statusArray['transponder_response'] == 'SUCCESS'){
    //all is well
  }else{
    //all is not well
    $statusArray['transponder_code'] = get_headers($url, 1);
    //send code and response via email to admin
  }

  return $statusArray;
}

The transponder code is such:

$dbInfo['host'] = '';
$dbInfo['dbname'] = '';
$dbInfo['user'] = '';
$dbInfo['pass'] = '';

$link = mysqli_connect($dbInfo['host'], $dbInfo['user'], $dbInfo['pass'], $dbInfo['dbname']);

if(mysqli_connect_errno()){
  printf('Connect failed: %s\n', mysqli_connect_error());
  exit();
}

if($result = mysqli_query($link, 'SELECT 1')){
  if(mysqli_num_rows($result)) echo 'SUCCESS';
  mysqli_free_result($result);
}

mysqli_close($link);

A 'SUCCESS' response means that the site domain is resolving, PHP is working and DB is working.

Can anyone point me in a direction to make this more accurate and effective for site monitoring?

When you say checkStatus($domain), the parameter should either be actually just a domain (without protocol identifier), or you should just specify the full URI. When in doubt, you shouldn't hardcode /transponder.php.

As for the task of status monitoring - testing the database connectivity is certainly not wrong, but keep in mind that far more can go wrong. If you really want to check if the site is working, don't test against a minimal test script, but a real page of that site. Preferably one which does involve as many systems as possible, and check if the output is the expected.

Why? As I said before, there are more possible errors than just the database connection. Commonly you can also run into problem with the session storage, failing server side caches (APC, memcached and alike), resource shortages (e.g. PHP going OOM), and various more.

You should probably also record response times, not only response status. If your server fails to respond timely, it's a warning sign that you should react to immediately before there is a complete outage.

Last, but not least, don't just test a single status page. If you have access to it, rather check the error logs if there were any events in the past interval. Especially during load spikes, not necessarily every request is going to fail. But an accumulation of failures can be easily read from the logs, and should be dealt with. Make sure your application does log warnings and errors properly.


Saving the Universe - Code Jam 2008 Qualification problem A I'm trying to brush up on my Python, so I'm going through some simple problems.  Big concerns are style, though any algorithmic problems I'm open to suggestions on.

Basically you have an input file where the first line is the number of test cases.

The following lines are composed of the cases. Each case starts with a number S followed by S unique lines representing search engines.  Then there is a number Q followed by Q lines representing queries. Each query is the name of a search engine. You have to run the queries in sequence, but you can batch them up; however you cannot run a query on the corresponding search engine. The program should return the minimum number of switches between engines required.  I know that the result is correct (passes the tests on the site), so again focus is mainly style (not just PEP, but also pythonic-ness so to speak).

Source: Problem A. Saving the Universe

import fileinput

def solve(inFile):
    #discard list of possible values
    S = int(next(inFile))
    for i in range(0,S): #Feels wrong doing this one at a time
        next(inFile)

    count = 0
    Q = int(next(inFile))
    queue = set()
    #iterate over queries, adding each one to a set until the set contains all
    #S possible queries. Then increment count and reset set to only include
    #the newest query since we had to have cleared the queue before it arrived
    while Q > 0 :
        Q -= 1
        temp = next(inFile)
        queue.add(temp)
        if len(queue) == S:
            queue -= queue   #Feels wrong but queue=set(temp) doesn't work
            queue.add(temp)
            count += 1
    return count

if __name__ == "__main__":
    inFile = fileinput.input()
    T = int(next(inFile))
    for i in range(1,T+1):
        print("Case #", end="") #Feels wrong having four prints
        print(i, end="")
        print(": ", end="")
        print(solve(inFile))

#Feels wrong having four prints

Indeed.

    print "Case %d: %d" % (i, solve(inFile))

is good enough.
#Feels wrong but queue=set(temp) doesn't work

Yes. The set constructor requires an iterable. Consider queue = set([temp]).
#Feels wrong doing this one at a time

I don't think you can do anything about this. As a side note, what really feels wrong is that these parts of the test cases are totally irrelevant to the solution; only the number of engines really matter. Kudos for realizing this.

As usual, a no naked loops principle dictates to factor this loop out into a function:

    def discard_names_as_useless(inFile, S):

Naming

Q and S only make sense for those who read the problem statement. Consider instead something like number_of_queries and number_of_engines.
import fileinput

Using fileinput seems like an overkill. It is only useful if you need to process multiple files in a row. In this case an unadorned raw_input() would work just fine.
PS

Be careful describing the problem statement. Your description

  The program should return the minimum number of batches required.

raised an immediate red flag: this code must be off-by-one. I had to read the original to realize that they ask for the number of _engine switches__.


Program to generate numbers, finding average and largest values and rounding the largest to 100 As this code is not in English, I will explain it first:

Creates ten numbers in an array between 0 and 100 as student grades.
Calculates the largest number and average value of these ten numbers in the array.
100, the largest grade, is subtracted from the largest number in the array. (e.g 100-96=4)
New value (which is 4 in this case, explained in example above) is added to all numbers in the array. It's also printed just below average value and largest value.
New values in the array that are unable to surpass the average value are printed down below, which prints a sentence in Turkish that roughly translates to:

"%dth student with %d grade has failed the lesson by staying under the average value."

#include<time.h>
#include<stdio.h>
#include<stdlib.h>

int main() {

int sum=0,fark,avg,imax;
int puan[10];
srand(time(NULL));
for(int i=0; i<10; i++)
{
    puan[i]=rand()%101;
    printf("%d. ogrencinin notu = %d ",i+1,puan[i]);
    printf("\n");
    if(i==0)
    {
        imax=puan[i];
    }
    else if(puan[i]>imax)
        {
            imax=puan[i];
        }
    sum += puan[i];
}
avg=sum/10;
fark= 100 - imax;
printf("\nEn yuksek not = %d ve not ortalamasi %d.\n",imax,avg);
printf("Her ogrencinin notuna %d puan eklendi.\n",fark);
printf("\n");
for(int i=0;i<10;i++)
{
    puan[i] += fark;
}
for(int i=0;i<10;i++)
{
    if(puan[i]<avg)
    {
        printf("%d. ogrenci aldigi %d notuyla ortalamanin altinda kalarak dersten basarisiz olmustur.\n",i+1,puan[i]);
    }
}
return 0;
}

Can I shorten this code? Is there a shorter way to make it work in the same way? This was the best my friend and I could come up with.

Just a quick approach. It could be improved more. What you want is a way to reduce the two loops into one.

Some comments about the code: We initialized imax in order to reduce the if -else statement and also we combined the 2 foor loops in the end into one. What I would like to achieve even more is to reduce the final two for loops into one. But I think this is not possible for this case. But I might be wrong.

#include <time.h>
#include <stdio.h>
#include <stdlib.h>

int main() {
    int sum = 0, imax = 0;
    int puan[10];
    srand(time(NULL));
    for(int i=0; i<10; i++)
    {
        puan[i]=rand()%101;
        printf("%d. ogrencinin notu = %d \n",i+1,puan[i]);
        if(puan[i]>imax)
        {
            imax=puan[i];
        }
        sum += puan[i];
    }
    float avg=sum/10.0f;
    int fark = 100 - imax;
    printf("\nEn yuksek not = %d ve not ortalamasi %f.\n",imax,avg);
    printf("Her ogrencinin notuna %d puan eklendi.\n\n",fark);
    for(int i=0;i<10;i++)
    {
        puan[i] += fark;
        if(puan[i]<avg)
        {
                printf("%d. ogrenci aldigi %d notuyla ortalamanin altinda kalarak dersten basarisiz olmustur.\n",i+1,puan[i]);
        }
    }

    return 0;
}


Finding the missing value in a proportion Basically, I'd like to be able to solve a proportion after entering in all but one term. Specifically, I'm working on creating a calculator for the Combined Gas Law.

$$\frac{P_1\cdot V_1}{T_1} = \frac{P_2\cdot V_2}{T_2}$$

Right now, what I have is a program that takes in two double arrays for the original and final values, and a String that represents which term to solve for. However, this seems really repetitive. Is there a way to solve for the "missing term" or even create a missing term directly without having to involve many "if" or "switch" statements? At least if I could do a "for-each loop" to find the specific term that I had to solve for, and then it would "fill-in the proportion"

    public static double CombinedGasLaw ( double [] original, double [] end, String query)
{
    double p1 = original[0];
    double v1 = original[1];
    double t1 = original[2];
    double p2 = end[0];
    double v2 = end[1];
    double t2 = end[2];
    switch (query.toLowerCase())
    {
        case "v":
            return p1*v1*t2/(t1*p2);
        case "p":
            return p1*v1*t2/(t1*v2);
        case "t":
            return p2*v2*t1/(p1*v1);
    }
}
   public static void main(String[] args)
{
    double p1 = 1;
    double v1 = 1;
    double t1 = 1;
    double [] original = {p1, v1, t1};
    double p2 = 1;
    double v2 = 1;
    double t2 = 1;
    double [] end = {p2, v2, t2};
    String search = "v";
    double fin = CombinedGasLaw(original, end, search);
    System.out.println("All of the original input except for the missing value");
    System.out.println("the missing value");
}

All in all, is there a way to create a similar program or improve this such that I ONLY need to enter necessary values, and the remaining value could be returned

public static double CombinedGasLaw ( double [] original, double [] end, String query)

As a general rule, methods have verb names.  Noun names represent objects or classes.

To get what you want, you need a way to represent a missing value.  One possibility in Java is a null.  Consider

public class CombinedGasLaw {

    private double pressure;
    private double volume;
    private double temperature;
    private double constant;

    public CombinedGasLaw(double pressure, double volume, double temperature) {
        this.pressure = pressure;
        this.volume = volume;
        this.temperature = temperature;
        constant = pressure * volume / temperature;
    }

    public CombinedGasLaw(double[] values) {
        this(values[0], values[1], values[2]);
    }

    public double getConstant() {
        return constant;
    }

    public static int findIndexOfNull(Object [] values) {
        for (int i = 0; i < values.length; i++) {
            if (values[i] == null) {
                return i;
            }
        }

        return -1;
    }

    public Double solve(Double [] end) {
        switch (findIndexOfNull(end)) {
        case 0:
            return constant * end[2] / end[1];
        case 1:
            return constant * end[2] / end[0];
        case 2:
            return end[0] * end[1] / constant;
        }

        return null;
    }

    public static void main(String[] args) {
        CombinedGasLaw original = new CombinedGasLaw(1, 1, 1);
        Double [] end = {1.0, null, 1.0};
        Double fin = original.solve(end);
        System.out.println("The missing value:  " + fin);
    }

}

Now we have a class named CombinedGasLaw and we can name the method solve, as in solve for the missing value.

We pass the array of values with one missing.

We search for the missing value with a for loop.  Note that this will break if there is more than one missing value.

This version also allows us to find multiple triples and only calculates the constant value once.

There are two constructors because I don't know how you get your input.  I wouldn't use an array for original but perhaps you had some reason to do so.  So I made it so that you can call the constructor with an array.  If so, it just calls the other constructor.  I don't use that constructor though, so you could just delete it.

If there is a name used both by an object field and a method parameter, Java defaults to using the parameter.  You can override this by saying this.pressure, which tells it to use the field instead.  So one easy way to assign values to fields in a constructor is to use the same name for both sides.

The this. is not needed any other time, although you can use it if you want.  It's basically saying that it is the pressure associated with the current object.  Absent a method parameter with the same name, that's the default, so you don't normally need it.  Documentation.


How can I clean up (DRY) this verbose jQuery code? I had posted this on StackOverflow — someone pointed me here.

My code is working, but it's incredibly verbose and I know that it can be made more compact. I just don't know how :-)

I'm building an audio portfolio. When a button is clicked, a sequence of events happens. When another button is clicked, all other active buttons are killed and the sequence for that specific button runs.

The buttons are invisible and are placed on a visualisation of a switch. When clicked, an image of the switch flicked into its "activated" state has its class of "display: none" removed. That should give the user the impression that actually flicking a switch starts playing audio.

Like so:

$(function(){

  // FIRST BUTTON
  $('.button01').click(function() {
    if ($('.switch01').hasClass('activated')){

    // So if button.button01 is clicked and img.switch01 has class "activated"

      // Kill all audio
      $('audio').each(function(){ this.pause(); this.currentTime = 0; });

      // Turn this switch off
      $('.switch01').removeClass('activated');

      // Kill all info cards showing the playback controls
      $('.audio-info-card').addClass('d-none');
    } else {

    // If button.button01 is clicked and img.switch01 DOESN'T have class "activated"

      // Turn all other switches off
      $('.switch02, .switch03').removeClass('activated');

      // Kill all info cards
      $('.audio-info-card').addClass('d-none');

      // Activate this switch and info card
      $('.switch01').addClass('activated');
      $('.audio-info-card#card01').removeClass('d-none');

      // Kill all audio
      $('audio').each(function(){ this.pause(); this.currentTime = 0; });

      // Start this audio
      $('#audio01-player')[0].play();
    }
  });

  // SECOND BUTTON
  $('.button02').click(function() {
    if ($('.switch02').hasClass('activated')){

    // So if button.button02 is clicked and img.switch02 has class "activated"

      // Kill all audio
      $('audio').each(function(){ this.pause(); this.currentTime = 0; });

      // Turn this switch off
      $('.switch02').removeClass('activated');

      // Kill all info card showing the playback controls
      $('.audio-info-card').addClass('d-none');
    } else {

    // If button.button02 is clicked and img.switch02 DOESN'T have class "activated"

      // Turn all other switches off
      $('.switch01, .switch03').removeClass('activated');

      // Kill all info cards
      $('.audio-info-card').addClass('d-none');

      // Activate this switch and info card
      $('.switch02').addClass('activated');
      $('.audio-info-card#card02').removeClass('d-none');

      // Kill all audio
      $('audio').each(function(){ this.pause(); this.currentTime = 0; });

      // Start this audio
      $('#audio02-player')[0].play();
    }
  });

There are 16 buttons. I realize this code is stupid but JS / jQuery isn't my strong suit :-D

Fortunately, the code works, but any help making this simpler would be greatly appreciated!

Factor out detailed code into subfunctions

One relatively mechanical thing you could do is transform all of the commented blocks into functions. For a small section, something like this

  // Kill all audio
  $('audio').each(function(){ this.pause(); this.currentTime = 0; });

  // Turn this switch off
  $('.switch01').removeClass('activated');

  // Kill all info cards showing the playback controls
  $('.audio-info-card').addClass('d-none');

becomes

  killAllAudio();
  turnSwitchOff( '.switch01' );
  killAllInfoCards();

  //...

function killAllAudio(){
  $('audio').each(function(){ this.pause(); this.currentTime = 0; });
}

function turnSwitchOff( switch ){
  $(switch).removeClass('activated');
}

// Kill all info cards showing the playback controls
function killAllInfoCards(){
  $('.audio-info-card').addClass('d-none');
}

After this, your top-level algorithm should be much clearer. At the top level it should almost look like pseudocode, except the comments are all calls to subfunctions which do one small named task.

Factoring out code into subfunctions also helps you see "more code" by abstracting away small details. With shorter functions, you can see more of them on the screen at once.


Implementation of API to create a company account in a database I'm working on an API that has a lot of controller functions like this:

def create = Action.async { implicit request =>

    if (request.body.asJson.isEmpty) {
        Future.successful(BadRequest("Missing body"))
    }
    else {
        val body = request.body.asJson.get.as[JsObject]
        val companyID = (body \ "company" \ "id").validate[String]
        val parsedAccount = (body \ "account").validate[Account]

        // Check that we have all of the fields we need
        if (parsedAccount.isError) {
            Future.successful(BadRequest("Missing account data"))
        } else if (companyID.isError) {
            Future.successful(BadRequest("Missing company data"))
        }
        else {

            // Insert the new account
            val account = parsedAccount.get
            (for {
                _ <- primaryDAO.insert(account, companyID.get)
                account <- primaryDAO.get(account.id)
            } yield account).map {
                case account => Created(account)
            }.recover {
                case e => BadRequest(e)
            }
        }
    }
}

I was hoping that I would be able to do something more like this (using early returns):

def create = Action.async { implicit request =>

    if (request.body.asJson.isEmpty) {
        return Future.successful(BadRequest("Missing body"))
    }

    val body = request.body.asJson.get.as[JsObject]
    val companyID = (body \ "company" \ "id").validate[String]
    val parsedAccount = (body \ "account").validate[Account]

    // Check that we have all of the fields we need
    if (parsedAccount.isError) {
        return Future.successful(BadRequest("Missing account data"))
    }
    if (companyID.isError) {
        return Future.successful(BadRequest("Missing company data"))
    }

    // Insert the new account
    val account = parsedAccount.get
    (for {
        _ <- primaryDAO.insert(account, companyID.get)
        account <- primaryDAO.get(account.id)
    } yield account).map {
        case account => Created(account)
    }.recover {
        case e => BadRequest(e)
    }
}

However this is not possible because the return statement only returns from my nested function (and back into the Action.async)

I am wondering what I can do in place of early returns (which I would use in imperative programming languages) to make my code cleaner.

The primary DAO implements a generic trait that I use for most of my DAOs and looks like this:

trait DAOGet[A <: BaseModel] {
    def get(pk: String): Future[Option[A]]
    def all: Future[Seq[A]]
    def all(page: Int, perPage: Int): Future[Seq[A]]
}

trait DAOInsert[A <: BaseModel] extends DAOGet[A] {
    def insert(model: A): Future[Any]
}

trait DAOUpdate[A <: BaseModel] extends DAOGet[A] {
    def update(model: A): Future[Int]
}

trait DAODelete[A <: BaseModel] {
    def delete(pk: String): Future[Int]
}

trait CRUDDAO[A <: BaseModel] extends DAOGet[A] with DAOInsert[A] with DAOUpdate[A] with DAODelete[A]

Prelude

I'll assume the following code :

trait BaseModel{
  def pk:String
  def id:String=pk
}
case class Account(name:String) extends BaseModel{
  override val pk = name
}

As the internal structure of the Account class. I made it extends BaseModel so it can be used with the following fake DAO and my sample compiles. I had to add an insert which takes both an Account and a company Id.

class AccountDAO extends CRUDDAO[Account]{
  override def insert(model: Account): Future[Any] = Future.successful(model)

  def insert(model: Account,companyId:String): Future[Any] = model.id match{
    case "account1"|"account2" => Future.successful(model)
    case _ => Future.failed(new RuntimeException("Unable to save account"))
  }

  override def update(model: Account): Future[Int] = ???

  override def get(pk: String): Future[Option[Account]] = pk match {
    case "account1" => Future.successful(Some(Account(pk)))
    case "account2" => Future.successful(None)
    case _ => Future.failed(new RuntimeException("no such account"))
  }

  override def all: Future[Seq[Account]] = ???

  override def all(page: Int, perPage: Int): Future[Seq[Account]] = ???
}

I made the original code sample compile by adapting the bottom of the code as Created(account) wouldn't compile here is what it looks like.

Initial code

class Companies @Inject()(primaryDAO:AccountDAO)(implicit ec:ExecutionContext) extends Controller {
  implicit val AccountReads = Json.format[Account]

  def create = Action.async { implicit request =>
    if (request.body.asJson.isEmpty) {
      Future.successful(BadRequest("Missing body"))
    }
    else {
      val body = request.body.asJson.get.as[JsObject]
      val companyID = (body \ "company" \ "id").validate[String]
      val parsedAccount = (body \ "account").validate[Account]

      // Check that we have all of the fields we need
      if (parsedAccount.isError) {
        Future.successful(BadRequest("Missing account data"))
      } else if (companyID.isError) {
        Future.successful(BadRequest("Missing company data"))
      }
      else {
        // Insert the new account
        val account = parsedAccount.get
        (for {
          _ <- primaryDAO.insert(account, companyID.get)
          account <- primaryDAO.get(account.id)
        } yield account).map {
          case Some(a) => Created(a.id)
          case None => InternalServerError("Unable to create Account")
        }.recover {
          case e => BadRequest(e.getMessage)
        }
      }
    }
  }
}
object Companies extends Companies(new AccountDAO)(play.api.libs.concurrent.Execution.defaultContext)

Following are some sample request/reponses using httpie:

$> echo '{"company":{"id":"1"}, "account":{"name":"account1"}}'| http :9000/foobar
HTTP/1.1 201 Created
Content-Length: 8
Content-Type: text/plain; charset=utf-8
Date: Thu, 28 Apr 2016 20:17:19 GMT

account1

$> echo '{"company":{"id":"1"}, "account":{"name":"account2"}}'| http :9000/foobar
HTTP/1.1 500 Internal Server Error
Content-Length: 24
Content-Type: text/plain; charset=utf-8
Date: Thu, 28 Apr 2016 20:18:45 GMT

Unable to create Account

$> echo '{"company":{"id":"1"}, "account":{"name":"account3"}}'| http :9000/foobar
HTTP/1.1 400 Bad Request
Content-Length: 22
Content-Type: text/plain; charset=utf-8
Date: Thu, 28 Apr 2016 20:21:19 GMT

Unable to save account

$> echo '{ "account":{"name":"account3"}}'| http :9000/foobar
HTTP/1.1 400 Bad Request
Content-Length: 20
Content-Type: text/plain; charset=utf-8
Date: Thu, 28 Apr 2016 20:21:44 GMT

Missing company data

$> echo '{"company":{"id":"1"}, "account":{}}'| http :9000/foobar
HTTP/1.1 400 Bad Request
Content-Length: 20
Content-Type: text/plain; charset=utf-8
Date: Thu, 28 Apr 2016 20:22:07 GMT

Missing account data

Embracing the HTTP framework

Now there is a way to write this using early returns, I'll show it for completeness sake but you really really don't want to do that as I'll explain below. Return doesn't have a type so you are forced to explicitely provide a return type which is impossible in an anonymous function (the block after Action.async is just an anonymous function). You can easily extract your code to a named method with explicit types and use that as the action body:

class Companies @Inject()(primaryDAO:AccountDAO)(implicit ec:ExecutionContext) extends Controller {
  implicit val AccountReads = Json.format[Account]

  def doCreate(implicit request:Request[AnyContent]):Future[Result]={
    if (request.body.asJson.isEmpty) {
      return Future.successful(BadRequest("Missing body"))
    }

    val body = request.body.asJson.get.as[JsObject]
    val companyID = (body \ "company" \ "id").validate[String]
    val parsedAccount = (body \ "account").validate[Account]

    // Check that we have all of the fields we need
    if (parsedAccount.isError) {
      return Future.successful(BadRequest("Missing account data"))
    }
    if (companyID.isError) {
      return Future.successful(BadRequest("Missing company data"))
    }

    // Insert the new account
    val account = parsedAccount.get
    (for {
      _ <- primaryDAO.insert(account, companyID.get)
      account <- primaryDAO.get(account.id)
    } yield account).map {
      case Some(a) => Created(a.id)
      case None => InternalServerError("Unable to create Account")
    }.recover {
      case e => BadRequest(e.getMessage)
    }
  }
  def create = Action.async(doCreate)
}
object Companies extends Companies(new AccountDAO)(play.api.libs.concurrent.Execution.defaultContext)

I said that doing this is wrong and you really don't want to do this. Quoting Rob Norris (tpolecat) :

  If you find yourself in a situation where you think you want to return early, you need to re-think the way you have defined your computation

So let's do some rethinking :) I'll posit that What you really want isn't so much using early returns as it is avoiding deep nesting of if/else clauses.

Let's have a look at the types we are manipulating :
request.body.asJson returns an Option[JsValue]. The current implementation tests to check if it is empty and returns a
BadRequest. Play offers a similar and much cleaner way to check that you actually receive an application/json body for your endpoint (I'll leave the wrapping code to concentrate on the action itself for now) using a specific body parser:

def create = Action.async(parse.json) { implicit request =>
 val body = request.body
 val companyID = (body \ "company" \ "id").validate[String]
 val parsedAccount = (body \ "account").validate[Account]

 // Check that we have all of the fields we need
 if (parsedAccount.isError) {
   Future.successful(BadRequest("Missing account data"))
 }
 if (companyID.isError) {
   Future.successful(BadRequest("Missing company data"))
 } else {
   // Insert the new account
   val account = parsedAccount.get
   (for {
     _ <- primaryDAO.insert(account, companyID.get)
     account <- primaryDAO.get(account.id)
   } yield account).map {
     case Some(a) => Created(a.id)
     case None => InternalServerError("Unable to create Account")
   }.recover {
     case e => BadRequest(e.getMessage)
   }
 }
}

Using a body parser in the action will enforce the media type for the endpoint (in this case it will have to be a form of json). Trying to call it with a content type such as application/x-www-form-urlencoded will fail with a 415 Unsupported Media Type error, passing an invalid json body will yield a 400 BadRequest for you :

$> echo 'coucou'| http --form :9000/foobar
HTTP/1.1 415 Unsupported Media Type
Content-Length: 2163
Content-Type: text/html; charset=utf-8
Date: Thu, 28 Apr 2016 20:41:15 GMT

$> echo "coucou"| http :9000/foobar
HTTP/1.1 400 Bad Request
Content-Length: 2289
Content-Type: text/html; charset=utf-8
Date: Thu, 28 Apr 2016 20:43:30 GMT

Embracing the Json library

The next step consists of leveraging play-json's validation facilities. First let's define a reader which enforces all the protocol constraints for your endpoint :

import play.api.libs.json._
import play.api.libs.functional.syntax._
implicit val CreateDTOReads =
 (
   (__ \ "company" \ "id").read[String] and
   (__ \ "account").read[Account]
 ).tupled

Now we can use that to fully validate the incoming payload and reject it if it is incorrect:

def create = Action.async(parse.json) { implicit request =>
  val createDto:JsResult[(String,Account)] = request.body.validate(CreateDTOReads)
  // Check that we have all of the fields we need
  if (createDto.isError) {
    Future.successful(BadRequest("Missing account or company data"))
  } else {
    // Insert the new account
    val (companyId,account) = createDto.get
    (for {
      _ <- primaryDAO.insert(account, companyId)
      account <- primaryDAO.get(account.id)
    } yield account).map {
      case Some(a) => Created(a.id)
      case None => InternalServerError("Unable to create Account")
    }.recover {
      case e => BadRequest(e.getMessage)
    }
  }
}

Notice that at this point we have lost a bit of precision since I don't distinguish between the two errors anymore. The information is still there, captured in the errors of the JsResult. You could use pattern matching or even a cast to get a JsError out of the JsResult and once you have a JsError you get the list of all validation errors for each path which you can manipulate and translate as you like. For instance :

def create = Action.async(parse.json) { implicit request =>
    val createCommand:JsResult[(String,Account)] = request.body.validate(CreateDTOReads)
    // Check that we have all of the fields we need
    if (createCommand.isError) {
      val errors = createCommand.asInstanceOf[JsError]
      Json.prettyPrint(JsError.toJson(errors))
      Future.successful(BadRequest(Json.prettyPrint(JsError.toJson(errors))))
    } else {
      // Insert the new account
      val (companyId,account) = createCommand.get
      (for {
        _ <- primaryDAO.insert(account, companyId)
        account <- primaryDAO.get(account.id)
      } yield account).map {
        case Some(a) => Created(a.id)
        case None => InternalServerError("Unable to create Account")
      }.recover {
        case e => BadRequest(e.getMessage)
      }
    }
  }

returns something like :

$> echo '{"coucou":""}'| http :9000/foobar
HTTP/1.1 400 Bad Request
Content-Length: 173
Content-Type: text/plain; charset=utf-8
Date: Fri, 29 Apr 2016 08:41:35 GMT

{
  "obj.account" : [ {
    "msg" : [ "error.path.missing" ],
    "args" : [ ]
  } ],
  "obj.company.id" : [ {
    "msg" : [ "error.path.missing" ],
    "args" : [ ]
  } ]
}

This is still not looking very nice since this is not the idiomatic way to extract information from a JsResult. The proper way is to fold over the JsResult. The fold method signature on a JsResult is fold[X](errors: (Seq[(JsPath, Seq[ValidationError])]) => X, valid: (A) => X): X). In our case we want X to be a Future[JsResult], and can write it like this :

def create = Action.async(parse.json) { implicit request =>
  val createCommandResult:JsResult[(String,Account)] = request.body.validate(CreateDTOReads)
  // Check that we have all of the fields we need
  createCommandResult.fold(
    errors => Future.successful(BadRequest(Json.prettyPrint(JsError.toJson(errors)))),
    createCommand => {
      val (companyId,account) = createCommand
      (for {
        _ <- primaryDAO.insert(account, companyId)
        account <- primaryDAO.get(account.id)
      } yield account).map {
        case Some(a) => Created(a.id)
        case None => InternalServerError("Unable to create Account")
      }.recover {
        case e => BadRequest(e.getMessage)
      }
    }
  )
}

Single Responsibility Principle

Now we are getting there but the valid case is not looking so good. This is because the create action handles too many responsibilities. At the REST endpoint level you should only handle HTTP protocol concerns :

content negotiation
deserialization of the payload (can include some validation)
serialization of the responses

Let's extract the business logic, however simple, to an AccountService class :

@Singleton
class AccountService @Inject() (primaryDAO: AccountDAO){
  def createAccount(companyId:String,account:Account)(implicit ec: ExecutionContext) : Future[Option[Account]] =
    for {
      _ <- primaryDAO.insert(account, companyId)
      account <- primaryDAO.get(account.id)
    } yield account
}

Now our endpoint only handles the HTTP translation logic :

def create = Action.async(parse.json) { implicit request =>
  val createCommandResult:JsResult[(String,Account)] = request.body.validate(CreateDTOReads)
  // Check that we have all of the fields we need
  createCommandResult.fold(
    errors => Future.successful(BadRequest(Json.prettyPrint(JsError.toJson(errors)))),
    createCommand => {
      val (companyId,account) = createCommand
      val createdAccountF: Future[Option[Account]] = accountService.createAccount(companyId, account)
      createdAccountF.map {
        case Some(a) => Created(a.id)
        case None => InternalServerError("Unable to create Account")
      }.recover {
        case e => BadRequest(e.getMessage)
      }
    }
  )
}

The error handling code:

createdAccountF.map {
  case Some(a) => Created(a.id)
  case None => InternalServerError("Unable to create Account")
}.recover {
  case e => BadRequest(e.getMessage)
}

Is a good candidate for abstraction. If you wanted to always return Json for instance you could have the following  :

object JsonResultMapper extends Results {
  import play.api.libs.json.Writes

  def jsonOk[A](subject: A)(implicit writer: Writes[A]) = Ok(Json.toJson(subject))

  def jsonNotfound(msg: String) = NotFound(Json.obj("reason" -> msg))

  def exception2Location(exception: Exception): String =
    Option(exception.getStackTrace)
      .flatMap(_.headOption)
      .map(_.toString)
      .getOrElse("unknown")

  def jsonInternalServerError(msg: String, cause: Exception) = {
    val jsonMsg = Json.obj(
      "reason" -> msg,
      "location" -> exception2Location(cause)
    )
    InternalServerError(jsonMsg)
  }

  def toJsonResult[A](subjectOptionFuture: Future[Option[A]],noneMsg: => String = "NotFound")
                             (implicit writer: Writes[A]): Future[SimpleResult] = {
    subjectOptionFuture.map {
      case Some(subject) => jsonOk(subject)
      case None          => jsonNotfound(noneMsg)
    }.recover {
      case e: Exception => jsonInternalServerError(e.getMessage, e)
    }
  }
}

and then write your action as  :

def create = Action.async(parse.json) { implicit request =>
  val createCommandResult:JsResult[(String,Account)] = request.body.validate(CreateDTOReads)
  // Check that we have all of the fields we need
  createCommandResult.fold(
    errors => Future.successful(BadRequest(Json.prettyPrint(JsError.toJson(errors)))),
    createCommand => {
      val (companyId,account) = createCommand
      val createdAccountF: Future[Option[Account]] = accountService.createAccount(companyId, account)
       JsonResultMapper.toJsonResult(createdAccountF, s"Unable to create account")
    }
  )
}

You could stop there and I will for the purpose of this review, but there are still things which can probably be improved. I'll give you a couple leads to further improve:

PrimaryDao protocol

The Future[Option[Account]] may not be a good signature for primaryDAO.get(account.id) or for AccountService#Create.

As you can see, it has 2 errors paths and 1 happy path. However when serializing it, the happy path and first error path are processed together in the same block, then the second error path (exception raised) in a different block.

Some would argue that one error path is a business error while the other is a technical error which makes it ok.

Whether we hide this in the ResultMapper or not I personally don't like it. To get rid of it an depending on your team standards, you can go for:

A BusinessException such as AccountNotFound which is thrown instead of returning an Option
A custom composition of Future and Option (see http://www.edofic.com/posts/2014-03-07-practical-future-option.html and http://loicdescotte.github.io/posts/scala-compose-option-future/ )
A ScalaZ monad transformer which does the same as the previous option in a generic way

In the same logic Future[Any] is not a very good signature for :

trait DAOInsert[A <: BaseModel] extends DAOGet[A] {
  def insert(model: A): Future[Any]
}

I strongly suggest changing that to

trait DAOInsert[A <: BaseModel] extends DAOGet[A] {
  def insert(model: A): Future[A]
}

and having the insert return the saved instance if it is possible. This would allow you to distinguish between : there was an error while inserting vs I couldn't read the instance which are not necessarily the same.

Using specific types

customerId is a string which carries very little information, creating and using a CustomerId type would probably prove very useful if it is used throughout your application.

Disclaimer

I don't know enough of the business to properly name things in my refactoring. Naming is probably the single most important thing when writing code and it is known to be one of the hardest (with invalidating caches).

Final code

package controllers.company

import scala.concurrent.{ExecutionContext, Future}

import com.google.inject.{Inject, Singleton}
import play.api.libs.json.Json
import play.api.mvc.{Action, Controller}

trait BaseModel {
  def pk: String

  def id: String = pk
}

case class Account(name: String) extends BaseModel {
  override val pk = name
}

trait DAOGet[A <: BaseModel] {
  def get(pk: String): Future[Option[A]]

  def all: Future[Seq[A]]

  def all(page: Int, perPage: Int): Future[Seq[A]]
}

trait DAOInsert[A <: BaseModel] extends DAOGet[A] {
  def insert(model: A): Future[Any]
}

trait DAOUpdate[A <: BaseModel] extends DAOGet[A] {
  def update(model: A): Future[Int]
}

trait DAODelete[A <: BaseModel] {
  def delete(pk: String): Future[Int]
}

trait CRUDDAO[A <: BaseModel] extends DAOGet[A] with DAOInsert[A] with DAOUpdate[A]

class AccountDAO extends CRUDDAO[Account] {
  override def insert(model: Account): Future[Any] = Future.successful(model)

  def insert(model: Account, companyId: String): Future[Any] = model.id match {
    case "account1" | "account2" => Future.successful(model)
    case _ => Future.failed(new RuntimeException("Unable to save account"))
  }

  override def update(model: Account): Future[Int] = ???

  override def get(pk: String): Future[Option[Account]] = pk match {
    case "account1" => Future.successful(Some(Account(pk)))
    case "account2" => Future.successful(None)
    case _ => Future.failed(new RuntimeException("no such account"))
  }

  override def all: Future[Seq[Account]] = ???

  override def all(page: Int, perPage: Int): Future[Seq[Account]] = ???
}

@Singleton
class AccountService @Inject() (primaryDAO: AccountDAO){
  def createAccount(companyId:String,account:Account)(implicit ec: ExecutionContext) : Future[Option[Account]] =
    for {
      _ <- primaryDAO.insert(account, companyId)
      account <- primaryDAO.get(account.id)
    } yield account
}

@Singleton
class Companies @Inject()(accountService: AccountService)(implicit ec: ExecutionContext) extends Controller {
  implicit val AccountReads = Json.format[Account]

  import play.api.libs.functional.syntax._
  import play.api.libs.json._
  implicit val CreateDTOReads =
    (
      (__ \ "company" \ "id").read[String] and
      (__ \ "account").read[Account]
    ).tupled

  def create = Action.async(parse.json) { implicit request =>
    val createCommandResult:JsResult[(String,Account)] = request.body.validate(CreateDTOReads)
    // Check that we have all of the fields we need
    createCommandResult.fold(
      errors => Future.successful(BadRequest(Json.prettyPrint(JsError.toJson(errors)))),
      createCommand => {
        val (companyId,account) = createCommand
        val createdAccountF: Future[Option[Account]] = accountService.createAccount(companyId, account)
        createdAccountF.map {
          case Some(a) => Created(a.id)
          case None => InternalServerError("Unable to create Account")
        }.recover {
          case e => BadRequest(e.getMessage)
        }
      }
    )
  }
}

object Companies extends Companies(new AccountService(new AccountDAO()))(play.api.libs.concurrent.Execution.defaultContext)


Thoughts on organizing code for multiple mysql queries in php scripts I have a web application where I connect with a MySQL database using PDO and in some scripts there are a lot of queries one after the other which don't necessarily concern the same tables. i.e I will execute a SELECT statement in one, and then depending on some values I will UPDATE another and finally go and DELETE from another one.

The thing is I only recently learned PHP and MySQL and because I wasn't sure and wanted to be careful and to find easily any problems (and a little because I am a little ocd and anal about silly things like uniformity and coding style) in every query I used the following format

try {
    $statement = "
        UPDATE/SELECT   ...
        FROM/SET    ...
        WHERE   ...";
    $query = $dbcnx->prepare($statement);
    $flag = $query->execute();
}
catch (PDOException $e) {
    $errorMsg = "...";
    error_log($errorMsg,3,'../../xxx.log');
    $response = ...;
    $dbcnx->null;
    return $response;
}
$result = $query->fetch/fetchAll/fetcColumn...

so I could find where any problem would occur (try/catch) and to be safe against injections and invalid characters (prepare) (I had some personal checks but I am pretty sure that a function specifically made for would be better).

When I had one or two queries it was fine but after the code grew it became a little too much code for little action/substance (like 16 lines for one query...)

So I would like some advise.
How do I make my code more manageable?
Is there some fundamental error in my logic on structure (the way I wrote it)?
Is there some rule for using try/catch? Is it more for developing and debugging and afterwards you can remove some blocks of it?
Is prepare enough for rudimentary security on queries?

I was thinking of making a function of just this block of code and calling it with the statement as a parameter. So I would just 'type' the query in the main body and then call the function where it would be prepared, executed and then return the result. Of course then I would always use fetchAll and would return an associative array but I think as long the datasets are small the memory usage would be fine (in any case in nowadays systems it should take A LOT to notice a difference, I think)...

EDIT: I was told about abstraction layer, which I admit I am a little fuzzy about. Basically it is a class which is instantiated every time the script/function runs and wrap the functions used repeatedly (along with the checking I do) inside it?

You're on the right track, what you're looking for is a DAL (Data Abstraction Layer), a class that represents a database connection.
This will allow you to move your common code into the class so you can simply construct your SQL statement and pass it in, with the method returning the expected data.

At a minimum you'd typically have, but aren't limited to:

Constructor (__construct) to create a new database connection using the relevant permissions.
Method to run your select statement (after input sanitization), returning a result set.
Method to run statements that don't return a result set.
Destructor (__destruct) to clean up and free any resources.

This would give you the following advantages:

Code reuse: only 1 copy of logging, connection initialization code, in 1 place.
Code encapsulation: The details are hidden behind a public interface so you can easily fix bugs and add new features without breaking existing SQL calls.
Pass in a complete SQL statement for centralized execution of SQL code.
Optionally pass in a parameter to specify calling Fetch/FetchAll/FetchColumn.
Easier debugging since your class will very quickly become very reliable as it gets heavily used and bugs are noticed and fixed.
Logging would be done automatically whenever you ran an SQL statement.
Easily add additional methods, such as input validation/sanitization.
Additional methods for specific purposes depending on your requirements.


Java class to compute and get a MD5 hash as a string Here is my class file to compute MD5 hashes on a file. I wanted to be able to easily calculate them without having to create a lot of overhead in the code using the class.

Can anyone tell me a list of improvements to make this class acceptable?

package Md5Generator;

/**
 *
 * @author jacob
 */
 import java.io.IOException;
 import java.security.*;
 import java.nio.file.Files;
 import java.nio.file.Paths;
 import java.nio.file.Path;

 /**
  * The point of this class it to compute the MD5 sum of a file
  */
 public class Md5 {
// two instance variables, one to store the file path and one to store the MD5 sum
private String path;
private String md5Sum;

/**
 * Constructor that takes a file path and and calcs the MD5 sum
 *
 * @param filePath the string that contains the full path to the file
 */
public Md5(String filePath) {
    path = filePath;
    calcMd5(path);
}

private void calcMd5(String filePath) {
    //create a messagedigest object to compute an MD5 sum
    try {
        MessageDigest md = MessageDigest.getInstance("MD5");
        //create a input stream to get the bytes of the file
        Path path = Paths.get(filePath);
        //read the bytes from the file and put them in the message digest
        md.update(Files.readAllBytes(path));
        //digest the bytes and generate an MD5 sum thats stored in an array of bytes
        byte[] hash = md.digest();
        //convert the byte array to its hex counter parts and store it as a string
        md5Sum = toHexString(hash);
    } catch (IOException | NoSuchAlgorithmException ex) {
        ex.printStackTrace();
    }

}

private String toHexString(byte[] bytes) {
    char[] hexArray = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F'};
    char[] hexChars = new char[bytes.length * 2];
    int v;
    for (int j = 0; j < bytes.length; j++) {
        v = bytes[j] & 0xFF;
        hexChars[j * 2] = hexArray[v / 16];
        hexChars[j * 2 + 1] = hexArray[v % 16];
    }
    return new String(hexChars);
}

/**
 * Returns the MD5 sum as a String
 *
 * @return the string that contains the MD5 sum
 */
public String getMd5Sum() {
    return md5Sum;
}
}

THe most striking issue I can see in your code is that you're severely limited in the size of files you can compute the hash on. This line of code is particularly jarring:

md.update(Files.readAllBytes(path));

You read the entire file in to memory... which limits you to two things:

the size of your file is limited to the size of your memory ... unless
you have lots of memory, but the file is >2GB at which point you can't create a larger array of bytes.

The reason that the MessageDigest methods contain the update method, is to deal with larger files, and allow you to hash them in parts.

I would use this as an opportunity to learn the somewhat older NIO features: Channels, and Buffers.

Consider the code:

try (MessageDigest md = MessageDigest.getInstance("MD5")) {
    try (FileChannel fc = FileChannel.open(Paths.get(filePath))) {
        long meg = 1024 * 1024;
        long len = fc.size();
        long pos = 0;
        while (pos < len) {
            long size = Math.min(meg, len - pos);
            MappedByteBuffer mbb = fc.map(MapMode.READ_ONLY, pos, size);
            md.update(mbb);
            pos += size;
        }
    } catch (IOException ioe) {
        ....
    }
    byte[] hash = md.digest();
    .....
} catch ( NoSuchAlgorithmException nsae) {
    ....
}

The advantage of the above code is that is plays much closer to the operating system. In theory, it should be the fastest possible way for a Java program to read the file, and it should never transfer the actual file content in to the Java memory space. It removes at least one data copy between the file represented in the OS on disk, and the copy in memory.

There is no limit to the size of file you compute the hash on.


Ackermann Function of (4,2) How efficient is this program? How could I make it more efficient?

package main;

import java.math.BigInteger;

public class Ackermann {

    public static void main(String[] args) {
        System.out.println(ack(BigInteger.valueOf(4),BigInteger.valueOf(2)));

    }

    public static BigInteger ack(BigInteger a, BigInteger b) {
        BigInteger ans;
        if (a.equals(BigInteger.ZERO)) ans = b.add(BigInteger.ONE);
        else if (b.equals(BigInteger.ZERO)) ans = ack(a.subtract(BigInteger.ONE),BigInteger.valueOf(1));
        else ans = ack(a.subtract(BigInteger.ONE), ack(a,b.subtract(BigInteger.ONE)));
        return (ans);
    }

}

The Ackermann function is not designed to be efficient... ;-) It is designed to be a counter-proof to a theoretical construct.

But, the reality is that the BigInteger is required. The conditions are required.

About the only things I can suggest are:

use better names
return-early logic
the ans variable is unnecessary
always use braces for conditional blocks.
BigInteger.ONE is already defined, you have used it in some places already, no need for BigInteger.valueOf(1)
call it ackermann not ack

I would have:

public static BigInteger ackermann(BigInteger a, BigInteger b) {
    if (a.equals(BigInteger.ZERO)) {
        return b.add(BigInteger.ONE);
    }
    if (b.equals(BigInteger.ZERO)) {
        return ackermann(a.subtract(BigInteger.ONE),BigInteger.ONE);
    }
    return ackermann(a.subtract(BigInteger.ONE), ackermann(a, b.subtract(BigInteger.ONE)));
}


Custom Error Handler Library in C++ Is this code clean and optimized? When taking memory snapshots, it seems a bit high.
Anything I have done wrong or can do to optimize it?
#pragma once
#include <iostream>
#include <windows.h>
#include <sstream>
#include <Psapi.h>

// Author: https://github.com/TheRacc2
// Other Help: https://github.com/dgoguerra/, for the bytes_to_size function (modified from the "humanSize" function found in https://gist.github.com/dgoguerra/7194777)
// Repository: https://github.com/TheRacc2/error_handler/
// Report issues here: https://github.com/TheRacc2/error_handler/issues

constexpr const char* name = "Error Handler";

namespace helpers {
    // Convert bytes into readable sizes, E.G: 1024B -> 1KB, 1024000B -> 1MB
    //
    // call as such:
    // const char* sign = bytes_to_size(1024);
    const char* bytes_to_size(int bytes) {
        const char* units[] = { "B", "KB", "MB", "GB", "TB" };
        const int size = sizeof(units) / sizeof(units[0]) - 1;

        int i = 0;
        double outside_bytes = bytes;

        if (bytes > 1024)
            for (; (bytes / 1024) > 0 && i < size; i++, bytes /= 1024)
                outside_bytes = bytes / 1024;

        static char final[12];
        snprintf(final, 12, "%.01f %s", outside_bytes, units[i]);
        return final;
    }
}

namespace error_handler {
    // Use this in a "try, catch" situation, the send to developers function doesn't need to be provided.
    //
    // call as such:
    // try {
    //     your_function_or_code();
    // } CATCH(send_log_to_developer(ex))
    #define CATCH(send_to_developers_function) \
    catch (const std::exception& ex) { \
        std::stringstream error; \
        error << ex.what(); \
        error << "\n\n"; \
        error << "Would you like to report this to the developers? Should your request be seen, this crash may be prevented."; \
        \
        if (MessageBoxA(nullptr, error.str().c_str(), name, MB_YESNO) == IDYES) { \
            send_to_developers_function; \
            MessageBoxA(nullptr, "Thank you for reporting this!", name, MB_OK); \
        } \
    }

    // Call this when something needs to go wrong, formatted like "printf". The second argument doesn't need to be provided.
    //
    // call as such:
    // THROW("You didn't use this right! You used this like: %s", "throw()")
    //
    // output:
    // "You didnt use this right! You used this like: throw()"
    #define THROW(text, ...) \
    error_handler::throw_ex(text, __FILE__, __FUNCTION__, __LINE__, __VA_ARGS__);

    // You shouldn't manually call this function, as you have to provide the file, function and line.
    // Use the "THROW" macro instead.
    template <typename... Args>
    void throw_ex(const char* text, const char* file, const char* function, const int& line, Args... args) {
        char message[128];
        snprintf(message, 128, text, args...);

        std::stringstream stream;
        stream << message << "\n\n";

        PROCESS_MEMORY_COUNTERS_EX pmc;
        K32GetProcessMemoryInfo(GetCurrentProcess(), reinterpret_cast<PROCESS_MEMORY_COUNTERS*>(&pmc), sizeof(pmc));

        stream <<
            "{\n" <<
            "    File: \"" << file << "\",\n"
            "    Function: \"" << function << "\",\n"
            "    Line: " << line << ",\n"
            "    Memory: \"" << helpers::bytes_to_size(pmc.PrivateUsage) << "\"\n"
            "}"
        ;

        throw std::exception(stream.str().c_str());
    }
}

Make bytes_to_size() reentrant
Since bytes_to_size returns a pointer to a static buffer, it is not reentrant. This will be a problem if you have multiple threads throwing exceptions at the same time. There are two solutions:

Have bytes_to_sign() take a pointer to a buffer as an argument, and have it write the text into that.
Have it return a std::string.

Avoid mixing C and C++ string formatting
It's very weird to see both snprintf() and std::stringstream used at the same time. Now you will get the worst of both worlds: type unsafety and possible truncation from snprintf(), and the overhead of dynamically growing memory from std::stringstream. Pick one and stick with it.
Consider using std::format
C++20 introduced std::format, which gives you type-safe string formatting. If you want to be compatible with older versions of C++, you can use the {fmt} library.
About units
The SI prefixes are powers of 1000, if you want to make sure people are not confused, use the IEC binary prefixes, so KiB, MiB, GiB and so on. I would also add all the defined prefixes, so up to YiB.
Consider allowing different exception types to be thrown
Your THROW() adds helpful information to the exception message, but the exception type is fixed to std::exception. That means that it is hard for any code that wants to catch exceptions to determine what exactly went wrong has very little information. Maybe you can add the exception type as an argument to the macro?
#define THROW(type, text, ...) \
    error_handler::throw_ex<type>(text, __FILE__, __FUNCTION__, __LINE__, __VA_ARGS__);

template <typename Exception, typename... Args>
void throw_ex(const char* text, ...) {
    ...
    throw Exception(stream.str().c_str());
}

Then it can be used like so:
if (value > MAX_VALUE) {
    THROW(std::out_of_range, "value %d larger than maximum %d", value, MAX_VALUE);
}

C++20 will avoid the need for macros
C++20 introduces std::source_location which avoids the need to use macros to get the filename and line number at the place where the exception is thrown.