Calculator Object Oriented Design I am aiming to implement a calculator in an object-oriented way.

Here is the solution by using the strategy pattern.

Looking forward to some valuable comments.

package oopdesign.calculator;

public class AdditionStrategy implements CalculationStrategy {

    @Override
    public int calculate(int value1, int value2) {
        return value1 + value2;
    }
}

package oopdesign.calculator;

public interface CalculationStrategy {

    int calculate(int value1, int value2);
}

package oopdesign.calculator;

public class Calculator {

    public static Calculator instance = null;

    CalculationStrategy calculationStrategy;

    public void setCalculationStrategy(CalculationStrategy calculationStrategy) {
        this.calculationStrategy = calculationStrategy;
    }

    public static Calculator getInstance(){
        if(instance == null){
            instance = new Calculator();
        }
        return instance;
    }

    public int calculate(int value1, int value2) {
       return calculationStrategy.calculate(value1, value2);
    }
}

package oopdesign.calculator;

public class CalculatorMain {

    public static void main(String[] args) {

        Calculator c = Calculator.getInstance();

        c.setCalculationStrategy(new AdditionStrategy());
        System.out.println(c.calculate(5 ,2));

        c.setCalculationStrategy(new SubtractionStrategy());
        System.out.println(c.calculate(5 ,2));

        c.setCalculationStrategy(new MultiplicationStrategy());
        System.out.println(c.calculate(5 ,2));

        c.setCalculationStrategy(new DivideStrategy());
        System.out.println(c.calculate(5 ,2));
    }
}

package oopdesign.calculator;

public class DivideStrategy implements CalculationStrategy {

    @Override
    public int calculate(int value1, int value2) {
        return value1 / value2;
    }
}

package oopdesign.calculator;

public class MultiplicationStrategy implements  CalculationStrategy{

    @Override
    public int calculate(int value1, int value2) {
        return value1 * value2;
    }
}

package oopdesign.calculator;

public class SubtractionStrategy implements CalculationStrategy {

    @Override
    public int calculate(int value1, int value2) {
        return value1 - value2;
    }
}

Review:
From extensibility and in pro of having the possibility of including in future versions more operations, it is a good approach. The code is very simple, and it is easy to read so it is very good your design proposal.

The main purpose of applying design patterns is to simplify things, to reach the maximum level of abstraction and allow you to write meaningful code, not just repeat stuff

So, you did good.
However, there are some observations:
package oopdesign.calculator;

//Singleton is a good approach for this problem
public class Calculator {

    //By default any object is null
    //Do not put it as public, you have the getInstance method
    private static Calculator instance;

    //You are limiting the operations to handle
    CalculationStrategy calculationStrategy;

    //This is not a Singleton if you allow the default constructor (its public by default)
    private Calculator() {
    }

    public void setCalculationStrategy(CalculationStrategy calculationStrategy) {
        this.calculationStrategy = calculationStrategy;
    }

    public static Calculator getInstance() {
        if (instance == null)
            instance = new Calculator();
        return instance;
    }

    //You should think about handle the most general data type (this case double)
    public double calculate(double value1, double value2) {
       return calculationStrategy.calculate(value1, value2);
    }
}

package oopdesign.calculator;

public class CalculatorMain {

    public static void main(String[] args) {

        Calculator c = Calculator.getInstance();

        //There is a problem with it, you need to instanciate the strategies
        //each time you need to use it
        c.setCalculationStrategy(new AdditionStrategy());
        System.out.println(c.calculate(5,2));

        //It requires space, plus you are not being efficient by storing
        //there operations (calculation strategies)
        c.setCalculationStrategy(new SubtractionStrategy());
        System.out.println(c.calculate(5,2));

        c.setCalculationStrategy(new MultiplicationStrategy());
        System.out.println(c.calculate(5,2));

        c.setCalculationStrategy(new DivideStrategy());
        System.out.println(c.calculate(5,2));
    }
}

An alternative
import java.util.HashMap;
import java.util.Map;

public class Calculator {

    private static Calculator instance;

    //search in Constant time (approximately)
    private Map<String, CalculationStrategy> calculationStrategies;

    private Calculator() {
        calculationStrategies = new HashMap<>();
    }

    public void addCalculationStrategy(String name, CalculationStrategy strategy) {
        calculationStrategies.put(name, strategy);
    }

    public static Calculator getInstance() {
        if (instance == null)
            instance = new Calculator();
        return instance;
    }

    //double b... means that there may be 0 to n parameters
    //consider that there are unitary operators or functions in a calculator
    public double calculate(String name, double a, double... b) {
        return calculationStrategies.get(name).calculate(a, b);
    }
}

package oopdesign.calculator;

public class Main {

    public static void main(String[] args) {
        Calculator calculator = Calculator.getInstance();

        //Use a lambda instead
        calculator.addCalculationStrategy("+", (a, b) -> a + b[0]);
        //[b] is taken as an array but is a variadic parameter
        calculator.addCalculationStrategy("-", (a, b) -> a - b[0]);
        calculator.addCalculationStrategy("*", (a, b) -> a * b[0]);
        calculator.addCalculationStrategy("/", (a, b) -> a / b[0]);
        calculator.addCalculationStrategy("Abs", (a, b) -> Math.abs(a));
        calculator.addCalculationStrategy("Cos", (a, b) -> Math.cos(a));
        calculator.addCalculationStrategy("Sin", (a, b) -> Math.sin(a));

        System.out.println(calculator.calculate("+", 1, 3));
        System.out.println(calculator.calculate("-", 1, 3));
        System.out.println(calculator.calculate("*", 1, 3));
        System.out.println(calculator.calculate("/", 1, 3));
        System.out.println(calculator.calculate("Abs", -66));
        System.out.println(calculator.calculate("Cos", 75));
        System.out.println(calculator.calculate("Sin", 28));
        System.out.println(calculator.calculate("+", 666, 777));
    }
}

About double b... read this post about Variadic function parameters, as I said, it is a way to have multiple parameters, From 0 To N parameters
Thanks for reading this answer.


Min and Max initialization Is it a good practice to initialize the max to -100000 and min to 100000? Is there any other way to initialize both min and max to 0?

 import javax.swing.*;
    import java.util.*;

public class arrayTajba
{
    public static void main(String[] args) throws Exception
    {
        String userStringInput = " ";
        String display = " ";
        int max = -100000;
        int min = 100000;
        int total = 0;
        double average;
        int i = 0;
        int [] num = new int [5];

        for (i = 0; i < num.length; i++)
        {
            userStringInput = JOptionPane.showInputDialog(null,"Please enter 5 numbers " + (i+1),"Input Table",JOptionPane.QUESTION_MESSAGE);
            num[i] = Integer.parseInt(userStringInput);

            display+=num[i] + ", ";
            total+=num[i];

            if (num[i] < min)
            {
                min = num[i];
            }
            if (num[i] > max)
            {
                max = num[i];
            }
        }
            average = total / num.length;

        JOptionPane.showMessageDialog(null,"The numbers you have entered are:\n" + display + "\nSum of all numbers is: " + total + "\nAverage is: " + average + "\nMinimum number is: " + min + "\nMaximum number is: " + max,"Output Table" ,JOptionPane.INFORMATION_MESSAGE);
        }
    }i

Java has a couple of constants available that are commonly used in situations like this:

    int max = Integer.MIN_VALUE;
    int min = Integer.MAX_VALUE;

Using these two values ensures you cover the possible spectrum of valid input values.

In addition, a sadistic user would enter 5 large values, and cause your total value to overflow, creating a bad result. You should declare your total as a long value:

    long total = 0;

That will prevent overflow (unless you have 2 billion input values.... ;-)

Finally, the last statement is a problem, it does integer division:

        average = total / num.length;

It should convert one of the values to a double, so that floating-point division happens instead:

        average = total / (double)num.length;


Reverse input program in C99 I am writing simple program to reverse its input and output the reversed version. I came up with this code. So how can I improve it? Something tells me that int lengthSoFar is bad in both printArray and reverse functions.

#include <stdio.h>
#include <string.h>

#define ARRSIZE 1000

//function declarations...
char *reverse(char *a, int l);

void printArray(char *a, int l);

int main(void) {
    int c = 0;
    int i = 0;
    char buff[ARRSIZE];
    while ((c = getchar()) != EOF) {
        if (i > ARRSIZE) printArray(reverse(buff, i), i);
        if (c != '\n') {
            buff[i]=c;
            i++;
        }else{
            printArray(reverse(buff, i), i);
            memset(buff, 0, sizeof buff);
            i=0;
        }
    }
    return 0;
}

char *reverse(char *array, int lengthSoFar) {
    int i = 0;
    int j = lengthSoFar - 1;
    static char arrayToReturn[ARRSIZE];
    while (i < lengthSoFar && j >= 0) {
        arrayToReturn[i] = array[j];
        i++;
        j--;
    }
    return arrayToReturn;
}

void printArray(char *array, int lengthSoFar) {
    for (int i = 0; i < lengthSoFar; i++) {
        putchar(array[i]);
    }
}

Take a destination for the reversed string

Instead of a local pointer, make reverse a void function:

void reverse(char * dst, const char * src, size_t length) {
    for(size_t i = 0; i < length; ++i) {
        dst[i] = src[length - i - 1];
    }
}

The names dst (destination) and src (source) and their order stem from standard library functions like strncpy.

size_t is an unsigned integer type, and large enough to index an array that spans your whole memory. We're allowed to define size_t i in the for loop, therefore our variable doesn't live any longer than necessary.

But you already did that for printArray, so I guess you knew that :)

void printArray(const char * array, size_t length) {
    for (size_t i = 0; i < length; i++) {
        putchar(array[i]);
    }
}

It's good practice to use const <type> * if you don't want to change the data by accident.

Use a function to ask for the string

To keep your main sleek and your code more reusable, provide a function to get the string from the user instead:

int getInput(char * dst, size_t max_len) {
    int length = 0;
    int c;

    while((c = getchar()) != EOF && length < max_len) {
        if(c == '\n')
            return length;

        dst[length++] = c;
    }
    return c == EOF ? EOF : length;
}

This will return EOF if getchar() returned EOF, and the number of read characters otherwise.

Putting all back together

I omit the functions I've already covered above, but if we use them, we end up with the following program:

#include <stdio.h>
#include <string.h>

#define ARRSIZE 1000

int getInput(char * dst, size_t max_len);
void reverse(char * dst, const char * src, size_t length);
void printArray(const char * array, size_t length);

int main(void) {
    int length = 0;
    char buff[ARRSIZE];
    char reversed[ARRSIZE];

    while ((length = getInput(buff, ARRSIZE)) != EOF) {
        reverse(buff, reversed, length);
        printArray(reversed, length);
    }
    return 0;
}

Which, in my opinion, is rather readable. Note that your memset wasn't necessary. After all, we're going to overwrite the values in buff with the next input.


VBA More effective way of data trimming My code consists of Split, Mid, Left, Right and Replace functions which are used to trim my data into a more neat manner. However, it's too long and inefficient, it takes too much time to go through all this because I have up to thousands of files that will go through this.

My main code goes through a folder and goes through all the files inside the folder to find and search for strings and then copy it out into a new worksheet.

