I made a Rock paper scissors game with Python 3.8 If you could rate my code, 1-10. What will it be and provide reasoning on why you gave that answer, i feel like i could of written this code better but i don't know what i am missing or what i could have done to make it better

import random
print('Lets play Rock Paper Scissors')

user_point = 0     #To keep track of the points
bot_point = 0

for tries in range (1,4):

    try:
        user_guess = input('Rock Paper Scissors? ')
        choices = ['Rock','Paper','Scissors']

        bot_guess  = random.choice(choices) #Will randomly pick from the list 'choices'

        while user_guess not in choices:#if the user tries to put in anything other then the choices given
            print('Please enter the choices above!')
            user_guess = input('Rock Paper Scissors? ')
    except ValueError:
        print('Please choose from the choices above ')  #Just in case user tries to put a different value type
        user_guess = input('Rock Paper Scissors? ')

    #DEBUG = "The bot did " + bot_guess

    #print(DEBUG)

    if user_guess == bot_guess:
        print('Tie!')
    elif user_guess == "Rock" and bot_guess == "Paper":
        print('The bot earns a point!')
        bot_point += 1
    elif user_guess == 'Paper' and bot_guess == "Rock":
        print('The user earns a point!')
        user_point += 1
    elif user_guess == 'Paper' and  bot_guess == 'Scissors':
        print('The bot earns a point')
        bot_point += 1
    elif user_guess == 'Scissors' and bot_guess == 'Paper':
        print('The user earns a point')
        user_point += 1
    elif user_guess == 'Rock' and  bot_guess == 'Scissors':
        print('The user earns a point')
        user_point += 1
    elif user_guess == 'Scissors' and bot_guess == 'Rock':
        print('The bot earns a point')
        bot_point += 1

print('After ' + str(tries) + ' tries. ' + ' The score is')
print('The User: ' + str(user_point))
print('The Bot: ' + str(bot_point))

if user_point > bot_point:
    print('THE USER IS THE WINNER!!!')
else:
    print('THE BOT IS THE WINNER!!!')

I give it a 0 because I tied the bot and the bot still wins.

Just kidding.

Without considering some of the ways this could be made simpler and more compact (ie. strictly from a style guide point of view), this would be rated exactly a 4.88/10 (at least, that's what pylint is claiming):

************* Module rps
rps.py:3:13: C0303: Trailing whitespace (trailing-whitespace)
rps.py:6:47: C0303: Trailing whitespace (trailing-whitespace)
rps.py:7:13: C0303: Trailing whitespace (trailing-whitespace)
rps.py:9:19: C0326: No space allowed before bracket
for tries in range (1,4):
                   ^ (bad-whitespace)
rps.py:9:21: C0326: Exactly one space required after comma
for tries in range (1,4):
                     ^ (bad-whitespace)
rps.py:13:25: C0326: Exactly one space required after comma
        choices = ['Rock','Paper','Scissors']
                         ^ (bad-whitespace)
rps.py:13:33: C0326: Exactly one space required after comma
        choices = ['Rock','Paper','Scissors']
                                 ^ (bad-whitespace)
rps.py:15:19: C0326: Exactly one space required before assignment
        bot_guess  = random.choice(choices) #Will randomly pick from the list 'choices'
                   ^ (bad-whitespace)
rps.py:17:0: C0301: Line too long (106/100) (line-too-long)
rps.py:21:110: C0303: Trailing whitespace (trailing-whitespace)
rps.py:21:0: C0301: Line too long (110/100) (line-too-long)
rps.py:32:22: C0303: Trailing whitespace (trailing-whitespace)
rps.py:35:23: C0303: Trailing whitespace (trailing-whitespace)
rps.py:38:22: C0303: Trailing whitespace (trailing-whitespace)
rps.py:41:23: C0303: Trailing whitespace (trailing-whitespace)
rps.py:44:23: C0303: Trailing whitespace (trailing-whitespace)
rps.py:45:58: C0303: Trailing whitespace (trailing-whitespace)
rps.py:59:37: C0303: Trailing whitespace (trailing-whitespace)
rps.py:1:0: C0114: Missing module docstring (missing-module-docstring)
rps.py:6:0: C0103: Constant name "user_point" doesn't conform to UPPER_CASE naming style (invalid-name)
rps.py:7:0: C0103: Constant name "bot_point" doesn't conform to UPPER_CASE naming style (invalid-name)

-----------------------------------
Your code has been rated at 4.88/10

It is OK to ignore the last 2 messages. Pylint thinks variable that are declared outside functions are supposed to be constants. But in this case the script is so short that creating functions doesn't make sense.

formatting on the trailing comments:

bot_guess  = random.choice(choices) #Will randomly pick from the list 'choices'
while user_guess not in choices:#if the user tries to put in anything other then the choices given

much nicer to do: <code><one space>#<one space><comment>

bot_guess  = random.choice(choices) # will randomly pick from the list 'choices'
while user_guess not in choices: # if the user tries to put in anything other then the choices given

The if-elif statements used to determine the winner can be simplified quite a bit. Think about it like this: there are only 3 outcomes, so you should only need 3 checks, at most. There are several ways to do it, but here is one I thought of:

    if user_guess == bot_guess:
        print('Tie!')
    elif choices.index(user_guess) == (choices.index(bot_guess) + 2) % 3:
        print('The bot earns a point!')
        bot_point += 1
    else:
        print('The user earns a point')
        user_point += 1

The list choices = ['Rock','Paper','Scissors'] is treated as a constant, so declare it at the top of the file, right after the import statements (and it actually should be in all UPPER CASE). Since the content won't change, it would be good practice to use a tuple instead of a list (just swap the square brackets [...] for parens (...)).
Python 3.4 introduced a feature called f-strings that make string formatting a lot cleaner. Read the link for all †he details, but the main takeaway from it is that if you prefix a string literal with an f, it allows for string interpolation "on the fly":

print('After ' + str(tries) + ' tries. ' + ' The score is') # NO!!!
print(f'After {tries} tries, the score is') # YES!!!!

The try-except block is not needed. You can replace it with:

user_guess = ''
bot_guess  = random.choice(choices)
while True:
    user_guess = input('Rock Paper Scissors? ')
    if user_guess in choices:
        break
    else:
        print("you must pick a choice from the list!")

You could also combine the user_point and bot_point into a single points variable, and assign negative points for the bot and positive points for the user. This is kind of like taking the users side: you earn points by winning, and lose them when the bot wins. The downside is that you can't keep track of how many points each player has earned, only the difference between the 2.

So in the end, I get this:

#!/usr/bin/env python3

import random

CHOICES = ('Rock', 'Paper', 'Scissors')
points = 0
print('Lets play Rock Paper Scissors')
for tries in range(1, 4):
    user_guess = ''
    bot_guess = random.choice(CHOICES)
    while True:
        user_guess = input('Rock Paper Scissors? ')
        if user_guess in CHOICES:
            break
        else:
            print("you must pick a choice from the list!")
    print(f"The bot did {bot_guess}")
    if user_guess == bot_guess:
        print('Tie!')
    elif CHOICES.index(user_guess) == (CHOICES.index(bot_guess) + 2) % 3:
        print('The bot earns a point!')
        points -= 1
    else:
        print('The user earns a point!')
        points += 1
if points > 0:
    print(f'After {tries} tries, THE USER IS THE WINNER BY {points}!!!')
elif points == 0:
    print(f'After {tries} tries, IT\'S A TIE!!!')
else:
    print(f'After {tries} tries, THE BOT IS THE WINNER BY {points * -1}!!!')


Converting an image from polar to cartesian coordinates I have the following code which has too many loops inside it (almost 13 billion). This code is actually for image processing. I am first making a matrix of dimension 1024*360. This is the value of the projection data given to me. f is my image in the polar coordinate. The first loop gives me f in the polar-coordinate. f[r,phi] where r varies from 0 to 1024 and phi varies from 0 to 90. h is a filter that I have used in the first loop. In the second I am converting the image from polar coordinate to Cartesian coordinate. The image in the Cartesian coordinate is of dimension 725*725. I  have set the range of x and y as (1,726).

I doubt that it is right too. But when I had it as (0,725) earlier, it showed me error "dividing with zero" as I use atany(y/x). Hence I changed it. And after that I am converting the image to grayscale. I am new to Python and I guess my algorithm seems to slow down the execution. Is there any other way to avoid these loops? I haven't been able to display the image until now.

from __future__ import division
import scipy.io
import numpy as np
import pylab as pl
import matplotlib.pyplot as plt
import math
from math import sin
from math import pow
from math import degrees
from math import atan
import Image

q = np.zeros((1024,360))

x = scipy.io.loadmat('/home/prakriti/Project/A.mat')
A = np.array((x.values()))
q[:,0:120] = A[0][240,:,:]
del x
del A

y = scipy.io.loadmat('/home/prakriti/Project/B.mat')
B = np.array((y.values()))
q[:,120:240] = B[0][240,:,:]
del y
del B

z = scipy.io.loadmat('/home/prakriti/Project/C.mat')
C = np.array((z.values()))
q[:,240:360] = C[0][240,:,:]
del z
del C

f = np.zeros((1024,90))  #In polar co-ordinate

D = 0.28                #Source to isocenter distance
h = np.zeros(1024)
h[0:512] = np.linspace(0,1,512)
h[513:] = np.linspace(1,0,511)

for r in range(0,1024) :
  for phi in range(0,90) :
    for b in range(0,360) :
      for s in range(0,1024) :
        U = (D + r*sin(b-phi))/D
        l = math.pow(U,-2)
        k = D/(math.pow((math.pow(D,2)+math.pow(s,2)),0.5))
        f[r,phi] = 0.5*l*k*q[s,b]*h[s]

I =np.zeros((725,725))

for x in range(1,726) :
  for y in range(1,726) :
    r = math.trunc(math.pow((math.pow(x,2)+math.pow(y,2)),0.5))
    phi = math.trunc(math.degrees(math.atan(y/x)))
    I[x,y] = f[r,phi]

I8 = (((I - I.min()) / (I.max() - I.min())) * 255.9).astype(np.uint8)
img = Image.fromarray(I8)
img.save("Fanbeamreconstruction.png")
im = Image.open("Fanbeamreconstruction.png")
im.show()

Someone also pointed out that may be in looping over b and s I am overwriting f[r,phi] 360*1024 times and only the last one is used. What could this possible mean?

Well for starters, you can use xrange instead of range which will not create an entire list before you iterate, just create values as you need them. You can also move parts of the calculation to upper levels in your loops to decrease the work being done every cycle.

D_pow2 = math.pow(D,2)

for r in xrange(0,1024):
    for phi in xrange(0,90):
        for b in xrange(0,360):
            U = (D + r*sin(b-phi))/D
            l = math.pow(U,-2)

            for s in xrange(0,1024):
                k = D/(math.pow(D_pow2 + math.pow(s,2)),0.5))
                f[r,phi] = 0.5*l*k*q[s,b]*h[s]


