Doubly linked list implementation in C I'm quite new to C, and I'm looking for feedback on this (doubly) linked list code.

Once I get to the add and remove methods, I feel like the code goes downhill. I'm looking for any potential errors I haven't spotted, and improvements that could be made for efficiency/tidiness.

struct node {
    int data;
    struct node *next;
    struct node *previous;
};

struct listset {
    struct node *head;
    struct node *tail;
    int current_elements;
};

struct listset * new_listset(){
    struct listset *new_list = malloc(sizeof(struct listset));
    new_list ->head = NULL;
    new_list ->tail = NULL;

    return new_list;
};

struct node *listset_lookup(struct listset *this, int item){
    struct node *curr_elem = this->head;
    if(!curr_elem) return NULL;
    while(item != curr_elem->data){
        if(curr_elem->next==NULL)return NULL;
        curr_elem = curr_elem->next;
    }
    return curr_elem;
}

void listset_add(struct listset *this, int item){
    struct node *new_node = malloc(sizeof(struct node));
    new_node -> data = item;
    new_node ->next = NULL;
    if(!this->head) {
        new_node->previous = NULL;
        this->head = this->tail = new_node;
    }
    else{
        this->tail->next = new_node;
    }
    this->current_elements++;
}
void listset_remove(struct listset *this, int item){
    struct node *elem_remove = listset_lookup(this, item);
    if(!elem_remove) return;
    if(elem_remove->next && elem_remove->previous) elem_remove->previous->next = elem_remove->next;
    else if (elem_remove->previous && !elem_remove->previous){
        elem_remove->previous->next=NULL;
        this->tail = elem_remove->previous;
    }
    else{
        this->tail = NULL;
        this->head = NULL;
    }
    free(elem_remove);
}

Your code is actually "off-topic" because it doesn't work.  You should really
include a short main that illustrates its use and allows it to be tested.  All the same, here are some comments.

Be consistent with spacing.  My preference is for no spaces around -> and
for a space after keywords (if, while etc).

Variable naming.  Your names are not to my taste, often too long:

Rename previous as prev, current_elements as n_elements
Rename this globally as list - this means nothing whereas list is
clearly a list.
Rename item as data.  It is called data within the node structure so
why not be consistent?
new_listset would perhaps be more consistent with the other functions if
named listset_new.  It also needs a void parameter list.

Function listset_lookup tests the wrong condition in the loop.  Better to
test for the end of the list:

struct node *listset_lookup(const struct listset *list, int data)
{
    struct node *n = list->head;
    while (n) {
        if (data == n->data) {
            break;
        }
        n = n->next;
    }
    return n;
}