'Test RowA(1)
wks.Cells(BlankRow, 1).Replace What:="testtest         : ", Replacement:="", LookAt:=xlPart, _
    SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
    ReplaceFormat:=False

'Start RowB(2)
wks.Cells(BlankRow, 2).Replace What:="  Started at: ", Replacement:="", LookAt:=xlPart, _
    SearchOrder:=xlByRows, MatchCase:=False, SearchFormat:=False, _
    ReplaceFormat:=False

'Temp RowC(3)
Dim strSearchC As String
Dim cCell As Range
Dim s3 As String
strSearchC = "testflyy"

Set cCell = wks.Cells(BlankRow, 3).Find(What:=strSearchC, LookIn:=xlFormulas, _
LookAt:=xlPart, SearchOrder:=xlByRows, SearchDirection:=xlNext, _
MatchCase:=False, SearchFormat:=False)

If Not cCell Is Nothing Then
s3 = Split(cCell.Value, ":")(1)
s3 = Mid(s3, 1, 3)
cCell.Value = s3
End If

'Type RowD(4)
Dim strSearchD As String
Dim dCell As Range
Dim s4 As String
strSearchD = "testflyy"

Set dCell = wks.Cells(BlankRow, 4).Find(What:=strSearchD, LookIn:=xlFormulas, _
LookAt:=xlPart, SearchOrder:=xlByRows, SearchDirection:=xlNext, _
MatchCase:=False, SearchFormat:=False)

If Not dCell Is Nothing Then
s4 = Split(dCell.Value, "_", 3)(2)
s4 = Mid(s4, 1, 3)
dCell.Value = s4
End If

'No RowF(6)
Dim strSearchF As String
Dim fCell As Range
Dim s6 As String
strSearchF = "homebeestrash_archivetestts"

Set fCell = wks.Cells(BlankRow, 6).Find(What:=strSearchF, LookIn:=xlFormulas, _
LookAt:=xlPart, SearchOrder:=xlByRows, SearchDirection:=xlNext, _
MatchCase:=False, SearchFormat:=False)

If Not fCell Is Nothing Then
s6 = Split(fCell.Value, "testts")(1)
s6 = Mid(s6, 1, 2)
fCell.Value = s6
End If

'End RowG(7)
Dim strSearchG As String
Dim gCell As Range
Dim s7 As String
strSearchG = "homebeestrash_archivetestts"

Set gCell = wks.Cells(BlankRow, 7).Find(What:=strSearchG, LookIn:=xlFormulas, _
LookAt:=xlPart, SearchOrder:=xlByRows, SearchDirection:=xlNext, _
MatchCase:=False, SearchFormat:=False)

If Not gCell Is Nothing Then
s7 = Split(gCell.Value, "reports")(1)
s7 = Split(s7, "Report")(0)
s7 = Left(s7, 8) & " " & Right(s7, 6)

'Month RowH(8)
wks.Cells(BlankRow, 8).Value = WorksheetFunction.Transpose(Left(s7, 4) & "-" & Mid(s7, 5, 2) & "-" & Mid(s7, 7, 2)) '<~~ get 2019-03-02
gCell.Value = s7
End If

Basically these are to trim the already extracted data in each respective rows worksheet, Row A and B is just simply replacing the data with nothing, for Row C onwards to Row H,, i will search if the string exists on the particular row and then use Split or Right, Left, Mid functions.

Question: Is there any other way to write it more efficiently because there are too many codings with formats like this and I feel like its slowing my program down a lot.

So I had a look at your code and I got confused to what you were trying to do with find. So I had to test it out for myself. If I understand it correctly you are using it to figure out if a string contains the value you are looking for. If it does you continue processing if not you skip it.

However, this can be done by fetching the cell value and performing InStr <> 0 to do the same thing.

I wrote a quick VBA to see the performance of find in relation to InStr. The results of this was (as you can test yourself now) that the .Find method took aproximately 5 seconds to compute, whereas the InStr one finishes instantly. When running on 100000 lines it takes almost a minute to complete, whereas the InStr one finishes instantly. So since you perform the Cells(x,y).Find for 4 separate columns in thousand of files I would say that this is your main culprit for timeloss.

Sheet1.Cells(1, 2).Value = TimeValue(Now)

For i = 0 To 10000
Dim x
Set x = Sheet1.Cells(1, 1).Find(What:="12", LookIn:=xlFormulas, _
    LookAt:=xlPart, SearchOrder:=xlByRows, SearchDirection:=xlNext, _
    MatchCase:=False, SearchFormat:=False)
    If Not x Is Nothing Then
        Sheet1.Cells(1, 4) = "Yup"
    End If
Next i
Sheet1.Cells(1, 3).Value = TimeValue(Now)
Sheet1.Cells(2, 2).Value = TimeValue(Now)
For i = 0 To 10000
Set x = Sheet1.Cells(2, 1)
    If InStr(x, "12") <> 0 Then
        Sheet1.Cells(2, 4) = "Yup"
    End If
Next i
Sheet1.Cells(2, 3).Value = TimeValue(Now)

On top of that you are running Cells(x,y).Replace which is about twice as slow as just setting the value of the Replace as seen below.

wks.Cells(BlankRow, 1) = Replace(wks.Cells(BlankRow, 1),"string to remove","")

The methods you are running for Find and Replace are supposed to be executed on Ranges for the maximum efficiency. Say if you instead running replace on each cell one by one you execute it on the entire column straight away like this:

wks.Columns(1).Replace What:="testtest         : ", Replacement:="", LookAt:=xlPart, _
        SearchOrder:=xlByColumns, MatchCase:=False, SearchFormat:=False, _
        ReplaceFormat:=False

Then you will notice that the Replace is performed in less than a second for all rows. Making it a lot faster if you have massive files.


Implement Scala-like Trampoline to gain tail call optimization in Rust On the topic of tail call optimization, I found two RFCS: 271 and 1888.

Until this gets implemented, I wanted to do something inspired from Scala's Trampoline functionality.

The actual implementation is far from a full-fledged tail call optimization in that it replaces recursive function calls with iterative ones, but it has its use for keeping the stack depth low. My actual state of work is as follows:

enum Trampoline<A, R> {
    Continue(A, R),
    End(R),
}

impl<A, R> Trampoline<A, R> {
    fn start(function: &Fn(A, R) -> Trampoline<A, R>, mut arg: A, mut sum: R) -> R
    {
        loop {
            match function(arg, sum) {
                Trampoline::Continue(r_arg, r_sum) => {
                    arg = r_arg;
                    sum = r_sum;
                },
                Trampoline::End(result) => return result,
            }
        }
    }
}

fn recurse_trampolin(arg: i32, sum: i32) -> Trampoline<i32, i32> {
    if arg == 0 {
        Trampoline::End(sum)
    } else {
        Trampoline::Continue(arg - 1, sum + arg)
    }
}

fn recurse_normal(arg: i32, sum: i32) -> i32 {
    if arg == 0 {
        sum
    } else {
        recurse_normal(arg - 1, sum + arg)
    }
}

fn main() {
    println!("{}", recurse_normal(5, 0));
    println!("{}", Trampoline::start(&recurse_trampolin, 5, 0));
}

Apart from using references for compatibility with non-Copy types, can this construction be further optimized or the code simplified/beautified?

Run Rustfmt. It will automatically fix such things as

Opening curly braces go on the same line as the function signature (unless the function signature is already multiple lines).
There is no comma at the end of a match arm that uses curly braces.

Your function name has a typo: recurse_trampolin
Instead of taking a trait object reference (&Fn(...) -> ...), take a generic. This allows for better optimizations and avoids unnecessary indirection.

enum Trampoline<A, R> {
    Continue(A, R),
    End(R),
}

impl<A, R> Trampoline<A, R> {
    fn start<F>(function: F, mut arg: A, mut sum: R) -> R
    where
        F: Fn(A, R) -> Trampoline<A, R>,
    {
        loop {
            match function(arg, sum) {
                Trampoline::Continue(r_arg, r_sum) => {
                    arg = r_arg;
                    sum = r_sum;
                }
                Trampoline::End(result) => return result,
            }
        }
    }
}

fn recurse_trampoline(arg: i32, sum: i32) -> Trampoline<i32, i32> {
    if arg == 0 {
        Trampoline::End(sum)
    } else {
        Trampoline::Continue(arg - 1, sum + arg)
    }
}

fn recurse_normal(arg: i32, sum: i32) -> i32 {
    if arg == 0 {
        sum
    } else {
        recurse_normal(arg - 1, sum + arg)
    }
}

fn main() {
    println!("{}", recurse_normal(5, 0));
    println!("{}", Trampoline::start(recurse_trampoline, 5, 0));
}


Roman numerals to Arabic number Is there any way to make this for loop more concise or efficient? I fell like there should be a way that you can cahnge the for loop so that it is about half the size that it is know. By the way, the array list defaultArabic has the values of {1,5,10,50,100,500,1000). Also, I'm not sure if you need to know this, but the goal of this code is the convert tradition roman letter such as (I,V,X,etc) to our form of numbers. Additionally, I am not looking for a whole new for loop just a revised one, as this for loop works correctly. However, it is just really long, bulky, and repetitive.

for (c = 0; c < 15; c++)
{
    findRomans = romans.get(c);
    if (findRomans.equals("I"))
    {
        if (romans.get(c + 1).equals("I"))
        {
            arabic += defaultArabics.get(0);
        }
        else
        {
            arabic -= defaultArabics.get(0);
        }
    }
    else if (findRomans.equals("V"))
    {
        if (romans.get(c + 1).equals("I") || romans.get(c + 1).equals("V"))
        {
            arabic += defaultArabics.get(1);
        }
        else
        {
            arabic -= defaultArabics.get(1);
        }
    }
    else if (findRomans.equals("X"))
    {
        if (romans.get(c + 1).equals("I") || romans.get(c +  1).equals("V") || romans.get(c + 1).equals("X"))
        {
            arabic += defaultArabics.get(2);
        }
        else
        {
            arabic -= defaultArabics.get(2);
        }
    }
    else if (findRomans.equals("L"))
    {
        if (romans.get(c + 1).equals("I") || romans.get(c + 1).equals("V") || romans.get(c + 1).equals("X") || romans.get(c + 1).equals("L"))
        {
            arabic += defaultArabics.get(3);
        }
        else
        {
            arabic -= defaultArabics.get(3);
        }
    }
    else if (findRomans.equals("C"))
    {
        if (romans.get(c + 1).equals("I") || romans.get(c + 1).equals("V") || romans.get(c + 1).equals("X") || romans.get(c + 1).equals("L") || romans.get(c + 1).equals("C"))
        {
            arabic += defaultArabics.get(4);
        }
        else
        {
            arabic -= defaultArabics.get(4);
        }
    }
    else if (findRomans.equals("D"))
    {
        if (romans.get(c + 1).equals("I") || romans.get(c + 1).equals("V") || romans.get(c + 1).equals("X") || romans.get(c + 1).equals("L") || romans.get(c + 1).equals("C") || romans.get(c + 1).equals("D"))
        {
            arabic += defaultArabics.get(5);
        }
        else
        {
            arabic -= defaultArabics.get(5);
        }
    }
    else if (findRomans.equals("M"))
    {
        if (romans.get(c + 1).equals("I") || romans.get(c + 1).equals("V") || romans.get(c + 1).equals("X") || romans.get(c + 1).equals("L") || romans.get(c + 1).equals("C") || romans.get(c + 1).equals("D") || romans.get(c + 1).equals("M"))
        {
            arabic += defaultArabics.get(6);
        }
        else
        {
            arabic += defaultArabics.get(6);
        }
    }
}
return arabic;