Lowest common ancestor in recursive tree traversal LCA = Lowest Common Ancestor

The following code finds the lowest common ancestor in tree of nodes, where a node can have two parents, left and right.

The tree looks like this:

     J-K
    /   \
 E-F-G-H-I

And K and H input, the output is F

public TreeSet<Node> find(Node node, TreeSet<Node> nodeSet) {
    nodeSet.add(node);
    if (node.getLeft() != null) {
        find(node.getLeft(), nodeSet);
    }
    if (node.getRight() != null) {
        find(node.getRight(), nodeSet);
    }

    return nodeSet;
}

which is called here:

TreeSet<Node> nodeList1 = new TreeSet<Node>();
TreeSet<Node> nodeList2 = new TreeSet<Node>();
nodeList1 = find(node1, nodesList1); // node1 = K
nodeList2 = find(node2, nodesList2); // node2 = H
Iterator iterator = nodeList1.descendingIterator();
  while (iterator.hasNext()){
    Node node = (Node) iterator.next();
    if(nodeList2.contains(node)){
      return node.getValue();
    }
}

and a Node looks like you'd expect and implements comparable.

Is my implementation O(n), I think it is, but can it be improved?

The relvant snippet of Node:

class Node implements Comparable<Node> {

    private String value;
        private Node left;
        private Node right;
        @Override
        public int hashCode() {
            return this.getValue().hashCode();
        }

        @Override
        public int compareTo(Node o) {
            return this.getValue().compareTo(o.getValue());
        }

You seem to be have a few misconceptions, as @rolfl points out.

First, the data structure you have described and illustrated does not fit the definition of a tree.  Rather, I would characterize your data structure as a graph of maximum in-degree 2.

Second, you seem to be under the impression that the TreeSet internally contains a tree that resembles part of your original graph.  Actually, a TreeSet is nothing more than a SortedSet; the fact that the SortedSet is implemented by using a tree internally is irrelevant to you.  nodeList1.descendingIterator() does not produce an iterator that walks from root to leaf.  Rather, it yields nodes reverse-sorted according to the value contained in the node.

As a result, the code does not work.


How to simplify this C++ CRTP example? I wanted a simple thing: being able to override a base class and just add one single getter. The bad thing is the base class does override some operators and will return the wrong type. I, therefore, came across the curiously recurring template pattern which helped me to write the following.
However, I am still not very satisfied. I wrote a very similar question here which helped me to learn about CRTP.
#include <algorithm>
#include <array>
#include <iostream>

using namespace std;

template <typename C, typename T, int N>
class PointCRTP
{
   protected:
    array<T, N> elements;

   public:
    PointCRTP() = default;
    PointCRTP(array<T, N> el) : elements(el) {}

    C operator+(C& other)
    {
        C c;
        transform(elements.begin(), elements.end(), other.elements.begin(),
                  c.elements.begin(), plus<int>());
        return c;
    }

    T operator[](int k) { return elements[k]; }

    friend std::ostream& operator<<(std::ostream& os, C const& x)
    {
        os << "P(";
        for (auto k : x.elements) os << k << ", ";
        return os << "\b\b" << ')';
    }
};

template <typename T, int N>
struct Point : public PointCRTP<Point<T, N>, T, N> {
    using PointCRTP<Point<T, N>, T, N>::PointCRTP;
};

template <typename T>
struct Point2D : public PointCRTP<Point2D<T>, T, 2> {
    Point2D() = default;
    Point2D(T x, T y) : PointCRTP<Point2D<T>, T, 2>({x, y}) {}

    T getX() { return PointCRTP<Point2D<T>, T, 2>::elements[0]; }
    T getY() { return PointCRTP<Point2D<T>, T, 2>::elements[1]; }
};

int main()
{
    Point<int, 3> p({1, 2, 3});
    Point<int, 3> q({3, 4, 2});
    Point<int, 3> c = p + q;
    std::cout << c << " = " << p << " + " << q << endl;

    Point2D<int> r(1, 2);
    Point2D<int> s(3, 4);
    Point2D<int> t = r + s;
    std::cout << t << " = " << r << " + " << s << endl;
}

Is there a way to make this example simpler (shorter)?

Avoid special cases
A lot of problems come from the fact that you have slightly different ways to construct a 2D point and to get its elements than you have for an arbitrary dimensional point. You should avoid that first, this will make your life much easier.
Here is a class Point that supports getX() and getY() with compile-time checking that the size of elements is large enough, and a constructor that takes a variable number of arguments, so you don't need to use braces when constructing a Point:
template <typename T, int N>
class Point
{
   std::array<T, N> elements;

public:
    template<typename... E>
    Point(E&&...e): elements({std::forward<E>(e)...}) {}

    Point operator+(const Point& other)
    {
        Point c;
        std::transform(elements.begin(), elements.end(), other.elements.begin(),
                  c.elements.begin(), plus<T>());
        return c;
    }

    const T& operator[](size_t k) const
    {
        return elements[k];
    }

    friend std::ostream& operator<<(std::ostream& os, Point const& x)
    {
        os << "P(";
        for (auto& k : x.elements) os << k << ", ";
        return os << "\b\b" << ')';
    }

    template<std::enable_if_t<(N > 0), int> = 0>
    const T& getX() const {
            return elements[0];
    }

    template<std::enable_if_t<(N > 1), int> = 0>
    const T& getY() const {
            return elements[1];
    }
};

There are various ways to enable/disable member functions at compile time, the above uses std::enable_if_t which completely disables a function. You can also use a compile-time assert, like so:
T getY() const {
    static_assert(N > 1, "this point doesn't have a y coordinate");
    return elements[1];
}

Although as pointed out in the comments, this has the drawback that you cannot check at compile time if a given Point type supports getX(), getY().
To make a Point2D convenience class, use using:
template<typename T>
using Point2D = Point<T, 2>;


Validating a form and alerting the server if passed This is my first script in object-oriented JavaScript (jQuery 1.5.2). Its purpose is to simply validate a form (nothing fancy at the moment just checks if any value is present on required fields) and if validation is passed it submits the form via Ajax and alerts the server's response.

It would be great if more experienced programmers could review this and tell me what could be improved and what's dead wrong.

Here is the full working page:

<form action="script.php" method="post">
     Choose Username

     <input type="text" name="username" />

     Choose Password

     <input type="text" name="password" />

     Message to Admin

     <textarea name="message"></textarea>

     <button type="submit">Submit</button>
</form>

Contents of script.php

<?php