Notice also that the parameter list is const (as you don't alter it).  Also
note that writing this with a short variable name makes it much more readable
compared to the dense text that results from a longer name.  In a ten line
function with only 3 variables there is really nothing wrong (and a lot right)
with using such a short name.

Your listset_add is missing two terms in the else clause and should test
for malloc failure (and return something to show whether it failed).  I
would also rename new_node as simple n - again, it is a short function (as
most should be) and so this is ok.

static struct node* listset_add(struct listset *list, int data)
{
    struct node *n = malloc(sizeof(struct node));
    if (n) {
        n->data = data;
        n->next = NULL;
        if (!list->head) {
            n->prev = NULL;
            list->head = list->tail = n;
        } else {
            n->prev = list->tail; // new
            list->tail->next = n;
            list->tail = n;       // new
        }
        list->n_elements++;
    }
    return n;
}

Your listset_remove is hard to read and wrong.  Again, a shorter variable
name would help.  You should also avoid long lines such as:

if(elem_remove->next && elem_remove->previous) elem_remove->previous->next = elem_remove->next;

This is much clearer as:

if (n->next && n->previous) {
    n->previous->next = n->next;
}

All the same, it is perhaps wrong.  And this:

else if (elem_remove->previous && !elem_remove->previous){

is quite clearly not sensible.  You also don't decrement the element count.
The correct function is:

static void listset_remove(struct listset *list, int data)
{
    struct node *n = listset_lookup(list, data);
    if (!n) return;

    if (n->prev) {
        n->prev->next = n->next;
    } else {
        list->head = n->next;
    }
    if (n->next) {
        n->next->prev = n->prev;
    } else {
        list->tail = n->prev;
    }
    list->n_elements--;
    free(n);
}

Here's what I used to test it:

static void listset_print(const struct listset *list)
{
    struct node *n = list->head;
    while (n) {
        printf("%d ", n->data);
        n = n->next;
    }
    printf(" : ");

    n = list->tail;
    while (n) {
        printf("%d ", n->data);
        n = n->previous;
    }
    printf("\n");
}

int main(void)
{
    struct listset *list = new_listset();
    listset_add(list, 1);
    listset_add(list, 2);
    listset_add(list, 3);
    listset_add(list, 4);
    listset_add(list, 5);
    listset_print(list);
    listset_remove(list, 6);
    listset_print(list);
    listset_remove(list, 1);
    listset_print(list);
    listset_remove(list, 5);
    listset_print(list);
    listset_remove(list, 3);
    listset_print(list);
    listset_remove(list, 2);
    listset_print(list);
    listset_remove(list, 4);
    listset_print(list);
    return 0;
}


Convert HTML color to OLE I have this function that takes a string which represents a html-color and returns the OLE value for it. If the string is null or empty or it can't be parsed, it uses a default value.
However I have to write that one line containing the default value twice and I don't see how I could remove this redundancy in the best possible way.

I know this isn't a big deal but I am curious to see what you guys might suggest for this :)
What do you think is the best possible way to remove this redundancy?

private int GetOleFromHTML(string stringRep)
{
    Color c;
    if (string.IsNullOrWhiteSpace(stringRep))
    {
        c = Color.FromArgb(224, 224, 224);
    }
    else
    {
        try
        {
            c = ColorTranslator.FromHtml(stringRep);
        }
        catch
        {
            c = Color.FromArgb(224, 224, 224);
        }
    }

    return ColorTranslator.ToOle(c);
}

Since a method has to be executed to get this color, you cannot declare the color as constant. A static readonly field is what comes closest to a const.

private static readonly Color DefaultColor = Color.FromArgb(224, 224, 224);

private int GetOleFromHTML(string stringRep)
{
    Color c;
    if (string.IsNullOrWhiteSpace(stringRep))
    {
        c = DefaultColor
    }
    else
    {
        try
        {
            c = ColorTranslator.FromHtml(stringRep);
        }
        catch
        {
            c = DefaultColor
        }
    }

    return ColorTranslator.ToOle(c);
}

Another approach would be to directly store the OLE color as int.

const int DefaultOleColor = 14737632; // R=224, G=224, B=224

private int GetOleFromHTML(string stringRep)
{
    if (string.IsNullOrWhiteSpace(stringRep))
    {
        return DefaultOleColor;
    }
    try
    {
        return ColorTranslator.ToOle(ColorTranslator.FromHtml(stringRep));
    }
    catch
    {
        return DefaultOleColor;
    }
}

Or calculated explicitly:

const int DefaultOleColor = 256 * (256 * 224 + 224) + 224;

Or with bit shift operations

const int DefaultOleColor = 224 << 16 | 224 << 8 | 224; // My preferred variant.

These expressions can be used to initialize the constant as they can be fully evaluated at compile time.

You can test these variants easily in the Immediate Window of Visual Studio. You must qualify the names with the namespaces for the first variant (System.Drawing.ColorTranslator.ToOle(System.Drawing.Color.FromArgb(224, 224, 224))).

Avoiding repetition is not the only reason for having constants. Constant declarations give a name to otherwise "magic" values. See Magic number (programming) (Wikipedia).

This variant avoids the repetition by restructuring the code, but still uses a constant.

const int DefaultOleColor = 224 << 16 | 224 << 8 | 224;

private int GetOleFromHTML(string stringRep)
{
    if (!String.IsNullOrWhiteSpace(stringRep))
    {
        try
        {
            return ColorTranslator.ToOle(ColorTranslator.FromHtml(stringRep));
        }
        catch
        {
        }
    }
    return DefaultOleColor;
}


Store location with some excluded path This code stores the last know location in the session. The location is not stored if it is one of the exceptions in the if statement.

def store_location
    if request.fullpath != '/users/sign_in' &&
     request.fullpath != '/users/sign_up' &&
     request.fullpath != '/users/password' &&
     request.fullpath != '/users/sign_out' &&
     request.fullpath != '/accounts' &&
     request.fullpath != '/accounts/new' &&
     !request.fullpath.include?('/users/confirmation') &&
     request.fullpath != '/' &&
     !request.xhr?

     session[:previous_url] = request.fullpath
   end
  end

I need to make it more simple.

this is already simple, it follows Devise wiki: https://github.com/plataformatec/devise/wiki/How-To:-Redirect-back-to-current-page-after-sign-in,-sign-out,-sign-up,-update

but if you really want to make it shorter and remove "if" you can use skip_before_filter on controller level, i.e.

class AccountsController < ApplicationController
  skip_before_filter :store_last_page, only: [:new, :index]


Enormous Input Test - reducing program runtime This is my solution to SPOJ Problem 442.  How can I make this code run faster? What are some techniques that can be used to make this faster? I am getting a Time Limit Exceed Error here.

import java.util.Scanner;

class main{
   public static void main(String args[]){
      Scanner input = new Scanner(System.in);
      int n,k,m=0;
      n = input.nextInt();
      k = input.nextInt();

      for(int c = 0 ; c < n ; c++){
         if(input.nextInt() % k == 0) m++;
      }

      System.out.println(m);
    }
}

The main reasons are

Scanner is pretty slow
System.in is unbuffered

By using a BufferedReader instead of the Scanner you get it to barely acceptable levels. The following approach runs in roughly 8.2 seconds and get's accepted.

public class Main {
    public static void main(String args[]) throws java.io.IOException {
        java.io.InputStreamReader isr = new java.io.InputStreamReader(System.in);
        java.io.BufferedReader br = new java.io.BufferedReader(isr, 16 * 1024);
        String[] line0 = br.readLine().split(" ");
        int n, k, m = 0;
        n = Integer.parseInt(line0[0]);
        k = Integer.parseInt(line0[1]);
        for (int c = 0; c < n; c++) {
            if (Integer.parseInt(br.readLine()) % k == 0)
                m++;
        }

        System.out.println(m);
    }
}

But it's still really slow compared to the top scoring solutions that run in under 2 seconds.

I've got it down to a bit over 2 seconds by reading into a byte[] buffer and doing custom number parsing based on that buffer roughly like the following incomplete piece (which just prints the parsed numbers)

public class Main {
    public static void main(String args[]) throws java.io.IOException {
        byte[] buffer = new byte[16 * 1024];
        int currentNumber = 0;
        boolean inNumber = false;
        int read;
        while((read = System.in.read(buffer)) >= 0) {
            for (int i = 0; i < read; i++) {
                char c = (char) buffer[i];
                if (c >= '0' && c <= '9') {
                    inNumber = true;
                    currentNumber = currentNumber * 10 + (c - '0');
                } else if (inNumber) {
                    inNumber = false;
                    System.out.println("I've read number: " + currentNumber);
                    currentNumber = 0;
                }
            }
        }
    }
}

See http://ideone.com/lQ4ImB for above code with input

The reason why this is so much faster is basically that there is almost 0 overhead in reading & parsing the input into numbers. BufferedReader in comparison is transforming each line into a String (thus creating a new Object), then parses that String. Above approach allocates no new Objects besides the single byte[] buffer.


C++ implementation of Tic-Tac-Toe game I have written this simple code for Tic Tac Toe.

#include <iostream>
#include <vector>

void draw_board(const std::vector< std::vector<char> >& vec)
{
    std::cout << "    0   1   2  \n";
    std::cout << "  +---+---+---+\n";
    for (int i = 0; i < vec.size(); i++)
    {
        std::cout << i << " " ;
        for (int j = 0; j < vec[i].size(); j++)
        {
            std::cout << "| " << vec[i][j] << " ";
        }
        std::cout << "|";
        std::cout << '\n';
        std::cout << "  +---+---+---+\n";
    }
}

void enter(int row, int col, char ch, std::vector< std::vector<char> >& vec)
{
    vec[row][col] = ch;
}

bool check(const std::vector< std::vector<char> >& vec)
{
    //to check diagonals
    if ((vec[0][0] == 'X' && vec[1][1] == 'X' && vec[2][2] == 'X')
        || (vec[0][2] == 'X' && vec[1][1] == 'X' && vec[2][0] == 'X'))
        {
            std::cout << "Player X won this game\n";
            return true;
        }
    else if ((vec[0][0] == 'O' && vec[1][1] == 'O' && vec[2][2] == 'O')
             || (vec[0][2] == 'O' && vec[1][1] == 'O' && vec[2][0] == 'O'))
             {
                std::cout << "Player O won this game\n";
                return true;
             }

    //to check horizonatal and vertical
    for (int i = 0; i < vec.size(); i++)
    {
        if ((vec[i][0] == 'X' && vec[i][1] == 'X' && vec[i][2] == 'X')
              || (vec[0][i] == 'X' && vec[1][i] == 'X' && vec[2][i] == 'X'))
              {
                  std::cout << "Player X won this game\n";
                  return true;
              }
        else if((vec[i][0] == 'O' && vec[i][1] == 'O' && vec[i][2] == 'O')
              || (vec[0][i] == 'O' && vec[1][i] == 'O' && vec[2][i] == 'O'))
              {
                  std::cout << "Player O won this game\n";
                  return true;
              }
    }
}

void start(std::vector< std::vector<char> >& vec)
{
    int row, col, res = 0;
    char ch;

    std::vector<int> index = {0, 1, 2};

    for (int i = 0; i < vec.size(); i++)
    {
        for (int j = 0; j < vec[i].size(); j++)
        {
            vec[i][j] = ' ';
        }
    }

    for (int i = 0; i < 9;)
    {
        if (i == 0 || i%2 == 0)
        {
            ch = 'X';
            std::cout << "Chance to enter X\n";
        }
        else
        {
            ch = 'O';
            std::cout << "Chance to enter O\n";
        }
        std::cout << "Enter row number ";
        std::cin >> row;
        std::cout << "Enter column number ";
        std::cin >> col;
        if (vec[row][col] == ' ' || vec[row][col] == ' ')
        {
            enter(row, col, ch, vec);
            draw_board(vec);
            if ( i >= 2)
            {
                res = check(vec);
                if (res == 1)
                {
                    break;
                }
            }
            i++;
        }
        else
        {
            std::cerr << "This position already contains a character\n";
        }

    }
    if (res == 0)
    {
        std::cout << "This game draws\n";
    }
}

int main()
{
    std::vector< std::vector<char> > board(3, std::vector<char>(3));
    draw_board(board);
    start(board);
}

Check your inputs in enter()

You don't check that the user entered a valid vector index. For example, if I enter a 3 as the row and/or column number the program crashes. You should reject an input if the user enters an invalid row and/or column number (like you do if the user selects a space that is already taken).

All control paths of check() should return a value

You only ever return true in check(). If none of the check conditions are true then you should explicitly return false at the end of the function.

Use the correct type for indexing

You have for loops like for (int i = 0; i < vec.size(); i++), which compare the signed integer i to the unsigned integer vec.size(). Use std::vector< std::vector<char> >::size_type.

Use a type alias / type definition for the std::vector< std::vector<char> > container

Use a type alias like using container = std::vector< std::vector<char> >; if your compiler supports it, otherwise use a type definition like typedef std::vector< std::vector<char> > container;. You can put it after the #include statements so you can use it for all your functions. This is useful for a couple of reasons:

It's a shorter name for the container. This comes in handy when referring to the container and/or its size_type, for example: container::size_type is shorter and easier to read than std::vector< std::vector<char> >::size_type.
It's easier to change the container. You may decide later to use a std::array instead, in which case you only need to modify the using statement rather than all instances of std::vector< std::vector<char> >.


This python code converts integers to strings and strings to ints without the inbuilt functions "int()" and "str()" It is my solution from a challange from edabit.com named "Drunken Python". Thats the task:
You need to create two functions to substitute str() and int(). A function called int_to_str() that converts integers into strings and a function called str_to_int() that converts strings into integers.
I hope you can tell me things I could do better and if the algorithms I used (e.g. to calculate the length of an int) are optimal.
# A function called int_to_str() that converts integers into strings and a function called str_to_int() that
# converts strings into integers without using the in built functions "int()" and str().

def str_to_int(num_str):
    dec_places = {11: 10000000000, 10: 1000000000, 9: 100000000, 8: 10000000, 7: 1000000, 6: 100000, 5: 10000, 4: 1000,
                  3: 100, 2: 10, 1: 1}
    char_digit = {'0': 0, '1': 1, '2': 2, '3': 3, '4': 4, '5': 5, '6': 6, '7': 7, '8': 8, '9': 9}
    num = 0
    length = len(num_str)
    for i in range(length):
        x = char_digit[num_str[0 - (i + 1)]] * dec_places[i + 1]
        num = num + x
    return num

def calculate_length(num):
    div = 10
    i = 1
    while num / div >= 1:
        div = div * 10
        i = i + 1
    return i

def int_to_str(num_int):
    word = ""
    div = 10
    char_digit = {0: '0', 1: '1', 2: '2', 3: '3', 4: '4', 5: '5', 6: '6', 7: '7', 8: '8', 9: '9'}
    length = calculate_length(num_int)
    for i in range(length):
        x = (num_int % div) // (div // 10)
        word = char_digit[x] + word
        num_int = num_int - x
        div = div * 10
    return word
```

Keep it simple
Your functions are way too complicated. First, you can convert a single digit from integer to character by using ord() and the fact that the ordinals of the characters '0' to '9' are consecutive. Furthermore, you don't need to calculate the length of an integer if you reverse the way you build up the string. Here are example implementations that are simpler:
def str_to_int(num_str):
    num = 0

    for char in num_str:
        num *= 10
        num += ord(char) - ord('0')

    return num

def int_to_str(num_int):
    word = ""

    while num_int:
        word = chr(ord('0') + num_int % 10) + word
        num_int //= 10

    return word or "0"

Integers can be negative
Your code (and my example simplification above) does not handle negative integers.


Functional composition of member function predicates In C++11, it is en vogue to use small lambda expressions to produce ad-hoc predicates. However, sometimes it's fun to create predicates in a functional, "point-free" way. That way, no function bodies need to be inspected, the code is mildly self-documenting, and it's a great icebreaker at parties.

Suppose I have a std::vector<std::vector<int>> v, and I want to remove all the empty inner vectors. The lambda-style remove_if call might look like this:

std::remove_if(v.begin(),
               v.end(),
               [](std::vector<int> const & w) -> bool { return w.empty(); });

This is verbose and redundant. The point-free approach uses std::mem_fn:

std::remove_if(v.begin(),
               v.end(),
               std::mem_fn(&std::vector<int>::empty));

This works fine. But now the problem: Suppose I want to further compose this predicate, say by negating it. In the lambda this would be a trivial change. But for the functional notation, we would like to use the library function std::not1 to produce a unary_negate wrapper.

However, the obvious std::not1(std::mem_fn(&std::vector<int>::empty)) does not compile. The problem appears to be that the result type of std::mem_fn defines its argument_type member type as a pointer to the class, not a reference; cf. 20.8.10/2:

  The simple call wrapper shall define two nested types named argument_type and result_type as synonyms for cv T* and Ret, respectively, when pm is a pointer to member function with cv-qualifier cv and taking no arguments, where Ret is pm’s return type.

This breaks the composability with std::not1.

I have written this type mangling work-around which strips the unwanted pointer:

#include <type_traits>

template <typename T>
struct result_type_mangler : T
{
    using argument_type = typename std::remove_pointer<typename T::argument_type>::type;
    result_type_mangler(T t) : T(t) { }
};

template <typename T>
result_type_mangler<T> mangle(T t)
{
    return result_type_mangler<T>(t);
}

Now I can compose the predicates:

std::not1(mangle(std::mem_fn(&std::vector<int>::empty)))

Is this a legitimate work-around? Can member function predicates be composed in an easier way? And why is the member function wrapper's argument_type defined in such a weird way?

The verbosity of C++14 generic lambdas should be much lower than std::mem_fun

std::remove_if(v.begin(),
               v.end(),
               [](auto const & w) { return w.empty(); });

If you want to remove non-empty vectors, you can also do something like

std::function<bool(std::vector<int>)> is_empty = [](auto const & w) { return w.empty(); };
std::remove_if(v.begin(),
               v.end(),
               std::not1(is_empty));

Oh and the -> bool in your question is already superfluous in C++11 for single-line lambdas. Generic lambdas are currently supported by Clang >= 3.4, GCC 4.9 and MSVC 2013 November CTP.

I think Scott Meyers even has an Item "Prefer lambdas over bind" in his upcoming book Effective C++11/14.


Binary numbers challenge I don't know if this is exactly refactoring but I recently completed a programming challenge to learn more about Ruby's iterators. However, I feel that the code is extremely "un-Ruby like". Would anyone have any suggestions as to how I could improve it?

numbers = []
binary = []
binary_count = 0

File.open('decimals.txt').each_line do |line|
  numbers.push line.chomp
end

numbers.each do |number|
  binary.push number.to_i.to_s(2)
end

binary.each do |binary_num|
 binary_count += binary_num.count("1")
end

puts binary_count

Yes, I know it's a basic problem. It took me roughly five minutes to complete but it was more about just getting the syntax of the iterators down.

I would not open a stream instead use IO.readlines and enum methods:

puts File.readlines("decimals.txt").reduce(0) { |total, line| total += line.chomp.to_i.to_s(2).count("1") }


Rotating strings in Java I have this Java class for producing rotations of an input string:

StringRotator.java

package net.coderodde.util;

import java.util.Objects;

/**
 * This class implements a data type for rotating strings.
 *
 * @author Rodion "rodde" Efremov
 * @version 1.6 (Mar 24, 2016)
 */
public class StringRotator {

    /**
     * The source string. All rotations will be produced from this string.
     */
    private final String string;

    /**
     * Caches the length of the source string in order to by a little of
     * efficiency.
     */
    private final int length;

    /**
     * Caches the current rotation. This field is set to {@code null} after
     * every effective rotation. Whenever the user requests the rotated string,
     * if this field is not {@code null}, the cached string is returned.
     * Otherwise, a rotated string is built and set to this field, and finally
     * returned.
     */
    private String cachedString;

    /**
     * Index of the first logical character component in the source string.
     */
    private int finger;

    public StringRotator(String string) {
        this.string = Objects.requireNonNull(string,
                                             "The input string is null.");
        this.length = string.length();
    }

    public void rotate(int offset) {
        int oldFinger = finger;
        finger += offset;
        finger %= string.length();

        if (finger != oldFinger) {
            cachedString = null;
        }
    }

    @Override
    public String toString() {
        if (cachedString != null) {
            return cachedString;
        }

        return cachedString = buildString();
    }

    private String buildString() {
        StringBuilder sb = new StringBuilder(length);

        for (int i = 0; i < length; ++i) {
            sb.append(string.charAt((finger + i) % length));
        }

        return sb.toString();
    }

    public static void main(final String... args) {
        StringBuilder sb = new StringBuilder();

        for (char c = 'a'; c <= 'z'; ++c) {
            sb.append(c);
        }

        String input = sb.toString();

        for (int i = 0; i < input.length(); ++i) {
            StringRotator string = new StringRotator(input);
            string.rotate(i);
            System.out.println(string);
        }
    }
}

Please, tell me anything that comes to mind.

Simplicity

To rotate a string, only the string itself and the offset of rotation are needed.

A class is a way to store data in an orderly manner, but for the task of rotating a string, you do not need such storage.

In Java not needing a class is signalled by the use of static methods.

Also, using  String.substring you can avoid the manual looping. In short all your program can be reduced to:

public static String rotate(String s, int offset) {
  int i = offset % s.length();
  return s.substring(i) + s.substring(0, i);
}

The example usage is also simplified, as you do not need to build a new class:

public static void main(final String... args) {
  for (int i = 0; i < 6; i++) {
    System.out.println(rotate("abcdefg", i));
  }
}

The drawback of this approach is that it may be slower and/or use more memory, but unless you want to rotate a string tens of millions of times per second it should not matter.


filter and map functions I am a JavaScript programmer beginning with Go.  I tried to replicate filter and map functions like JavaScript's, as an exercise.
Here is what I came up with.
The filter function:
func Filter(slice interface{}, cb func(i interface{}) bool) []interface{}{
sliceValues := reflect.ValueOf(slice)
newSlice := []interface{}{}
for i := 0; i < sliceValues.Len(); i++{
    v := sliceValues.Index(i).Interface()
    passed := cb(v)
    if passed{
        newSlice = append(newSlice, v)
    }
}
return newSlice
}

Usage:
a := []int {1,2,3}

result := Filter(a,func(i interface{}) bool{
    v,_:= i.(int)
    return v > 1
})

fmt.Println(result) // [2 3]

The map function:
func Map(slice interface{}, cb func(i interface{}) interface{}) []interface{}{
sliceValues := reflect.ValueOf(slice)
newSlice := []interface{}{}
for i := 0; i < sliceValues.Len(); i++{
    v := sliceValues.Index(i).Interface()
    modifiedValue := cb(v)
    newSlice = append(newSlice, modifiedValue)
}
return newSlice

}
Usage:
a := []int {1,2,3}

result := Map(a,func(i interface{}) interface{}{
    v,_:= i.(int)
    return v + 1
})

fmt.Println(result) // [2 3 4]

From a mathematics and science genius:

"If I have seen further it is by standing on the shoulders of Giants." Isaac Newton
Wikipedia: Standing on the shoulders of giants

For Go giants, you might start with Rob Pike.

GitHub - robpike/filter: Simple apply/filter/reduce package.
I wanted to see how hard it was to implement this sort of thing in Go, with as nice an API as I could manage. It wasn't hard.
Having written it a couple of years ago, I haven't had occasion to use it once. Instead, I just use "for" loops.
You shouldn't use it either.

Take Rob Pike's advice and use "for" loops.


Python Etiquette for Functions in Multiple Files Here's the context: My code is meant to automate testing of OCR (Image to Text programs) for a science project which could easily be read and understood my others. The code includes sets of functions which serve different purposes. Basically, there are functions which deal with the drawing and saving of pages (drawtext(), createpage(), etc.), functions which run various OCR on an image (tesseractocr(), etc.) and a function which computes the difference between two strings (levenshtein()).

Having all these functions in main.py results, in my opinion, in a cluttered piece of code. Is it better generally to:

Keep all those functions in main.py so that the entire code is in one file.
Put all these functions in one functions.py file
Put each set of functions in a file, resulting in a levenshtein.py, ocr.py, and pagemaker.py in addition to the main.py.

Honestly, I feel that either 2 or 3 is the choice to make, I'm just not sure. Additionally, what level of abstraction would you usually want in a main.py file? Should the file look like

import ocr, levenshtein, pagemaker
import "a few other things"
define vars, etc

for page in pages:
    correct = pagemaker.drawpage(input1,input2,(input3,input4),etc) #also saves image
    tesseract_out = ocr.tesseract("img.png")
    tesseract_dist = levenshtein.levenshtein(correct, tesseract_out)
    print(f"Tesseract Distance: {tesseract_dist}")
    #etc for other ocr

Or is that too abstract?
Also, is command-line argument parsing (which I didn't include above) generally in the main.py file or should you simply make a file called parseocrargs.py and run

pages, color, etc = parseocrargs.argparse(aruments grabbed earlier)

I guess the main question(s) are: Should a standalone (doesn't interact with other functions) but lengthy/ugly function go in a separate file than the other functions? Should almost all functions not be in main.py? If not, how many should be? Should main.py be extremely abstracted? If not, how abstracted should it be?

Putting all of your code into a single file is bad.  As a very VERY loose rule of thumb I try to avoid having any source file be more than a thousand lines of code, but the real rule to follow is that code should be broken up into modules that have coherent and comprehensible purposes, and those modules should live in their own files.

The origin of rules of thumb like "no file longer than a thousand lines" is that if a file is really long, it's no longer comprehensible by a normal human brain, and so you need to think about how to break your logic down into smaller and more mentally-digestible pieces.  A function (or group of interrelated functions) that is long but is self-contained and doesn't interact with anything else would make a lot of sense to have in its own file/module.

I'd probably do my argument parsing right in the main function unless the arg parsing was so complex that it felt like it needed to be its own module.


Finding maximum deviation It was a part of my coding challenge and the last one, but I failed because it failed to yield a result in two seconds for some of sample inputs (out of six samples, three passed but three failed for not meeting the time constraint).

Basically, you are supposed to find the maximum deviation from given number of consecutive integers. There are two inputs; the first argument is an array of integers and the second argument is the number of consecutive integers. And you print the maximum deviation. For example, if you have [10, 1, 5, 2, 6, 3] and 3 as arguments, the output should be 9 since [10, 1, 5] would yield the maximum deviation of 9 (10-1).

Can this be refactored to be faster?

def find_deviation(integers, range)
  max = 0
  (0..integers.size-range).each do |n|
    array = integers.slice(n, range)
    diff = array.max - array.min
    max = diff if diff > max
  end
  puts max
end

Some notes:

find_deviation(v, d): Try to write more meaningful names for variables. Specially, I'd give a plurar name to v, since it's a collection.
max = 0, each, inline if: All of this denote you think in imperative terms. Some notes on functional programming with Ruby.

I'd write, with a functional approach:

def find_deviation(values, m)
  values.each_cons(m).map { |xs| xs.max - xs.min }.max
end

Now, this function has exactly the same time complexity than yours (though it may be faster or slower depending on how Ruby works). The complexity is: len(values) = n -> O(n*m). Note that you can use Enumerable#minmax to avoid one traversal, but it's still the same complexity.

To make it faster here's an idea, even though it's not trivial to implement: use a structure with O(log n) times for insertion/deletion/max/min (a binary search tree is ideal for this) to hold values of the sliding window, this way the overall complexity is O(nlog m). I guess some of the tests have big values of m so the trivial approach O(nm) is too slow.

[edit] Just for fun, I wrote a O(n log m) implementation in Haskell (I found no BST gem for Ruby):

deviation :: (Num a, Ord a) => [a] -> Int -> a
deviation [] _ = 0
deviation xs m = maximum $ [Tree.maximum w - Tree.minimum w | w <- windows]
  where
    windows = scanl shift (Tree.fromList $ take m xs) (zip xs (drop m xs))
    shift window (old, new) = Tree.insert (Tree.delete window old) new


Sorting an array of numbers by descending frequency Somebody asked me to create a code that orders unique numbers of an array according to their frequency, ie:

{1,3,3,4,4,4,4,4,2,2,5,5,5,5}

to

{4,5,3,2,1}

I'm a beginner and only started learning C last week so my code probably isn't optimal. I managed to get it in just over 100 lines without worrying too much about space. I haven't messed with memory allocation yet but I should start looking into it.

Any tips or feedback?

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

void CalculateFrequency(int numbers[], int frequency[]) //Populates an array with the frequency of numbers in another
{
    int hasSorted = 0;
    do
    {
        hasSorted = 0;
        for(int currentInt=0; currentInt<10; currentInt++)
        {
            for(int i=0; i<10; i++)
            {
                if(numbers[i] == currentInt)
                {
                    frequency[currentInt]++;
                    hasSorted = 1;
                }
            }
        }
        if(hasSorted == 0)
        {
            break;
        }
    }while(hasSorted == 0);
}
void SortByFrequency(int numbers[], int frequency[])    //Sorts an array according to the frequency of the numbers
{
    int hasSorted = 0;
    int temp = 0;
    do
    {
        hasSorted = 0;
        for(int i=0; i<10; i++)
        {
            for(int i=0; i<10; i++)
            {
                if(frequency[numbers[i]] < frequency[numbers[i+1]])
                {
                    temp = numbers[i+1];
                    numbers[i+1] = numbers[i];
                    numbers[i] = temp;
                    hasSorted = 1;
                }
            }
        }
        if(hasSorted == 0)
        {
            break;
        }
    }while(hasSorted == 0);
}
int CountUniqueNumbers(int array[], int arrayLength)    //Counts unique numbers in an array
{
    int count = 1; //At least 1 number
    for(int i=0; i<arrayLength-1; i++)
    {
        if(array[i] != array[i+1])
        {
            count++;
        }
    }
    return count;
}
int PopulateArrayByFrequencyAndReturnLength(int numbers[], int sortedByFrequency[])
{
    int k = 0;
    sortedByFrequency[0] = numbers[0];
    for(int i=0; i<10; i++)
    {
        if(numbers[i] != numbers[i+1])
        {
            sortedByFrequency[k] = numbers[i];
            k++;
        }
    }
    return k;
}
int counter;

int main(void)
{
    int numbers[10] = {1,2,2,2,5,7,8,8,8,8};
    int frequency[10] = {0,0,0,0,0,0,0,0,0,0};
    int sortedByFrequency[10] = {0,0,0,0,0,0,0,0,0,0};
    int sortedByFrequencyTrueLength = 0;
    int differentNumbers = 1;   /*The array must have at least 1 number*/
    int sizeofNumbersArray = 10;
    int uniqueNumbersInArray = 0;
    int i = 0;
    /*print the numbers*/
    printf("Numbers:\t");
    for(int i=0; i<10; i++)
    {
        printf("%d ", numbers[i]);
    }
    puts("");
    /*Perform functions*/
    CalculateFrequency(numbers, frequency);
    SortByFrequency(numbers, frequency);
    /*Get amount of unique numbers in the array*/
    uniqueNumbersInArray = CountUniqueNumbers(numbers, sizeofNumbersArray);
    /*Poupulate the unique number frequency array and get the true length*/
    sortedByFrequencyTrueLength = PopulateArrayByFrequencyAndReturnLength(numbers, sortedByFrequency);
    //Print unique number frequency array
    printf("Numbers sorted by frequency:\t");
    for(i=0; i<sortedByFrequencyTrueLength; i++)
    {
        printf("%d ", sortedByFrequency[i]);
    }
    puts("");

    return 0;
}

I see a couple occurrences of:

do
{
    hasSorted = 0;
    ...other things...
    if(hasSorted == 0)
    {
        break;
    }
}while(hasSorted == 0);

Did you mean if(hasSorted == 1) in those? If not, then you might want to just get rid of the whole if statement and replace the while condition with while(hasSorted != 0).

Also, comments that describe the whole function could look better on a separate line right before the function, so that the line doesn't get too long.


Best way to get rid of code repetition? I currently have this bit of code, but I need to repeat it for red green and blue.  Is there a way I can do it without copying and pasting the code 3 times?

yellow.setOnClickListener(new View.OnClickListener() {
    public void onClick (View v)   {

        switch (buttonCount) {
        case 1:
            empty1.setImageResource(R.drawable.yellow);
            buttonCount++;
            guess1= Colour.YELLOW;
            break;
        case 2:
            empty2.setImageResource(R.drawable.yellow);
            buttonCount++;
            guess2=Colour.YELLOW;
            break;
        case 3:
            empty3.setImageResource(R.drawable.yellow);
            buttonCount++;
            guess3=Colour.YELLOW;
            break;
        case 4:
            empty4.setImageResource(R.drawable.yellow);
            buttonCount++;
            guess4=Colour.YELLOW;
            break;
        case 5:
            empty5.setImageResource(R.drawable.yellow);
            buttonCount++;
            guess5=Colour.YELLOW;
            break;

        }
    }
});

If you look at your naming schema, you have emptyX and guessX, where X is an index. This is an indicator that you should be using an array or a list:

EmptyType empties[5] = { empty1, empty2, ... };
GuessType guesses[5] = { guess1, guess2, ... };

yellow.setOnClickListener(new View.OnClickListener() {
    public void onClick (View v) {
        assert buttonCount < 5;
        empties[buttonCount - 1].setImageResource(R.drawable.yellow);
        buttonCount++;
        guesses[buttonCount - 1] = Colour.YELLOW;
    }
})

If you have to do the same thing for three colours, either copy&paste this small snippet, or extract it into another method, which you can call with appropriate colours:

private void doStuff(EventSource source, Color color, ImageResource resource) {
  source.setOnClickListener(new View.OnClickListener() {
    public void onClick (View v) {
        assert buttonCount < 5;
        empties[buttonCount - 1].setImageResource(resource);
        buttonCount++;
        guesses[buttonCount - 1] = color;
    }
  });
}

  doStuff(yellow, Color.YELLOW, R.drawable.yellow);
  doStuff(blue,   Color.BLUE,   R.drawable.blue  );
  doStuff(green,  Color.GREEN,  R.drawable.green );

This still looks like you could choose better data structures, so that the Color and the corresponding ImageResource and EventSource are explicitly connected.


Becoming a Perl bandit I've posted a few questions about natas a wargame. But I'm stuck at natas29 it has to do with Perl. I've never coded in Perl before.

Thus to help me beat the last few levels, I decided it is time to learn Perl. And what better way to learn it, then with wargames from overthewire. I made a small Perl program that beats the first few levels of bandit (in combination with some bash commands)

I'm specifically looking for answers that provide insights on how certain Perl operation could be abused, but any review is welcome.

#!/usr/bin/perl
use strict;
use warnings;

use Net::SSH::Perl;

my ($hostname, $username, $password, $port, $command) = ("bandit.labs.overthewire.org", "bandit0", "bandit0", "2220", "");

sub next_level {
    my ($user) = @_;
    $user =~ s/(\d+)\z/ $1 + 1 /e;
    return $user;
}

sub bandit_level {
    my ($pass, $user, $c) = @_;
    my $ssh = Net::SSH::Perl->new("$hostname", port=>$port, debug=>0);
    $ssh->login("$user","$pass");
    my ($stdout, $stderr, $exit) = $ssh->cmd("$c");
    my $new_pass = $stdout;
    return $new_pass;
}

sub read_file {
    my %hash;
    open CONFIG, "commands.txt" or die;
    while (my $line = <CONFIG>) {
        chomp $line;
        my ($lvl, $cmd) = split /\:/, $line;
        $hash{$lvl} = $cmd;
    }
    close CONFIG;
    return %hash;
}

my %hash = read_file;
my $bandit_count = scalar(keys %hash);

while ($bandit_count-- > 0)
{
    print "$username:$password\n";
    $command = $hash{$username};
    $password = bandit_level($password, $username, "$command");
    chomp $password;
    $password =~ s/\s//g;
    $username = next_level($username);
}

With commands.txt being this:

bandit0:cat readme
bandit1:cat ./-
bandit2:cat 'spaces in this filename'
bandit3:cat inhere/.hidden
bandit4:cat inhere/./-file07
bandit5:cat inhere/./maybehere07/.file2
bandit6:cat /var/lib/dpkg/info/bandit7.password
bandit7:cat data.txt | grep millionth | sed -e 's/^millionth//'
bandit8:cat data.txt | sort | uniq -u
bandit9:strings data.txt | grep '=' | cut -d '=' -f11 | tail -c 36
bandit10:base64 -d data.txt

Not yet another perl hacker ;)

This answer is going to talk about your style, and best practices. Some of it may be subjective. I'll go through the code line by line.

use strict;
use warnings;

Good job! Modern Perl programs should always start with strict and warnings. If you want more detailed information on what's wrong, you can add use diagnostics, but remember to remove it before you ship, because it is really heavy and just a debugging aid. You can also turn on use warnings 'all'.

my ($hostname, $username, $password, $port, $command) = ("bandit.labs.overthewire.org", "bandit0", "bandit0", "2220", "");

As you can see from the scrollbar, this line is long. For two or three variables, assigning them as a list is fine, but in this case, I would break them into five statements. It's way clearer, especially since $username and $password have the same value.

In addition, you do not need to put numbers into quotes, unless you need a leading zero (i.e. for a ZIP, ISBN or EAN code).

Some people prefer single quotes '' when there is no interpolation, but that really doesn't matter and is a question of personal preference. For the empty string, I prefer to use q{}, as that clear shows the intention of an empty string, over a forgotten string or a typo that made it to production, because of the pair delimiters.

my $hostname = 'bandit.labs.overthewire.org';
my $username = 'bandit0';
my $password = 'bandit0';
my $port     = 2220;
my $command  = q{};

$user =~ s/(\d+)\z/ $1 + 1 /e;

This line is fine, but I would like to point out the /x modifier, that allows you to use comments on the pattern part of the substitution. You can also change the delimiter, so this could (but in this case should probably not) become:

$user =~ s{
    (\d+) # grab lots of numbers
    \z    # at the end of the line
}{ $1 + 1 }ex;

my ($pass, $user, $c) = @_;

What is $c? I guess it is the command, but it's not as nicely verbose as your other variables. I do a lot of work with the Catalyst framework, and to me, $c is hardwired to the Catalyst context object. One-letter variable names are seldom the right choice. Always go for clarity.

my $ssh = Net::SSH::Perl->new("$hostname", port=>$port, debug=>0);

You do not need to quote variable names. New Perls will probably optimize this away, but it's ugly and might introduce bugs. You've done that a couple of times (though not consistently), and I'm not going to mention it again.

Also, your spacing here is inconsistent. You are using spaces around the equal (=), so why not do the same around the fat comma (=>)?

my $ssh = Net::SSH::Perl->new($hostname, port => $port, debug => 0);

my ($stdout, $stderr, $exit) = $ssh->cmd("$c");
my $new_pass = $stdout;
return $new_pass;

You only need the $stdout variable. The other two are discarded, so there's no reason to assign them at all. You can assign things to undef in a list context, and those parts of the list on the right hand side will be thrown away.

my (undef, $two, undef) = (1, 2, 3);

Since you only want the first value, there's no need to even do that. In list context the assignment will start with the right-most values, and all the later elements on the RHS will be discarded if there is nothing to assign them to on the LHS.

The assignment to $new_pass is not necessary because you just return that value. I know it has some kind of documentary value, but you can just name your variable like that in the first place. $stdout and $stderr only need to be named that if you need to distinguish them. But in this case, you don't care how Net::SSH::Perl got your password.

my ($new_pass) = $ssh->cmd($c);
return $new_pass;

However, you might want to check for errors somehow. Not sure what those commands return. You maybe want to keep $exit to look at it.

my %hash;

This is a really bad name. The sigil % already tells us it's a hash. This is like naming your child Child. The hash holds levels and commands. Why not name it %commands?

open CONFIG, "commands.txt" or die;
while (my $line = <CONFIG>) {
    # ...
}

This is a big one. Don't use glob filehandles. The CONFIG thing is global. It's the same everywhere in your program. Even in modules from CPAN. If you were to call some function while that CONFIG filehandle is opened, and that module were also using one with the same name, it would interfere with yours. Instead, use a lexical filehandle.

This has the added benefit that you don't have to use close explicitly in this case, as the lexical variable will go out of scope at the end of the function call anyway, and thus the filehandle will implicitly be closed.

You also should use the three-argument form of open. Your two arg without a mode implies reading, which is fine with this fixed string filename here. But what if the name comes from somewhere else? What if it's |rm -rf /? Ooops. You just opened a pipe to a command that deleted your harddrive. Bad luck. Instead, always pass the mode as the second argument, and that kind of thing cannot happen.

You do have or die, which is good, but your error will be Died at .... That's not very helpful. Instead, at least include the error that was returned by open, which is in $!. Putting the filename in that error message is helpful too.

open my $fh, '<', 'commands.txt' or die "Can't open commands.txt: $!";
while (my $line = <$fh>) { ... }

my ($lvl, $cmd) = split /\:/, $line;

The colon : is not a meta character in regex. You don't need to escape it.

 my $bandit_count = scalar(keys %hash);

You don't need the parentheses here. In fact, you hardly ever need them in Perl, which makes the code read more like English.

And there's the %hash again... Actually it's another %hash. Using the same variable name in an outer and inner scope it not the best idea, as it's ambiguous.

my $bandit_count = scalar keys %hash;

while ($bandit_count-- > 0)
{

Your style of placing the curlies is inconsistent. Above you had them on the same line as the while loop, now they are in the same line. Decide for one.

That loop could be implemented in a lot of ways. Since you don't care about the loop variable, the way you picked is not very Perlish at all. In fact, you're prone to getting an off-by-one error (or two?). The $foo-- decrement operator returns the current value of the variable, and then reduces it by one. Is that really what you need here?

It would be easier to get rid of the $bandid_count variable completely, and simply use a range with a for or foreach loop. Both are the same syntactically. Some people prefer the short for, but I think for the non-C style loop the foreach is easier to read.

foreach (1 .. keys %commands) {
     # ...
}

You can leave out the [scalar][9] here as well, as the .. range operator already puts keys into scalar context.

print "$username:$password\n";

Instead of print and the newline, you can use say, which is shorter and includes the trailing line break. It's included in Perls starting from version 5.10 and needs to be turned on with use feature 'say' or a feature bundle.

In total, this code is pretty solid. If you really just started learning, you've done a great job. But then you are probably an experienced developer already, which is worth a lot.

If you wanted to replace the whole read_file sub with something more Perlish, you could do this:

sub read_file {
    return do {
        open my $fh, '<', 'commands.txt'
            or die "Can't open commands.txt: $!";
        map { chomp; split /:/ } <$fh>;
    };
}

The code makes use of a couple of features. The do block groups a number of statements into one. It's useful to know that every block's return value is always the return value of the last statement. That's also true for subs, so if there is no explicit return, chances are something will go wrong at some point.

map is like a foreach loop, and you probably know it from other languages that implement the map and reduce pattern. In the block, the topic variable $_ is used to iterate the list that comes out of the <$fh> diamond operator, which will just return the whole file as a list of lines in list context. Both chomp and split work on $_ implicitly if no arguments are passed, which makes this really concise.

If I may suggest another resource, the book Beginning Perl by Curtis "Ovid" Poe is great for developers that want to switch to Perl, as it has a nice walk-through, but does not explain everything down to how programming works, like Learning Perl would. To get more of the new stuff that's in the language, take a look at chromatic's Modern Perl, which is available for free online. Finally, there are some nice tricks in Effective Perl by Joseph N. Hall, Joshua A. McAdams and brian d foy. Several of the authors of these books are frequent users on Stack Overflow too.

Finally, something that might make your life a bit easier as these challenges get harder is Object::Remote. It allows you to execute Perl code on remote systems via SSH, without installing any modules there. You can use everything you have locally as long as it's compatible with the Perl that's there remotely, down to 5.8. Here is a great talk about it by the author from the German Perl Workshop 2014.


Processing lots of JSON objects I want to process a ton of JSON objects as quickly as possible.

I've done my best to make the processing as concise and efficient as I can.

How can I make it more efficient?

private void processButton_Click(object sender, EventArgs e)
{
    // For measuring how long the processing takes.
    var stopwatch = Stopwatch.StartNew();

    // Get the IDs of the completed RootObjects.
    // There are no duplicate IDs.
    var completed = new List<string>();
    foreach (var file in Directory.EnumerateFiles("C:\\Completed", "*.json"))
    {
        completed.AddRange(
            JsonConvert.DeserializeObject<List<RootObject>>(File.ReadAllText(file)).Select(o => o.id));
    }
    Console.WriteLine($"completed.Count: {completed.Count}");

    // Get the unfinished RootObjects.
    //
    // 78,198 of the unfinished RootObjects share their ID with another.
    // The duplicates are removed in the next step. (#2)
    //
    // The unfinished RootObjects also contain ALL of the completed RootObjects.
    // The completed RootObjects are ignored in the next step. (#1)
    var unfinished = new List<RootObject>();
    foreach (var file in Directory.EnumerateFiles("C:\\Unfinished", "*.json"))
    {
        unfinished.AddRange(JsonConvert.DeserializeObject<List<RootObject>>(File.ReadAllText(file)));
    }
    Console.WriteLine($"unfinished.Count: {unfinished.Count}");

    var processed =
        unfinished.Where(o => !completed.Contains(o.id)) // (#1) Ignore all completed RootObjects.
            .GroupBy(o => o.id).Select(objects => objects.First()) // (#2) Remove all duplicate RootObjects.
            .ToList();
    Console.WriteLine($"processed.Count: {processed.Count}");

    stopwatch.Stop();
    Console.WriteLine($"stopwatch.ElapsedMilliseconds: {stopwatch.ElapsedMilliseconds}");
}

// Output:
//      complete.Count: 35649
//      unfinished.Count: 250315
//      processed.Count: 136468
//      stopwatch.ElapsedMilliseconds: 75875

If you require any further information, let me know.

LINQ (convenient)

You could rewrite the loops with LINQ so this

var completed = new List<string>();
foreach (var file in Directory.EnumerateFiles("C:\\Completed", "*.json"))
{
  completed.AddRange(
      JsonConvert.DeserializeObject<List<RootObject>>(File.ReadAllText(file)).Select(o => o.id));
}

could become this

var completd =
    Directory.EnumerateFiles("C:\\Completed", "*.json")
    .Select(File.ReadAllText)
    .Select(json => JsonConvert.DeserializeObject<List<RootObject>>(json).Select(x => x.id))
    .SelectMany(x => x)
    .ToList();

You can do the same with the other loop.

When using LINQ you might experience some performance hit but don't be discouraged from using it. Use LINQ first because it's easy to use and read. Only if you are not satisfied with its performance rewrite it with a normal loop.

In most cases you won't even notice that LINQ is in action.

Run a profiler if you're not sure. Don't blindly rewrite everything with loops because LINQ potentialiy could slow something down unless you really, really, really need to.

var processed =
  unfinished.Where(o => !completed.Contains(o.id)) // (#1) Ignore all completed RootObjects.
      .GroupBy(o => o.id).Select(objects => objects.First()) // (#2) Remove all duplicate RootObjects.
      .ToList();

I think this is the most inefficient part in your code because the completed.Contains method is a O(n) operation.

There are two alternatives for it: either use a HashSets or the Except extension that

  [..] returns those elements in first that do not appear in second. It does not also return those elements in second that do not appear in first.

In order to make it work and make its usage easier you should get entire RootObjects from the first query instead of just the ids and you'll need a custom comparer for the RootObject that you can even reuse for the Distinct extension so you can replace the

.GroupBy(o => o.id).Select(objects => objects.First())

with

.Distinct(new RootObjectComparer())

The new query is now much easier to understand and does not requrie any comments. It also swaps the Distinct with the Except so it has less items to process.

var processed =
    unfinished
    .Distinct(new RootObjectComparer())
    .Except(completed, new RootObjectComparer())
    .ToList();

The custom comparer.

class RootObjectComparer : IEqualityComparer<RootObject>
{
    public bool Equals(RootObject left, RootObject right)
    {
        return left.id == right.id;
    }

    public int GetHashCode(RootObject obj)
    {
        return obj.id;
    }
}

LINQ free (fast)

You could try to mix LINQ and the HashSet into this where you not only provide the root-objects to the constructor but also the custom comparer.

A hash-set stores only unique objects so there's no need to filter them.

var unfinished = new HashSet<RootObject>(
    Directory
        .EnumerateFiles("C:\\Unfinished", "*.json")
        .Select(File.ReadAllText)
        .Select(json => JsonConvert.DeserializeObject<List<RootObject>>(json))
        .SelectMany(x => x),
    new RootObjectComparer()
);

The fastet version should be however this LINQ free one. Here you import the lists directly into the hash-set.

var unfinished = new HashSet<RootObject>(new RootObjectComparer());

foreach (var file in Directory.EnumerateFiles("C:\\Unfinished", "*.json"))
{
    unfinished.UnionWith(
        JsonConvert.DeserializeObject<List<RootObject>>(File.ReadAllText(file)));
}

Then you exclude the completed items with

unfinished.ExceptWith(completed);


Check whether two strings are anagrams Inspired by Monday's Reddit daily programmer challenge, I wrote a function to check whether two strings are anagrams. It doesn't take the same input as the challenge, though, since I don't want to handle user input (yet).

For the lazy ones, the task is to write a function that checks whether the letters in the first word can be shuffled (and upcased/downcased) to create the second word. Punctuation and whitespace isn't important.

This is more or less the first code I've written in Rust, so any kind of feedback is welcome:

/// Returns the sorted alphabetic symbols of a string in lowercase.
fn normalize(s : &str) -> Vec<char> {
    let mut characters : Vec<char> = s.to_lowercase()
        .chars()
        .filter(|x| x.is_alphabetic())
        .collect();
    characters.sort();
    characters
}

/// Returns true if the sets of alphabetic symbols coincide.
pub fn is_anagram(a : &str, b : &str) -> bool{
    normalize(a) == normalize(b)
}

#[test]
fn is_anagram_test() {
    assert!( is_anagram("Clint Eastwood","Old West Action"));
    assert!( is_anagram("wisdom","mid sow"));
    assert!(!is_anagram("Reddit","Eat Dirt"));
    assert!( is_anagram("Schoolmaster","The classroom"));
    assert!( is_anagram("Vacation Times", "I'm Not as Active"));
    assert!( is_anagram("12431b53c11a5324", "abc"));
}

I'm not particularly fond of using a mut Vec<char> just to use sort, but I guess it's the same situation as in C++, where std::sort will also modify a container in-place. The code was written with previous experience in C++, but with the following Haskell code in mind:

isAnagramOf :: String -> String -> Bool
isAnagramOf = (==) `on` (sort . map toLower . filter isLetter)

There is no space before : in types.
There is a space after the return type
There is a space between function arguments, after the comma.
str::to_lowercase allocates a new string. It's a potential optimization to iterate through each character and convert to lowercase. Profiling should be done if performance matters.
Could use itertools sorted to reduce some boilerplate. Does the same thing under the hood.

extern crate itertools;
use itertools::Itertools;

/// Returns the sorted alphabetic symbols of a string in lowercase.
fn normalize(s: &str) -> Vec<char> {
    s.chars()
        .flat_map(char::to_lowercase)
        .filter(|c| c.is_alphabetic())
        .sorted()
}

/// Returns true if the sets of alphabetic symbols coincide.
pub fn is_anagram(a: &str, b: &str) -> bool {
    normalize(a) == normalize(b)
}

#[test]
fn is_anagram_test() {
    assert!( is_anagram("Clint Eastwood", "Old West Action"));
    assert!( is_anagram("wisdom", "mid sow"));
    assert!(!is_anagram("Reddit", "Eat Dirt"));
    assert!( is_anagram("Schoolmaster", "The classroom"));
    assert!( is_anagram("Vacation Times", "I'm Not as Active"));
    assert!( is_anagram("12431b53c11a5324", "abc"));
}

Ultimately, you won't be able to achieve the same particular kind of elegance as the Haskell solution, but I'd hope that the performance would be better.


C# Parser of strings with list of assignments I have implemented class which manages communication with measuring device, which sends list of assignments, like variableName=value. These assignments are always separated by space. There are two options, the first is simple assignment variableName=value (without space around equal sign), the second is "array assignment", like variableName[num]= value_1 ... value_num (here is space after equal sign and between single values).
Here is my code for parser:

    /// <summary>
    /// Parse variable value "y" from assignment pair "x=y"
    /// </summary>
    /// <param name="s">string where "x=y" pairs are separated by space</param>
    /// <param name="varName">left side of assignment</param>
    /// <returns>right side of assignment</returns>
    private static string ParseEqualValue(string s, string varName)
    {
        int startPos = s.IndexOf("=", s.IndexOf(varName)) + 1;
        int endPos = s.IndexOf(" ", startPos);
        return s.Substring(startPos, endPos - startPos);
    }

    /// <summary>
    /// Parse variable values after "=" from string
    /// </summary>
    /// <param name="s">string in format: varName[num]= x1 .. xnum</param>
    /// <param name="varName">left side of assignment</param>
    /// <returns>right side of assignment in the list</returns>
    private static List<string> ParseEqualValues(string s, string varName)
    {
        List<string> list = new List<string>();

        // position of ShapeX..
        int varPos = s.IndexOf(varName);

        // parsing of numPts value in []
        int numStartPos = s.IndexOf("[", varPos) + 1;
        int numEndPos = s.IndexOf("]", numStartPos);
        int num = 0;
        int.TryParse(s.Substring(numStartPos, numEndPos - numStartPos), out num);

        int startPos = s.IndexOf("=", varPos) + 2;

        for (int i = 0; i < num; i++)
        {
            int endPos = s.IndexOf(" ", startPos);
            if (endPos < 0)
                endPos = s.Length;

            list.Add(s.Substring(startPos, endPos - startPos));
            startPos = endPos + 1;
        }

        return list;
    }

And here how I use it:

    /// <summary>
    /// Create ColdTest cut from ProfRoll answer
    /// </summary>
    /// <param name="s">Answer from ProfRoll</param>
    /// <returns></returns>
    public static Cut CreateColdTestCut(string s)
    {
        Cut cut = new Cut();
        cut.pts = new List<PointF>();

        int numShapes;

        double.TryParse(ParseEqualValue(s, "AvgTemp"), out cut.pyroTemp);
        double.TryParse(ParseEqualValue(s, "MinTemp"), out cut.tempMin);
        double.TryParse(ParseEqualValue(s, "MaxTemp"), out cut.tempMax);

        int.TryParse(ParseEqualValue(s, "NumShapes"), out numShapes);

        List<int> numPts = new List<int>();

        for (int i = 0; i < numShapes; i++)
        {
            List<string> xPts = ParseEqualValues(s, string.Format("ShapeX{{0}}", i));
            List<string> yPts = ParseEqualValues(s, string.Format("ShapeY{{0}}", i));

            if (xPts.Count != yPts.Count)
                break;

            numPts.Add(xPts.Count);

            for (int j = 0; j < xPts.Count; j++)
            {
                try
                {
                    cut.pts.Add(new PointF(float.Parse(xPts[j]), float.Parse(yPts[j])));
                }
                catch { }
            }
        }

        cut.isColdTest = true;
        cut.time = DateTime.Now;
        cut.numCurves = cut.numPts.Length;

        return cut;
    }

I will be glad for any critical comments.

I'm not sure exactly where to start with reviewing this code. There are many things that jump out as unsafe or awkwardly approached.

If my source string contained:

foo1=bar1 foo=bar

Then querying for 'foo'

int startPos = s.IndexOf("=", s.IndexOf(varName)) + 1;

Will incorrectly return the index of 'bar1' not 'bar'.

Use positional data to locate the keys within the string. All keys begin immediately after a space, and end immediately before an equals sign, and contain no spaces or equal signs.

Start by splitting the string on the '='. Your first key will be result[0]. Find the last index of a ' ' character in result[1]. Everything before that is your first value. Iterate your split results to build a Dictionary<string, string> and a Dictionary<string, string[]>, processing the value of key as appropriate based on the assignment type. You can then check if a particular key is defined by looking for it in the dictionaries, and get the value directly.

Is there a purpose to specifying the number of array elements in the key, when you already know the number from the value? How do you propose resolving the difference in the case of an incorrectly constructed string?

"a[1]= x y z"

Should either be a[3]= x y z or a[1]= x, but which takes precedence. If you can control the incoming format, and are not dealing with something beyond your control, use a[]= x y z and don't rely on the count being placed in the key.


Simple Makefile to build and run scala program I wrote this simple Makefile but it ended up like a spaghetti code. I used basename multiple times. I have a special case for first.spec.scala.run and follow.spec.scala.run and maybe more antipattern code that I am not aware of. I am not happy with my code and I appreciate any feedback or review. Thank you.
SPECS=First Follow
EXAMPLES_PATH=../..
ROOT_PATH=../${EXAMPLES_PATH}
SCALAV=2.12
APSLIB=${ROOT_PATH}/lib/aps-library-${SCALAV}.jar
SCALA_FLAGS=.:${APSLIB}
APS2SCALA=${ROOT_PATH}/bin/aps2scala

all: $(addsuffix Spec.compile, $(SPECS)) $(addsuffix Spec.run, $(SPECS))

%.generate:
    ${APS2SCALA} -DCOT -p ${EXAMPLES_PATH}:${ROOT_PATH}/base $*

%.run:
    @scala -cp ${SCALA_FLAGS} $(basename $@)

GrammarUtil.compile: grammar.generate
    scalac -cp ${SCALA_FLAGS} grammar.scala $(basename $@).scala

first.compile:
    scalac -cp ${SCALA_FLAGS} $(basename $@).scala

follow.compile:
    scalac -cp ${SCALA_FLAGS} $(basename $@).scala

Spec.compile:
    scalac -cp ${SCALA_FLAGS} $(basename $@).scala

FirstSpec.compile: Spec.compile grammar.generate GrammarUtil.compile first.generate first.compile
    scalac -cp ${SCALA_FLAGS} $(basename $@).scala

FollowSpec.compile: Spec.compile grammar.generate GrammarUtil.compile follow.generate follow.compile
    scalac -cp ${SCALA_FLAGS} $(basename $@).scala

clean:
    rm -f *.class grammar.scala first.scala follow.scala

I don't know Scala, but I do know Make, so I hope this helps.
Firstly, the makefile is missing .DELETE_ON_ERROR:, which pretty much every makefile needs, to avoid part-written output files appearing up-to-date when a compilation command fails (e.g. is interrupted).
We also want .PHONY: for those targets that don't produce files, such as all and clean.  (And clean should use $(RM) rather than rm -f, for increased portability.)
I think that we want a straightforward pattern rule that says that we can make a file called foo.compile given an input called foo.scala:
%.compile: %.scala
    scalac -cp ${SCALA_FLAGS} $<

It looks to me that -cp is one of the flags to scalac, so that should probably be in ${SCALA_FLAGS} rather than in the command.
I'm not convinced that your rules actually create the files they claim to.  For example, GrammarUtil.compile depends on grammar.generate, but then actually uses grammar.scala - should grammar.generate actually be grammar.scala there?


Calling an Oracle Stored Procedure with C# Code I came across this code in a project I inherited. I am curious if all the nested using statements make sense to anyone else. Also, wondering if this code could not be written in a better/leaner way?

    private static Dictionary<string, List<string>> GetSubscriptionKeyList(string proc)
    {
        var addResults = new List<string>();
        var deleteResults = new List<string>();

        using (var conn = new OracleConnection(conStr))
        {
            conn.Open();
            using (var cmd = new OracleCommand(proc, conn))
            {
                cmd.CommandType = CommandType.StoredProcedure;

                using (var adds = new OracleParameter
                {
                    ParameterName = "p_add",
                    OracleDbType = OracleDbType.RefCursor,
                    Direction = ParameterDirection.Output,
                    Value = DBNull.Value
                })
                {
                    using (var deletes = new OracleParameter
                    {
                        ParameterName = "p_delete",
                        OracleDbType = OracleDbType.RefCursor,
                        Direction = ParameterDirection.Output,
                        Value = DBNull.Value
                    })
                    {
                        cmd.Parameters.Add(adds);
                        cmd.Parameters.Add(deletes);

                        using (var rdr = cmd.ExecuteReader())
                        {
                            while (rdr.Read())
                            {
                                addResults.Add(rdr.GetString(0));
                            }
                            rdr.NextResult();
                            while (rdr.Read())
                            {
                                deleteResults.Add(rdr.GetString(0));
                            }

                        }
                    }
                }
            }
            conn.Close();
        }
        var result = new Dictionary<string, List<string>>
                         {
                             {"Adds", addResults},
                             {"Deletes", deleteResults}
                         };
        return result;
    }

I could see using it for the connection, command and reader since they implement IDisposable and the documentation for them does say to always call Dispose().

For the parameters it seems not only unnecessary but in my opinion it really decreases readability.  I would kill the using statements for the parameters and make them simple declarations.

For what it's worth, here's how I'd write it.

private static Dictionary<string, List<string>> GetSubscriptionKeyList(string proc)
{

    var result = new Dictionary<string, List<string>>();
    result.Add("Adds", new List<string>());
    result.Add("Deletes", new List<string>());

    using (var conn = new OracleConnection(conStr))
    {
        conn.Open();
        using (var cmd = new OracleCommand(proc, conn))
        {
            cmd.CommandType = CommandType.StoredProcedure;

            cmd.Parameters.Add(new OracleParameter("p_add", OracleDbType.RefCursor, ParameterDirection.Output));
            cmd.Parameters.Add(new OracleParameter("p_delete", OracleDbType.RefCursor, ParameterDirection.Output));

            using (var rdr = cmd.ExecuteReader())
            {
                while (rdr.Read())
                {
                    result["Adds"].Add(rdr.GetString(0));
                }
                rdr.NextResult();
                while (rdr.Read())
                {
                    result["Deletes"].Add(rdr.GetString(0));
                }

            }
        }
        conn.Close();
    }
    return result;
}

Obviously some things are a matter of taste, like whether to declare the parameters on a separate line and whether to keep the initializers there for the dictionary.  (I thought it was more clear to separate them.)


Split and word count in go Could it be written better?

package main

import (
    "code.google.com/p/go-tour/wc"
    "fmt"
)

func WordCount(s string) map[string]int {
    dict := make(map[string]int)

    splited := Split(s)
    for _, string := range splited {
        _, present := dict[string]
        if present {
            dict[string]++
        } else {
            dict[string] = 1
        }
    }
    return dict
}

func Split(s string) []string{
    arraySize := 1
    for i := 0; i < len(s); i++ {
        if s[i] == ' ' {
            arraySize++
        }
    }
    array := make([]string, arraySize)

    currentStrInd := 0
    currentStr := ""
    for i := 0; i < len(s); i++ {
        if s[i] == ' ' {
            array[currentStrInd] = currentStr
            currentStrInd++
            currentStr = ""
        } else {
            currentStr += string(s[i])
        }
    }
    array[arraySize - 1] = currentStr
    return array;
}

func main() {
    fmt.Println(Split("I am learning Go!"))
    wc.Test(WordCount)
}

For simply counting word occurrencies, yes:

package main

import (
    "fmt"
    "regexp"
)

func get_words_from(text string) []string{
    words:= regexp.MustCompile("\\w+")
    return words.FindAllString(text, -1)
}

func count_words (words []string) map[string]int{
    word_counts := make(map[string]int)
    for _, word :=range words{
        word_counts[word]++
    }
    return word_counts;
}

func console_out (word_counts map[string]int){
    for word, word_count :=range word_counts{
        fmt.Printf("%v %v\n",word, word_count)
    }
}

func main() {
    text := "I am learning Go! Go is a nice language to learn."
    console_out(count_words(get_words_from(text)))
}

Supposing, splitting by \w+ gives in most cases, what you want.

Another solution would be (\\b[^\\s]+\\b). Depends on your demands.

Go play with it!


Showing and hiding text excerpts Below is my jQuery scriptfile, all the functionality works (except for the click events on IE). I bring this before you guys to ensure I am effectively separating concerns and writing efficient scripts.

( function( $ ) {

  $( document ).ready(function() {
    setTimeout(textFill, 50);
    fitheadliner();
    showExcerpt();
    hideExcerpt();
  });

  $(window).resize( function() {
    fitheadliner();
    textFill();
  });

  function showExcerpt() {
    $('img.moreInfo').each(function() {
      var $excerpt = $(this),
        $excerptParent = $excerpt.parent();

      $excerpt.click(function() {
        var $boxInfo = $excerptParent.children('.modalExcerpt');
        $boxInfo.css("display", "initial");
      });
    });
  }

  function hideExcerpt() {
    $('.xBox').each(function() {
      var $thisBox = $(this);
      $thisBox.click(function() {
        $thisBox.parent().css("display", "none");
      });
    });

    $('.modalExcerpt').mouseleave(function() {
      if ($(window).width() > 700) {
        $(this).css("display", "none");
      };
    });
  }

  function fitheadliner() {
    $(".bigHeadline").each(function() {
    var
      $parent = $(this).parent(),
      $headline = $(this);

    var
      textW = $headline.width(),
      parentW = $parent.width(),
      ratio = parentW / textW;

    var
      originalSize = parseFloat($headline.css('font-size')),
      newSize = originalSize * (.99 * ratio);

      $headline.css("font-size", newSize);
    });
  }

function textFill() {
  $(".textFill").each(function() {
    var $text = $(this),
      $parent = $text.parent(),
      textW = $text.width(),
      textH = $text.height(),
      parentW = $parent.width(),
      parentH = $parent.height(),
      ratio = (parentW + parentH) / (textW + textH),
      originalSize = parseFloat($text.css('font-size'));

     var
      newSize = originalSize * (.9 * ratio);

    $text.css("font-size", newSize);

  });
}

} )( jQuery );

Looks good overall. Only a few minor quibbles:

Comments. Always a good idea. Write some.
What's with the show-and-immediately-hide-excert stuff in the ready event handler? I have no idea (I no comments to help me).
Don't "outdent" when inside a block (i.e. function or control flow construct). Your var declarations should be indented like anything else.

function () {
  // indent
  // everything
  // inside
  // the function
}

Don't put a linebreak right after var; put the first variable on the same line.
Personally, I indent following declaration lines by 4 spaces, to make them line up:

var textW = $headline.width(),
    parentW = $parent.width(),
    ratio = parentW / textW;

// more code

Tools like jslint yell at me for doing that, but I find it makes things more readable.
Combine your declarations. In fitheadliner you have 3 groups of var declarations in a row. I understand the grouping - it makes sense - but I'd either use a separate var for each line, or one big chunk. If you want to keep the grouping, you can separate declaration and assignment, e.g.:

function fitHeadings() {
  $(".bigHeadline").each(function() {
    var $headline, $parent,
        textWidth, parentWidth, ratio,
        originalSize, newSize;

    $headline = $(this);
    $parent = $headline.parent();

    textWidth = $headline.width();
    parentWidth = $parent.width();
    ratio = parentWidth / textWidth;

    originalSize = parseFloat($headline.css('font-size'));
    newSize = originalSize * (0.99 * ratio);

    $headline.css("font-size", newSize);
  });
}

A couple of notes to the above:

I've assigned $headline first, to avoid calling $(this) twice to also get the $parent.
I've added a zero to 0.99 because why not? Yeah, the shorthand works, but it really doesn't do anything. Writing 0.99 is a perfectly natural way to write a number.
Oh, and I've renamed the function to use camelCase to separate the words, and change headings to Headlines, since a) it's plural, and b) they're headings, not "headliners".

Of course, some might say you're overdoing the variable declarations for something rather trivial. This would work just as well:

function fitHeadlines() {
  $(".bigHeadline").each(function() {
    var $this = $(this), $parent,
        ratio = $this.parent().width() / $this.width(),
        newSize;

    newSize = 0.99 * ratio * parseFloat($headline.css('font-size'));
    $headline.css("font-size", newSize);
  });
}

By the way, why multiply by 0.99? Again, comments would be nice.
A more jQuery-esque way to attach the same event handler to multiple elements would (in most situations) be to attach the handler to a common parent object, and use the selector as a filter:

$(document).on('click', 'img.moreInfo', function(event) {
  // handle click
});

Also, a more jQuery-esque way to write a document ready handler is to just say:

$(function () {
  // this is run on document ready
)};

Be consistent with your whitespace. You have:

( function( $ ) {

and:

$( document ).ready(...)

but then you have:

$(window).resize(...)

I'd stick the latter; the other ones are maybe a bit to "spacey".


Resource Manager in C++ for Games I'm currently writing a small game, needing some advice for things I could do better on this resource management system. Speed is not crucial, and I have no memory budget.

The goal is to load and store resources so they can be reused and accessed with a string. That way, all resources are loaded at once, and can be reused by this singleton from anywhere else in the code. This system currently works, I'm just looking for ways to improve it or better leverage STL's features.

The Resource class

class Resource
{
public:
    Resource();
    virtual ~Resource();

    virtual void loadResource() = 0;
    virtual void unloadResource() = 0;

    void setResourceId(unsigned id);
    unsigned getResourceId() const;

    void setResourcePath(const std::string& path);
    std::string getResourcePath() const;

protected:
    unsigned mResourceId;
    std::string mResourcePath;

};

It's a simple base class with getters and setters, along with pure virtual loadResource() and unloadResource() functions that each derivative will override.

The ResourceManager class

class ResourceManager : public Singleton<ResourceManager>
{
public:
    ResourceManager();
    virtual ~ResourceManager();

    void addResource(Resource* resource, const std::string& name, const std::string& path);

    template <typename T>
    T* getResource(const std::string& name) {
        return dynamic_cast<T*>(mResources.find(name)->second);
    }

    static ResourceManager& getInstance();
    static ResourceManager* getInstancePtr();

private:
    std::map<std::string, Resource*> mResources;

};

The addResource implementation

void ResourceManager::addResource(Resource* resource, const std::string& name, const std::string& path)
{
    resource->setResourcePath(path);
    resource->setResourceId(mResources.size());
    resource->loadResource();

    mResources.insert(std::pair<std::string, Resource*>(name, resource));
}

The unloadResource function is called for every Resource in ResourceManager's destructor.

Usage example: loading the resource (with a hypothetical Texture class)

mResourceManager->addResource(new Texture, "test", "Resources/test.png");

Usage example: accessing the resource

mResourceManager->getResource<Texture>("test");

Like I said earlier, speed/memory isn't crucial, so all solutions are welcome.

The ResourceManager class
You should probably use a std::unordered_map<> (from the <unordered_map> header) instead of a std::map, since resources don't really need to be ordered as far as I can see. This will give you some speed, even if you're not concerned by that.
Consider throwing an exception from the getResource() function if the dynamic_cast<> fails. Right now, it is up the caller to check that the returned pointer is not null, it might be better if the function simply throws an exception so that client code does not have to wrap every single call with a check.
This is up to you, since now the client might have to wrap calls in try-catches. However, it certainly makes errors pop up a lot more visibly and at the call location, instead of possibly later if the client forgets to check for null.
The addResource implementation
You never check that resource is not null, which a pointer can be. Consider checking or taking a Resource& instead to ensure that you are operating on an existing value.
Your insertion into the map looks like this:
mResources.insert(std::pair<std::string, Resource*>(name, resource));

std::map<> and std::unordered_map<> both have the emplace() member function. The function takes the arguments to construct the key-value-pair internally (thus avoiding copy/move operations). You can instead write your return statement like so:
mResources.emplace(name, resource);

This looks cleaner and is more efficient.

Be careful with your map, I don't know what unload resource does, but it better call delete if that's (dynamically) how you're allocating your resources. However, a better idea might instead to use std::unique_ptr<> with a custom deleter. Here's a mock implementation:
#include <iostream>
#include <memory>

struct resource
{
    void unload_resource() { std::cout << "unloaded\n"; }
};

struct resource_free
{
    void operator()(resource* r)
    {
        r->unload_resource();
        delete r;
    }
};

int main()
{
    std::unique_ptr<resource, resource_free> pres{ new resource };
}

This will simplify the implementation of the destructor as std::unique_ptr<> takes care of properly releasing your resource. You can make an alias for ease of use: using resource_ptr = std::unique_ptr<...>. This allows you to write your map variable declaration in an easier to read way:
std::unordered_map<std::string, resource_ptr> mResources;


Sieve of Eratosthenes in C I have attempted to implement the Sieve of Eratosthenes in C. It works rather quickly with values tested up to ten million: time ./soe.out 10000000 returns real: 0m0.218s (this is of course, not including the result printing at the end which takes the majority of the time).

Is my code the most efficient it could be, and would implementing a segmented sieve instead be a good idea for improvement?

#include <stdio.h>
#include <string.h>
#include <math.h>
#include <errno.h>
#include <stdlib.h>

#define true (1)
#define false (0)
typedef unsigned char bool;

#define MAX 10000000

/* to_int: converts a character array to an integer, returns -1 on error, -2 on out of limits */
int to_int(char* inp) {
    int len = strlen(inp);
    unsigned int out = 0, prev_out = 0, mult = 1;

    if (len == 0)
        return -1;

    for (int i = len - 1; i >= 0; i--) {
        if (inp[i] < 48 || inp[i] > 57)
            return -1;

        prev_out = out;
        out += (inp[i] - 48) * mult;
        mult *= 10;

        /* detect wrapping */
        if (out < prev_out)
            return -2;
    }

    return out;
}

int main(int argc, char **argv) {
    if (argc < 2) {
        fprintf(stderr, "Syntax: number to find primes up to (max. %d)\n", MAX);
        return EXIT_FAILURE;
    }

    unsigned int max = to_int(argv[1]);

    if (max == -1) {
        fprintf(stderr, "Syntax: number to find primes up to (you typed %s, which is not a valid number)\n", argv[1]);
        return EXIT_FAILURE;
    }

    if (max > MAX || max == -2) {
        printf("Warning: the number you typed was outside of the limit. It has been set to %d.\nPress the return key to continue...\n", MAX);
        max = MAX;
        getchar();
    }

    max++;

    /* Set up the list */
    bool *list = NULL;
    if ((list = malloc(max)) == NULL) {
        fprintf(stderr, "Error! Could not allocate the requested amount of memory: %s\nExiting...\n", strerror(errno));
        return EXIT_FAILURE;
    }

    memset(list, true, max);

    int iteration = 0;
    int max_sqrt = sqrt(max);

    for (int i = 2; i <= max_sqrt; i++) {
        if (list[i])
            for (int j = i*i; j <= max; j += i)
                list[j] = false;
    }

    for (int i = 1; i < max; i++)
        if (list[i])
            printf("%d: prime\n", i);

    free(list);

    return EXIT_SUCCESS;
}

This works really well and is very easy to read! Here are some suggestions.

Don't Reinvent the Wheel

There are plenty of ways to convert a string to a number in C. There's no need to write your own. You could instead use atoi(), strtol() or sscanf().

Use the Right Comparison

You made max be an unsigned int, but then you compare it against -1 and -2. Those values are not unsigned. I would make some named constants for those comparisons. Something like:

static const unsigned int ERROR_NO_NUMBERS = UINT_MAX;
static const unsigned int ERROR_MAX_BAD_VALUE = ERROR_NO_NUMBERS - 1;

Note that the value for ERROR_NO_NUMBERS depends on the size of an int on your platform. I assumed it was 32 bits.

Prefer const to #define

Preprocessor macros created with #define are problematic. They can lead to very hard to understand code with surprising effects. For defining a constant it's not a huge deal, but it's good to get into the habit of using const for constants and inline functions for calculations when possible instead of macros because of the problems they have.

Performance

Aside from using a different algorithm, I don't see a lot of ways to improve the speed of this. One minor improvement might come from filling the array with alternating true/false values at the start so you iterate over fewer values. Something like this:

// Skip the first 4 values
list [ 0 ] = true;
list [ 1 ] = true;
list [ 2 ] = true;
list [ 3 ] = true;
// This value represents 4 bools in order false, true, false, true
const bool FALSE_TRUE_FALSE_TRUE[] = {false, true, false, true};
// Set all even values over 3 to false since we know they're divisible by 2
memset_pattern4(&list [ 4 ], FALSE_TRUE_FALSE_TRUE, max - 4);

You can then skip checking for 2 and start at 3:

for (int i = 3; i <= max_sqrt; i++) {
    if (list[i])
        for (int j = i*i; j <= max; j += i)
            list[j] = false;
}

Unused Variables

The iteration variable doesn't appear to be used anywhere. It should be removed.

Error Handling

I find your error handling a little odd. (Although it's great that you have it! It's often missed. Nice work.) You check to see if there's no number, and if so, output an error and exit. But if there is a number but it's out of range, you use the maximum allowed value and continue on. I'd make them both do the same thing. Either both exit or both use the maximum and continue on. (Personally, I think stopping is smarter.)


Plugin for showing cross-sells on the single product page in WooCommerce I made a custom plugin for showing cross-sells on the single product page in WooCommerce, however i need to know if this is secure and possibly act as an attack vector. Additionally, is there a better way than a while loop for iterating through the products?

if ( ! defined( 'ABSPATH' ) ) {
    exit;
}

if ( in_array( 'woocommerce/woocommerce.php', apply_filters( 'active_plugins', get_option( 'active_plugins' ) ) ) ) {

function show_cross_sell_in_single_product(){
    $crosssells = get_post_meta( get_the_ID(), '_crosssell_ids',true);
    $args = array(
        'post_type' => 'product',
        'posts_per_page' => 4,
        'post__in' => $crosssells
        );
    $products = new WP_Query( $args );
    if ( $products ) : ?>

<?php esc_html_e( 'Passend dazu', 'woocommerce' ); ?>

          <?php woocommerce_product_loop_start(); ?>

          <?php while ( $products->have_posts() ) : $products->the_post();
              wc_get_template_part( 'content', 'product' );
          endwhile; // end of the loop. ?>

          <?php woocommerce_product_loop_end(); ?>

      <?php endif;

    wp_reset_postdata();

}

add_action('woocommerce_after_single_product_summary', 'show_cross_sell_in_single_product', 10, 2);
/* todo removeaction woocommerce_after_single_product_summary, show_related */
}

The code looks ok to me from a security point of view. You're not accessing the database directly and it looks like you are escaping the output ( assuming that the template part does the same ).

A couple of suggestions:

Instead of looking for the filename in the list of active plugins, use function_exists or class_exists to check to see if the plugin is enabled by looking for a function or class defined by the plugin. The reason being that options can be cached and filenames can be changed. Function/Class names rarely change and it's much cleaner looking.

if ( class_exists( 'WC_Query' ) ) {
    // Your plugin code.
}

Check to make sure that you're actually getting data returned in $crosssells = get_post_meta( get_the_ID(), '_crosssell_ids',true); before using it in the query. If there is no data, there's no point in querying the database.

// Put this before the query
$crosssells = get_post_meta( get_the_ID(), '_crosssell_ids',true);
if ( empty( $crosssells ) {
    return;
}

WP_Query will always return something and so if( $products ) will always be true. Use if( $products->have_posts() ) instead. This will only return true if there were posts returned from the query.

To answer your question, the while loop is fine. That's what WordPress core and the bundled theme use.

The only other suggestion I would make is for performance. Add a couple of items to your query args:

    $args = array(
        'post_type' => 'product',
        'posts_per_page' => 4,
        'post__in' => $crosssells,

        /**
         * If you're not paginating, this skips MYSQL_CALC statments
         */
        'no_found_rows' => true,

        /**
         * Skips updating meta cache
         */
        'update_post_meta_cache' => false,

        /**
         * Skips updating term cache.
         */
         'update_post_term_cache' => false,
    );

Hope it helps!


Shell script to display environment variables This shell script writes my environment variables. If there is an argument, the shell script will grep for the argument in the environment variables.

PAGER=more
if type less > /dev/null;then PAGER=less; fi
echo $PAGER
if [ -z ${1+x} ]; then printenv|"$PAGER"; else printenv|grep "$1"|"$PAGER"; fi

If the above script is correct then I will work on my shell to be able to parse it. It can parse everything except the last line, which I added today.

Readability

Just because Bash let's you cram a lot of statements on a single line doesn't mean you should.
I recommend to split this up to multiple lines,
and also to put spaces around |, like this:

#!/bin/bash

PAGER=more
if less >/dev/null; then
    PAGER=less
fi
echo $PAGER

if [ -z ${1+x} ]; then
    printenv | "$PAGER"
else
    printenv | grep "$1" | "$PAGER"
fi

But if you like compact writing style,
a reasonable compromise is to replace the first if statement with an alternative writing style using && like this:

less >/dev/null && PAGER=less

Error handling

I'm not sure if it's intentional,
but if the less command doesn't exist (but in most systems it does),
this line will emit an error message:

if type less >/dev/null

To avoid errors, you want to suppress stderr too in addition to stdout:

if type less >/dev/null 2>&1

Prefer simple solutions

This is hacky, cryptic:

if [ -z ${1+x} ]; then

I suggest a much more readable, simple equivalent:

if [ -z "$1" ]; then

Don't repeat yourself

The printenv command is repeated in both branches of the last if statement.
You could move it in front of the if, like this:

printenv | \
if [ -z "$1" ]; then
    "$PAGER"
else
    grep "$1" | "$PAGER"
fi

However, in this writing style it's important that the \ on the line before the if is the last character on the line directly in front of the line break.


Determine if input word can be created from a list of letters I am looking for a more elegant way to validate in an input word can be created from a list of letters.

# Necessary variables (change as needed to test your code).
user_word = 'BANANA'
letters = ['N', 'B', 'N', 'A', 'A', 'P', 'M', 'S', 'A']

# This function checks whether a word entered by the user contains appropriate letters.
# It receives the letters and the word as parameter values, and returns True or False.
def validate_word(word, letters):
    from collections import Counter
    Count = Counter(letters)
    for letters in word:
        if not Count[letters]:
            return False
        Count[letters] -= 1
    return True

# Print the return value of the function to test it.
print(validate_word(user_word, letters))

As commented the function needs to return True if all the letters in the word are in the list, False otherwise.

Imports

In general, imports should be listed at the top of the script, not inside function definitions.  So instead of this:

def validate_word(words, letters):
    from collections import Counter
    ...

Write:

from collections import Counter

def validate_word(words, letters):
    ...

Variable names

In Python, variable names should be snake_case.  Words beginning with upper case letters are reserved from classes, like Counter, so Count should be called count.

Test Code

user_word and letters are declared far from the code that uses them.  They should be declared near where they are used, so at the bottom of the script, and ideally inside a __name__ == '__main__' guard:

from collections import Counter

def validate_word(word, letters):
    ...

if __name__ == '__main__':
    user_word = 'BANANA'
    letters = ['N', 'B', 'N', 'A', 'A', 'P', 'M', 'S', 'A']
    print(validate_word(user_word, letters))

A String is a List of Characters

The Counter class accepts an iterable for its construction argument, such as the list of letters you want to count.  But a string is also an iterable list of characters, so instead of a list, you could simply pass in a string, and it would work just as well:

if __name__ == '__main__':
    user_word = 'BANANA'
    letters = 'NBNAAPMSA'
    print(validate_word(user_word, letters))

Counters

You are counting up the counts of letters you are allowed to use.  Why not count up the count of letters in the user_word?

>>> Counter('NBNAAPMSA')
Counter({'A': 3, 'N': 2, 'B': 1, 'P': 1, 'M': 1, 'S': 1})
>>> Counter('BANANA')
Counter({'A': 3, 'N': 2, 'B': 1})

Then instead of looping over the letters of the user word, and subtracting 1 from the letter count, you could compare the letter counts directly.

Or ... you could subtract one from the other ...

>>> counts = Counter('NBNAAPMSA')
>>> counts.subtract('BANANA')
>>> counts
Counter({'P': 1, 'M': 1, 'S': 1, 'N': 0, 'B': 0, 'A': 0})
>>>

As long as all values in the counter are non-negative, the word is valid.

>>> all(count >= 0 for count in counts.values())
True

From this, a simpler function can be created.  Left to student.


More elegant way to construct the S3 path name with given options I have a method that generates the S3 key prefix according to the provided args. However, my gut feeling is that this method is not as elegant as it should be, maybe I just don't have an eye for it. Care to instruct me on how to make this little bit less sketchy?

private String generateFullQualifiedS3KeyPrefix(DataStream dataStream, Options options) {
    String environmentName, applicationName, streamName = applicationName = environmentName = null;
    if (dataStream != null) {
        environmentName = dataStream.getEnvironmentName();
        applicationName = dataStream.getApplicationName();
        streamName = dataStream.getStreamName();
    }
    String year, month, date, groupById = date = month = year = null;
    if (options != null) {
        year = String.valueOf(options.getYear());
        month = String.format("%02d", options.getMonth());
        date = String.format("%02d", options.getDate());
        groupById = options.getGroupById();
    }

    String[] arr = new String[]{environmentName, applicationName, streamName, groupById, year, month, date};
    StringJoiner filePath = new StringJoiner("/");
    for (int i = 0; i < arr.length; i++) {
        if (arr[i] == null) {
            break;
        }
        filePath.add(arr[i]);
    }
    return filePath.toString() + "/";
}

The local variables make this very verbose:
each variable is declared, initialized, re-assigned, and referenced.
(They each appear 3-4 times in the code.)
You could instead accumulate values in a list.

Creating an array that may contain null values,
then filtering out the null values in a second step feels like a waste that's easy to avoid.
You could instead accumulate non-null values in a list.

Using StringJoiner is efficient to join a string from multiple parts.
Then at the end the filePath.toString() + "/" is an inefficient string concatenation that could have been easily avoided by appending an empty string to filePath.

And instead of using StringJoiner, you could use String.join.

Consider this alternative:

private String generateFullQualifiedS3KeyPrefix(DataStream dataStream, Options options) {
    List<String> values = new ArrayList<>();

    if (dataStream != null) {
        values.add(dataStream.getEnvironmentName());
        values.add(dataStream.getApplicationName());
        values.add(dataStream.getStreamName());
    }

    if (options != null) {
        values.add(String.valueOf(options.getYear()));
        values.add(String.format("%02d", options.getMonth()));
        values.add(String.format("%02d", options.getDate()));
        values.add(options.getGroupById());
    }

    values.add("");

    return String.join("/", values);
}