There is a common pattern followed in all the if-else blocks in the given code. This can be moved to a function or a loop.

Here the pseudo code does this using a loop. As the condition in each successive block is cumulative, it is tracked as a separate variable modified by each successive iteration.

arr = {"I", "V", "X", "L", "C", "D", "M"}; // Store these in an array
for (c = 0; c < 15; c++) {
    findRomans = romans.get(c);
    condition = false;
    for(index = 0; index < size(arr); index++) {
        condition = condition || romans.get(c + 1).equals(arr[index]);
        if (findRomans.equals(arr[index])) {
            int multiplier = condition ? 1 : -1;
            arabic += multiplier * defaultArabics.get(index);
        }
    }
}
return arabic;

This code is just more concise than the original one. There is no efficiency gained here.


10x10 bitmapped square with bits This program prints out a 10x10 square and uses only bit operations. It works well, but I don't like that global array.

Can you tell me if the code is proper or not?

#include <iostream>

const std::size_t HEIGHT = 10;
const std::size_t WIDTH = 10;

char graphics[WIDTH/8][HEIGHT];

inline void set_bit(std::size_t x, std::size_t y)
{
    graphics[(x) / 8][y] |= (0x80 >> ((x) % 8));
}

void print_screen(void)
{
    for (int y = 0; y < HEIGHT; y++)
    {
        for (int x = 0; x < WIDTH/8+1; x++)
        {
            for (int i = 0x80; i != 0; i = (i >> 1))
            {
                if ((graphics[x][y] & i) != 0)
                    std::cout << "*";
                else
                    std::cout << " ";
            }
        }
            std::cout<<std::endl;
    }
}

int main()
{
    for(int x = 0; x < WIDTH; x++)
    {
        for(int y = 0; y < HEIGHT; y++)
        {
            if(x == 0 || y == 0 || x == WIDTH-1 || y == HEIGHT-1)
                set_bit(x,y);
        }
    }
    print_screen();
    return 0;
}

That global array is indeed not good.  You'll need to pass around an array, but you shouldn't do it with a C-style array.  Doing that will cause it to decay to a pointer, which you should avoid in C++.  If you have C++11, you could use std::array, which will be set at an initial size.  But if you don't have C++11, and also want to adjust the size, use an std::vector.  You can also compare the two here.  Either way, you'll be able to pass any of them around nicely, and it's something you should be doing in C++ anyway.

To match your environment, the following code does not utilize C++11.

I'll use std::vector here, but this can be done with other STL storage containers.  Here's what a 2D vector would look like:

std::vector<std::vector<T> > matrix; // where T is the type

This type does look long, and you may not want to type it out each time.  To "shorten" it, you can use typedef to create an alias (which is not a new type):

typedef std::vector<std::vector<T> > Matrix;

With that, you can use this type as such:

Matrix matrix;

and create the 2D vector of a specific size.

However, this is where the syntax gets nasty (especially lengthy).  It's not set to a specific size, so you can just push vectors into it to increase the size.  For a fixed size (using your size and data type), you'll have something like this:

std::vector<std::vector<char> > matrix(HEIGHT, std::vector<char>(WIDTH));

This can be made shorter by having another typedef to serve as a dimension of the matrix.  This will also make it a little clearer what the vector means in this context.

typedef std::vector<char> MatrixDim;

It is then applied to the Matrix typedef:

typedef std::vector<MatrixDim> Matrix;

The 2D initialization will then become this:

Matrix matrix(HEIGHT, MatrixDim(WIDTH));

Now you can finally use this in main() and pass it to the other functions.  Before you do that, you'll need a different loop counter type.  With an STL storage container, you should use std::size_type.  With std::vector<char>, specifically, you'll have:

std::vector<char>::size_type;

You can use yet another typedef for this:

typedef MatrixDim::size_type MatrixDimSize;

Here's what the functions will look like with the changes (explanations provided).  I've also included some additional changes, which are also explained.  The entire program with my changes applied and produces the same output as yours.

setbit():

inline void set_bit(Matrix& matrix, MatrixDimSize x, MatrixDimSize y)
{
    matrix[(x) / 8][y] |= (0x80 >> ((x) % 8));
}

An additional parameter of type Matrix is added.  The matrix is passed in by reference and modified within the function.
The std::size_t parameters were replaced with the MatrixDimSize type.

print_screen():

void print_screen(Matrix const& matrix)
{
    for (MatrixDimSize y = 0; y < HEIGHT; y++)
    {
        for (MatrixDimSize x = 0; x < WIDTH/8+1; x++)
        {
            for (int i = 0x80; i != 0; i >>= 1)
            {
                std::cout << (((matrix[x][y] & i) != 0) ? '*' : ' ');
            }
        }

        std::cout << "\n";
    }
}

A parameter of type Matrix is added.  The matrix is passed in by const&, which is necessary as the function displays the matrix but does not modify it.  It's also cheaper to pass it this way as opposed to copying (passing by value).
MatrixDimSize is added for the loop counter types.
The if/else is replaced with an equivalent ternary statement.
A newline is done with "\n" as opposed to std::endl.  The latter also flushes the buffer, which is slower.  You just need the former.
i = (i >> 1) is shortened to i >>= 1.

Main():

int main()
{
    Matrix matrix(HEIGHT, MatrixDim(WIDTH));

    for (MatrixDimSize x = 0; x < WIDTH; x++)
    {
        for (MatrixDimSize y = 0; y < HEIGHT; y++)
        {
            if (x == 0 || y == 0 || x == WIDTH-1 || y == HEIGHT-1)
            {
                set_bit(matrix, x, y);
            }
        }
    }

    print_screen(matrix);
}

Both matrix vector typedefs are applied.
MatrixDimSize is added for the loop counter types.
The matrix is passed to and modified only by set_bit().

It is passed to print_screen() and is not modified.


File class and large constructor I have some dispute with a colleague. I work with some REST-service, from which I get always same type data.

class File():
    def __init__(self, name, created, modified, media_type, path, md5, type, mime_type, size,
                 public_key=None, public_url=None, preview=None):
        self.public_url = public_url
        self.preview = preview
        self.name = name
        self.created = created
        self.modified = modified
        self.path = path
        self.type = type
        self.media_type = media_type
        self.md5 = md5
        self.mime_type = mime_type
        self.size = size

The following code snippet demonstrate, how I work with File class:

def get_list_of_all_files(self):
    url = self._base_url + "/resources/files"
    r = requests.get(url, headers=self.base_headers)
    json_dict = r.json()

    files = []

    for item in json_dict["items"]:
        f = File(**item)
        files.append(f)

    return files

My colleague says that the constructor is very large, and this code is horrible. Is he right? How should look like this code?

There are three ways I know of to deal with large container classes:

large constructors
post-constructor modifiers
sub-categories

None of them are 'nice'.

Large Constructor

Your code is an example of the large constructor.

Post Construction

The post-constructor modifier system will have a bunch of 'set' methods, and you call them like:

 myFile.setSize(...);
 myFile.setMD5(...);

Sub-Categories

The sub-cateogory option is not awful, it is done by grouping related parameters together. For example, the two times could be one class:

class FileTimes():
    def __init__(self, created, modified):
        self.created = created
        self.modified = modified

then the file locations could be grouped as well

class FileLocation():
    def __init__(self, path, public_url=None):
        self.public_url = public_url
        self.path = path

FileGroup would be, perhaps, the various 'types', like type, media_type, mime_type.

Once you have sub-categorized your parameters, the constructor becomes simpler, like:

def __init__(self, name, locations, times, types, md5, size, public_key=None, preview=None):

But, though you have simplified this class's constructor, you now have to build up a hierarchy of data to initialize it.... which is not simple.

Recommendation

As for me, given the nature of your JSON data, it appears that the large constructor is the lesser of the evils. It is 'tailored' to match the JSON data format, and it works well.

Your question:

  My colleague says that the constructor is very large, and this code is horrible. Is he right?

The answer is: yes, it is large, and the code is ugly.... and he's right. But, not everything can be small, and pretty. This is the lesser of the evils.

I would not change it. Your class has a lot of related data, it has to get in there somehow.

Other Issues

Your constructor has the input public_key but does not store that as a property.

You should consider using a generator in your get_list_of_all_files: you should yield each File, instead of returning the completed list.


Basic Sieve of Erosthenes This is a basic command line program that, given a number, returns a list of primes up to and including that number. Also handles a lack of command line arguments and non-number arguments.

I ran into a bunch of problems with Options while I was writing this, which led to me using unwrap() out of desperation. I'd rather never use unwrap, and instead handle None or Err or whatever. Additionally, I'd prefer not to have everything so mutable.

// A bad implementation of the Sieve of Eratosthenes

// Usage:
//  primesTo x = comma-separated list of all primes up to x

use std::env;

// Returns a vector of primes from 2 to max
fn primes_to(max: u32) -> Vec<u32> {
    let mut all: Vec<u32> = (2..max+1).collect();
    let mut primes: Vec<u32> = vec!();

    while all.len() > 0 {
        let (a, b) = sieve(all, primes);
        all = a;
        primes = b;
    }
    primes
}

// Sieve a non-empty list
fn sieve(list: Vec<u32>, mut primes: Vec<u32>) -> (Vec<u32>, Vec<u32>) {
    // We have a Vec<u32>
    // We want a u32 and a Vec<u32>
    let mut i_list = list.into_iter();
    let head: u32 = i_list.next().unwrap();
    let mut tail: Vec<u32> = i_list.collect();
    tail.retain(|&x| x % head != 0);
    primes.push(head);
    (tail, primes)
}

fn prettyprint_primes(mut primes: Vec<u32>) -> () {
    let temp = primes.pop().unwrap();
    for prime in primes {
        print!("{:}, ", prime);
    }
    print!("{:}", temp);
}

fn main() {
    let args: Vec<String> = env::args().collect();
    let arg: Option<&String> = args.get(1);

    match arg {
        None => {
            println!("Please enter a number to receive primes.");
            println!("Usage:");
            println!("\ttoPrime x");
        },
        Some(x) => {
            match x.parse::<u32>() {
                Ok(max) => {
                    let primes: Vec<u32> = primes_to(max);
                    prettyprint_primes(primes);
                },
                _ => println!("That's not a number; please enter a number"),
            }
        }
    }
}

I think this is one of the few "Sieves of Erosthenes" that actually implements the sieving method; congratulations! ^_^

Run rustfmt:

Spaces go around binary operators:

let mut all: Vec<u32> = (2..max + 1).collect();

vec! should always be called with square brackets:

let mut primes: Vec<u32> = vec![];

There's no comma after the curly brace of a match arm.

Run Clippy:

warning: length comparison to zero
  --> src/main.rs:13:11
   |