    while (list($key, $value) = each($_POST)) {
        echo $_POST[$key].' - ';
    }

?>

Its only a small Quibble with your code but I find the naming of formObj a little odd. For me I see this as a FormValidator or FormExtender or some such.

Another point would be the for(i in ....) should be for(var i in ...) it will run either way and I believe it is just a matter of coding style.

Also you have functions like:

validate: function(theForm, required({....

I think it helps debugging if you do this instead:

validate: function FormValidate(....

It has the disadvantage of polluting the namespace but makes debugging easier as the function is now not anonymous.

If you were making this a class to instantiate you would write it like so:

var FormValidator = function(theForm)
{
    this.formElement = theForm;
    return this;
}

FormValidator.prototype = {

     validate : function(theForm, required) {
            //...
     },
     inputListener : function(theField) {
            //...
     },
     ajaxSubmit : function(formToSubmit) {
            //...
     }

};

var mainForm = new FormValidator(document.forms[0]);

If you were doing a  Singleton pattern something like:

var FormValidator = function()
{
    return this;
}

function GetFormValidatorInstance()
{
    return window.__formValidatorInstance || new FormValidator();
}

These last two are probably overkill if you just want a simple Component. A more in depth example of the above can be found at https://codereview.stackexchange.com/questions/1658/javascript-code-class-structure


Test thousands of combinations quickly I have a physical robot which has a function, turn(degrees). For different robots, there will be small differences in the accuracy of the turn. I have fine turning variables, multiplier and adder. The degrees are multiplied by the multiplier, and then adder is added on.

I have made a program to brute-force the optimal values for fine tuning. The program is very simple, unoptimized, and slow.

The code:

from datetime import datetime

# in the format (request degrees, actual degrees)
real_results = (
(90, 99),
(100, 109),
(110, 121),
(125, 140),
(130, 149),
)
max_adder = 100
max_multiplier = 100
precision = 0.01

#        amount off, adder, multiplier
best_setting = [9999, 0, 0]

start_time = datetime.now()
# this lets me "step" in precision increments, because range only accepts
# integers, I tried making my own function but it was 3 times slower
# stackoverflow.com/questions/7267226
for adder in range(int(max_adder / precision)):
    for multiplier in range(int(max_multiplier / precision)):
        amount_off = 0
        for result in real_results:
            # the * precision turns it from an integer into steps of 0.01
            amount_off += abs(
                result[0] * (multiplier*precision) + (adder*precision) - result[1])
        if amount_off < best_setting[0]:
            best_setting[0] = amount_off
            best_setting[1] = adder
            best_setting[2] = multiplier

print("Best settings are:")
print("adder: {:0.3f}".format(1 / (best_setting[1] * precision) if best_setting[1] else 0))
print("multiplier: {:0.3f}".format(1 / (best_setting[2] * precision) if best_setting[2] else 0))
print("Average amount off: {:.4f}".format(best_setting[0] / len(real_results) if best_setting[0] else 0))
print("That run took:", datetime.now() - start_time)

How can I make my program faster?
The idea I currently have is having the second for loop (adder) could start in the middle, and act like a basic P controller. I'll work on this while I wait for replies :)

Clarification:

The data set will be manually created by someone requesting a turn, and recording what the actual turn was. This means the data set will usually be around 4 to 8 results long.

The precision is how accurate we want the result to be. Ideally it would be as precise as possible, but since the robot has a fair amount of unpredictability, 2 d.p is fine.

The program finds the number that the results were multiplied by. In order to find the number that will give the most accurate turn, we must divide 1 by the number.

numpy.arange: yes, I should have used that because that is exactly what my code emulates. I didn't know about that yesterday :P

Algorithm

You seem to essentially be doing linear regression with a least absolute deviations optimisation criterion. There are various standard approaches, although I'm not entirely sure about their applicability.

Problem: the task implemented by this code is not well specified. The precision variable serves to quantise the gradient and intercept of the line, but it's impossible to tell from the given specification and comments whether this quantisation is inherent to the problem domain or whether it's there to control the runtime of the search.

If the quantisation is inherent then some of the standard techniques will give starting points but you'll need to test the four nearest valid parameters (rounding each parameter up and down).

Given the size of your data, the approach which tries all lines through two points is probably going to be the most efficient, but again I'm not sure whether the real scenario has a larger dataset.

Code

start_time = datetime.now()
...
print("That run took:", datetime.now() - start_time)

I believe that timeit is the standard way of doing this in Python.

for adder in range(int(max_adder / precision)):
    for multiplier in range(int(max_multiplier / precision)):
        ...
                result[0] * (multiplier*precision) + (adder*precision) - result[1])

It took some thought to decipher the intent here, especially since precision's meaning is not documented. It would be clearer to use numpy.arange or to implement something similar.

print("adder: {:0.3f}".format(1 / (best_setting[1] * precision) if best_setting[1] else 0))
print("multipliertiply: {:0.3f}".format(1 / (best_setting[2] * precision) if best_setting[2] else 0))

Huh? Where did those reciprocals come from? Also, you seem to have started changing multiply to multiplier and got distracted part-way through.


remove redundance in compare-functions I wrote some compare-functions that I pass to other methods in javascript:

function equal(a, b) {
    if (a instanceof Date) return a.getTime() == b.getTime();
    return a == b;
}

function lessThan(a, b) {
    if (a instanceof Date) return a.getTime() < b.getTime();
    return a < b;
}

function greaterThan(a, b) {
    if (a instanceof Date) return a.getTime() > b.getTime();
    return a > b;
}

function lessOrEqual(a, b) {
    return equal(a, b) || lessThan(a, b);
}

function greaterOrEqual(a, b) {
    return equal(a, b) || greaterThan(a, b);
}

Is there a way to get this a bit more compact? If I want to add another comparable type I always have to modify the first three functions.

No need to check if the variables are instanceof Date. A dates value is its time so when you make (less than/greater than) comparisons with a Date instance (x) you're essentially comparing it to +x or x.valueOf() which will be the same as x.getTime(). Therefore the below will always be equivalent:

function equal(a, b) {
    if (a instanceof Date && b instanceof Date) return +a == +b; //compare the 2 dates times as noted in the above statement +x == x.getTime() == x.valueOf()
    return a == b;
}

function lessThan(a, b) {
    return a < b;
}

function greaterThan(a, b) {
    return a > b;
}


Javascript array_count_values function I am using an array_count_values function since I have translated the code from php to javascript and javascript does not have this function, but not sure on the performance of it. I'm sure it can be faster. It currently takes 50ms for 32000 iterations each time it is called.

I have one that returns a simple array:

function array_count_values(arr) {//returns array
    let a = [], prev;
    arr.sort();
    for ( let i = 0; i < arr.length; i++ ) {
        if ( arr[i] !== prev ) {
            a.push(1)
        } else {
            a[a.length-1]++;
        }
        prev = arr[i];
    }
    return a;
}

And another that returns an object:

function array_cnt_values(arr) {//returns object
    let a = [], prev;
    arr.sort();
    for ( let i = 0; i < arr.length; i++ ) {
        if ( arr[i] !== prev ) {
            a[arr[i]] = 1
        } else {
            a[arr[i]]++;
        }
        prev = arr[i];
    }
    return a;
}

Your first function just gives an array of counts, without anyway to tell what each of them are counting.

Your second function is better, as the index shows what's being counted. You can use the index to see if it already exists, which means you don't have to sort your array.

if( a[arr[i]] )

This works fine if all values are positive integers. But what if you had strings? It would still work, but you couldn't loop through the result easily. You might also run into problems if you have values equal to existing array properties like length.

Instead you can use a Map, which is similar to the associative array in php.

function array_count_values(arr) {
    let a = new Map();
    for ( let i = 0; i < arr.length; i++ ) {
        if ( a.has(arr[i]) ) {
            a.set(arr[i], a.get(arr[i])+1)
        } else {
            a.set(arr[i], 1)
        }
    }
    return a;
}


Removing equivalent authors in a list I have a list of authors, where seeking duplicity. The indexes of these duplicates are stored in a list of lists named duplicity. For example, at index 0 in the list duplicity is nested list, which refer to indexes of authors list with duplicates author A, index 1 author duplicity B, etc.
I also have list edge, where indexes of nodes(authors) are stored that are connected by an edge. Program passes through and removes duplicate authors by list duplicity.

For example I have stored in duplicity list indexes 1, 2, 3. These refers to authors list, where are stored names of authors. It also refers to edge list, where are stored start and end point of edges. For example if I have authors stored on indexes 1,2,3 and they are connected altogether, than the edge list is 1,2, 1,3, 2,3. Because authors on indexes 1,2,3 are same, I need make them one. So I pass edge list and replace all 2 and 3 to 1.

The problem is that progress of code below is very slow. I'm sure that in the code is something useless which raise complexity, but I can not figure it out. I think, the main failure in my code is the first part with the for loops. It many times passes authors list and if there are tens of thousands items, it's really slow. Can you help me optimize this code?

 for (int i = 0; i < authors.GetDuplicates().Count; i++)
        {
            for (int j = 0; j < edge.Count; j++)
            {
                for (int k = 0; k < authors.GetDuplicates()[i].Count - 1; k++)
                {
                    if (edge[j] == authors.GetDuplicates()[i].ElementAt(k))
                    {
                        edge[j] = authors.GetDuplicates()[i].ElementAt(k + 1);
                    }
                }
            }
        }

 public static List<List<int>> GetDuplicates(this IList<string> source)
    {
        HashSet<string> itemsSeen = new HashSet<string>();
        HashSet<string> itemsYielded = new HashSet<string>();
        List<List<int>> duplicates = new List<List<int>>();
        List<int> dupLow = new List<int>();
        HashSet<int> temp = new HashSet<int>();

        int c = 0;

        foreach (string item in source)
        {
            if (!itemsSeen.Add(item))
            {
                if (itemsYielded.Add(item))
                {
                    if (item != "-")
                    {
                        int w = 0;

                        for (int j = 0; j < authors.Count; j++)
                        {
                            if (authors[j] == authors[c])
                            {
                                temp.Add(w);
                            }
                            w++;
                        }

                        dupLow = new List<int>(temp);
                        duplicates.Add(dupLow);
                        temp.Clear();
                    }
                }
            }
            c++;
        }
        return duplicates;
    }

EDIT:
We have four authors, A and B are authors of one book and C and A are authors of different book. Authors within one book are connected by edge. We can see, there are two authors "A". So we replace in edges index of author A (index 0) to 3. At the end books are connected by common author.

authors | edge | remove dup. | bad result
[0] A    [0] 0   [0] 3    [0] 4
[1] B    [1] 1   [1] 1    [1] 1

[2] C    [2] 2   [2] 2    [2] 2
[3] A    [3] 3   [3] 3    [3] 3

The result should look like this:

I'm not sure I follow the code, but I would try removing the repeated calls to GetDuplicates:

foreach (var duplicate in authors.GetDuplicates())
{
    for (int j = 0; j < edge.Count; j++)
    {
        for (int k = 0; k < duplicate.Count - 1; k++)
        {
            if (edge[j] == duplicate[k])
            {
                edge[j] = duplicate[k + 1];
            }
        }
    }
}

I've also replaced the calls to ElementAt with use of the indexer, as I think it's clearer.


"Can a given array be made strictly increasing by removing one number?" The premise of the problem is that given an array of numbers, can you remove only one to make it a strictly increasing sequence?

[1, 1, 1, 2, 3] would be false because if you remove one of the 1s the array would still start off non-increasing because of the other two remaining 1s before it increased.
[1, 2, 3, 4, 99, 5, 10] would be true because you can remove the 99 and the remaining array [1, 2, 3, 4, 5, 10] would be strictly increasing.
[1, 88, 2, 3, 4, 99, 5, 6] would be false because removing any number would still leave an array that would not strictly increase.

For [1, 1] it would return true because even though removing one of the 1s would only leave one 1 which by definition can't increase sequentially because it is only one thing, I did not write the problem and this is the response they wanted.

and so on....

My questions

I've clearly made this code too complex, but it does work. Below my code is the tests that I used to check it with the expected returns commented out next to each.

How can I refactor this code to simplify it? Is there a way to do this without using chunk_while?

def almost_increasing_sequence(sequence)
  array_of_arrays = sequence.chunk_while {|i, j| i < j}.to_a
  if array_of_arrays.length == 1
    puts true
  elsif array_of_arrays.length > 2
    puts false
  else
    if (array_of_arrays[0].last <=> array_of_arrays[1].first) == 1
      if (array_of_arrays[0][-2] <=> array_of_arrays[1][0]) == -1
        puts true
      elsif (array_of_arrays[0].last <=> array_of_arrays[1][1]) == -1
        puts true
      elsif (array_of_arrays[0][-2]) == nil
        puts true
      else
        puts false
      end
    else
      puts true
    end
  end
end

almost_increasing_sequence([1, 2, 3, 4, 99, 5, 6]) #true
almost_increasing_sequence([1, 3, 2]) #true
almost_increasing_sequence([10, 1, 2, 3, 4, 5]) #true
almost_increasing_sequence([0, -2, 5, 6]) #true
almost_increasing_sequence([1, 2, 3, 4, 3, 6]) #true
almost_increasing_sequence([1, 1]) #true
almost_increasing_sequence([100, 200, 300, 400, 99, 500, 600]) #true
almost_increasing_sequence([1, 2, 1, 2]) #false
almost_increasing_sequence([1, 2, 3, 4, 5, 3, 5, 6]) #false
almost_increasing_sequence([40, 50, 60, 10, 20, 30]) #false
almost_increasing_sequence([1, 3, 2, 1]) #false
almost_increasing_sequence([1, 4, 10, 4, 2] ) #false
almost_increasing_sequence( [1, 1, 1, 2, 3]) #false
almost_increasing_sequence([10, 1, 2, 3, 4, 5, 6]) #true
almost_increasing_sequence([5, 7, 8, 90, 91, 92, 93]) #true

I'm not completely sure I've interpreted the requirements correctly, but I think this should do it:

def almost_increasing_array?(array, tolerance)
  unmatched = 0
  array[1..-1].each_with_index do |item, index|
    unmatched += 1 if array[index] >= item
    return false if unmatched > tolerance
  end
  true
end

The tolerance is extra but it basically says this:

Ignore the first item in the array
For the rest, compare them to the previous item
If the previous item was more than this, increment 'unmatched' count.
If more than the tolerance didn't increase, false. It's not 'almost increasing'

It looks like it works for all cases.

almost_increasing_array?([1, 3, 5], 1) # true
almost_increasing_array?([5, 4, 3, 2, 1], 1) # false
almost_increasing_array?([1, 2, 5, 4], 1) # true (only one decrease)
almost_increasing_array?([1, 7, 5, 4], 1) # false (two decreases)
almost_increasing_array?([1, 7, 5, 4], 2) # true (two decreases, but we said that's okay)

You could easily simplify by removing the tolerance argument (hard coding 1).

def almost_increasing_array?(array)
  unmatched = 0
  array[1..-1].each_with_index do |item, index|
    unmatched += 1 if array[index] >= item
    return false if unmatched > 1
  end
  true
end

Weaknesses:

This includes an array which is fully increasing, too.
This will fail as soon as an issue is found, but it may run slowly on huge arrays where there's a decrease right at the end.

Strengths:

It's pretty simple to understand.
This doesn't even have to be an array, it'll work for an instance of any Ennumerable class. Such as ActiveRecord::Relation, or Hash.


Detect whether a string has a letter 'b' 3 characters after 'a' I'm relatively new to JavaScript and wonder whether my code is 'acceptable' for a practice exercise. Essentially, the function (successfully) returns true or false if the provided string has a letter 'b' 3 characters after a letter 'a' - e.g.:

Input:"after badly" - Output:"false"
Input:"Laura sobs" - Output:"true"

Could somebody advise me how it could be improved? Although it works, I wonder whether the best functions are used and whether its 'readability' could be improved (i.e. return trueOrFalse.some(answer) ).

function bThreeAfterA(a) {
 var b = (a.split(' ').join('')).split('a'); // creates array

 var trueOrFalse = b.map(function(c, i){ // puts into array true/false for each index
  if (c[2] == 'b') {
   console.log('value: ' + c[2] + ' is b; true');
   return true;
  } else {
   console.log('false');
   return false;
  }
 });

 var answer = function(el) {
 // checks whether any index is true
  return el === true;

};

 return trueOrFalse.some(answer); // return true/false
}

Using a regular expression is definitely the way to go — that's exactly what they are good at doing.  The following regex looks for 'a', followed by any number of spaces, followed by a non-space character, followed by any number of spaces, followed by a non-space character, followed by any number of spaces, followed by 'b'.

function bThreeAfterA(str) {
    return /a *[^ ] *[^ ] *b/.test(str);
}

console.log('after badly', bThreeAfterA('after badly'));
console.log('Laura sobs', bThreeAfterA('Laura sobs'));

You should be aware, though, that you are using .some() suboptimally.  The .some(callback) method stops executing as soon as the callback returns a true value.  But you've already built trueOrFalse by analyzing the entire string, instead of taking advantage of that short-circuiting.  Therefore, if you use .some(), you shouldn't also use .map().

function bThreeAfterA(str) {
    // Array of characters without spaces
    var chars = str.split(' ').join('').split('');
    return chars.some(function(c, i, chars) {
        return c == 'a' && chars[i + 3] == 'b';
    });
}

console.log('after badly', bThreeAfterA('after badly'));
console.log('Laura sobs', bThreeAfterA('Laura sobs'));


JavaScript class for a GameRoom I am new to object oriented design with JavaScript and am trying to make a PublicGame class with some methods that puts itself in a map of games. It calls killGame on itself if there aren't enough players. I feel like my design is way off. I won't be able to test my design though until the rest of my group members finish their parts, so I could use some feedback.

Here's a link to my state diagram. Everything in yellow is the game.

'use strict';

// var GameSockets = require(‘GameSockets’);
var Games = {};
var id_counter = 0;
var minPlayers = 3;
var maxPlayers = 6;

function PublicGame (players) {
    this._id = id_counter++;
    this.players = players;
    this.gameSocket = new GameSockets.registerPlayers(this.players, this._id, this.playerDisconnects);

    this.judge = this.setJudge();

    this.killGame = function() {
        delete Games[this._id];
    };

    // When a player presses leave game
    this.playerExits = function(playerToRemove) {
        // Delete player from players array
        this.players.splice(this.players.indexOf(playerToRemove),1);

        // If less than min players
        if (this.players.length < minPlayers) this.killGame();

        // If less than max players
        if (this.players.length < maxPlayers) {
                this.needsPlayers = true;
        }

       gameSockets.kickPlayer(playerToRemove);
    };

    // When a player disconnects without warning, e.g. closes window
    this.playerDisconnects = function(playerToRemove) {
        // Delete player from players array
        this.players.splice(this.players.indexOf(playerToRemove),1);

        // If less than min players
        if (this.players.length < minPlayers) this.killGame();

        // If less than max players
        if (this.players.length < maxPlayers) {
                this.needsPlayers = true;
        }
    };

    this.selectJudges = function() {
        this.judge = this.players.pop();
        this.players = this.players.unshift(this.judge);
    };

    this.setWinner = function(winner) {
        this.winner = winner;
    };

    Games[this._id] = this;
}

You're not using pseudo-private properties by way of trapping vars inside the constructor. Thus, there is no incentive to putting the methods on the constructor. It only increases your memory usage because the functions are created per instance and not shared across instances. Suggesting you move them out to the prototype instead.

function PublicGame(){...}
GameRoom.prototype.playerExits = function(){...};
GameRoom.prototype.playerDisconnects = function(){...};
// and so on

this.killGame = function() {
    delete Games[this._id];
};

Make sure you actually destroy the game properly, and not just remove its reference from the Games object. Make sure no handlers are referenced, no sockets running etc. Otherwise, the GC will never pick them up.

function PublicGame (players) {

  this.players = players;

  this.playerExits = function(playerToRemove) {

    this.players.splice(this.players.indexOf(playerToRemove),1);

In JS, we usually assume stuff because of the lack of type information and rely on the engine throwing an error when something is executed with the wrong data.

However, in this case, you're assuming players is an array but never check if it really is during instance creation. The problem will come in later when you start doing array operations. In this case, splice will throw, but only later when a player exits. Then you scratch heads, lose hair and learn that your game has been running with broken data.

Guard players. Make sure it's an array and throw early before your code starts to churn with potentially broken data.

this.selectJudges = function() {
    this.judge = this.players.pop();
    this.players = this.players.unshift(this.judge);
};

You don't really need to pop off the last player to be the judge. Just put the judge in the beginning of the array to begin with and just pick him out using:

this.judge = this.players[0];

this._id = id_counter++;

Wondering if a counter would be a good game ID. What if you start to persist games across app restarts? Or run multiple instances of the game in parallel? They'll all start with 0 and numbers will collide. I suggest you use something like a GUID. They're usually unique and chances of collision are rare. Here's one from StackOverflow.


Abstracting async hooks out from components A component displays a list of values and I'm trying to hide everything about how those values are gotten, essentially divorcing concerns about state away from the component itself so that in the future any changes to which API data is fetched from, what store is used, etc won't effect the component.
First, is this a good idea at all or should mount/unmount actions be handled directly in the component?
Second, if it is a good approach, how is my implementation? I'm feeling comfortable in JS but am not an expert
ContentList.js:
import React from 'react'

import './ContentList.css'

import { ContentCardContainer } from '../ContentCard/ContentCard'
import { getPostSummaries } from '../../Utils/ContentAPI'

import { Grid } from '@material-ui/core'

export function ContentListContainer(props) {
  const posts = getPostSummaries()

  return <ContentList posts={posts} />
}

function ContentList(props) {
  return  (
    <div className='content-list'>
      <Grid container >
        {props.posts.map((post, id) =>
          <Grid item xs={12} sm={12} lg={6}>
            <ContentCardContainer
              key={id}
              id={id}
              title={post.title}
              summary={post.summary}
              link={'post/' + post.title}
            />
          </Grid>
        )}
      </Grid>
    </div>
  )
}

ContentApi.js:
import { useEffect } from 'react'
import { useDispatch, useSelector } from 'react-redux'
import { setContent, setSummaries } from '../Store/Actions'
import axios from 'axios'

const apiRoot = '/api'

export const getPostByID = (id) => {
    const dispatch = useDispatch()

    useEffect(() => {
      axios.get(apiRoot+'/post', {params: {id}})
        .then(resp => dispatch(setContent(resp.data.content)))
        .catch(() => dispatch(setContent('')))

        return () => dispatch(setContent(''))
    }, [])

    return useSelector(state => state.content)
}

export const getPostSummaries = () => {
  const dispatch = useDispatch()

  useEffect(() => {
    axios.get(apiRoot+'/post-summaries')
      .then(resp => dispatch(setSummaries(resp.data.posts)))
      .catch(() => dispatch(setSummaries([])) )
  }, [])

  return useSelector(state => state.summaries)
}

The general approach looks just fine. The logic behind data retrieval isn't entirely related to the rendering of components, so putting it into a custom hook instead makes good sense, especially if other components might need the data as well.
A few suggestions:

React recommends that the function names of custom hooks start with use, to clearly indicate that it's a hook and that other hooks (like useEffect) can be used inside it.
If other components might need to use the summaries as well, or if this component may get completely unmounted and then re-mounted again, consider whether you can use the summaries that were retrieved previously instead of fetching them again. (Or maybe you do want to fetch them again regardless - think about what you want.)
There doesn't really appear to be error handling, other than setSummaries([]). It would be more user friendly to indicate to the user if there's an error (and, at least in the development environment, to log the error).
Having both a ContentListContainer and a ContentList seems odd, since ContentListContainer is empty except for the custom hook call. If you aren't planning to add more stuff to the container, consider removing ContentListContainer entirely and putting the hook call into ContentList instead.


Age verification in Objective-C I'm creating some age-verification functionality for an iOS app. The user is presented with a UIDatePicker object, and the latest selectable date should be today minus 18 years. How vulnerable to inaccuracy is my code? How could it be leaner?

-(void)validateAge {

    NSDateComponents *today = [[NSCalendar currentCalendar] components:NSDayCalendarUnit | NSMonthCalendarUnit | NSYearCalendarUnit fromDate:[NSDate date]];
    NSInteger day = [today day];
    NSInteger month = [today month];
    NSInteger year = [today year];

    int correctYear = year - 18;

    NSDateComponents *correctAge = [[NSDateComponents alloc] init];
    [correctAge setDay:day];
    [correctAge setMonth:month];
    [correctAge setYear:correctYear];

    NSCalendar *calendar = [NSCalendar autoupdatingCurrentCalendar];

    UIDatePicker *agePicker = [[UIDatePicker alloc] initWithFrame:CGRectMake(0, 240, 320, 200)];
    [agePicker setDatePickerMode:UIDatePickerModeDate];
    [agePicker setMaximumDate:[calendar dateFromComponents:correctAge]];
    [self.view addSubview:agePicker];
    return;
}

Check this method. Pass value as minus 18 (-18)

-(NSDate *)offsetYear:(int)numOfYears date:(NSDate*)date {
    NSCalendar *calendar = [[NSCalendar alloc] initWithCalendarIdentifier:NSGregorianCalendar];
    NSDateComponents *offsetComponents = [[NSDateComponents alloc] init];
    [offsetComponents setYear:numOfYears];

    return [gregorian dateByAddingComponents:offsetComponents
                                  toDate:date options:0];
}


Having a parent respond to a child's event I have a parent object (a display window) and it has a few child objects (buttons and controls on the window). I would like one child object to hide itself when another is clicked. Currently, I have my parent object bind to a custom event expected to be fired from the child object. Then, the parent object passes the command on to its other child.

Example:

//Namespace for the 'Orders' page.
$(function Orders() {
    "use strict";
    var orders = $('#Orders');

    var ordersSearchResult = new OrdersSearchResult();
    var ordersSearchDisplay = new OrdersSearchDisplay();
    var ordersEditDisplay = new OrdersEditDisplay();

    ordersSearchResult.selector.bind('searchResultLinkClicked', function () {
        ordersEditDisplay.hide();
        ordersSearchDisplay.fadeIn();
    });
});

I have two questions / concerns about this code:

I expose a property called 'selector' for ordersSearchResult. This is a direct reference to the jQuery DOM element. As I understand it this is bad practice -- objects should expose methods to affect their DOM elements, but not the DOM elements themselves. Previously, I passed the onSearchResultLinkClicked anonymous function into a method of ordersSearchResult which bound the event to the selector internally. I felt that that implementation was counter-intuitive, though, as it did not read very clearly to see anonymous functions being passed into 'binding' methods.
ordersEditDisplay and ordersSearchDisplay expose methods hide() and fadeIn() which affect their DOM elements, but ordersEditDisplay/ordersSearchDisplay are not DOM elements themselves. Is it bad practice to 'confuse' other developers like this? The code could read: ordersEditDisplay.selector.hide() but I did not want to expose the selector publically.

It's really hard to judge if exposing selector is the "right" thing to do without reviewing more of your source code.
Also, it depends on your architecture design. If you're using a Model View Controller approach, then yes. Elements associated with the view will be exposed.
Check out some of the examples for Backbone.js.
Here are some other factors that come into play:

Coding standards
size of your source
how strictly you're required to follow Object Oriented Programming, OOP.
who will maintain your code.

The code you provided looks great. As long as you produce consistent readable code that has a good level of data abstraction and works, then you shouldn't worry about the small details.
If you're still concerned about it though then just create an adapter for the .bind().
Old Code:
ordersSearchResult.selector.bind('searchResultLinkClicked', function () {
    ordersEditDisplay.hide();
    ordersSearchDisplay.fadeIn();
});

New Code:
...
OrdersSearchResult.prototype.bind = function(eventName, fn ){
    this.selector.bind( eventName, fn );
};
...
var ordersSearchResult = new OrdersSearchResult();
ordersSearchResult.bind('searchResultLinkClicked', function () {
    ordersEditDisplay.hide();
    ordersSearchDisplay.fadeIn();
});

Here are a few additional tips.
1) Name variables and properties based on their overall purpose and type.

Programmer A: There's a problem with orders in your code.
Programmer B: Which one? Are you talking about the function, jQuery object, element id, or something else?

To avoid this situation, create unique names assuming the language is case insensitive.
Old code:
$(function Orders() {
    "use strict";
    var orders = $('#Orders');
    ...

New code:
$(function OrdersFunc() {
    "use strict";
    var $orders = $('#OrdersTable');
    ...

More info here: "How to name variables"
2) Add the dollar sign as a prefix for jQuery variables.
It would make more sense to rename selector as $el.
3) Try not to create method names that are the same from a different library API.
ordersEditDisplay.hide() sounds like ordersEditDisplay returns a jQuery object, but that isn't the case.
Instead of ordersEditDisplay.hide();, try ordersEditDisplay.hideView();
4) Use the Singleton Pattern or an object literal to create a namespace.
Great examples can be found here
Final Code:
var mySite = {};
mySite.Orders = {};
mySite.Orders.init = function() {
    "use strict";
    var $orders = $('#OrdersTable');

    var ordersSearchResult = new OrdersSearchResult();
    var ordersSearchDisplay = new OrdersSearchDisplay();
    var ordersEditDisplay = new OrdersEditDisplay();

    ordersSearchResult.$el.bind('searchResultLinkClicked', function () {
        ordersEditDisplay.hideView();
        ordersSearchDisplay.fadeInView();
    });
};
$(mySite.Orders.init);


Is this an efficient implementation of the Roman Numeral Converter? Is this an efficient way to implement the Roman Numeral Converter ?

So first, I put all the digits of the parameter number into an array.

Then I loop through this array, and multiply a 10 power based number to the current digit.

I use the resulted number in multiple statements, to compare it with special cases or numbers bigger or smaller than those cases.

For each if statement, when it is true, I concatenate a corresponding roman numeral to the whole.

At the end I return the final Roman Numeral as a string.

function convertToRoman(num) {

  var digitArr = []; // 2

  while(num !== 0){
    digitArr.push(num % 10); // 2
    num = Math.floor(num / 10); // num = 0
  }

  var len = digitArr.length;
  var romanNumber = "";
  var numb;
  var number;

  for(var i = len-1;i >= 0;i--){ // 1 0
    number = Math.pow(10, i) * digitArr[i]; // 3

    if(number >= 1000){
      for(var j = 0;j < digitArr[i];j++)romanNumber += "M";
    }else if(number === 900){
      romanNumber += "CM";
    }else if(number > 500){
      numb = number / 100 - 5;
        romanNumber += "D";
        for(var k = 0;k < numb;k++)romanNumber += "C";
    }else if(number === 500){
      romanNumber += "D";
    }else if(number === 400){
      romanNumber += "CD";
    }else if(number < 400 && number >= 100){
      numb = number / 100;
        for(var l = 0;l < numb;l++)romanNumber += "C";
    }else if(number === 90){
      romanNumber += "XC";
    }else if(number >= 60 && number < 90){
      numb = number / 10 - 5;
      romanNumber += "L";
      for(var z = 0;z < numb;z++)romanNumber += "X";
    }else if(number === 50){
      romanNumber += "L";
    }else if(number === 40){
      romanNumber += "XL";
    }else if(number < 40 && number >= 10){
      numb = number / 10;
      for(var x = 0;x < numb;x++)romanNumber += "X";
    }else if(number === 9){
      romanNumber += "IX";
    }else if(number < 9 && number >= 6){
      numb = number - 5;
      romanNumber += "V";
      for(var y = 0;y < numb;y++)romanNumber += "I";
    }else if(number === 5){
      romanNumber += "V";
    }else if(number === 4){
      romanNumber += "IV";
    }else if(number < 4){
      for(var p = 0;p < number;p++){
        romanNumber += "I";
      }
    }
  }

  return romanNumber;
}

See link above in comments for some more elegant solutions. So I am not going to focus on algorithm, which I think can certainly be improved.

Some thoughts on your code itself:

Comments don't seem to make sense or add value.
No need for all the if-else stuff. Would be much clearer with just if conditions and breaks, IMO.
I don't understand why you build digitArray the way you do. How about something like num.toString().split('').map(function(t){return parseInt(t)}) or Array.from(num.toString()).map(Number) (ES6)?
Your spacing around conditional and flow-control structures (or really, lack thereof) makes your code hard to read.
Indentation is inconsistent.
Seems to be odd use of vertical whitespace.


PHP DateTime increments, a neater / more efficient way? I'm trying to get to grips with the DateTime class in PHP. Previously I've used date() and mktime() when playing with dates.

Here's the head of a calendar table, and I'm pretty sure I'm doing this in a stupid way! That fact that I'm setting the format each time, and modifying the original date with each Interval, seems 'lumpy':

<?php $date = new DateTime('2013-04-01'); ?>
<?php echo $date->format('Y'); ?>
<?php echo $date->format('D jS');?>
<?php $date->add(new DateInterval('P1D')); echo $date->format('D jS');?>
<?php $date->add(new DateInterval('P1D')); echo $date->format('D jS');?>
<?php $date->add(new DateInterval('P1D')); echo $date->format('D jS');?>
<?php $date->add(new DateInterval('P1D')); echo $date->format('D jS');?>
<?php $date->add(new DateInterval('P1D')); echo $date->format('D jS');?>
<?php $date->add(new DateInterval('P1D')); echo $date->format('D jS');?>

Can anyone point me towards a better way to do this. I'm planning to create a function to spit these calendars out, but I've provided an inline example here so you can see the way I'm using DateTime etc for now.

Thanks

I like it.
You could use loops and use a variable for 'DateInterval(P1D)'.

<?php $date = new DateTime('2013-04-01');
      $day = new DateInterval('P1D');
?>
<?php echo $date->format('Y'); ?>
<?php echo $date->format('D jS');?>
<?php
for($i = 0;$i <= 6; $i++){
    $date->add($day)
    echo ''.$date->format('D jS').'	';
}?>


Calculating sum of smallest prime factors for 2 < n < 10**12 I am trying to calculate the sum of the smallest prime factors of n, where 2 < n < 10**12, and get the remainder of this sum when divided by 10**9:

require 'prime'
puts "Started at #{Time.now}."
num = 10**12
sum = 0
(2..num).each { |n| sum += n.prime_division[0][0] }
sum = sum % 10**9
p sum
puts "Finished at #{Time.now}."

Understanding that the smallest prime factor of any even number is 2, and the smallest prime factor of any prime number is itself, I made the following function:

def findSmallestPrimeFactor(number)
  return 2 if number.even?
  return number if Prime.prime? number
  return number.prime_division[0][0]
end

However I feel this would have no effect on the program's run time as #prime_division works well enough on any number.

Is there any way to shorten the runtime of this program? I have been told that efficient implementation will allow the program to finish in under 1 minute, however I can't see how I can do this without iterating over every number between 2 and one trillion (10**12), which will take several days regardless.

Sorry, too long for a comment (this isn't a real review, but it isn't a CR what you need).

This is Project Euler 521 and providing a full answer here would spoil the fun. If you really want to cheat, google it out.

  Is there any way to shorten the runtime of this program?

There are many ways and you'll need more than one:

Iterating till 10**12 alone is too slow, even if you did just a simple sum (one hour maybe).
Determining if a number is a prime, even with a good sieve takes quite some time as well.

Without much thinking, I agree with you that the runtime will be many days.

Your question is actually not about code optimization, but about finding a smarter algorithm. Smarter by a few orders of magnitude. A wonderful hint can be found in forum for problem 10, page 5.

It's solvable without this wonderful idea, too, but it took my computer half an hour. Instead of factoring numbers, I produced them as products, which is much faster when you really want all of them. The next idea is that you don't really need to produce them, counting is enough.


PseudoBrain FizzBuzz Thoughts Some recent discussions on this site involved the Fizz-Buzz game - I have never actually implemented this, but for the record this is what I put together:

public class FizzBuzzConverter
{
    private readonly int _fizz;
    private readonly int _buzz;

    public FizzBuzzConverter(int fizz, int buzz)
    {
        _fizz = fizz;
        _buzz = buzz;
    }

    public string Convert(int value)
    {
        if (value % (_fizz*_buzz) == 0) return "FizzBuzz";
        if (value % _buzz == 0) return "Buzz";
        if (value % _fizz == 0) return "Fizz";
        return value.ToString();
    }
}

When people implement fizzbuzz, they tend to run a loop going from 1 to 100 and outputting the results to some console. According to Wikipedia, that's not how the game plays:

  Players generally sit in a circle. The player designated to go first says the number "1", and each player thenceforth counts one number in turn. However, any number divisible by three is replaced by the word fizz and any divisible by five by the word buzz. Numbers divisible by both become fizz buzz. A player who hesitates or makes a mistake is eliminated from the game.

So I'm thinking of going that way - and since I've never actually coded anything remotely resembling AI since I tweaked some Duke Nukem 3D configuration files in 1997, I'd like to get some feedback and refactoring thoughts about this:

public class PseudoBrain
{
    private readonly FizzBuzzConverter _converter;
    public PseudoBrain(FizzBuzzConverter converter)
    {
        _converter = converter;
    }

    public virtual string Think(string lastResult, int value)
    {
        var random = new Random();
        var ai = random.Next(0, 99);

        var result = _converter.Convert(value);

        if (ai < 85)
        {
            // thinks straight most of the time.
            return result;
        }
        else
        {
            Hesitate();
            if (ai < 95) return result;

            // could be right, could be wrong. hesitant anyway.
            return lastResult == "fizz" ? value.ToString() : "buzz";
        }
    }

    private void Hesitate()
    {
        Thread.Sleep(500);
    }
}

This brain works, ..it's just a little too hesitant:

Still, it's not too dumb:

I'm sure I could tweak the percentages to come up to something decently smart enough to implement a game with 5-6 of those "brains" playing against a human player, who would win if all the brains fried before his.

How's this PseudoBrain class doing? Don't Hesitate(), just say what you Think()!

I'd first create an interface that your FizzBuzzConverter class can implement:

public interface IConverter
{
    string Convert(int value);
}

public class FizzBuzzConverter : IConverter
{
    // ...
}

Then the PseudoBrain can have any implementation of the interface injected into it:

public class PseudoBrain
{
    private readonly IConverter _converter;

    public PseudoBrain(IConverter converter)
    {
        _converter = converter;
    }

    // ...
}

This will allow for easier unit testing via mocking or writing and using new Converters.

Lastly, I have a bit of a squicky feeling when I see random number generators created over and over (as Think() will be called repeatedly). So I put that at the class level:

public class PseudoBrain
{
    private readonly Random _random = new Random();
    private readonly IConverter _converter;

    public PseudoBrain(IConverter converter)
    {
        _converter = converter;
    }

    public virtual string Think(string lastResult, int value)
    {
        var ai = _random.Next(0, 99);

        // ...
    }

    // ...
}


Optionally creating a directory path based based on existence in NodeJS I've been doing some reading and tutorials. As such I've contrived a node based project to apply what I've learned. I thought this was a good example as it had some conditional async calls and error handling.

Any and all criticism of not following current JS best practices welcome.

/**
 * mkLocalCfg function,
 * makes a local config directory structure and empty config file.
 * in the form "~/.config/nobjs/nobjs_config.json"
 *
 * @param {mkLocalCfgCallback} cb - The callback that handles the response.
 *
 */
function mkLocalCfg(cb) {
    //validate root folder exists, if not make it.
    if (!admin.hasHomeConfigDir()) {
        fs.mkdir(os.homedir() + "/.config", "775", function(err) {
            //stop here if you can't make it.
            if (err) {cb(err); return;}
        });
    }
    fs.mkdir(admin.getConfigLocation(false), "775", function(err) {
        //can't make config folder end it here
        if (err) {cb(err); return;}
        //write config file, return null or err on error
        fs.writeFile(admin.getConfigLocation(true), JSON.stringify({blogs: {}}), function(err) {
            if (err) {cb(err); return;}
            cb(null);
        });
    });
}

There's one main issue I currently see with your code and that is that if the admin does not have a home configuration directory (admin.hasHomeConfigDir()) then it will be created asynchronously. The issue with this it that your second call won't wait for this, leading to a race condition where you're trying to make (and write to) a path that might not exist yet.

One solution for this is to have a function inside your function that handles the "second half" of your creation (i.e. the config location creation and file writing).

function createConfiguration(callback) {

    // Make the configuration directory and write to the configuration.
    var writeConfiguration = function() {
        fs.mkdir(admin.getConfigLocation(false), "775", function(err) {
            if (err) {
                callback(err);
                return;
            }

            // Write the configuration, either forwarding an error to the
            // callback if an error occurs or nothing at all on success.
            fs.writeFile(admin.getConfigLocation(true), JSON.stringify({blogs: {}}), function(err) {
                if (err) {
                    callback(err);
                    return;
                }

                callback(null);
            });
        });
    };

    // If the admin home configuration directory exists, don't attempt to
    // create it.
    if (admin.hasHomeConfigDir()) {
        writeConfiguration();
    } else {
        // Otherwise create the directory and write the configuration.
        fs.mkdir(os.homedir() + "/.config", "775", function(err) {
            if (err) {
                callback(err);
                return;
            }

            writeConfiguration();
        });
    }
}

I tidied up your code a bit, mostly renaming things (e.g. renaming the function to have a simpler name, renaming arguments to be more readable) and rewording the comments.

Another issue I have noticed with your code is that if the directory that admin.getConfigLocation(false) returns already exists, an error will be thrown. You should probably check whether or not the configuration location exists before creating it.

Overall it's rather good and well written code, although I don't really have much experience in the field of "JS standards" so to speak.


A Mixin Comparator class Idea:
In C++, often a straightforward implementation of comparison operators is needed. In C++11, they can be conveniently implemented using std::tie. The following mixin class automatically enables comparison operators by deriving a class T from Comparable<T>.

Implementation: (please review this part, requires C++14)

// A mixin class that makes derived classes comparable
template<typename T>
struct Comparable {
    // nothing is needed here
};

// Default: Assume the client class offers a tie method
// Alternative: The client class can override this method
template<typename T>
auto tie(const Comparable<T>& a_object)
{
    return static_cast<const T&>(a_object).tie();
}

template<typename T>
bool operator==(const Comparable<T>& a_lhs, const Comparable<T>& a_rhs)
{
    return tie(a_lhs) == tie(a_rhs);
}

template<typename T>
bool operator!=(const Comparable<T>& a_lhs, const Comparable<T>& a_rhs)
{
    return tie(a_lhs) != tie(a_rhs);
}

template<typename T>
bool operator<(const Comparable<T>& a_lhs, const Comparable<T>& a_rhs)
{
    return tie(a_lhs) < tie(a_rhs);
}

template<typename T>
bool operator>(const Comparable<T>& a_lhs, const Comparable<T>& a_rhs)
{
    return tie(a_lhs) > tie(a_rhs);
}

template<typename T>
bool operator<=(const Comparable<T>& a_lhs, const Comparable<T>& a_rhs)
{
    return tie(a_lhs) <= tie(a_rhs);
}

template<typename T>
bool operator>=(const Comparable<T>& a_lhs, const Comparable<T>& a_rhs)
{
    return tie(a_lhs) >= tie(a_rhs);
}

Usage Example: Client code then could look like the following. The implementation of the tie function automatically defines a lexicographical ordering for all supported operations.

struct Client : public Comparable<Client>
{
    auto tie() const
    {
        return std::tie(m_a, m_b);
    }

    int m_a = 1;
    int m_b = 2;
};

Client a, b;
b.m_b = 5;

const bool isEqual = (a == b);
const bool isNotEqual = (a != b);
const bool isSmaller = (a < b);
const bool isGreater = (a > b);
const bool isSmallerOrEqual = (a <= b);
const bool isGreaterOrEqual = (a >= b);

Advantage: There is no need anymore to implement many comparison operators by hand for many classes individually. If there are many different classes requiring comparison operators with straightforward semantics this can save quite some code.

ADL

This can be improved. First - you're requiring a tie() member function, it'd be better to make that more of a customization point. Second - you're putting all the operators in global scope when it'd be better to hide them. Both issues can be solved with some ADL.

The advantage of making an operator a non-member friend declared inline is that it can only be found when comparing precisely what you want. The downside to what you're doing right now is the following compiles:

struct B : Comparable<B> {
    ...
};

struct D {
    operator B() { ... }
};

D d1 = ...;
D d2 = ....;
if (d1 == d2) { ... }

D isn't comparable - but this code would still compile via the B conversion, which may be surprising!

We can instead write equality like so:

namespace cmp {
    template <class T>
    auto tie(const T& val) -> decltype(val.tie()) {
        return val.tie();
    }

    template <class T>
    struct Comparable {
        friend bool operator==(const T& lhs, const T& rhs) {
            return tie(lhs) == tie(rhs);
        }
    };
}

Note that the operator here takes Ts and not Comparable<T>s. We know what the derived type is, so we can just use it. The nonqualified call to tie lets us write something like:

namespace foo {
    struct B : Comparable<B> {
        int i;
    };

    int tie(const B& b) { return b.i; }
}

Customization is nice.

Split it up

EqualityComparable and LessThanComparable are different concepts. If you split up the responsibilities, it'll make your class more useful:

namespace cmp {
    template <class T>
    struct EqualityComparable {
        friend bool operator==(const T& lhs, const T& rhs) { return tie(lhs) == tie(rhs); }
        friend bool operator!=(const T& lhs, const T& rhs) { !(lhs == rhs); }
    };

    template <class T>
    struct LessThanComparable {
        friend bool operator<(const T& lhs, const T& rhs) { return tie(lhs) < tie(rhs); }
        friend bool operator<=(const T& lhs, const T& rhs) { return !(rhs < lhs); }
        // etc.
    };

    template <class T>
    struct TotallyOrdered : EqualityComparable<T>, LessThanComparable<T>
    { };
}

Cross-Compare

If two classes have comparable tie()s, why not support that too?

template <class T, class U>
struct EqualityComparable2
{
    friend bool operator==(const T& lhs, const U& rhs) { return tie(lhs) == tie(rhs); }
    friend bool operator==(const U& lhs, const T& rhs) { return rhs == lhs; }
    // same for !=
};

Go wild.


Performance concerns when converting GTK image to greyscale I have a function which takes GTK Image (It is RGB) and returns a representation of this image as list of grey levels.

This is my code:

grayscaleFromRGB :: Image -> IO [Word8]
grayscaleFromRGB img = do
    imgType <- img `get` imageStorageType
    if imgType == ImageEmpty
        then
            return (replicate 256 0)
        else do
            pixbuf <- imageGetPixbuf img
            pixels <- (pixbufGetPixels pixbuf :: IO (PixbufData Int Word8))
            rgbList <- getElems pixels
            nChannels <- pixbufGetNChannels pixbuf
            return (map convert $ chunksOf nChannels rgbList)

    where
        convert channels = round $ foldl (+) 0 $ zipWith (*) [0.3, 0.59, 0.11] $ map fromIntegral channels

But I have a problem: this function uses too much memory and CPU time. For example for an image with a size of 500x500 it takes nearly 100MB of memory. Opening an image with a size of 2000x1000 it use more than 1GB of memory and it takes a lot of time.

How can it be written in an efficient way? What is the reason for this behavior?

Update:

Whole code which use this function (this program takes too much RAM):

import Graphics.Rendering.Cairo
import Graphics.UI.Gtk
import Data.List.Split
import Data.Array.MArray
import Data.Array.IO
import Data.Word
import Data.List
import Foreign.Storable

grayscaleFromRGB :: Image -> IO [Word8]
grayscaleFromRGB img = do
    imgType <- img `get` imageStorageType
    if imgType == ImageEmpty
        then
            return (replicate 256 0)
        else do
            pixbuf <- imageGetPixbuf img
            pixels <- (pixbufGetPixels pixbuf :: IO (PixbufData Int Word8))
            rgbList <- getElems pixels
            nChannels <- pixbufGetNChannels pixbuf
            return (map convert $ chunksOf nChannels rgbList)

    where
        convert channels = round $ foldl' (+) 0 $ zipWith (*) [0.3, 0.59, 0.11] $ map fromIntegral channels

main :: IO ()
main= do
     initGUI
     window <- windowNew
     set window [windowTitle := "Hello Cairo",
                 windowDefaultWidth := 300, windowDefaultHeight := 200,
                 containerBorderWidth := 30 ]

     image <- imageNew
     let menuBarData = [
                  ("File", [("Open", openPressed image),
                            ("Exit", mainQuit)
                           ])
                      ]

     tab <- tableNew 5 10 True

     menuBar <- createMenuBar menuBarData

     scrolled <- scrolledWindowNew Nothing Nothing
     scrolledWindowAddWithViewport scrolled image

     tableAttachDefaults tab menuBar    0 10 0 1
     tableAttachDefaults tab scrolled   0 5 1 10

     containerAdd window tab

     widgetShowAll window
     onDestroy window mainQuit
     mainGUI

createMenuBar menuBarData = do
    menuBar <- menuBarNew

    mapM_ (setMenuEntries menuBar) menuBarData
    return menuBar

    where
        setMenuEntries menuBar (entryName, items) = do
            entry <- menuItemNewWithLabel entryName
            menu <- menuNew
            mapM_ (addItemToEntry menu) items
            menuItemSetSubmenu entry menu
            menuShellAppend menuBar entry

        addItemToEntry menu (name, action) = do
            item <- menuItemNewWithLabel name
            item `on` menuItemActivated $ action
            menuShellAppend menu item

openPressed :: Image -> IO ()
openPressed image = do
    chooser <- fileChooserDialogNew
                (Just "Open file")
                Nothing
                FileChooserActionOpen
                [("OK", ResponseOk),
                 ("Cancel", ResponseCancel)]

    widgetShow chooser
    res <- dialogRun chooser
    filename <- performResponse res chooser
    widgetHide chooser

    where
        performResponse ResponseCancel _ = return ()
        performResponse ResponseOk chooser = do
            Just fname <- fileChooserGetFilename chooser
            imageSetFromFile image fname
            grey <- grayscaleFromRGB image
            putStrLn $ show $ grey

By commenting out various parts of grayscaleFromRGB, we can deduce that what is taking the most time is getElems (and possibly the computations after return). Indeed, for a 2000x1000 picture, there are 2M pixels (x3 or 4 channels), and getElems is putting them in a list word by word. Lists are quite inefficient for storing large amounts of data like pictures (at least 4 or 5 extra words per element, and the GC comes with an extra x2 factor).

To reduce memory usage, we can write a custom loop to process the elements in the array as we read them, like below. To reduce that further, don't use lists to store large amounts of data. It still takes a while to read millions of elements from memory byte by byte; there may be a better way using a specialized data processing library but I don't know it.

grayscaleFromRGB :: Image -> IO [Word8]
grayscaleFromRGB img = do
    imgType <- img `get` imageStorageType
    if imgType == ImageEmpty
        then
            return (replicate 256 0)
        else do
            pixbuf <- imageGetPixbuf img
            pixels <- (pixbufGetPixels pixbuf :: IO (PixbufData Int Word8))
            nChannels <- pixbufGetNChannels pixbuf
            (_, bound) <- getBounds pixels
            loop pixels nChannels bound 0 []

    where
        -- the returned list will be reversed compared to the original code
        loop pixels n bound i acc | i + 2 > bound = return acc
        loop pixels n bound i acc = do
          let get i = fromIntegral <$> (readArray (pixels :: PixbufData Int Word8) i) :: IO Double
          a <- (0.3 *) <$> get i
          b <- (0.59 *) <$> get (i + 1)
          c <- (0.11 *) <$> get (i + 2)
          loop pixels n bound (i + n) $! (((:) $! round (a + b + c)) $! acc)


Example using Object lock synchronized blocks Here's my attempt at trying to understand synchronized() blocks using an Object lock, using wait() and notify():

import java.util.Scanner;

public class SyncTest {

    private Scanner scanner;
    private Object lock;
    private volatile String string;

    public SyncTest() {
        scanner = new Scanner(System.in);
        lock = new Object();
        string = "";
    }

    public void start() {

        //This thread will wait until the other thread notifies it.
        Thread t1 = new Thread(new Runnable() {
            @Override
            public void run() {
                while (true) {
                    synchronized (lock) {
                        try {
                            lock.wait();
                        } catch (InterruptedException e) {
                            e.printStackTrace();
                        }
                    }
                    System.out.println("String changed.");
                }
            }
        });
        t1.start();

        //This thread will notify Thread 1 if a condition is met.
        Thread t2 = new Thread(new Runnable() {
            @Override
            public void run() {
                while (true) {
                    if (!string.isEmpty()) {
                        synchronized (lock) {
                            lock.notify();
                        }
                        string = "";
                    }
                    try {
                        Thread.sleep(200);
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
            }
        });
        t2.start();

        //This is just a dummy thread to modify the string data.
        Thread t3 = new Thread(new Runnable() {
            @Override
            public void run() {
                while (true) {
                    string = scanner.nextLine();
                }
            }
        });
        t3.start();
    }

    public static void main(String[] args) {
        new SyncTest().start();
    }

}

Couple questions:

Is there any better way of achieving such functionality?
Is this how listeners work? How are they so efficient at listening to events? I mean, if I don't add the 200ms delay in t2 execution, the thread will consume a lot of processing power, so, how can I create a listener behavior?

It does look sound to me with one exception that you need to be aware of, as far as I can see:
if (!string.isEmpty()) {
    synchronized (lock) {
        lock.notify();
    }
    string = "";
}

The moment you set string to a new value, the first thread might not have visited the variable at this point. You can see that when outputting string in both threads. This is only important if thread #1 is going to do processing on the value.
Additionally, because of the Thread.sleep(200); you might miss input lines because string value might have changed an unidentified number of times during this period.

I mean, if I don't add the 200ms delay in t2 execution, the thread will consume a lot of processing power, so, how can I create a listener behavior?

The same way you did between #2 and #1, notify #2 from #3.

To fix these issues, you want to use something like a BlockingQueue. Thread #3 pushes the line to the Queue, thread #2 dequeues the next value and notifies #1. Or, #2 pushes it to a second Queue for #1.


Improving C file reading function I am programing in C language and have created the below functions to read files. There are two functions that I can use to read files. I wanted to know how I can further improve these two in terms of efficiency and out of these two which one would be better to use for reading files.

First is:

        char *read_data_from_file(char *data, char *path, int *fsz1)
        {
              int imgFD=0;
              struct stat fst;
              int fsz = *fsz1;
              fsz = 0;

              imgFD = open(path, O_RDONLY);
              if (imgFD <= 0){
                      *fsz1 = fsz;
                      return NULL;
              }

              fstat(imgFD, &fst);
              fsz = fst.st_size;
              printf("the size %s = %d inmfd = %d \n ", path, fsz, imgFD);
              if (fsz <= 0){
                      *fsz1 = fsz;
                      return NULL;
              }
              data = (char *)malloc(sizeof(char)*(fsz+1));
              read(imgFD, data, fsz);
              close(imgFD);

              *fsz1 = fsz;
              return (data);
      }

Another is:

      int read_data_from_file(int fd, void *buffer, int count)
      {
              void *pts = buffer;
              int status = 0, n;

              if(count < 0) return -1;
              while(status != count) {
                      n = read(fd, pts+status, count-status);
                      if(n < 1) return n;
                      status += n;
              }
              return (status);
      }

The calling portion of the above mentioned function is:

      int main(int argc, char *argv[])
      {
              struct stat sb;
              off_t len;
              char *p, *buffer = NULL;
              int fd;

              fd = open("test.txt" , O_RDONLY);
              if (fd == -1) {
                      perror ("open");
                      return 1;
              }
              if (fstat (fd, &sb) == -1) {
                      perror ("fstat");
                      return 1;
              }
              if (!S_ISREG (sb.st_mode)) {
                      fprintf (stderr, "%s is not a file\n", argv[1]);
                      return 1;
              }
              debug_print("printing start time and size : %d\n", sb.st_size);
              buffer = (char *)malloc(sizeof(char) * (sb.st_size + 1));

              read_data_from_file(fd, buffer, sb.st_size);

The two functions do not do the same thing so comparing their efficiency is not sensible.  A few comments on the code though (adding to those of @mjfgates)

first read_data_from_file:

path should be const
imgFD should be comapred <0 not <=0
check the return value of fstat instead of checking whether fsz is less than 0.
use perror to print failure reason after detecting open failure
fsz == 0 is valid and should return an empty buffer (ie return data and set *fsz1 to zero)
don't cast the return value of malloc
sizeof(char) is 1 by definition
check whether malloc succeeded.
you allocated fsz+1 bytes, presumably allowing for a terminator but didn't add one (\0)
check whether read succeeded.
check whether close succeeded (if pedantic).
why does data get brackets in return (data) and NULL not in return NULL?  Prefer the latter (no brackets).

second read_data_from_file:

pts should be char * or else you will get a compiler warning.  Are warnings enabled for you?
keywords (if, while, etc) preferred with a space - ie. if (...) instead of if(...)
if you are reading a normal file, the loop is not required. Just one read, as in the first version.
if you are reading something other than a file, then a loop is needed, but your loop is incorrect.  If the read call gave some data, you will get n = number read, if it got to end of file, you will get 0, if there was an error or if reading was interrupted before any daat was received you will get an error (-1) and errno will be set.  In the latter case you must work out whether there was an error or whether the call was interrupted  in which case you can loop.
if count is bigger than the actual file size, your loop never exits.
status is misnamed - it holds the number of bytes read so far.

main:

return EXIT_FAILURE or EXIT_SUCCESS
ok, it is a file you are reading so the loop in the second read_data_from_file is not necessary.
as before, don't cast malloc return and sizeof(char) is 1 by definition


Encoding strings using periodic table abbreviations I took a JavaScript challenge to finish a task related to logo of "Breaking  Bad" where letters in your first name and last name are spotted with elements of periodic table and its respective atomic number. I wrote the below code, any suggestions to improve performance or any best coding practices

  function Process() {
    var ellist = {
        "h": "1",
        "he": "2",
        "li": "3",
        "be": "4",
        "b": "5",
        "c": "6",
         .
         .
         .
        "Lv":"116",
    "Uus":"117",
    "Uuo":"118"
    };
    var fname = document.getElementById("firstname");
    var lname = document.getElementById("lastname");
    var splits = fname.split("");
    var value;

    for (var i = 0; i < splits.length; i++) {
        var onevalue = fname.indexOf(splits[i]);
        var singlev = fname.substring(onevalue, onevalue + 1);
        var doublev = fname.substring(onevalue, onevalue + 2);
        var triplev = fname.substring(onevalue, onevalue + 3);
        if (ellist[splits[i]] || ellist[doublev] || ellist[triplev]) {
            value = splits[i];

            if (ellist[doublev] || ellist[triplev]) {
                value = ellist[doublev];

                if (ellist[triplev]) {
                    value = ellist[triplev];
                         // some code here
                }
       // some code here
            }
    // some code here
        }
    }

Using the Process() function which contains the logic. The object ellist contains the list of elements of periodic table with its atomic number. First name is taken from textbox on webpage and stored in fname and similarly the last name in lname and in the for loop it contains the code which checks whether the firstname contains the string which matches the elemetns of periodic table. Any suggestions?

Any suggestions?

Yes, a few.

First off, split your function into parts (SRP), to separate the view (DOM elements and their values) from the logic (finding element names in strings).

var splits = fname.split("");
for (var i = 0; i < splits.length; i++) {
    var onevalue = fname.indexOf(splits[i]);

That doesn't make much sense to me. Don't you expect onevalue == i? If not, you might annotate this explicitly and/or make the comparison. Maybe it's inside the "some code"?

var doublev = fname.substring(onevalue, onevalue + 2);
var triplev = fname.substring(onevalue, onevalue + 3);

Notice that these will have the same value as singlev in the last [two] iterations of your loop, where the end is outside of the string.

if (ellist[splits[i]] || ellist[doublev] || ellist[triplev]) {
    value = splits[i];
    if (ellist[doublev] || ellist[triplev]) {
        value = ellist[doublev];
        if (ellist[triplev]) {
            value = ellist[triplev];

Ouch. Simplify this to

if (triplev in ellist) {
    value = ellist[triplev];
} else if (doublev in ellist) {
    value = ellist[doublev];
} else if (splits[i] in ellist) { // are you sure you don't want `singlev`?
    value = splits[i];
}


Mutable stack in Racket I'm learning Racket and have implemented a mutable stack, which is just a bunch of wrappers around an underlying struct containing a size and buffer list (so it's not optimal, in terms of computational complexity). After consulting #racket and reading the first half of Greg Hendershott's Fear of Macros, I was able to write the syntax transformations I wanted for my implementation.

(module stack racket
  (module stack-implementation racket
    (struct stack (size buffer) #:mutable)

    ;; size :: stack -> integer
    (define (size stack) (stack-size stack))

    ;; non-empty-stack? :: stack -> boolean
    (define (non-empty-stack? stack)
      (and
        (stack? stack)
        (positive? (stack-size stack))))

    ;; push! :: stack -> any (new item) -> integer (size)
    (define (push! stack item)
      (set-stack-buffer! stack
        (append (list item) (stack-buffer stack)))

      (set-stack-size! stack (+ (stack-size stack) 1))

      (stack-size stack))

    ;; pop! :: (non-empty) stack -> any (head)
    (define (pop! stack)
      (let ([head (car (stack-buffer stack))]
            [tail (cdr (stack-buffer stack))])

        (set-stack-buffer! stack tail)
        (set-stack-size! stack (- (stack-size stack) 1))

        head))

    ;; make
    (define-syntax-rule (make name)
      (define name (stack 0 '())))

    (provide make
             stack?
             (contract-out
               [size  (-> stack?            integer?)]
               [push! (-> stack? any/c      integer?)]
               [pop!  (-> non-empty-stack?  any)])))

  (require 'stack-implementation
           (for-syntax racket/syntax))

  ;; make-stack
  ;  Defines a stack under the specified symbol <name>
  ;  Plus defines <name>-size, <name>-empty?, <name>-push! and <name>-pop!
  (define-syntax (make-stack stx)
    (syntax-case stx ()
      [(_ name)
        (with-syntax ([(size-fn empty-fn push-fn pop-fn)
                       (map (lambda (fn) (format-id stx "~a-~a" #'name fn))
                            '("size" "empty?" "push!" "pop!"))])
          #'(begin
              (make name)
              (define (size-fn)      (size name))
              (define (empty-fn)     (zero? (size-fn)))
              (define (push-fn item) (push! name item))
              (define (pop-fn)       (pop!  name))))]))

  (provide make-stack
           stack?))

I'm completely new to Racket and Lisp, so I'm guessing there are many improvements I can make here (e.g., the duplication in the macro where I define the helper function IDs), besides using a better underlying data structure.

Many Lisp programs use cons cells quite liberally instead of adding a
separate stack structure, in that sense your concerns about complexity
are probably a bit less critical.  Since the stack is also carrying the
length of the list it actually supports at least the additional size
operator quite well.  Of course writing more code for an underlying
(resizable) vector could make sense, but again I wouldn't count on
that being necessary.

For the code I only have minor suggestions, as it's pretty well written
and clear what each part means; while I'm not that familiar with Racket
it's easily readable to me.

For the pattern (+/- x 1) there are also the functions add1 and
sub1 which could be used.
The non-empty-stack? could just check whether the buffer is
null? or empty? instead.  positive? sounds like there's some
risk that the stack size might get negative for some reason.  For the
same reason the contract can probably also be tightened to check for
non-negative integers.
(append (list x) y) is a bit shorter as (cons x y).


Proper way to map the object I have a simple task.

Array of people object comes in, up to a thousand.
Need to iterate over it, and create a custom payload for external service.

I have created a working solution, but I would like to see if I am totally off course, and what changes should I make.
I have reduced non-important details from the code (errors, and similar), so it might not work. But the point is to show how it works, and hopefully get some advice.
User struct {
   Name        string  `json:"name"`
   Lastname    string  `json:"lastname"`
 }

func HandleLambda(event events.SQSEvent) {
 var userRecords []map[string]interface{}

 for idx := range event.Records {
  record := event.Records[idx]
  ev, err := e.NewEvent(record.Body)

  if err != nil {
    continue
  }

  u, err := repository.NewUser(ev)

  if err != nil {
        continue
  }

  user := make(map[string]interface{})
  user["1"] = u.Name
  user["2"] = u.Lastname

  userRecords = append(userRecords, user)
  }

 uJSON, _ := json.Marshal(userRecords)
 // I send this to external service

 }

External service is using numbers as keys, that's why I need "1" and "2". I do not have control over it.
That's the way it works.

I guess I might as well then.  To prefix that with:  It looks fine as is,
there's a few details that could be improved, although those also depend
on your team and style guide.  Also read it in the context of a bigger
project, not everything always applies, especially if it's one-off code,
etc.
I'm gonna assume (since the formatting is a bit off) that you're using
gofmt or goimports already, if you don't, check them out, it helps
immensely.
For the User struct, even if the JSON name is lastname, consider the
Go-side name to be LastName to keep in line with normal style guides.
Depending on whether you need it the JSON annotation could also use an
omitempty, but that depends on your external service of course.
For the HandleLambda loop a few things stand out:

for idx := range event.Records might as well be
for idx, record := event.Records, no need have an extra line to get
the actual item.  And if you're never using it,
for _, record := event.Records even.
Errors are ignored?  Consider logging them if don't error our
immediately.
user := make(map ...) I'd usually make that a literal to avoid
repeating myself even more.
userRecords could perhaps already be pre-allocated to the right size. Then there'd be no need for append at all.  Or, like here, if you still need to use continue, pre-allocating the maximum length it could take, so it definitely wouldn't have to
be resized might also be a good idea.  Usually this also doesn't matter that much if you don't have a
lot of entries, but it's good to keep in mind for situations where it
does matter.
Also consider making the to-map conversion a new method on User, or
a helper function, you might want to test it or reuse in other places.
repository looks like a global variable, that's usually not the best
idea.  Consider making that a parameter for HandleLambda, or making
HandleLambda a method and put the repository as one of its
members.  It will help with testability in the end to not have
dependencies scattered around like that.

type User struct {
    Name     string `json:"name"`
    LastName string `json:"lastname"`
}

func (u *User) toMap() map[string]interface{} {
    return map[string]interface{}{
        "1": u.Name,
        "2": u.LastName,
    }
}

func HandleLambda(event events.SQSEvent) {
    userRecords := make([]map[string]interface{}, 0, len(event.Records))

    for _, record := range event.Records {
        ev, err := event.NewEvent(record.Body)
        if err != nil {
            continue
        }

        user, err := repository.NewUser(ev)
        if err != nil {
            continue
        }

        userRecords = append(userRecords, user.toMap())
    }

    uJSON, _ := json.Marshal(userRecords)
    // I send this to external service
}


Finding min value of an array using SIMD I have the following code to find the minimum sbyte value in an array. It is using System.Runtime.Intrinsics to perform a SIMD min on chunks of the array, and then loops over the resulting vector to find the true minimum.

public static sbyte Min( sbyte[] array )
{
  if( array.Length <= 0 )
    return 0;

  var length = array.Length;
  var stepSize = Vector128<sbyte>.Count;

  fixed ( sbyte* pStep = &array[ 0 ] )
  {
    var i = stepSize;
    var minVector = Avx.LoadVector128( pStep );
    for( ; i <= length - stepSize; i += stepSize )
      minVector = Avx.Min( minVector, Avx.LoadVector128( pStep + i ) );

    var _ = stackalloc sbyte[ stepSize ];
    Avx.Store( _, minVector );

    // Find min of minVector
    var min = sbyte.MaxValue;
    for( var j = 0; j < stepSize; j++ )
      if( min > _[ j ] )
        min = _[ j ];

    // Evaluate remaining elements
    if( i < length )
      while( i < length )
      {
        if( min > pStep[ i ] )
          min = pStep[ i ];
        i++;
      }

    return min;
  }
}

I feel like this is a slightly excessive amount of code, but seeing how this function is meant to handle arrays with lengths that can't be equally partitioned into a 128-bit register, I'm not sure if there's any cleaner way to do this.

I do intend to implement this method for all supported primitive types, but it seems like most of the code will just be duplicated.

Correctness

  if( array.Length <= 0 )
    return 0;

This piece of code is suspicious. Maybe you should return sbyte.MaxValue, maybe null, or maybe throw ArgumentException. (I can imagine that there were a Max method, both, together is used to find the range of values, then maybe 0 is a valid return value.) This is one of the rare times I'd appreciate a comment to explain such extrinsic information.

Useless Code

 if( i < length )
      while( i < length )

Lose the if.

Excessive code

When you are operating on fixed size chunks,
 - pad the input with some appropriate value (sbyte.MaxValue)
 - handle the last fixed size chunk of the input separately.

The last suggestion should read something like this, (which means I don't even guarantee that it will compile):

if (length < stepSize)
    throw new ArgumentException($"this method cannot be used for arrays shorter than {stepSize}")

Vector128<sbyte> minVector;
SetAllVector128(minVector, sbyte.MaxValue);
for (var i = 0; i < length - stepSize; i += stepSize)
    minVector = Avx.Min(minVector, Avx.LoadVector128(pStep + i));

minVector = Avx.Min(minVector, Avx.LoadVector128(pStep + length - stepSize));

Then you can use the last loop altogether;