13 |     while all.len() > 0 {
   |           ^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!all.is_empty()`
   |
   = note: #[warn(len_zero)] on by default
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#len_zero

You are declaring types way more frequently than you need to. Type inference can handle most of these cases. Other times, you can let the compiler infer a part of the type, such as with collect: let foo: Vec<_> = iter.collect().
Converting a Vec to an iterator, popping one element, then converting back to a Vec is expensive. Instead, just get the first element of the input using the index operator (foo[0]).
Once you've done that, you might as well just pass in a mutable reference to the list to avoid taking ownership of the list and passing back ownership and dealing with the tuple output.
Once you've done that, there's no reason to pass in the whole prime list just to append to it and get it back. Instead, return the new prime number from the function and append it later.
Use your function comments to enhance your function names, instead of them being dead text warning about how to call them ("non empty list").
Your desire to avoid unwrap is well-founded. Your code crashes if run with 1 as the argument. To properly format a comma-separated list, check out What's an idiomatic way to print an iterator separated by spaces in Rust? and In Rust, what is the best way to print something between each value in a container. TL;DR: use Itertools::format.
Using {:} as the format specifier is very unusual — I had to think hard about what it even meant. You should use just {}.
Don't take a Vec if you are just going to iterate over it.
Don't explicitly mark () as the return type of a function, that's the default.
Instead of collecting your arguments into a vector, just grab the single argument you care about using Iterator::nth.

extern crate itertools;

use itertools::Itertools;

use std::env;

fn primes_to(max: u32) -> Vec<u32> {
    let mut all: Vec<_> = (2..max + 1).collect();
    let mut primes = vec![];

    while !all.is_empty() {
        primes.push(sieve_non_empty_list(&mut all));
    }

    primes
}

fn sieve_non_empty_list(list: &mut Vec<u32>) -> u32 {
    let head = list[0];
    list.retain(|&x| x % head != 0);
    head
}

fn prettyprint_primes(primes: &[u32]) {
    print!("{}", primes.iter().format(", "))
}

fn main() {
    let arg = env::args().nth(1);

    match arg {
        None => {
            println!("Please enter a number to receive primes.");
            println!("Usage:");
            println!("\ttoPrime x");
        }
        Some(x) => {
            match x.parse() {
                Ok(max) => {
                    let primes = primes_to(max);
                    prettyprint_primes(&primes);
                }
                _ => println!("That's not a number; please enter a number"),
            }
        }
    }
}

I'd probably implement the sieve as an iterator, however. This has the effect of removing the need to store the primes (the caller can do so if needed), allowing the caller to get the next prime back quicker, and encapsulates the state of the list so we don't need to document that you can only call functions in certain states.

struct Sieve {
    list: Vec<u32>,
}

impl Sieve {
    fn new(up_to: u32) -> Self {
        Self {
            list: (2..up_to + 1).collect(),
        }
    }
}

impl Iterator for Sieve {
    type Item = u32;

    fn next(&mut self) -> Option<Self::Item> {
        let val = self.list.get(0).cloned();

        if let Some(head) = val {
            self.list.retain(|&x| x % head != 0);
        }

        val
    }
}

And it can be printed directly as:

let primes = Sieve::new(max);
print!("{}", primes.format(", "))


Dining philosophers problem in C++11 Is this implementation correct? Do you find any threading problem? Also, what parts of the code can be changed to be more C++11-ish?

#include <iostream>
#include <vector>
#include <thread>
#include <random>
#include <condition_variable>

namespace {
    int sleep_for_random_time() {
        std::random_device rd;
        std::mt19937 mt(rd());
        std::uniform_real_distribution<double> dist(1000, 4000);

        return dist(mt);
    }
}

class DiningPhilosopher {
public:
    DiningPhilosopher(int numOfPhilosophers_ = 5, int numOfForks_ = 5, int numOfEating_ = 3) : numOfPhilosophers(numOfPhilosophers_), numOfForks(numOfForks_), numOfEating(numOfEating_) {
        forks.push_back(new Fork);
        forks.push_back(new Fork);
        forks.push_back(new Fork);
        forks.push_back(new Fork);
        forks.push_back(new Fork);

        numAllowed = numOfForks - 1;
    }

    ~DiningPhilosopher() {
        for(const auto& fork : forks) {
            delete fork;
        }
    }

    DiningPhilosopher(const DiningPhilosopher&) = delete;
    DiningPhilosopher& operator = (const DiningPhilosopher&) = delete;

    void StartDining() {
        for(int i = 0; i < numOfPhilosophers; ++i) {
            threads.push_back(std::thread(&DiningPhilosopher::Philosopher, this, i));
        }

        std::for_each(threads.begin(),threads.end(), std::mem_fn(&std::thread::join));
    }
private:
    void WaitForPermission() {
        std::lock_guard<std::mutex> lock(permission);
        cv.wait(permission, [this] { return numAllowed > 0; });
        --numAllowed;
    }

    void GrantPermission() {
        std::lock_guard<std::mutex> lock(permission);
        ++numAllowed;

        if(numAllowed == 1) {
            cv.notify_all();
        }
    }

    void Think(int id) {
        std::chrono::milliseconds duration(sleep_for_random_time());
        std::this_thread::sleep_for(duration);
    }

    void Eat(int id) {
        this->WaitForPermission();
        forks[id]->mux.lock();
        forks[(id + 1) % numOfForks]->mux.lock();

        std::cout << "Philosopher "<< id << " is eating" << std::endl;
        this->Think(id);
        std::cout << "Philosopher "<< id << " has finished eating" << std::endl;

        forks[id]->mux.unlock();
        forks[(id + 1) % numOfForks]->mux.unlock();

        this->GrantPermission();
    }

    void Philosopher(int id) {
        for(int i = 0; i < numOfEating; ++i) {
            this->Think(id);
            this->Eat(id);
        }

        std::cout << "Philosopher " << id << " DONE" << std::endl;
    }

    struct Fork {
        std::mutex mux;
    };

    int numOfEating;
    int numOfPhilosophers;
    int numAllowed;
    int numOfForks;
    std::vector<std::thread> threads;
    std::vector<Fork*> forks;
    std::mutex permission;
    std::condition_variable_any cv;
};

int main()
{
    DiningPhilosopher dp;
    dp.StartDining();

    return 0;
}

struct Fork {
    std::mutex mux;
};
std::vector<Fork*> forks;

I think the problem you want to solve by using pointers here is that a std::mutex is not copyable nor movable. By using raw owning pointers, you'll get a bunch of unpleasant properties. For example, you have to implement a destructor, and might have to worry about exception safety.

Typically, using smart pointers instead of raw owning pointers is a better idea, since it avoids many of those unpleasant side-effects. But the original problem can be solved differently:

std::vector<std::mutex> forks; // data member
DiningPhilosopher(..) // constructor
  : // ...
  , forks(5)
{}

This uses a vector constructor that constructs 5 std::mutex in-place. It therefore does not require that the value type is movable. You cannot resize such a vector, though, nor can you add new entries via push_back etc, even if there's space left (capacity > size). If you have a fixed number of forks, you could use a std::array as well.

If you need to add more forks later, or need to resize:

std::vector<std::unique_ptr<std::mutex>> forks;
DiningPhilosopher(..) // constructor
  : // ...
  , forks(5)
{
    for(auto& fork : forks) fork = std::make_unique<std::mutex>();
}

make_unique is C++14, but can be easily implemented in C++11 as well.
It is also possible to use std::generate_n:

DiningPhilosopher(..) // constructor
  : // ...
{
    std::generate_n(std::back_inserter(forks), 5
                    []{ return std::make_unique<std::mutex>(); });
}

threads.push_back(std::thread(&DiningPhilosopher::Philosopher, this, i));

This can be replaced by the slightly simpler:

threads.emplace_back(&DiningPhilosopher::Philosopher, this, i);

Or even:

threads.push_back([this, i]{ Philosopher(i); });

std::for_each(threads.begin(),threads.end(), std::mem_fn(&std::thread::join));

alternative:

for(auto& t : threads) t.join();

One might argue about the use of "raw loops", but you don't need the position of each thread (the iterator) here. Therefore, I'd say that a for-each loop is fine as well; and it's shorter.

void Philosopher(int id)

The name of this function surprised me when I encountered &DiningPhilosopher::Philosopher earlier. It almost looked like a data member, since it's not formulated as a verb. From a code organization perspective, I think it should be earlier in the code, before the functions it calls. This allows a top-down reading from the higher to lower level functions.

 std::cout << "Philosopher "<< id << " is eating" << std::endl;
 this->Think(id);
 std::cout << "Philosopher "<< id << " has finished eating" << std::endl;

The philosopher thinks while they're eating? Ok, I think it is code reuse, but the name confused me. Why not introduce an intermediary function wait_for_random_time that is used to implement both thinking and (the actual) eating? Similarly, the function Eat doesn't just let a philosopher eat. It does more (waiting to eat etc.)

int sleep_for_random_time() {
    std::random_device rd;
    std::mt19937 mt(rd());
    std::uniform_real_distribution<double> dist(1000, 4000);

    return dist(mt);
}

glampert has already criticised the name of this function in his answer, the fact that it repeatedly creates a random_device as well as a mt19937, and the usage of double.

I'd also like to point out that creating both generators seems strange to me: First you get a (potentially pseudo-)random number from one random number generator (the random_device). Then, you use it to seed another random number generator, to create a pseudo-random number. The second step is unnecessary, if you trust the quality of your random_device implementation enough: It either produces true random numbers, or - and that's the Quality Of Implementation aspect - evenly distributed pseudo-random numbers. The following should be sufficient:

int sleep_for_random_time() {
    std::random_device rd;
    std::uniform_int_distribution<int> dist(1000, 4000);
    return dist(rd);
}

The original code followed a common pattern of creating pseudo-random numbers. Typically, one does not use the random device to create all random numbers because it is slow (it might run out of entropy, too, depending on the implementation). You'd therefore use it only once to initialize a single mersenne twister. Successive requests for pseudo-random numbers would use this single mersenne twister to produce pseudo-random numbers. For example:

 int sleep_for_random_time() {
     static std::mt19937 mt{ std::random_device{}() };
     std::uniform_int_distribution<int> dist(1000, 4000);
     return dist(mt);
 }

Arguably, even the distribution should be static, to allow recycling unused bits of the output of the generator. As Deduplicator points out, there could/should be one generator per thread. The above code uses one shared generator, which is not thread-safe, since dist(mt) modifies the state of the (now shared) generator. There are several ways to make this thread-safe, one of them is:

 int sleep_for_random_time() {
     thread_local std::mt19937 mt{ std::random_device{}() };
     std::uniform_int_distribution<int> dist(1000, 4000);
     return dist(mt);
 }

This associates an individual mersenne twister for each thread. Each of them is created once, for the lifetime of the thread.

this->Think(id);

Again this line, but this time in isolation. First, why is there a this? It seems to be used whenever you call a member function. Why?

Additionally, the id is currently unnecessary. If it was not, there should be a way IMHO to make sure that each philosopher thinks for themselves. Something like

Think();

should be sufficient; however it requires an object to represent each philosopher. Since you already have a thread object for each philosopher, that seems possible and simple to realize. These objects could also get (read-only!) access to the options such as numOfEating.


Table builder pattern Trying to combine functional style (immutable objects) and flexibility of property setters. For the sake of example, let’s say we have a soil types table with two attributes: Color and Name. I am looking for a way to alternate Names, but not Color. Here is how I solved it:

        // retrieving: all objects are immutable
        SoilTypes types = SoilTypes.Default;
        ISoilType clay1 = types.Clay;
        ISoilType clay2 = types[3];

        // derive an alternated immutable copy
        SoilTypes altTypes = types
            .With(tt =>
            {
                // tt.SensitiveFines.Color is still read only
                tt.SensitiveFines.Name = "Very sensitive fines!";
                tt[2].Name = "Purely Organic soil!";
            });

        // retrieving: everything is immutable
        ISoilType sensitiveFines = altTypes.SensitiveFines;

Where this interface is immutable:

public interface ISoilType
{
    Color Color { get; }
    string Name { get; }
}

And this class is mutable:

public class SoilType : ISoilType
{
    public static implicit operator SoilType((Color Color, string Name) tuple) =>
         new SoilType(tuple.Color, tuple.Name);

    internal SoilType(ISoilType source)
        : this(source.Color, source.Name)
    {
    }

    internal SoilType(Color color, string name)
    {
        Color = color;
        Name = name;
    }

    public Color Color { get; }
    public string Name { get; set; }
}

And this non-generic class is immutable:

public class SoilTypes : SoilTypes<ISoilType>
{
    public static SoilTypes Default = new SoilTypes(
        (White, "Undefined"),
        (Red, "Sensitive Fines"),
        (Green, "Organic Soil"),
        (Blue, "Clay"),
        (Orange, "Silty Clay"));

    public SoilTypes(params SoilType[] types)
        : base(types)
    {
    }

    public SoilTypes With(Action<SoilTypes<SoilType>> update)
    {
        var copy = this
            .Select(t => new SoilType(t))
            .ToArray();

        update(new SoilTypes<SoilType>(copy));
        return new SoilTypes(copy);
    }
}

while this generic base used in both situations:

public class SoilTypes<TType> : ReadOnlyCollection<TType>
    where TType : ISoilType
{
    internal SoilTypes(TType[] types)
        : base(types)
    {
    }

    public TType Undefined => this[0];
    public TType SensitiveFines => this[1];
    public TType OrganicSoil => this[2];
    public TType Clay => this[3];
    public TType SiltyClay => this[4];
}

I'm afraid this is not fully immutable becasue I am able to change the Name with a simple cast:

altTypes.Dump();
((SoilType)altTypes.SensitiveFines).Name = "foo";
altTypes.Dump();

The underlying data type is still SoilType so the interface does not protect the data from being overriden.

Consider a user that writes a function like this one because he doesn't like interfaces :-)

public static void foo(SoilType bar)
{
    bar.Name = "new name";
}

and calls it

foo((SoilType)altTypes.SensitiveFines);
altTypes.Dump();

Name changed. Unfortunatelly I have no idea how to prevent it yet.


Java multiple completablefuture I wrote the following snipet code within Cumulocity platform.

  runForTenant

must be in a separate thread. What is your an opinion about my code. is there any need for optimization? Thanks

LinkedList<CompletableFuture<Void>> asyncThreads = new LinkedList<CompletableFuture<Void>>();
        CompletableFuture<Iterable<ManagedObjectRepresentation>> response = new CompletableFuture<>();

        subscriptions.runForEachTenant(() -> {
            String tenant = subscriptions.getTenant();
            asyncThreads.add(CompletableFuture.runAsync(() -> {
                subscriptions.runForTenant(tenant, () -> {
                    Iterable<ManagedObjectRepresentation> objects = inventoryApi
                            .getManagedObjectsByFilter(customInventoryFilter).get().allPages();
                    if (!ObjectUtils.isEmpty(objects)) {
                        response.complete(objects);
                    }

                });
            })); // end of runAsync()
        });

        boolean allThreadsAreDone = true;
        do { // until all threads are done ...
            try {
                return response.get(50, TimeUnit.MILLISECONDS); // wait for first result
            } catch (TimeoutException e) {
                // device with UUID Not found in time

                // check all threads are done/closed/stopped in all fashions
                allThreadsAreDone = true;
                for (int i = 0; allThreadsAreDone && i < asyncThreads.size(); i++) {
                    allThreadsAreDone = asyncThreads.get(i).isDone();
                }
            } catch (ExecutionException execExc) {
                logger.error("Error on lookup: " + execExc.getLocalizedMessage());
            } catch (InterruptedException interExc) {
                logger.error("Error on lookup: " + interExc.getLocalizedMessage());
            }

        } while (!allThreadsAreDone);

I want to optimize the last path of the code:

boolean allThreadsAreDone = true;
    do { // until all threads are done ...
        try {
            return response.get(50, TimeUnit.MILLISECONDS); // wait for first result
        } catch (TimeoutException e) {
            // device with UUID Not found in time

            // check all threads are done/closed/stopped in all fashions
            allThreadsAreDone = true;
            for (int i = 0; allThreadsAreDone && i < asyncThreads.size(); i++) {
                allThreadsAreDone = asyncThreads.get(i).isDone();
            }
        } catch (ExecutionException execExc) {
            logger.error("Error on lookup: " + execExc.getLocalizedMessage());
        } catch (InterruptedException interExc) {
            logger.error("Error on lookup: " + interExc.getLocalizedMessage());
        }

    } while (!allThreadsAreDone);

I have one advice, but since the code is not compiling, it's a bit hard to make a proper review.

Instead of using an java.util.LinkedList, I suggest that you use the java.util.concurrent.ExecutorService to handle the threads and use it to check the end of the threads.

        ExecutorService taskExecutor = Executors.newFixedThreadPool(4);

        CompletableFuture<Iterable<ManagedObjectRepresentation>> response = new CompletableFuture<>();

        subscriptions.runForEachTenant(() -> {
            String tenant = subscriptions.getTenant();
            taskExecutor.execute(CompletableFuture.runAsync(() -> {
                subscriptions.runForTenant(tenant, () -> {
                    Iterable<ManagedObjectRepresentation> objects = inventoryApi
                            .getManagedObjectsByFilter(customInventoryFilter).get().allPages();

                    if (!ObjectUtils.isEmpty(objects)) {
                        response.complete(objects);
                    }

                });
            }));
        });

        taskExecutor.shutdown();
        try {
            taskExecutor.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS);
        } catch (InterruptedException e) {

        }

        // The threads are finished.

Ref: https://stackoverflow.com/questions/1250643/how-to-wait-for-all-threads-to-finish-using-executorservice


Avoiding Python tricks to find anagrams What are anagrams?
An anagram of a string is another string that contains the same characters, only the order of characters can be different. For example, “abcd” and “dabc” are an anagram of each other
def anagram_check(string1,string2):
    list1 = [char1.lower() for char1 in string1 if char1 != " "]
    list2 = [char2.lower() for char2 in string2 if char2 != " "]
    if len(list1) != len(list2):
        return False
    else:
        list1.sort()
        list2.sort()
        return list1 == list2

print(anagram_check('clint eastwood','old west action'))

Question
I have started to learn data structures and algorithms, and my question is: Am I using too many Python tricks to solve this problem or do I need to think of a more basic approach to solve this?

Taking advantage of the work previously submitted by @hjpotter92 and adding an extra twist:
def anagram_check(*args ):
    return len( { tuple(sorted(x.lower().replace(' ', ''))) for x in args } ) == 1

This, potentially, gains an additional pythonicity score by avoiding repetition of the sorted(x.lower().replace( ...) construct. Instead, the construct is applied to both arguments in the set comprehension, and the test to see if the resulting set has a single element provides the answer. This implementation also generalizes the API to cover more that two strings if wanted, returning True if all strings given are anagrams of each other (thanks to Heap Overflow for pointing this out).
If you really want a function which is limited to two strings, you might prefer:
def anagram_check(string1, string2):
    return len({tuple(sorted(x.lower().replace(' ', ''))) for x in [string1,string2]}) == 1

The code can be made less terse and more legible by defining a function to act on each string:
def make_unique(x):
     return tuple( sorted( x.lower().replace(' ', '') ) )

def anagram_check(*args):
     return len( { make_unique(x) for x in args } ) == 1


Copying directories in Windows 10 I wrote a program to copy directories in Windows 10. I have tested copying a directory (696MB) in 3 different ways:

xcopy: time taken - 52 seconds
ctrl+c: time taken - 78 seconds
My program: time taken - 193 seconds

What can I do to improve the performance of my program to be as fast as xcopy?

#include <stdio.h>
#include <string.h>
#include <time.h>
#include <io.h>
#include <Windows.h>

int copyfile(const char *inputfile, const char *outputfile);
void CopyDirectory(const char *src_dir, const char *dst_dir);

int main(void)
{
    clock_t t1 = clock();

    // src directory must end with \\*
    /* dst directory must end with \\ */
    CopyDirectory("C:\\Users\\green\\Downloads\\Folder1\\*",
                  "C:\\users\\green\\desktop\\Folder1\\");

    clock_t t2 = clock();
    printf("time elapsed : %.4f secs\n", (double)(t2 - t1) / CLOCKS_PER_SEC);

    puts("Press any key to continue...");
    getchar();
    return 0;
}

void CopyDirectory(const char *src_dir, const char *dst_dir)
{
    intptr_t hFile;
    struct _finddata_t c_file;

    char *ptr;
    char buf[MAX_PATH + 20];
    char tmp_dir[MAX_PATH + 20];
    char f_name[MAX_PATH + 20];

    strcpy(buf, src_dir);
    ptr = buf + strlen(buf) - 1;

    if ((hFile = _findfirst(src_dir, &c_file)) == -1L) {
        fprintf(stderr, "Failed to open directory %s\n", src_dir);
        return;
    }
    CreateDirectoryA(dst_dir, NULL);
    while (_findnext(hFile, &c_file) == 0) {
        if (c_file.attrib & _A_SUBDIR) {
            if (strcmp(c_file.name, "..")) {
                /*......*/
                strcpy(tmp_dir, dst_dir);
                strcat(tmp_dir, c_file.name);
                strcat(tmp_dir, "\\");
                /*......*/
                *ptr = '\0';
                strcat(buf, c_file.name);
                strcat(buf, "\\*");
                CopyDirectory(buf, tmp_dir);
            }
        }
        else {
            strcpy(tmp_dir, dst_dir);
            strcat(tmp_dir, c_file.name);

            *ptr = '\0';
            strcpy(f_name, buf);
            strcat(f_name, "\\");
            strcat(f_name, c_file.name);
            copyfile(f_name, tmp_dir);
            strcpy(tmp_dir, dst_dir);
        }
    }
    _findclose(hFile);
}

int copyfile(const char *inputfile, const char *outputfile)
{
    static FILE *fp_I;
    static FILE *fp_O;

    fp_I = fopen(inputfile, "rb");
    if (!fp_I) {
        perror(inputfile);
        return -1;
    }

    fp_O = fopen(outputfile, "wb");
    if (!fp_O) {
        fclose(fp_I);
        perror(outputfile);
        return -1;
    }

    static size_t nr;
    const static size_t bufsize = 512 * 512;
    static char buf[bufsize];

    while ((nr = fread(buf, 1, bufsize, fp_I)) > 0) {
        fwrite(buf, 1, nr, fp_O);
    }

    fclose(fp_I);
    fclose(fp_O);
    return 0;
}

Re-architect to do lots of reading, then lots of writing - then loop.

OP's code presently reads file A, writes file A, reads file B, writes file B, ...

Reading file A, file B,.... partial file Z and then writing those into a large buffer is certainly faster.  Of course it is more complicated as code needs to pick-up where it left off in the middle of file Z.

I first noticed this huge saving BITD when XCOPY *.* came out as it read many files first versus the old COPY *.*.  This paradigm has been consistently true with my subsequent coding experiences.

A central issue, that unless you get deeper into the DISK I/O, code is limited to trying to do what it thinks the file system wants best in order to perform fast.

Using multiple threads (one for reading, one for writing) may help too.


Find the maximum number in an array of numbers The task is to find the maximum number in an array of numbers.
I have taken every number in the array to be byte-sized.
I have written the code such that the result is stored in ch at the end.

The code is written to run on Intel 8086.

Code optimization and improvement required.

   org 100h

    ;registers: bx - to store the base address of the array of numbers
    ;si : to store the index of the current number in the array
    ;dl: to store the iterator value of the for loop in the program
    ;dh: to store the value of n(size of the array)
    ;ch: to store the maximum element
    ;ah: temporary register to store the current element, and to compare with
    ;the number in ch

   lea bx,array
   mov si,0h
   mov dl,0h
   mov dh,n

   cmp dl,dh
   jge end

   mov cx,0h
   ;to move the first value into the min-register to compare with other elements
   mov ch,[bx+si]
   inc dl
   inc si

loop:
   cmp dl,dh
   jge end

   mov ah,[bx+si]
   inc si
   inc dl
   cmp ah,ch
   jle loop
   mov ch,ah
   jmp loop

end:

   mov ax,0h
   mov dx,0h
   mov bx,0h
   mov si,0h

   ret

array DB 4h,45h,32h,23h,9h
n DB 5

Undefined Behaviour

If you execute this code with n DB 0, it returns without setting ch to any value, leaving the result as undefined.

Signed Length

mov dl,0h
mov dh,n

cmp dl,dh
jge end

If n happens to be between 128 and 255, the jge will treat the comparison as if it was a signed comparison, and since 0 is not greater than or equal to any value between -128 and -1, inclusive, immediately jump to the end, again leaving ch as an undefined value.

You should use jae instead, so you can have up to 255 values in array.

Signed comparison

Does your array of values represent unsigned bytes, or signed bytes?  If your array contains 0FFh, is that the largest possible value, or one smaller than zero?

If the data is supposed to be signed, you're good.  If it was supposed to be unsigned bytes, you again should change the jle to jbe.

Unnecessary Register Usage

You execute mov cx,0h, but nowhere do you use cl, so are unnecessarily destroying cl.

You execute mov ah,[bx+si] and use mov ax,0h to clean-up at the end.  Again, nowhere have you used al, so you've unnecessarily destroyed al.

If you used cl instead of ah, you wouldn't need to destroy ax at all.

Use of si appears unnecessary.  Instead of based-index addressing mode:

mov cl,[bx+si]
inc si

you could use register indirect addressing mode:

mov cl,[bx]
inc bx

Better Registers

cx is the standard for loop counts.  ax is the standard for function return values.  si is the standard for source indexing.  So ...

Have your function store and return the maximum in al.  Use ah to fetch values from the array, and zero it at the end.  As a bonus, ax could also be considered the return value.  (If the array is intended to hold signed data, then use cbw to sign extend al into ax instead of zeroing ah.)

If you used cx for your array length, and decrement to zero, you could use one LOOP instruction instead of a pair of instructions to perform the compare-and-branch.  As another bonus, it will already be zero by the end, so you won't need to clear it.

Store the array address in si, and use cx for double duty by using based-index addressing (mov ah, [cx+si]) to index into your array.  Of course, you'll be searching backwards, which still fine for finding the maximum.  You'd just have to be careful to include the 0th value, and exclude the nth value (dec cx before the fetch, not afterwards).

Final tally:

return value in ax
cx zeroed (automatically)
si cleared (manually)
All other registers unchanged.


Count to target with javascript extended I previously asked for a review at Count to target with JavaScript, and it was a great help. I'm now looking for comments on the extended version. This code very simply counts in the direction of a particular target number, with the count slowing down as the target gets nearer.  The target number is stored in a nearby file and is often modified by other processes (so this should display a gradual change towards the new number.

Please be as brutal as you like with ways to improve the code or the algorithm (I'm aware there should be comments) - I'm a recreational programmer and would like to be improving my skills. This is absolutely my first use of AJAX and I'm amazed it worked at all :)

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN "http://www.w3.org/TR/html4/strict.dtd">

  <title>Count to a particular target</title>

Starting


Find Missing Numbers in an int list I have an alternative algorithm to the same problem as Finding missing items in an int list.

My implementation includes optional min and max bounds to selectively fill in the list.

The linked implementation is much simpler.

My Questions are:

How do our two algorithms compare, efficiency wise?
Besides the algorithm linked, are there more efficient algorithms?
Did I identify and does my algorithm work for the edge cases?

I tried not to use min and max because I heard that they are \$\mathcal{O}(n)\$.

However, I used the .index method for lists. I am not sure how efficient .index is. I would think that it would be \$\mathcal{O}(\log n)\$ for a sorted but I am not sure if or how the .index function would know that a given list is sorted.

Additionally, I used exceptions, which do give a performance penalty.

7 Cases

One missing number - Normal
Multiple Missing Numbers - Normal
No missing numbers - Normal
Repeats - Normal
Array too small - Case for empty array: returns an array of the numbers from min_val to max_val inclusive
Missing numbers at end - Filled in based on the min and max number
Numbers out of range - Ignored those which were out of range

from bisect import bisect_left

def find_missing(int_array, min_val = False, max_val = False):

"""Doctest

>>> find_missing([1,2,3,5,6,7], 1, 7)

[4]
>>> find_missing([2,3,6,4,8], 2, 8)

[5, 7]
>>> find_missing([1,2,3,4], 1, 4)

[]
>>> find_missing([11,1,1,2,3,2,3,2,3,2,4,5,6,7,8,9],1,11)

[10]
>>> find_missing([-1,0,1,3,7,20], -1, 7)

[2, 4, 5, 6]

>>> find_missing([-2,0,3], -5, 2)

[-5, -4, -3, -1, 1, 2]

>>> find_missing([2],4,5)

[4, 5]

>>> find_missing([3,5,6,7,8], -3, 5)

[-3, -2, -1, 0, 1, 2, 4]

>>> find_missing([1,2,4])

[3]

"""
if len(int_array) == 0:
    return list(range(min_val, max_val+1))

int_array = sorted(int_array)
first_in_int_array = int_array[0]
last_in_int_array = int_array[len(int_array)-1]

if not min_val:
    min_val = first_in_int_array

if not max_val:
    max_val = last_in_int_array
result = []

#Checking to see if the min & max values currently exist
#If they do not then add them to the resultant array as they are missing numbers
if min_val < first_in_int_array:
    int_array.insert(0, min_val)
    result.append(min_val)

if max_val > last_in_int_array:
    int_array.insert(len(int_array), max_val)
    result.append(max_val)

#Dealing with case where max_val < last_in_int_array but not in array
#Example: max_val = 2 int_array = [1,3]
#Also dealing with the analogous case for min_val and first_in_int_array
#Deal with this using exceptions
try:
    min_pos = int_array.index(min_val)

except Exception as e:
    result.append(min_val)
    min_pos = bisect_left(int_array, min_val)
    int_array.insert(min_pos, min_val)

try:
    max_pos = int_array.index(max_val)
except Exception as e:
    result.append(max_val)
    max_pos = bisect_left(int_array, max_val)
    int_array.insert(max_pos, max_val)

for i in range(min_pos,max_pos):
    difference = int_array[i+1] - int_array[i]
    if difference != 1 and difference != 0:
        result += range(int_array[i]+1,int_array[i+1])

result.sort()
return result

You say 'Doctest'  but then you format them badly with unecessary newlines and you do not actually run the tests.
Your code is much more complex than it should be.
When writing a programme remember that it should be the simplest possible.

The function down below behaves exactly like yours but:

It is 3 lines of code instead of 50.
The doctest is properly formatted so that you can actually run it.
The doctest is actually run when the script is executed as the main file.

:

import doctest

def find_missing(integers_list,start=None,limit=None):
    """
    Given a list of integers and optionally a start and an end, finds all
    the integers from start to end that are not in the list.

    'start' and 'end' default respectivly to the first and the last item of the list.

    Doctest:

    >>> find_missing([1,2,3,5,6,7], 1, 7)
    [4]

    >>> find_missing([2,3,6,4,8], 2, 8)
    [5, 7]

    >>> find_missing([1,2,3,4], 1, 4)
    []

    >>> find_missing([11,1,1,2,3,2,3,2,3,2,4,5,6,7,8,9],1,11)
    [10]

    >>> find_missing([-1,0,1,3,7,20], -1, 7)
    [2, 4, 5, 6]

    >>> find_missing([-2,0,3], -5, 2)
    [-5, -4, -3, -1, 1, 2]

    >>> find_missing([2],4,5)
    [4, 5]

    >>> find_missing([3,5,6,7,8], -3, 5)
    [-3, -2, -1, 0, 1, 2, 4]

    >>> find_missing([1,2,4])
    [3]

    """
    start = start if start is not None else integers_list[0]
    limit = limit if limit is not None else integers_list[-1]
    return [i for i in range(start,limit + 1) if i not in integers_list]

def _test():
    doctest.testmod()

if __name__ == "__main__":
    _test()


Using a for-loop to fill 20 picturebox number 'i' with image I implemented as follows and it works (I got the solution here), but I really wonder if I can do that without having to define an array. For 20 it's a hassle but can be done, but what if you have an hundred to do? Is there a more "aesthetic" way to do that?

public void afficheuneimagecombo()
    {
 for (int i = 0; i <= 20; i++)
       {
            Item imguse = new Item();
            pictureBoxs[i].Load(imguse.getImage());
            pictureBoxs[i].SizeMode = PictureBoxSizeMode.StretchImage
        }

      }

PictureBox[] pictureBoxs = new PictureBox[20];

public Form1()
        {
            InitializeComponent();

            pictureBoxs[0] = pictureBox1;
            pictureBoxs[1] = pictureBox2;
            pictureBoxs[2] = pictureBox3;
            pictureBoxs[3] = pictureBox4;
            pictureBoxs[4] = pictureBox5;
              [...]
            pictureBoxs[20] = pictureBox20;

            afficheuneimagecombo()
        }

Assuming you have a container for each of your PictureBox controls do the following:

Use a List control:

  List<PictureBox> boxes = new List<PictureBox>();

Search for your picture boxes:

  foreach (Control c in YourContainerName.Controls)
  {
    if (c is PictureBox)
    {
      boxes.Add(c);
    }
  }

Then in your method:

public void afficheuneimagecombo()
{
  foreach (PictureBox pb in boxes)
  {
    Item imguse = new Item();
    pb.Load(imguse.getImage());
    pb.SizeMode = PictureBoxSizeMode.StretchImage;
  }
}


Take an Integer and print it in a Different Base Recursively (c++) This was for a school project in my Data Structures class (I am still a very very young programmer). The purpose was to pass an interger and a base into a function and print out the result of changing that integer into that base. The project guidelines state that we can only pass an integer that is >= 0 and <= 1000000000.
The Main Function uses ifstream to stream a text document with the input into the main function.
I would like to know if there is anyway I can improve upon the BaseConverter function or anything else in the program before I submit it for grading.
I will put the full program below:
#include <iostream>
#include <fstream>
using namespace std;

/****************************************************************************************************
Function Name: BaseConverter
Description: The purpose of this function is to take the user decimal number and base that are passed
             to it and use them to recursively convert the decimal number into the base. The function
             prints the correct result to the problem onto the screen and returns void.
*****************************************************************************************************/

void BaseConverter(int num, int base)
{
    if (num == 0)
    {
        cout << "";
    }

    if (num >= base)
    {
        BaseConverter(num / base, base);

        if (num % base > 9)
        {
            cout << char(num % base + 55);
        }
        else
        {
            cout << num % base;
        }
    }

    else
    {
        cout << num % base;
    }
}

/*****************************************************************************************************
MAIN PROGRAM
******************************************************************************************************/

int main()
{
    ifstream fin("numbers.txt");

    int userNum, base;

    //sentinel control loop
    fin >> userNum >> base;
    while (userNum >= 0)
    {
        //display for input to ensure that correct input is being passed
        cout << userNum << " " << base << endl;

        if (userNum > 1000000000)
        {
            cout << "Decimal number too large for this program" << endl;
            cout << endl;
        }
        else
        {
            // call for function and output display
            cout << userNum << " in base " << base << " is ";
            BaseConverter(userNum, base);
            cout << endl;
            cout << endl;
        }

        fin >> userNum >> base;
    }

    fin.close();
    return 0;
}

1. Names/Magic numbers
What is the magic number 55 in your base converter function?
I suggest giving it a name.
2. Bounds / Magic numbers / validation
Also, what are the upper and lower bounds on which bases are allowed? Only 2-9 since you are comparing with 9? That should also be specified and checked when reading the input.
3. Code formatting / style
Your code has a lot of space (empty lines and unnecessarily many newlines).
Unless your school requires you to follow that particular style, I suggest reformatting it into a more readable style. You can for example paste it into this formatter to get a more concise and readable code.
https://codebeautify.org/cpp-formatter-beautifier
For example, this is the output of reformatting part of your code.
void BaseConverter(int num, int base) {
  if (num == 0) {
    cout << "";
  }

  if (num >= base) {
    BaseConverter(num / base, base);

    if (num % base > 9) {
      cout << char(num % base + 55);
    } else {
      cout << num % base;
    }
  } else {
    cout << num % base;
  }
}

4. cout multiple inputs
You can chain inputs to cout so you don't need to do this
cout << endl;
cout << endl;

But rather
cout << endl << endl;
5. File reading
It doesn't seem that the requirements for the assignment are to read from a file. If that is correct, I think you should skip the file reading and read from cin or a similar terminal input instead.


Correct implementation of Factory pattern in C++ I am primarily a java programmer and for the first time working in C++.

I have implemented a Factory design pattern in C++.

    class TaskFactory
    {
    public:
        typedef Task* (*create_callback)(map<string, string> vMap);
        static void register_task(const string& type, create_callback cb); // function for registering the classes with Factory
        static void unregister_task(const string& type); // function for unregistering the classess with factory
        static Task* create_task(const string& type,map<string, string> vMap); // function for creating instance of Tasks classes on the basis of class type
    private:
        static map<string, create_callback> mTasks; // map to store class types along with callback
    };

Following is the factory implementation.

    map<string, TaskFactory::create_callback> TaskFactory::mTasks;

    //registers the task with the factory
    void TaskFactory::register_task(const string& type, TaskFactory::create_callback cb){
        mTasks[type]= cb;
    }
    //unregisters the task with the factory
    void TaskFactory::unregister_task(const string& type){
        mTasks.erase(type);
    }
    //creates instance of Task subclasses on the basis of task
    Task* TaskFactory::create_task(const std::string& type, map<string, string> vMap){
        map<string, TaskFactory::create_callback>::iterator it=mTasks.find(type);
        if(it==mTasks.end()){
            return NULL;
        }else{
            return mTasks[type](vMap);
        }
    }

And my base Task class is below:

    class Task{
    private:
        map<string, string> vMap;
        Task(map<string, string>);
    public:
                    static Task* create(map<string, string>); // funtion is used for creating instance of the Task
            };

In code review I was told that Factory implementation is not doing its intended job. But in my opinion its working very well.
Is any thing wrong with this implementation?

Few small problems with the factory.

You are using a pointer to a method.

        typedef Task* (*create_callback)(map<string, string> vMap);

As the way of creating your object. This is very C like. It also introduces the possibilities of passing a NULL pointer (which is not good).

The more idiomatic way to to declare a functor:

struct TaskCreator
{
    // Leaving Task* for similarities to current code (I will get back to it).
    Task* operator()(std::map<std::string, std::string> vMap) const
    {
        return build(vMap);
    }
    private:
        virtual Task* build(std::map<std::string, std::string> vMap) const =  0;
};

Register objects

Now you should register objects derived from this class with the factory. This will prevent you accidentally passing NULL as a callback.

// This looks incant because there are no `*` here
// But there is a hidden '*' in create_callback
static void register_task(const string& type, create_callback cb);

// I would change this too:
// Notice I am not using `static` here. I will get to that in a second.
void register_task(const string& type, TaskCreator& cb);
                                          //     ^^^  That is important.
// Alternatively you can register via smart pointers.
// But I prefer to pass references. Then make the `TaskCreator` automatically
// Register themselves in their constructor.

Order of construction

You also have another invisible problem in the order of construction across compilation units. Remember that the order of construction of static storage duration objects is undefined across compilation units.

static map<string, create_callback> mTasks;

This object is constructed before main() is called. The problem is you do not know exactly when. As a result if you call register_task() before main() you don't know if it will work because the storage mTasks may not have been created yet.

There are a couple of ways of solving this. The easiest to use a static member of a function as the storage.

static map<string, create_callback>& getTaskStorage()
{
    static map<string, create_callback> mTasks;
    return mTasks;
}

But I would go the other way. And use a factory object pattern.

Factory object

You are using a set of static methods as your factory. I would swing this around and actually create a factory object. The problem with static functions is that it is hard to use test code with them. It is much easier to mock an object than static API.

Ownership

Last. You are not expressing ownership semantics for the created object.

static Task* create_task(const string& type,map<string, string> vMap);

A pointer Task* is totally useless in this situation as you have no idea if the ownership is being retained by the factory (and it will destroy it) or if ownership is being returned to the caller so he can destroy it.

Here you have two options:

Return a reference.

This indicates that the factory is retaining ownership of the created object. This means the object will be destroyed when the factory is destroyed (This implies a factory object).

Return a smart pointer

By returning a smart pointer you are indicating somthing else has ownership. The type of ownersip depends on the type of smart pointer (unique_ptr: ownership is returned to the caller, shared_ptr: ownership is shared and the loss of the last reference will delelte it).

Further Advice.

I would suggest some more specific recommendations. But unfortunately you do not provide the expected usage semantics of your factory. Thus I may write something that is tially usless. If you can write some more details on how you think the factory is going to be used I can give you some more concrete information.

Errors in the code:

class Task
{
     static Task* create(map<string, string>);
};

If you take this address it does not match:

typedef Task* (*create_callback)(map<string, string> vMap);

There is nothing in the standard that guarantees that the calling convention of a normal function matches the calling convention of static methods. It just happens to work on your compiler but it is non portable.

As stated above: What happens of somebody registers a NULL function.

return mTasks[type](vMap);

This line is likely to fail. Using your method you need to validate that the pointer is not NULL wither at the point of setting or at the point were it is used.


A binary search implementation for any datatype in C (continuation) This is an indirect follow-up to this post.

I've tried to assimilate all that was instructed and here's my attempt at binary search.

binary_search.h

#ifndef _BINARY_SEARCH_H
#define _BINARY_SEARCH_H

/*
 * Summary:
 *  Performs binary search on the given sorted array using the given
 * comparator function. Returns the index of the matching array
 * element (< count), and -1 if the element is not found.
 *
 *  Parameters are independent of type and hence this function
 * can be used for arrays of all data-types, provided that the size
 * (i.e. in terms of bytes) is properly specified.
 *
 * CAUTION: It will not work on unsorted arrays.
 *
 * Parameters:
 *  1. *x    - the address of the element to find in the array
 *  2. *base - the address of the sorted array from whence we start searching.
 *  3. count - the maximum number of elements we are willing to search against.
 *  4. size  - the size of each element of the array (in bytes)
 *  5. compare(void*, void*)
 *           - the function which is used for comparison. Zero is taken to be equality.
 */
void *binary_search(const void *x, const void *base, size_t count, size_t size,
                    int (*compare)(const void *, const void *));

#endif

binary_search.c

#include <stdlib.h>

void *binary_search(const void *x, const void *base, size_t count, size_t size,
                    int (*compare)(const void *, const void *))
{
    char *base_ptr = (char *) base;
    size_t lo = 0, hi = count - 1, mid;
    while (lo <= hi) {
        mid = lo + ((hi - lo) >> 1);
        int result = (*compare)(base_ptr + mid * size, x);
        if (result == 0) {
            return base_ptr + mid * size;
        } else if (result < 0) {
            lo = mid + 1;
        } else {
            hi = mid - 1;
        }
    }
    return NULL;
}

main.c

#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include "binary_search.h"

int compare(const void *a, const void *b)
{
    const double *p = a;
    const double *q = b;
    // The convention is to send the NaNs to the end, so
    // they are considered to be greater than others here.
    if (isnan(*p)) return +1;
    if (isnan(*q)) return -1;
    // An elegant way to convert the result to int:
    return (*p > *q) - (*p < *q);
}

int main(int argc, char const *argv[])
{
    double *a, *result;
    double x;
    size_t size, count, i;
    double aa = .1, bb = .1;
    size = sizeof(double);

    printf("Enter how many elements : ");
    scanf("%llu", &count);

    a = malloc(count * size);
    if (a == NULL) {
        printf("Error while trying to allocate memory.\n");
        return EXIT_FAILURE;
    }

    printf("Enter elements one-by-one:\n");
    for (i = 0; i < count; i++) {
        printf("%u : ", (i + 1));
        scanf("%lf", (a + i));
    }

    printf("Enter the element to look for : ");
    scanf("%lf", &x);
    result = binary_search(&x, a, count, size, compare);

    if (result) {
        printf("Element found. Index is %u\n", (result - a));
    } else {
        printf("Element not found.\n");
    }

    return EXIT_SUCCESS;
}

Same request. Suggestions and critique are welcome.

Update

In main.c, the line double aa = .1, bb = .1; is unnecessary. I forgot to remove it after debugging. You may disregard it.

Code does not handle an array size of 0 (count == 0). Simple to fix

//size_t lo = 0, hi = count - 1, mid;
//while (lo <= hi) {

size_t lo = 0, hi = count, mid;
while (lo < hi) {
  hi--;

Potential UB.  Consider the below if mid == 0.  IMO, better to use a variable hi_plus_1 instead of hi and adjust code accordingly to avoid size_t underflow.

        hi = mid - 1;  // hi could become SIZE_MAX

Avoid losing const-ness.

const void *base
// char *base_ptr = (char *) base;
const char *base_ptr = (const char *) base;

binary_search.c is missing the include of binary_search.h.  This is important to maintain the consistency of the declaration and definition.

binary_search.h should include <stdlib.h> for size_t.

#include "binary_search.h"

void *binary_search(const void *x, const void *base, size_t count, size_t size,
    int (*compare)(const void *, const void *)) {
  const char *base_ptr = (const char *) base;
  size_t lo = 0, hi_plus1 = count;
  while (lo < hi) {
    size_t mid = lo + (hi_plus1 - lo)/2;
    int result = (*compare)(base_ptr + mid * size, x);
    if (result == 0) {
      return base_ptr + mid * size;
    } else if (result < 0) {
      lo = mid + 1;
    } else {
      hi_plus1 = mid;
    }
  }
  return NULL;
}

Test code

Problem specifier.  Use "%zu" for size_t.  The 2 below hint OP is not compiling with all warnings enabled.  Double-check that all warnings are enabled for prompt automatic feedback.

 size_t count, i;
 // scanf("%llu", &count);
 scanf("%zu", &count);

 for (i = 0; i < count; i++) {
    // printf("%u : ", (i + 1));
    printf("%zu : ", (i + 1));

A size of 0 may cause for malloc() to return NULL and that is not an out-of-memory.

 a = malloc(count * size);
 // if (a == NULL) {
 if (a == NULL && count > 0) {
   printf("Error while trying to allocate memory.\n");

size = sizeof(double); lacks context.  Why sizeof(double)?  Certainly that is because that is the type a points to.  Then code that directly:

// size = sizeof(double);
size = sizeof *a;

For testing, useful to print the value sought and the value found.  Recommend %e for detail.

printf("Value sought %e\n", x);
printf("Value found  %e\n", x, *result);
// or for sufficient detail
printf("Value sought %.*e\n", DBL_DECIMAL_DIG - 1, x);
printf("Value found  %.*e\n", DBL_DECIMAL_DIG - 1, *result);

With compare(), note that 0.0 will match -0.0.  If that is not desired, perhaps below.  Simplifications may be possible.

int compare(const void *a, const void *b) {
    const double *p = a;
    const double *q = b;
    if (*a > *b) return 1;
    if (*a < *b) return -1;
    if (isnan(*p)) return +1;
    if (isnan(*q)) return -1;

    // values are equal and not NaNs
    if (!signbit(*a) == !signbit(*b)) return 0;
    if (signbit(*a)) return -1;
    return 1;
}


Python tic tac toe class I'm fairly new at programming and made a tic tac toe game to see if I could handle it. I managed to get it to work but I feel like my code is faaaar from perfection. I wanted to ask for some advices on how to improve it

class Game:

    def __init__(self):
        self.board = [0, 1, 2, 3, 4, 5, 6, 7, 8]
        self.win_conditions = ((0, 1, 2), (3, 4, 5), (6, 7, 8), (0, 4, 8), (2, 4, 6), (0, 3, 6), (1, 4, 7), (2, 5, 8))
        self.moves_count = 0

    def create_board(self):
        print()
        print(self.board[0], self.board[1], self.board[2])
        print(self.board[3], self.board[4], self.board[5])
        print(self.board[6], self.board[7], self.board[8])
        print()

    def p1(self):
            try:
                print("PLAYER X")
                question = int(input("Type where your X should be placed\n"))
                if self.board[question] != "X" and self.board[question] != "O":
                    self.board[question] = "X"
                    self.create_board()
                    self.moves_count += 1
                else:
                    print("someone already took that row")
                    self.p1()
                self.check_win()
            except ValueError:
                print("Write a NUMBER from 0 to 8")
                self.p1()

    def p2(self):
        try:
            print("PLAYER 0")
            question = int(input("Type where your 0 should be placed\n"))
            if self.board[question] != "X" and self.board[question] != "O":
                self.board[question] = "O"
                self.create_board()
                self.moves_count += 1
            else:
                print("someone already took that row")
                self.p2()
            self.check_win()
        except:
            print("Write a NUMBER from 0 to 8")
            self.p2()

    def check_win(self):
        for a in self.win_conditions:
            if self.board[a[0]] == self.board[a[1]] == self.board[a[2]] == "X":
                print("PLAYER X WINS")
                self.play_again()
            if self.board[a[0]] == self.board[a[1]] == self.board[a[2]] == "O":
                print("PLAYER O WINS")
                self.play_again()
            elif self.moves_count == 9:
                print("A DRAW!")
                exit()

    def play(self):
        while True:
            self.p1()
            self.p2()

    def run(self):
        self.create_board()
        self.play()

    def play_again(self):
        while True:
            question = input("Do you want to play again? Type y or n\n")
            if question == "y":
                print("GLHF")
                self.board = [0, 1, 2, 3, 4, 5, 6, 7, 8]
                self.run()
            elif question == "n":
                print("See you next time!")
                quit()
            else:
                print("Thats not a valid option")

cos = Game()
cos.run()

Let's start with a bit of theory about what a class is, why you would use one, and what makes for a good one. The key idea about classes is that a class is a blueprint for a thing, and that blueprint says how that thing works. Within the class, you define all the nitty gritty details and write code carefully to keep those details nice and orderly, and from outside the class you are free to completely forget about it and assume it just works.

The first change I would suggest with your code is separate the class that represents the game from the code that deals with the players. After all, the game is one thing that you can draw a ring around in your head, and the players are outside that ring.

Now, the game should take care of details like remembering who has played where, where someone is still allowed to play, what the board will look like if someone plays in a given square, whether this board represents a game that has been won or drawn or such like, and so on.
Meanwhile the players should take care of details like deciding where to play.
For example in the Game class you might have

def is_move_legal(self, move):
    return self.board[move] != "X" and self.board[move] != "O"

And then the players, instead of

if self.board[question] != "X" and self.board[question] != "O":

Would have something like

if current_game.is_move_legal(question):

You can imagine how it's easier to work out what the second version is doing while you're thinking about how to design the players, because you don't have to think about what the board is like. As a bonus, it's easy to tell at a glance because the function has been named in a descriptive way.

Following this approach to designing a class would be quite an extensive rewrite of your game, but it would make it much clearer

As a general rule in programming, if you find yourself writing substantially the same code twice, it is worth thinking about whether you can do it once and make up the differences with a couple of variables. In particular in this case, the p1 and p2 functions are completely identical except for what letter they print and calling the other one. So you could have just one of them, and pass the "X" or the "O" it has to print and play with as a parameter.

It's important to name your functions precisely, so you can tell what they do. For example create_board would probably be better named print_board.

Similarly it's important to name your variables so that it's clear what they represent. You call both variables that you get from user input question. I can see why, but a name like chosen_move would be clearer. (But on the topic of user input, well done for checking user input, including in play_again.)

Referring to check_win, it is generally a big of a warning sign if you find yourself using single letter variable names. Think about what a represents, and call it that.

Now for a few pesky details, which are worth the time to polish.

The second player is "O" rather than "0" which you sometimes print.

In check_win are you sure you want to exit the game after a draw, but allow a rematch otherwise?

In play you have an infinite loop of switching between the two players. That's not an inherently bad thing. So long as you have some way of breaking out of it once the game has run its course, looping is a perfectly valid way to repeatedly let the same bits of code (the player choices) run.
However, in this case it never even gets half way first the first loop, because p1 calls p2 directly, and that calls p1 and so on. This recursive approach is also a valid way to do that, but you should choose one and stick with it.


Facebook crawler redirect to force displaying own opengraph meta-tags For my website, I use the opengraph to send the right data to the crawler.

On the top of all pages on my website, I include the opengraph-script, like this:

if (preg_match("/facebookexternalhit/is", $_SERVER['HTTP_USER_AGENT'])) {
    include("./facebook-crawler.php");
    exit();
}

facebook-crawler.php:

        ## Infos for Crawling
        $server_path                = $_SERVER['REQUEST_URI'];
        $expl_path                  = explode("/", $server_path);
        $page_title                 = $expl_path[1];
        $page_data                  = "";
        $page_path_display          = "";

        if (isset($expl_path[2]) && $expl_path[2]!="") {
            $page_data              = $expl_path[2];
            $page_path_display      = "/".$page_title."/".$page_data;
        } elseif ($page_title!="") {
            $page_path_display      = "/".$page_title."/";
        } else {
            $page_path_display      = "/";
        }

function show_fbmeta_home() {

        global $site_url, $site_url_displayname, $fb_app_id, $fb_admins, $page_path_display, $count_cars, $count_member, $count_ps;

        get_statistics_general();

        $use_title      = "Lunatics Car Club Austria - est. 2014 - {Beta}";
        $use_text       = $count_member." Mitglieder, ".$count_cars." Autos, ".$count_ps."PS. Besuch unsere Website um mehr zu Erfahren!";
        $use_type       = "article";
        $use_pic        = $site_url."/images/home/r32-gruppenbild.jpg";

        $meta_string =
        '<meta property="og:title" content="'.$use_title.'" />
        <meta property="fb:app_id" content="'.$fb_app_id.'"/>
        <meta property="fb:admins" content="'.$fb_admins.'"/>
        <meta property="og:type" content="article" />
        <meta property="og:url" content="'.$site_url.'/" />
        <meta property="og:image" content="'.$use_pic.'" />
        <meta property="og:site_name" content="'.$site_url_displayname.'" />
        <meta property="og:description" content="'.$use_text.'" />
        ';

        return $meta_string;

}

        // Meta Cases
        switch($page_title) {
            case "": case "home":
                echo show_fbmeta_home();
                exit();
            break;
        }

        show_fbmeta_home();

Of course, the script would be extended for each link. If no function for the actual website is present, the home-page would be used.

Is this a legacy-style method, or, could this cause some problems in the future?

It would be better to assign $page_path_display this value ("/") from the beginning as all the cases use that as a prefix.
You're missing whitespace in this statement ($expl_path[2]!=""),  it should be $expl_path[2] != "".
In the following code block, you don't need to assign $meta_string as you return it straight away.

    return
    '<meta property="og:title" content="'.$use_title.'" />
    <meta property="fb:app_id" content="'.$fb_app_id.'"/>
    <meta property="fb:admins" content="'.$fb_admins.'"/>
    <meta property="og:type" content="article" />
    <meta property="og:url" content="'.$site_url.'/" />
    <meta property="og:image" content="'.$use_pic.'" />
    <meta property="og:site_name" content="'.$site_url_displayname.'" />
    <meta property="og:description" content="'.$use_text.'" />
    ';

In the following code block, you could just use an if-else instead of a switch

    switch($page_title) {
        case "": case "home":
            echo show_fbmeta_home();
            exit();
        break;
    }

for example:

if ($page_title == "" || $page_title == "home"){
    echo show_fbmeta_home();
    exit();
}

global, you should avoid using globals, because they're bad practice.