squaring a number using bitwise multiplication or using * I had written a code which is given in the following code 1,code1 & Driver program section of the post.
I have to ask which of the following code is more efficient and takes less time to execute on the C(GCC 4.9.2) compiler.
I had a little confusion in the following code which is multiplication of input1 *input1 which  I might think will takes lot more time so
Is there any way to optimize like using bitwise multiplication.
Code 1
int FindingMoves(int input1)
    {
        if(input1>0)
        {
               ++input1;
               input1 <<=1;
               ++input1;
               return input1*input1;

        }
        if(input1 == 0){return 8;}

    }

This is the first code which I had written using bitwise multiplication but fails to write a code to multiply two numbers(input1*input1).
Code 2
int FindingMoves(int input1)
    {
        if(input1>0)
        {

               return (2*(input1+1)+1)*(2*(input1+1)+1);

        }
        if(input1 == 0){return 8;}

    }

This is straight forward code which is a simpler version of above.
DriverProgram
int main() {
    int output = 0;
    int ip1;
    scanf("%d", &ip1);
    output = FindingMoves(ip1);
    printf("%d\n", output);
    return 0;
}

This is the driver program to above code snippets.
Help me here to write optimized and efficient code. Any mysteries will be welcome regarding macro or something.

Negative input causes undefined behavior
If input1 is negative, your code reaches the end of function without returning a value.  Either you should return something such as 0 in that case, or you should make the input type be unsigned.
Simplification using math
Your expression (2*(input1+1)+1) evaluates to 2*input1+3.  So to minimize the work done, you could do:
if (input1 > 0) {
    input1 += input1 + 3;
    return input1 * input1;
}

This has two adds, one multiply, and zero shifts.  This is one less shift than the code in "code1" and one less multiply than the code in "code2", so it should be faster than either of them.


REST service to list new, continuing, and terminated subscribers during some period I have a use case which says:

  Your REST service will consume a JSON object that contains a list of
  emails for subscribers at the start of the subscription period, and a
  list of subscribers at the end of the subscription period. Your
  service must return a list of subscribers who are new, subscribers who
  stayed, and subscribers who unsubscribed.

  Example JSON input:

{
    "start" : [
        "test1@test.com",
        "test2@test.com"
    ],
    "end" : [
        "test1@test.com",
        "test3@test.com"
    ]
}

  Example JSON output:

{
    "new" : [
        "test3@test.com"
    ],
    "stayed" : [
        "test1@test.com"
    ],
    "unsubscribed" : [
        "test2@test.com"
    ]
}

The way I implemented it is:

Controller

@POST
@Timed
@Consumes(MediaType.APPLICATION_JSON)
public SubscriptionResponse subscriptionDetail(SubscriptionRequest request) {
    return new SubscriptionResponse(request);
}

Response

public SubscriptionResponse (SubscriptionRequest request) {
        //Get the common subscriptions
        List<String> common = new ArrayList<String>();
        common.addAll(request.getEnd());
        common.retainAll(request.getStart());
        //Filter out unsubscribed
        List<String> unsubscribed = request.getStart();
        unsubscribed.removeAll(common);
        //Get the new subscription
        List<String> newSub = request.getEnd();
        newSub.removeAll(common);
        unsubscribed.removeAll(common);

        this.newSub = newSub;
        this.stayed = common;
        this.unsubscribed = unsubscribed;
    }

Is it the right way of implementing it? What are the pros (if any) and cons?

This is one way of implementing it.
Whether it's right or not depends on many conditions,
and we could only speculate.
Here are some example problems anyway,
that could be serious issues:

The current solution is inefficient for large lists, because for example, for list1.retainAll(list2) the worst case complexity is \$O(N M)\$ where \$N\$ and \$M\$ are the sizes of the two lists. (For more details, see the source code of ArrayList.retainAll. You could make that \$O(N)\$ by converting list2 to a Set.
The current solution modifies request.getStart() and request.getEnd(). Maybe that's not ok, then the current solution should be adjusted to work with copies instead.

On the other hand,
a good thing about the current solution is that it's short and idiomatic,
and it doesn't waste memory (at the expense of CPU).

Lastly,
a minor issue is that the comments are pointless,
they only state the obvious.
The code would be easier to read if you made those lines empty.


Class Putting Self in std::vector This is a small test to try and add a copy of a containing class into a vector. To me, it looks very ugly. I hate the fact I need to write a constructor based off of a pointer in order to add the class to a vector. Is there a better way to organize this?

#include <string>
#include <sstream>
#include <vector>
#include <iostream>

Support function (not the main point of the question):

template <class T>
std::string to_string(const T& number) {
    if (std::is_arithmetic<T>::value) {
        std::stringstream ss;
        ss << number;
        return ss.str();
    } // else
    return "to_string not implemented"; // I could throw here...
}

Main code:

class Return_Self_Test {
private:
    std::string name;

public:
    Return_Self_Test(const std::string& name_in) : name(name_in) { }

    Return_Self_Test(Return_Self_Test* rst) : name(rst->name) {
    }

    Return_Self_Test(const Return_Self_Test &obj) : name(obj.name) {
    }

    void output() const {
        std::cout << "name: " << name;
    }

    std::vector<Return_Self_Test> get_in_vector() const {
        std::vector<Return_Self_Test> rst;
        rst.emplace_back(Return_Self_Test("new one 1"));
        rst.emplace_back(*this);
        rst.emplace_back(Return_Self_Test("new one 2"));
        return rst;
    }
};

void return_self_test() {
    Return_Self_Test rst_a("Original");

    auto a_vect = rst_a.get_in_vector();
    for (size_t i = 0; i < a_vect.size(); i++) {
        std::cout << to_string(i) + ": ";
        a_vect.at(i).output();
    }
}

return_self_test() gets called from main;

Any comments welcome!

#include <iostream>
#include <ostream>
#include <string>
#include <vector>

class Return_Self_Test {
    std::string name;

    public:
    Return_Self_Test(std::string const& name_in) : name(name_in) { }

    std::string const& get_name() const noexcept {
        return name;
    }

    std::vector<Return_Self_Test> get_in_vector() const {

        using std::literals::string_literals::operator""s;

        std::vector<Return_Self_Test> rst;

        rst.emplace_back(Return_Self_Test("new one 1"s));
        rst.emplace_back(*this);
        rst.emplace_back(Return_Self_Test("new one 2"s));

        return rst;
    }
};

std::ostream& operator<<(std::ostream& os, Return_Self_Test const& rst) {
    return os << "Name: " << rst.get_name();
}

void return_self_test() {

    Return_Self_Test rst_a("Original");
    auto a_vect = rst_a.get_in_vector();

    for (size_t i = 0; i < a_vect.size(); i++) {
        std::cout << std::to_string(i) + ": " << a_vect[i] << '\n';
    }
}

This is my suggestion for a slightly more organized solution of your problem. Let's walk this through and see what I did differently from you, and why.

Constructors

C++ is actually reasonably nice when it comes to what special member functions have to be declared by the user and which ones are generated by the compiler. For most simple classes, you actually don't need to define anything at all! Copy constructor, move constructor, copy assignment operator, move assignment operator and destructor are all implicitly defined in your case, so I went ahead and removed your copy constructor definition.

You should also notice that I removed that pointer-to-self constructor you defined, which you also expressed concerns about. I have good news for you: This constructor is completely unnecessary! In fact, your original code does not even use it, and neither does mine.

Where is to_string?

I don't know why you felt compelled to write your own version of this function, but the standard provides something with the exact same functionality in the string header (well, apart from the fact that std::to_string will downright reject anything that is not a number).

Still, let's take some time and talk about your version of this common functionality. Three things come to my mind here:

Why do you take number by const reference? You are expecting numeric types, which are all small enough to be efficiently passed as values.
Having a normal if on a compile-time type trait is an slightly bad, if you have access to C++17. In that case, this should clearly be if constexpr (...).
Regard and handle errors as errors. Don't return some ill-defined dummy value if your function does not succeed. There are many ways to signal an error, from throwing an exception to returning an std::optional. In this special case, however, the error is actually diagnosable at compile time. Although this makes point two useless, check your type at compile time and SFINAE out if it is not a number. You will save yourself a lot of debugging headaches down the line.

Doing I/O Correctly

Maybe you have been wondering: "Hey, where did my output method go?". Probably, you have already realized that I turned it into a non-member operator<<, and I do have good reasons to do so:

Imagine you are working with a huge project with thousands and thousand of classes. Your task is to implement a logging system: Sometimes, things go wrong, and it is your task to collect all the error messages that result from this, order them by certain criteria, and send them to various outputs (e.g. a log file, stdout, a web socket). However, all classes only offer a single output() method, just as Return_Self_Test does, which writes a message to std::cout.

So you spend the next half a year doing the most boring, mind-numbing refactoring work anyone could possibly imagine.

Do you understand what I am getting at here? You are basically loading a gun to shoot yourself in the foot with in a few years when you write code like this. C++ offers a whole lot of flexibility through the whole istream/ostream concept, and you currently use exactly nothing of it.

This is why you should start writing operator<< instead of your own output or write or whatever methods. This makes integrating output routines into an already existing context much more pleasant, as you can just write these values to std::cout as you are used to with all of the basic types. Furthermore, using a general std::ostream instead of std::cout will save you a lot of suffering when you find yourself in a situation where you actually have to write to something that is not the standard output.


Scaling up the intelligent concatenation of 2 columns in R I've got a huge dataset in R which contains (among other things) 2 columns indicating "Term Code", which are "Term Code" and "Term Code1".

"Term Code" is the mostly-correct column, which usually contains a 6-digit code for year and academic term:

> head(data$Term_code)
[1] 201230 201230 201230 201230 201230 201230

however, over the years that this field was populated, on occassion this field was either left blank or populated with only the last 2 digits:

> head(data$Term_code[nchar(data$Term_code) < 6], 100)
  [1] NA NA NA 70 NA NA 10 NA NA 30 NA 30 NA 40 NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA
 [35] NA 10 NA 30 NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA
 [69] NA NA NA 50 NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA NA

A SAS analyst did his best to fill construct another field, "Term_code1", with the preferred 6-digit version of the code where it's missing or truncated in "Term_code".

Now I need to combine these 2 columns into a single, maximally-complete column vector within the dataframe.

I did this using a for loop, which works on the small scale, but is slow at best with Big Data and at worse breaks by hitting the max memory size (even when that value is maxed out).

This is how I wrote it:

  for(i in 1:nrow(data)){
    if(is.na(data$Term_code[i]) && !(is.na(data$Term_code1[i]))){
      data$Term_code[i] <- data$Term_code1[i]
    }
  } # Took a very long time to run, but improves data a lot

  for(i in 1:nrow(data)){
    if(nchar(data$Term_code[i]) < (nchar(data$Term_code1[i]))){
      data$Term_code[i] <- data$Term_code1[i]
    }
  } # Broke after hours due to memory constraint

How should I have written it?

While I can't provide the actual data for reproducibility, if the description of these 2 vectors is insufficient perhaps this can approximate it close enough for testing purposes:

a                  <- data.frame(matrix(nrow = 999999, ncol = 2))
a[, 1]             <- runif(min = 201210, max = 201560, n = 999999)
a[,1]              <- round(a[,1])
b                  <- sample(a[,1], 1000)
a[a[,1] %in% b, 1] <- NA
b                  <- sample(a[,1], 250)
a[a[,1] %in% b, 1] <- substr(b, 5, 6)
a[,2]              <- runif(min = 201210, max = 201560, n = 999999)
b                  <- sample(a[,2], 150)
a[a[,2] %in% b, 2] <- NA

R is well-suited for vector operations.
It's not well-suited for loops,
which you should avoid as much as possible.

Consider this condition in your code:

if(is.na(data$Term_code[i]) && !(is.na(data$Term_code1[i]))){
  data$Term_code[i] <- data$Term_code1[i]
}

How about if we remove the second condition after the &&:

if(is.na(data$Term_code[i])){
  data$Term_code[i] <- data$Term_code1[i]
}

The outcome won't change, right? If Term_code1 is not NA, then we'll get its value, if it's NA, well, we already had NA, so it makes no difference.

The good thing about this transformation is that now we can convert this directly to a vector operation without a loop:

no.term.code <- is.na(data$Term_code)
data$Term_code[no.term.code] <- data$Term_code1[no.term.code]

That is, set the values of Term_code to the values of Term_code1 where Term_code is NA.


Android class in Tic-Tac-Toe CellDrawableProvider is an Android class used in the game Tic-Tac-Toe. It provides drawables for cells of game board (drawables for figures X and O, and "Fire" if the cell is in the firing line). To save memory, this class uses a map to store the already created drawables.

class CellDrawableProvider {
    private final Map<Object, Drawable> drawables;
    private final Resources resources;

    CellDrawableProvider(Resources resources) {
        this.resources = resources;
        this.drawables = new HashMap<Object, Drawable>();
    }

    Drawable getCellDrawable(int moveIconId, int fireIconId) {
        if (fireIconId == android.R.color.transparent) {
            return getDrawable(moveIconId);
        } else {
            return getLayerDrawable(moveIconId, fireIconId);
        }
    }

    private Drawable getLayerDrawable(int moveIconId, int fireIconId) {
        Object layerDrawableKey = Pair.create(moveIconId, fireIconId);
        Drawable layerDrawable = drawables.get(layerDrawableKey);
        if (layerDrawable == null) {
            layerDrawable = createLayerDrawable(moveIconId, fireIconId);
            drawables.put(layerDrawableKey, layerDrawable);
        }
        return layerDrawable;
    }

    private Drawable createLayerDrawable(int moveIconId, int fireIconId) {
        Drawable[] layers = new Drawable[2];
        layers[0] = getDrawable(moveIconId);
        layers[1] = getDrawable(fireIconId);
        return new LayerDrawable(layers);
    }

    private Drawable getDrawable(int iconId) {
        Drawable drawable = drawables.get(iconId);
        if (drawable == null) {
            drawable = resources.getDrawable(iconId);
            drawables.put(iconId, drawable);
        }
        return drawable;
    }
}

I do not like the class name CellDrawableProvider and method name getCellDrawable().

Can you suggest better names?

You don't need to "cache" the drawables yourself. Android does that internally, as can be seen in the source code.

    Drawable dr = getCachedDrawable(isColorDrawable ? mColorDrawableCache : mDrawableCache, key);

    if (dr != null) {
        return dr;
    }

Therefore, you're actually not saving any memory at all. (Technically you're wasting memory by creating a map for it, but that's so little memory it barely counts)

Instead of creating a LayerDrawable you might want to use setBackgroundResource() and setImageResource() (assuming you're using ImageViews)

As for the name CellDrawableProvider, I don't see what would be wrong with it. It's purpose is clear: A provider for cell drawables.


Merge array of hashes, keeping duplicate values as arrays I am working on a Ruby gem to pull data from the Wikidata API.  The data is deeply nested, as it contains a lot of repetition.

I have written a piece of Ruby (sans Rails) code that enables an array of hashes to be merged together, with any duplicate key-value pairs being stored as an array under the same key.

For example:

[{:a=>1, :b=>2}, {:b=>3, :c=>4}, {:d=>5, :a=>6}].merge_hashes

gives:

{:a=>[1, 6], :b=>[2, 3], :c=>4, :d=>5}

I began with the following:

class Array
  def merge_hashes
    raise StandardError.new "Array is not an Array of Hashes" unless self.all? {|e| e.is_a? Hash}
    self.each_with_object({}) do |el, h|
      el.each do |k, v|
        if h[k].nil?
          h[k] = v
        else
          h[k] = Array.new([h[k]]) << v
          h[k] = h[k].flatten
        end
      end
    end
  end
end

And refactored to:

class Array
def merge_hashes
    raise StandardError.new "Array is not an Array of Hashes" unless self.all? {|e| e.is_a? Hash}
    self.each_with_object({}) { |el, h| el.each { |k, v| h[k].nil? ? h[k] = v : h[k] = (Array.new([h[k]]) << v).flatten } }
  end
end

Are there any other improvements I could make?  Is that an acceptable exception to throw?

I think the refactor is barely readable, the first version is better. Anyway, if you use functional abstractions, instead of doing a imperative processing from scratch, algorithms tend to be more clear.

In this case, we can use the abstraction Enumerable#map_by (a group_by variation where you can control both the grouping function and the accumulated values) and write:

require 'facets'

class Array
  def merge_hashes
    flat_map(&:to_a).map_by { |k, v| [k, v] }
  end
end

Note that this snippet always returns an array as a value, instead of the scalar/list you have.


Retrieving current time and computing a hash value Can someone critique this Java code?

package javaapplication;

import java.util.Date;
import java.lang.Long;

interface infix_operator {

    public int apply(int x, int y);
}

class f implements infix_operator {

    public int apply(int x, int y) {
        return x + (2 * y);
    }
}

class g implements infix_operator {

    public int apply(int x, int y) {
        return (2*x) + y;
    }
}

class h implements infix_operator {

    public int apply(int x, int y) {
        return x + y;
    }
}

class k implements infix_operator {

    public int apply(int x, int y) {
        return x * y;
    }
}

public class JavaApplication {

    static long to_64bit_signed_integer (long value)
    {
        long bit16 = value & 65535;
        value = (bit16 << 16) | bit16;
        value = (value << 16) | bit16;
        value = (value << 16) | bit16;
        return Long.reverse(value);
    }

    public static void main(String[] args) {
        long current_time_value = new Date().getTime();
        System.out.println("Current Time: " + current_time_value);

        infix_operator[] operator_array = {new f(), new g(), new h(), new k()};

        int [] node = new int[31];
        int [] to_signed_value = new int[] {0,-2,1,-1};
        int [] to_unsigned_value = new int[] {0,2,1,3};

        long value = to_64bit_signed_integer (current_time_value);

        value = value >> 2;

        for (int leaf_index=30; leaf_index>=15; leaf_index--)
        {
            node[leaf_index] = to_signed_value [(int)(value & 3)];
            value = value >> 2;
        }

        for (int internal_index = 14; internal_index >= 0; internal_index--)
        {
            int op = to_unsigned_value [(int)(value & 3)];
            int child_index = 2*internal_index + 1;
            node [internal_index] = operator_array[op].apply(node[child_index], node[child_index+1]);
            value = value >> 2;
        }

        System.out.println("Hash Value: " + node[0]);
    }
}

Just a couple of points which are more from a stylistic point-of-view rather than strict language requirements or algorithmic correctness:

Classes should be named upper-case: Car, House, TwoTimesPlusOne, Addition
Interfaces should also be named upper-case: InfixOperator
Variable names should use camel-case: leafIndex, unsignedValue
i, j and k are used as iterator variables (the more you reference a variable, the shorter it should be named)
Prefer lists over arrays and make use of the enhanced for loop

for(InfixOperator i : operators) {
    //...
}

Adhering to common language standards will make your code more easily understandable for other programmers.


Simple Multiclass C++ Project So I'm working through a beginners C++ book and they had this example demonstrating class hierarchy, which was originally in a single file. I tried recreating it with multiple files (header files + source files) to test my understanding, but I wanted to make sure that I did everything in the "optimal" way. Specifically, I had to use more #includes than I thought I would need to, so I'm wondering if I can make improvements there. Here is what the project looks like:
main.cpp
#include "FDeepDishPizza.h"
#include <iostream>
using std::cout, std::endl;

int main()
{
    cout << "Hello world!" << endl;
    FDeepDishPizza deepPizza(12, 13, 14, 15);
    deepPizza.DumpDensity();
    return 0;
}

FrozenFood.h
#ifndef FROZENFOOD_H_INCLUDED
#define FROZENFOOD_H_INCLUDED

class FrozenFood {
private:
    int Price;

protected:
    int Weight;

public:
    FrozenFood(int APrice, int AWeight);
    int GetPrice();
    int GetWeight();
};

#endif // FROZENFOOD_H_INCLUDED

FrozenFood.cpp
#include "FrozenFood.h"
#include <iostream>
using std::cout, std::endl;

FrozenFood::FrozenFood(int APrice, int AWeight) {
    Price = APrice;
    Weight = AWeight;
}

int FrozenFood::GetPrice() {
    cout << Price << endl;
}

int FrozenFood::GetWeight() {
    cout << Weight << endl;
}

FrozenPizza.h
#ifndef FROZENPIZZA_H_INCLUDED
#define FROZENPIZZA_H_INCLUDED

#include "FrozenFood.h"

class FrozenPizza : public FrozenFood {
protected:
    int Diameter;
public:
    FrozenPizza(int APrice, int AWeight, int ADiameter);
    void DumpInfo();
};

#endif // FROZENPIZZA_H_INCLUDED

FrozenPizza.cpp
#include "FrozenPizza.h"
#include <iostream>
using std::cout, std::endl;

FrozenPizza::FrozenPizza(int APrice, int AWeight, int ADiameter) :
    FrozenFood(APrice, AWeight) {
    Diameter = ADiameter;
}

void FrozenPizza::DumpInfo() {
    cout << "\tFrozen Pizza Info:" << endl;
    cout << "\t\tWeight: " << Weight << " ounces" << endl;
    cout << "\t\tDiameter: " << Diameter << " inches" << endl;
}

FDeepDishPizza.h
#ifndef FDEEPDISHPIZZA_H_INCLUDED
#define FDEEPDISHPIZZA_H_INCLUDED

#include "FrozenPizza.h"

    class FDeepDishPizza : public FrozenPizza {         //public inherit from FrozenPizza, which itself inherits FrozenFood
    private:
        int Height;
    public:
        FDeepDishPizza(int APrice, int AWeight, int ADiameter, int AHeight);
        void DumpDensity();
    };

    #endif // FDEEPDISHPIZZA_H_INCLUDED

FDeepDishPizza.cpp
#include "FDeepDishPizza.h"
#include <iostream>
using std::cout, std::endl;

FDeepDishPizza::FDeepDishPizza(int APrice, int AWeight, int ADiameter, int AHeight) :
    FrozenPizza(APrice, AWeight, ADiameter) {
        Height = AHeight;
    }

void FDeepDishPizza::DumpDensity() {
    cout << "\tDensity: ";
    cout << Weight * 2 << " pounds per cubic foot" << endl; //doesn't actually give density just a test
}

Don't use std::endl
Use "\n" instead of std::endl. The latter is equivalent to "\n", but also forces a flush of the output stream, which can be bad for performance.
Don't confuse getting a value with printing a value
You have functions like int GetPrice(), that look like they return the price as an int. However, in your implementation, you actually print the price to the screen, and return nothing. Either don't print and return the price, or rename the function void PrintPrice() and return nothing.
It's usually better to just return the value instead of printing them. The caller can then decide if they want to print the value, and if so how.
Consider adding operator<< overloads for printing
It's always best to make classes behave like standard types. When printing something, wouldn't it be nice if you could write the following?
FrozenPizza fp(1, 2, 3);
std::cout << fp;

You can if you create a friend operator<<(), like so:
class FrozenPizza: public FrozenFood {
    ...
    public:
    friend std::ostream &operator<<(std::ostream &os, const FrozenPizza &fp);
};

std::ostream &operator<<(std::ostream &os, const FrozenPizza &fp) {
    os << "\tFrozen Pizza Info:\n"
          "\t\tWeight: " << fp.GetWeight() << " ounces\n"
          "\t\tDiameter: " << fp.GetDiameter() << " inches\n";
}

The advantage is that you can now also "print" to different streams beside std::cout, like to a std::stringstream or a std::ofstream, making it much more generic.
Mark constants with const
In your classes, you initialize the member variables with some value, after which they can never be changed. In that case, make those member variables const, so that this is also clear to the compiler, which can then better optimize your code, and give an error if you accidentily do write to those variables:
class Frozenfood {
    const int Price;

protected:
    const int Weight;

public:
    ...
};

To be able to set these const variables in the constructor, use member intialization lists:
FrozenFood::FrozenFood(int APrice, int AWeight):
        Price{APrice},
        Weight{AWeight} {
}

I see you already use member initializer lists when initializing the base class, which is good.
Mark member functions that do not modify any member variables as const
Apart from making member variables const, you can also make member functions const. This tells the compiler that these functions do not change any member variables, which allows it to generate better code, produce errors if you do accidentily make changes, and allows those functions to be called on const objects.
So for example:
class FrozenFood {
    ...
    int GetPrice() const;
    int GetWeight() const;
};

int GetPrice() const {
    return Price;
}

int GetWeight() const {
    return Weight;
}

Consider using #pragma once
You are using header guards correctly. However, it can be a chore to add #ifndef..#define..#endif, and it is easy to accidentily copy&paste the header guard and forget to change the name. All major compilers support #pragma once. If you add that to the header files, the compiler will ensure it's only ever included once.
However, as pointed out by others, it is not standard C++ (yet). Also, C++20 introduces modules, which avoids the need for header guards.


Filling a row with values I started with VBA yesterday. Could this code be improved?

Private Sub Worksheet_Change(ByVal target As Range)
    If target.Column = 3 Then
        Call Modul1.AutoFill(target)
    End If
End Sub

Option Explicit

Sub AutoFill(activeCell As Range)
    Dim storedData As Range: Set storedData = GrabStoredData(activeCell.Value)
    If Not storedData Is Nothing Then
        Call FillRowWith(activeCell, storedData)
    End If
End Sub

Private Function GrabStoredData(itemName As String) As Range
    Dim itemCell As Range
    Set itemCell = Sheets("SourceSheet").Range("A:A").Find(what:=itemName, lookat:=xlWhole)

    If Not itemCell Is Nothing Then
        Set GrabStoredData = Sheets("SourceSheet").Range("B" & itemCell.row & ":G" & itemCell.row)
    End If
End Function

Private Sub FillRowWith(activeCell As Range, storedData As Range)
    Dim counter As Integer
    For counter = 1 To 6
        Sheets("TargetSheet").Cells(activeCell.row, activeCell.Column + counter).Value = storedData.Offset(0, counter - 1).Value
    Next counter
End Sub

We have a SourceSheet that works as the "Database". The SourceSheet has 7 columns, with the first one used to identify an item or order. The other 6 columns contain information.

Lets say we put an order in "TargetSheet" which already exists in "SourceSheet". It will find the order in "SourceSheet" and take the information in the 6 columns and replace 6 columns in "TargetSheet" with that information.

This demo should show it better.

:

Don't.

People read code downwards much better than they read across.
Almost all code everywhere operates under the convention "(at most) 1 statement/operation per line". Colons : are small and very easily missed. Just stick subsequent operations on newlines and you'll avoid a lot of confusion/errors down the line.

Call is fully deprecated

It's not needed anymore and your code will run just fine without it. Assuming you name your methods well, dropping Call will also means your code reads better.

You will need to modify your calling syntax slightly though. When you have a DoThing Sub, this is valid syntax:

Call DoThing(Arg1, Arg2)

But this is not:

DoThing(Arg1, Arg2)

Functions get brackets. Subs do not. Like so:

Var = Function(Arg1, Arg2)
DoThing Arg1, Arg2

Don't let renaming break your code

Call Modul1.AutoFill(target)

Sheets("SourceSheet").Range("B" & itemCell.row & ":G" & itemCell.row)

If that module gets renamed, or that sheet gets renamed, your code will break.

Fortunately there are easy solutions.

1: Don't qualify your module references

You shouldn't need to specify which module your subs are in. If you do, this implies wider problems with your code. If your methods are descriptively named and you don't do something horrible like having 2 identically-named methods in different modules, just call your method using its signature.

2: Sheets have Codenames

If you go to the properties window and select a Worksheet, there will be a (name) property. This is the sheet's Codename. By default, new sheets have codenames like Sheet1, Sheet2 etc. A Codename is effectively a global worksheet variable that always refers to that sheet.

So, assuming you give "SourceSheet" a Codename of sourceSheet then your code can simply be:

sourceSheet.Range("B" & itemCell.row & ":G" & itemCell.row)

And now your users can name the sheets whatever they like and your code will still run fine.

Be Explicit with your naming

Your method names are OK, but very, very ambiguous. AutoFill, GrabStoredData, FillRowWith, they all imply a level of generality that is at odds with what they actually do.

As a rule, names should be descriptive, then unambiguous and only then concise. Easy-To-Understand trumps Shorter-Name every time.

GrabStoredData(itemName). Not a clue what that does in particular.

GetRowRangeOfItemData(itemName). I immediately know exactly what that does. It's going to return a Range Object for a given item. This Range Object will be on a single row.

AutoFill(Cell). Fill What? Where? With what?

FillItemDetailsToRightOfCell(Cell). Takes about 0.2 seconds longer to read. Takes much less time to understand. Especially when someone else (or you in 6 months time) is coming back to this code with no memory of the internal workings.

FillRowWith(Range, Cell). If I see that I'm going to assume it, well, fills a row. No Idea what those variables are, particularly.

PasteRangeToRightOfCell(Range, Cell). Again, takes a fraction of a second longer to read, takes no time to understand exactly what's going on.


Test on remove function let's say I want to create a 'utils' package with several functions
working on files and folders. I want to do it once properly so I can use
them in all my personal projects without feeling to re invent the wheel
each time.
I also want to learn 2 items by the way:

exception handling, already read a lot about EAFP vs LBYL, I think       the following function suits well to EAFP.
unit tests with pytest tool, it seems simple enough to start with.

My function is:

def removeanything(src):
    """
    remove files or folders
    """
    try:
        os.remove(src)
        print('File is removed')
    except IsADirectoryError:
        shutil.rmtree(src)
        print('Folder is removed')
    except FileNotFoundError:
        print('File/folder is not existing')
    except PermissionError:
        print('Not allowed to suppress this file')

Do you have any comment to do regarding my exception handling? Did I forget one or more exceptions?

To go ahead, I have written this test function for pytest:

from pathlib import Path
import utils
#import pytest

def test_removeanything(tmp_path):
    d = tmp_path / '.tmp_dir'
    d.mkdir()
    f = d / '.tmp_file'
    f.touch()

    # Test permission
    d.chmod(mode=0o555)
    utils.removeanything(f)
    assert Path.exists(f)

    # Test file deletion
    d.chmod(mode=0o777)
    utils.removeanything(f)
    assert not Path.exists(f)

    # Test folder deletion
    utils.removeanything(d)
    assert not Path.exists(d)

As I am pretty newbie at testing, I would like to know if I include all reasonable things to assert for my function? Are there other way to do it more properly?
Is there a way to assert if expected error has been indeed raised by my function?

Thanks!

Doc comments:

You can be more clear in the doc comment about what the function does:

What is the input? (string, path)
Does it return anything? (no)
Does it throw exceptions? (yes, because os.remove and shutil.rmtree may throw)
What is the state after calling the function? (file/folder has been removed, except if there was a permission error or another error)

Exceptions:

Someone who calls removeanything("myfile") may expect that myfile does not exist anymore after the function call. However, in case of a permission error, it still does exist. I think this is an exceptional situation, so I recommend that you do not catch the PermissionError and instead propagate it to the caller.

Output:

Currently, the function communicates via print statements. This means that callers of the function have no way of finding out what actually happened. You could add a return value that indicates whether a file, a directory, or nothing was removed. Then you may think about removing the print statement, or enabling/disabling it via function argument, because users may want to remove files silently.

Tests:

It may be useful to separate the single test test_removeanything into multiple tests test_removeanything_deletes_file, test_removeanything_deletes_directory, test_removeanything_handles_permission_error. This way, if a test fails, the test name gives you some more information about what went wrong.

Often, functions that remove directories require them to be non-empty. Therefore, it makes sense to test the removal of both empty and non-empty directories.

If you change removeanything so that the PermissionError propagates to the user, you can use pytest.raises to test whether the exception was raised correctly.

Misc:

I think the name removeanything can be more specific. After all, the function does not remove a CD from my CD drive ;)


Convert data (adding and sorting) with JavaScript I have an array of data like this:

var data = [
    ['a', 2010, 12], ['a', 2010, 20], ['a', 2011, 43], ['a', 2012, 25], ['a', 2012, 15],
    ['b', 2010, 40], ['b', 2012, 65], ['b', 2013, 20], ['b', 2013, 10], ['b', 2013, 15],
    ['c', 2010, 13], ['c', 2010, 17], ['c', 2011, 22], ['c', 2011, 32], ['c', 2011, 10], ['c', 2012, 45], ['c', 2013, 10], ['c', 2013, 20]
]

and each array data is [name, year, value].

What I want to do here is to add values when name and year are the same and then sort data by year and change the order by name, so I want to convert data to:

var newData = [
    ['a', 2010, 32], ['b', 2010, 40], ['c', 2010, 30],
    ['a', 2011, 43], ['c', 2011, 64]
    ['a', 2012, 40], ['b', 2012, 65], ['c', 2012, 45],
    ['b', 2013, 45], ['c', 2013, 30]
]

I am doing this by taking multiple steps:

// find names
var sortByName = [];
var nameArr = [];
var namesFound = {};
for(var i = 0; i < data.length; i++) {
  if(namesFound[data[i][0]]) { continue; }
  nameArr.push(data[i][0]);
  namesFound[data[i][0]] = true;
}
// sort data by name by creating an array for each name (2D array to 3D array)
for (var i = 0; i < nameArr.length; i++) {
  var result = data.filter(function(arr) {
    return arr[0] == nameArr[i]
  })
  sortByName.push(result)
}
// add values with a same year
var newArr = sortByName.map(function(eachArr) {
  var sumBySameYear =  eachArr.reduce(function(acc, arr) {
    if (acc.year === arr[1]) {
      acc.result[acc.result.length - 1][2] += arr[2];
    } else {
      acc.result.push(arr);
      acc.year = arr[1]
    }
    return acc
  }, {result: [], year: null});
  return sumBySameYear.result
});
// 3D array to 2D array
var multiToSingleArr = [].concat.apply([], newArr);
// then sort new data by year and keep the order of name
var newData = multiToSingleArr.slice().sort(function(a, b) {
  return a[1] - b[1] || a[0].localeCompare(b[0]);
});

I definitely feel like I am taking unnecessary steps here, so how can I make this more concise?

My solution

I came with something more concise, readable and effective:

const convertData = data => {
  const obj    = {}, // Intermediate object {year => {letter => number}}
        result = [];

  data.forEach(array => {
    const [letter, year, number] = array;
    obj[year]         = obj[year] || {}; // Creating object if it doesn't exist
    obj[year][letter] = obj[year][letter] + number || number; // Create or increment property
  });

  Object.keys(obj).forEach(year => {
    Object.keys(obj[year]).forEach(letter => {
      result.push([letter, parseInt(year), obj[year][letter]]);
    });
  });

  return result;
};

/* ===== DEMO ===== */

const data = [
    ['a', 2010, 12], ['a', 2010, 20], ['a', 2011, 43], ['a', 2012, 25], ['a', 2012, 15],
    ['b', 2010, 40], ['b', 2012, 65], ['b', 2013, 20], ['b', 2013, 10], ['b', 2013, 15],
    ['c', 2010, 13], ['c', 2010, 17], ['c', 2011, 22], ['c', 2011, 32], ['c', 2011, 10], ['c', 2012, 45], ['c', 2013, 10], ['c', 2013, 20]
];

console.log(JSON.stringify(convertData(data)));
/* Output console formatting */
.as-console-wrapper { top: 0; }

Explanation

First part of my function (data.forEach()) builds intermediate object obj that looks like this afterwards:

{
  2010: {
    a: 32,
    b: 40,
    c: 30
  },
  2011: {
    a: 43,
    c: 64
  },
  2012: {
    a: 40,
    b: 65,
    c: 45
  },
  2013: {
    b: 45,
    c: 30
  }
}

The second part just iterates by years and for every year by it's letters and builds result array.

Optimized version

First part of my function (data.forEach() loop) can be replaced with

for (let i = 0; i < data.length; i++) {
  const [letter, year, number] = data[i];
  obj[year]         = obj[year] || {}; // Creating object if it doesn't exist
  obj[year][letter] = obj[year][letter] + number || number; // Create or increment property
}

It's simply change to the for loop, but it gives significant performance boost without compromising readability at all. Interestingly, despite .forEach()es are generally slower, replacing the rest of the code with regular loops has negative impact on performance.

ES5 version

function convertData(data) {
  var obj    = {}, // Intermediate object {year => {letter => number}}
      result = [];

  for (var i = 0; i < data.length; i++) {
    var letter = data[i][0],
        year   = data[i][1],
        number = data[i][2];
    obj[year]         = obj[year] || {}; // Creating object if it doesn't exist
    obj[year][letter] = obj[year][letter] + number || number; // Create or increment property
  };

  Object.keys(obj).forEach(function objForEach(year) {
    Object.keys(obj[year]).forEach(function yearForEach(letter) {
      result.push([letter, parseInt(year), obj[year][letter]]);
    });
  });

  return result;
}

/* ===== DEMO ===== */

var data = [
    ['a', 2010, 12], ['a', 2010, 20], ['a', 2011, 43], ['a', 2012, 25], ['a', 2012, 15],
    ['b', 2010, 40], ['b', 2012, 65], ['b', 2013, 20], ['b', 2013, 10], ['b', 2013, 15],
    ['c', 2010, 13], ['c', 2010, 17], ['c', 2011, 22], ['c', 2011, 32], ['c', 2011, 10], ['c', 2012, 45], ['c', 2013, 10], ['c', 2013, 20]
];

console.log(JSON.stringify(convertData(data)));
/* Output console formatting */
.as-console-wrapper { top: 0; }

Benchmark

@Flambino's solution:      17,241 ops/s ±1.2%
My solution:                      19,433 ops/s ±3.02%
My solution (optimized):  33,490 ops/s ±3.25%

Remarks to your code

Formatting

There are a few minor formatting problems:

You could combine multiple vars together,
Lack of spaces here and there, and multiple ones in other places,
Few semicolons are missing (despite optional, it's a good practice to have them),
Minor indentation issues.

Also you could add more spacing to all your blocks of code to aid readability.

Unnecessary continue

continue is very often a useful statement, although it shouldn't be used if it can be avoided without compromising readability. This in my opinion

for(var i = 0; i < data.length; i++) {
  if(namesFound[data[i][0]]) { continue; }
  nameArr.push(data[i][0]);
  namesFound[data[i][0]] = true;
}

could be replaced with

for (var i = 0; i < data.length; i++) {
  if (!namesFound[data[i][0]]) {
    nameArr.push(data[i][0]);
    namesFound[data[i][0]] = true;
  }
}

Loop iterator declaration

These two lines suggest that you treat variable i like if it had block scope, which is not the case:

for (var i = 0; i < data.length; i++) {
for (var i = 0; i < nameArr.length; i++) {

The way the code looks right now, after i is declared for the first time, it's available everywhere in the function containing it, or worse yet ― if it's all in a global scope, it's global variable. Block scope variable can be declared with ES6's let. If you don't want to or can't use it, you could declare i before both loops, and then go with i = 0 without var keyword.

Use strict equality operator whenever possible

This line

return arr[0] == nameArr[i];

could be easily replaced with

return arr[0] === nameArr[i];

That way no type conversion will be performed.

Don't declare functions within loops

You declare function withing your loops in your code. That way on each iteration you in fact create a completely new function.

Name your anonymous functions

Named function expressions may come in handy when it comes to debugging. Quoting "Named function expressions demystified" by Juriy "kangax" Zaytsev:

  In a nutshell, named function expressions are useful for one thing only — descriptive function names in debuggers and profilers.

Don't push it

sortByName[i] = result;

would be much more efficient than

sortByName.push(result);

Here's the benchmark:


Javascript Nested Classes Are there any potential issues with the following Javascript code:

var BobsGarage = BobsGarage || {}; // namespace

/**
 * BobsGarage.Car
 * @constructor
 * @returns {BobsGarage.Car}
 */
BobsGarage.Car = function() {

    /**
     * Engine
     * @constructor
     * @returns {Engine}
     */
    var Engine = function() {
        // definition of an engine
    };

    Engine.prototype.constructor = Engine;
    Engine.prototype.start = function() {
        console.log('start engine');
    };

    /**
     * Tank
     * @constructor
     * @returns {Tank}
     */
    var Tank = function() {
        // definition of a tank
    };

    Tank.prototype.constructor = Tank;
    Tank.prototype.fill = function() {
        console.log('fill tank');
    };

    this.engine = new Engine();
    this.tank = new Tank();
};

BobsGarage.Car.prototype.constructor = BobsGarage.Car;

/**
 * BobsGarage.Ferrari
 * Derived from BobsGarage.Car
 * @constructor
 * @returns {BobsGarage.Ferrari}
 */
BobsGarage.Ferrari = function() {
    BobsGarage.Car.call(this);
};
BobsGarage.Ferrari.prototype = Object.create(BobsGarage.Car.prototype);
BobsGarage.Ferrari.prototype.constructor = BobsGarage.Ferrari;
BobsGarage.Ferrari.prototype.speedUp = function() {
    console.log('speed up');
};

// Test it on the road

var car = new BobsGarage.Car();
car.tank.fill();
car.engine.start();

var ferrari = new BobsGarage.Ferrari();
ferrari.tank.fill();
ferrari.engine.start();
ferrari.speedUp();

// var engine = new Engine(); // ReferenceError

console.log(ferrari);

The goal is to have prototype inheritance and nested classes so that classes defined within BobsGarage.Car are not accessible outside the constructor of BobsGarage.Car but instances of them are accessible to derived classes, as shown in the test code.

Note: I am referring to the concept of Class in Javascript as defined on the MDN.

I don't see any issues with your code.

One suggestion would be to make your code a little DRYer by assigning to your prototype in 1 go and drop the jsdoc ( it adds a lot of lines, for little value ).

//Definition of an engine
var Engine = function() {};

Engine.prototype = {
  constructor : Engine,
  start : function() {
    console.log('start engine');
  }
};

Instead of

/**
 * Engine
 * @constructor
 * @returns {Engine}
 */
var Engine = function() {
    // definition of an engine
};

Engine.prototype.constructor = Engine;
Engine.prototype.start = function() {
    console.log('start engine');
};

Creating ferrari out of car is even more verbose, I would suggest 2 helper functions to keep the code more readable.

BobsGarage.Ferrari.prototype = {
  constructor : BobsGarage.Ferrari,
  speedUp : function() {
    console.log('speed up');
  },
}

extendPrototype( BobsGarage.Car , BobsGarage.Ferrari );

function extendPrototype( base , extension )
{
  extension.prototype = extend( base.prototype , extension.prototype );
}

function extend( o , extension )
{
  for( var key in extension )
    o[key] = extension[key];
  return o;
}

Obviously you would keep re-using extend and extendPrototype.


Simplifying function I'm making a chess game. On the board the columns go from a-h and rows from 8-1. I made this simple function so when someone inputs 'a' for the row it will give me the right row on an array same goes for columns, 7a would be [1][0]. Is there an easier way someone could point out for me?

Thanks in advance.

update:added small working sample of function.

#include <stdio.h>

int getColRow(char input)
{
    int val = 0;

    switch (input)
    {
    //row
    case 97:
        val = 0;
        break;
    case 98:
        val = 1;
        break;
    case 99:
        val = 2;
        break;
    case 100:
       val = 3;
       break;
    case 101:
       val = 4;
       break;
    case 102:
       val = 5;
       break;
    case 103:
       val = 6;
       break;
    case 104:
        val = 7;
        break;
    //col
    case 49:
        val = 7;
       break;
    case 50:
        val = 6;
        break;
    case 51:
        val = 5;
        break;
    case 52:
        val = 4;
        break;
    case 53:
        val = 3;
        break;
    case 54:
        val = 2;
        break;
    case 55:
        val = 1;
        break;
    case 56:
        val = 0;
        break;
    default:
        break;
    }
    return val;
    }

    int main(int argc, char *argv[])
    {
    char a = 'a';
    char b = 'b';
    char c = 'c';
    char d = 'd';
    char e = 'e';
    char f = 'f';
    char g = 'g';
    char h = 'h';

    char one = '1';
    char two = '2';
    char three = '3';
    char four = '4';
    char five = '5';
    char six = '6';
    char seven = '7';
    char eight = '8';

    printf("value of %c is %d ",a,getColRow(a));
    printf("\nvalue of %c is %d ",b,getColRow(b));
    printf("\nvalue of %c is %d ",c,getColRow(c));
    printf("\nvalue of %c is %d ",d,getColRow(d));
    printf("\nvalue of %c is %d ",e,getColRow(e));
    printf("\nvalue of %c is %d ",f,getColRow(f));
    printf("\nvalue of %c is %d ",g,getColRow(g));
    printf("\nvalue of %c is %d \n",h,getColRow(h));

    printf("\nvalue of %c is %d ",one,getColRow(one));
    printf("\nvalue of %c is %d ",two,getColRow(two));
    printf("\nvalue of %c is %d ",three,getColRow(three));
    printf("\nvalue of %c is %d ",four,getColRow(four));
    printf("\nvalue of %c is %d ",five,getColRow(five));
    printf("\nvalue of %c is %d ",six,getColRow(six));
    printf("\nvalue of %c is %d ",seven,getColRow(seven));
    printf("\nvalue of %c is %d \n",eight,getColRow(eight));

    return 0;
}

What you are doing is very basic, and the question really belongs on stackoverflow.com, but it has probably already been answered there, and therefore would be marked as a duplicate.

As I mentioned in the comment, this can be done using simple subtraction of characters when you include ctype.h>, you don't need a switch statement or anything that complcated. The example below should execute faster than the version with the switch statement, and it is a lot less code.

#include <ctype.h>

int getColRow(char input)
{
    int val = 0;
    unsigned char testValue = (unsigned char) input;

    if (isdigit(testValue))
    {
        return testValue - '0';
    }
    else if (isalpha(testValue))
    {
        testValue = tolower(testValue);
        return testValue - 'a';
    }

    fprintf(stderr, "Invalid character %c in getColRow(char input)\n", input);
    return val;
}

Most of the functions / macros in ctype.h take unsigned integers, and it is safer to use unsigned values. The unsigned char should automatically be promoted to an unsigned integer.


Checking and updating lines in a file I am trying to check a bunch of files that should have a leading | delimiter but sometimes don't. If the first line doesn't have the |, the rest of the file won't, if it does have it, then the rest of the file will.

I'm checking the first line which works and enters the update logic, which advances to the next row once I read the line in. So I added a writer.Write before I go into the Do While loop and it works, but it seems like there would be a better way of doing this.

    Dim fileList As FileInfo() = dirList.GetFiles

    For Each fiName In fileList
        sFullName = fiName.FullName
        sFileName = Path.GetFileNameWithoutExtension(fiName.ToString)
        sDir = fiName.DirectoryName
        Dim reader As New StreamReader(sFullName)
        Dim line As String = reader.ReadLine()

        If Not line.StartsWith("|") Then

            Dim sNewFullName As String = sDir + "\" + sFileName + "_Temp.txt"
            Dim writer As New StreamWriter(sNewFullName)
            Dim sLine As String

            sLine = "|" + line + System.Environment.NewLine
            writer.Write(sLine)

            Do While reader.Peek() <> -1
                sLine = "|" + reader.ReadLine() + System.Environment.NewLine
                writer.Write(sLine)
            Loop

            writer.Close()
            reader.Close()
            File.Delete(sFullName)
            File.Move(sNewFullName, sFullName)

        End If
        reader.Close()
    Next fiName

Building filenames should be done using the System.IO.Path.Combine() method. This ensures that you don't skip a "\" by accident and also enforces that no invalid path/file characters are used.
Strings shouldn't be added to other strings (if done more than once), better use System.Text.StringBuilder

As Mat's Mug already pointed out, your variable naming should be enhanced.

So

Dim sNewFullName As String = sDir + "\" + sFileName + "_Temp.txt"

would become

Dim newFullName As String = System.IO.Path.Combine(currentDirectoryName, currentFileName + "_Temp.txt")

As Mat's Mug also pointed out you need to extract some of the responsibilities to separate methods.

So let us introduce a method which takes a filename as input parameter and returns an array of Strings representing the content of the file.

Private Function GetFileContent(fileName As String) As String()
    Return System.IO.File.ReadAllLines(fileName)
End Function

next we introduce a function which takes a filename as input parameter and returns wether the first character of the first line is the "|" . We first check if the read line is maybe Nothing by using String.IsNullOrWhiteSpace(). This prevents an exception if the file would be empty.

Private Function FirstLineStartsWithPrefix(fileName As String, prefix As String) As Boolean
    Using reader As New System.IO.StringReader(fileName)
        Dim line As String = reader.ReadLine()
        Return Not String.IsNullOrWhiteSpace(line) AndAlso line.StartsWith(prefix)
    End Using
End Function

Next we introduce a method which takes a String array as input parameter and returns the prefixed content. We are using the Append() and AppendLine() method. Both methods are returning a StringBuilder object, so we can use it in a fluent way.

Private Function AddPrefixToLines(content As String(), prefix As String) As String
    Dim sb As New System.Text.StringBuilder(1024)
    For Each line As String In content
        sb.Append(prefix).AppendLine(line)
    Next
    Return sb.ToString()
End Function

Next we introduce a method which takes a FileInfo object as input parameter and returns nothing (a Sub).

Private Const pipePrefix As String = "|"
Private Const fileNamePostFix As String = "_Temp.txt"
Private Sub ProcessFile(currentFileInfo As FileInfo)

    Dim fullName As String = currentFileInfo.FullName

    If Not FirstLineStartsWithPrefix(fullName, pipePrefix) Then Exit Sub

    Dim fileContent As String() = GetFileContent(currentFileInfo.FullName)

    Dim prefixedContent As String = AddPrefixToLines(fileContent, pipePrefix)

    Dim pureFileName As String = Path.GetFileNameWithoutExtension(fullName)

    Dim newFileName As String = Path.Combine(currentFileInfo.DirectoryName, pureFileName & fileNamePostFix)

    File.WriteAllText(newFileName, prefixedContent)

    currentFileInfo.Delete()

    File.Move(newFileName, fullName)

End Sub

Now your initial code will look like

Dim fileList As FileInfo() = dirList.GetFiles

For Each fiName In fileList

    ProcessFile fiName

Next


Converting KML/XML to Javascript So, I have the boundaries of every states in the US, from the google map KML file.

This KML file is actually a XML file.

I'm converting this file to a JS file compatible with google map in order to draw polygons on my map.

Here is an excerpt from the file with all the coordinates:

<?xml version="1.0" encoding="UTF-8"?>
<kml xmlns="http://earth.google.com/kml/2.2">
<Document>
  <Placemark>
    <name>WA</name>
    <description><![CDATA[]]></description>
    <styleUrl>#style37</styleUrl>
    <Polygon>
      <outerBoundaryIs>
        <LinearRing>
          <tessellate>1</tessellate>
          <coordinates>
            -122.402015585862,48.2252165511906
            -122.368333031748,48.1281417374007
            -122.216991980112,48.0074395523983
            -122.230120864997,47.9691133009971
            -122.302922293019,47.9502148146411
            -122.394492319816,47.7741760704507
            -122.414815251142,47.6641799154458
            -122.382220450337,47.5954090424224
            -122.392633724016,47.510242430349
            -122.319738644767,47.3901148739843
            -122.325376306437,47.3443234291417
            -122.420837154063,47.3188444009071
            <!-- etc., 243 points altogether for WA state -->
          </coordinates>
        </LinearRing>
      </outerBoundaryIs>
    </Polygon>
  </Placemark>

Here is my code to convert the KML file to the JS file.

require 'rubygems'
require 'xmlsimple'

def read_xmlfile(file_name)
  file = File.open(file_name, "r")
  data = file.read
  file.close
  return data
end

target  = "states.js"

xml_data = read_xmlfile('Stateborders.xml')

data = XmlSimple.xml_in(xml_data, 'KeyToSymbol' => true)

content = "var stateBorders = {\n"
data[:document][0].each_with_index do |item, index|
  states = item[1]
  states.each_with_index do |state, index|
      name = state[:name][0]
      coord = state[:polygon][0][:outerboundaryis][0][:linearring][0][:coordinates][0].tr("\t", "").strip.gsub(/[\n]/,") ,new google.maps.LatLng(").gsub(" ) ,", "), ");
      coord = state[:polygon][0][:outerboundaryis][0][:linearring][0][:coordinates][0].tr("\t", "").strip
      coord2 = ""
      coord.split("\n").each do |ll|
        ll = ll.split(",")
        lat = ll[1].to_s.strip
        lat = lat.to_f.round(6).to_s
        lon = ll[0].to_s.strip
        lon = lon.to_f.round(6).to_s
        coord2 << "new google.maps.LatLng("+lat+", "+lon+"), "
      end
      statecoord = "'#{name}' : [#{coord2}],".gsub(", ],","],")
      content << statecoord+"\n"
  end
end
content << "};"

File.open(target, "w+") do |f|
  f.write(content)
end

The code itself is fast enough but i'm sure can be much simpler/cleaner. Also for the last state I have ], instead of ].

@Flambino has already explained the main problems in your code. I'd only add: 1) why do you use each_with_index if you don't actually use the indexes? 2) Ruby would have a terrible standard library if you needed 4 lines just to read the contents of a file: contents = File.read(path).

That's how I'd write it:

require 'nokogiri'
require 'json'

kml = Nokogiri::XML(open("Stateborders.xml"))
coordinates_by_state = kml.css("Placemark").map do |placemark|
  state = placemark.at_css("name").text.strip
  coordinates = placemark.at_css("coordinates").text.strip.lines.map do |line|
    line.split(",").map { |coord| coord.to_f.round(6).to_s }
  end
  [state, coordinates]
end.to_h

jscode = "var stateBorders = #{coordinates_by_state.to_json};"
File.write("stateborders.js", jscode)


Converting fractions to mixed numbers I've been making a program for an assignment that converts fractions to mixed numbers, and I'm done, but I was wondering if I could simplify it in any way, since it seems quite complicated right now.

Also, keep in mind that I am not allowed to use any Math or String methods to do this assignment, and I can only use +, -, *, and / as arithmetic operators. I also cannot use % or a modulus function.

I realize this is an assignment, and it's fine if you choose not to give me an answer. In fact, I'd actually prefer to have some tips or hints to help figure out how to solve it, rather than just telling me the answer.

Also, the 'c.print' is basically a print statement. I'm importing/using a different custom class, which is why it is different from the normal print statement.

    int numerator = 0; //can change to any value
int denominator = -2; //can change to any value
public int[] reducedFraction (int numerator, int denominator)
{
    int GCD = numerator;
    int tempN = numerator;
    int tempD = denominator;
    int[] fractionParts = {0, numerator, denominator};

    if (numerator == 0) //if the numerator is 0
    {
        return fractionParts;
    }

    //Making sure the program works with negative values by temporarily converting GCD and fractionParts[2] to positive values
    GCD = (numerator < 0) ? -numerator:
    numerator;

    tempD = (denominator < 0) ? -denominator:
    denominator;

    //Finding the GCD of the numerator and denominator
    while (GCD != tempD)
    {
        if (GCD > tempD)
            GCD -= tempD;
        else
            tempD -= GCD;
    }

    //Simplfying numerator and denominator
    fractionParts [1] /= GCD;
    fractionParts [2] /= GCD;

    //Temporarily making the numerator and denominator positive
    tempN = (fractionParts [1] < 0) ? -fractionParts [1]:
    fractionParts [1];

    tempD = (fractionParts [2] < 0) ? -fractionParts [2]:
    fractionParts [2];

    //if the numerator is not greater than the denominator return the simplified fraction.
    if (tempN < tempD)
    {
        return fractionParts;
    }

    //Finding value of whole number, numerator and possibly converting denominator to original sign
    fractionParts [0] = fractionParts [1] / fractionParts [2];
    fractionParts [1] = fractionParts [1] - (fractionParts [1] / fractionParts [2]) * fractionParts [2];

    if (fractionParts [1] < 0 && fractionParts [2] < 0) //if the numerator and denominator are both negative
    {
        fractionParts [1] = (fractionParts [1] < 0) ? -fractionParts [1]:
        fractionParts [1];

        fractionParts [2] = (fractionParts [2] < 0) ? -fractionParts [2]:
        fractionParts [2];
    }

    return fractionParts;
}

public void display ()
{
    if (reducedFraction (numerator, denominator) [1] == 0)
        c.print (reducedFraction (numerator, denominator) [0]);

    else if (reducedFraction (numerator, denominator) [0] == 0)
        c.print (reducedFraction (numerator, denominator) [1] + "/" + reducedFraction (numerator, denominator) [2]);

    else
        c.print (reducedFraction (numerator, denominator) [0] + " " + reducedFraction (numerator, denominator) [1] + "/" + reducedFraction (numerator, denominator) [2]);
}

First, a couple of tips:

Using an int[] to represent the result is a poor choice. It would be better to create a class for this, for example called MixedNumber. Then, instead of accessing the elements by indexes, you can access them by field name. It could also have an appropriate toString method to print itself
The handling of negative numbers is very poor. You could determine the sign of the number in one place early on, use it to multiply the integral part and that's it. That would get rid of many confusing redundant logic. The current code also has a bug related to it: for the input (3, -2) it gives back -1 1/-2 instead of -1 1/2
An elegant implementation of the gcd algorithm is this recursive function: return b == 0 ? a : gcd(b, a % b);
The printing of the number can be cleaner and simpler using String.format

I recommend to rewrite the function in terms of this class:

class MixedNumber {
    private final int integral;
    private final int numerator;
    private final int denominator;

    private MixedNumber(int integral, int numerator, int denominator) {
        this.integral = integral;
        this.numerator = numerator;
        this.denominator = denominator;
    }

    private static int gcd(int a, int b) {
        return b == 0 ? a : gcd(b, a % b);
    }

    static MixedNumber fromFraction(int numerator, int denominator) {
        // TODO: simplified main implementation comes here
    }

    @Override
    public String toString() {
        if (numerator == 0) {
            return "" + integral;
        } else if (integral == 0) {
            return String.format("%d/%d", numerator, denominator);
        } else {
            return String.format("%d %d/%d", integral, numerator, denominator);
        }
    }
}

Rather than printing the output, use unit tests to verify.
After you finished the implementation above,
with the bug that I mentioned fixed,
these unit tests should pass:

@Test
public void testZeroNumerator() {
    assertEquals("0", MixedNumber.fromFraction(0, -2).toString());
}

@Test
public void test_minus3_over_2() {
    assertEquals("-1 1/2", MixedNumber.fromFraction(-3, 2).toString());
}

@Test
public void test_3_over_minus2() {
    assertEquals("-1 1/2", MixedNumber.fromFraction(3, -2).toString());
}

@Test
public void test_minus3_over_minus2() {
    assertEquals("1 1/2", MixedNumber.fromFraction(-3, -2).toString());
}

@Test
public void test_123_over_82() {
    assertEquals("1 1/2", MixedNumber.fromFraction(123, 82).toString());
}

@Test
public void test_82_over_123() {
    assertEquals("2/3", MixedNumber.fromFraction(82, 123).toString());
}

Whatever IDE you use, there is certainly an easy way to add a JUnit4 test class and run it. Give it a try!


A clean tacit version of (n choose k) in J Listing all \$\binom{n}{k}\$ combinations of i is a well-solved problem in J. From that article, there is this implementation:

comb=: 4 : 0
 k=. i.>:d=.y-x
 z=. (d$<i.0 0),<i.1 0
 for. i.x do. z=. k ,.&.> ,&.>/\. >:&.> z end.
 ; z
)

used as follows:

   4 comb 6
0 1 2 3
0 1 2 4
0 1 2 5
0 1 3 4
0 1 3 5
0 1 4 5
0 2 3 4
0 2 3 5
0 2 4 5
0 3 4 5
1 2 3 4
1 2 3 5
1 2 4 5
1 3 4 5
2 3 4 5

I am interested primarily in clean, tacit version. Here's what I have so far:

comb=. ~.@:(/:~"1)@(i.@[ {"1 (i.@! A. i.)@])

but that feels overly complex and verbose. Is there a better tacit implementation?

Your initial verb is a good start. Your code could be shortened to 24 bytes

~.@(/:~@:{"1~i.)~!A.&i.]

A shorter version for 22 bytes can be found by permuting the binary string with k 1's.

[:I.@~.i.@!@]A.]{.1#~[

These are both O(n!) for C(n, k).


Building text inside a while loop I use the following code
<?php echo $obj->text; ?>
 in while loop.

Is this the best way?

Is there a better way, either to optimize or replace this code?

<?php
    $akhbarkotah1 = $conn->prepare("SELECT text,time FROM small WHERE active='0' ORDER BY time DESC LIMIT 10");
    $akhbarkotah1->execute();
    while($obj = $akhbarkotah1->fetch(PDO::FETCH_OBJ)){

?>

<?php echo $obj->text; ?>

<?php
    }
?>

It looks like you don't really need to exit out of the PHP tag on this one.

<?php
    $akhbarkotah1 = $conn->prepare("SELECT text,time FROM small WHERE active='0' ORDER BY time DESC LIMIT 10");
    $akhbarkotah1->execute();
    while($obj = $akhbarkotah1->fetch(PDO::FETCH_OBJ)){
        echo "
" . $obj->text . "
";
    }
?>

This simplifies the code and shortens it.  I think that I have the Syntax Correct.  I am rather new to PHP myself but I am pretty sure that this will work exactly the same.

If you leave your code the other way you should indent the HTML to match what is inside the PHP since it is really inside the PHP (kind of/sort of)  like this

<?php
    $akhbarkotah1 = $conn->prepare("SELECT text,time FROM small WHERE active='0' ORDER BY time DESC LIMIT 10");
    $akhbarkotah1->execute();
    while($obj = $akhbarkotah1->fetch(PDO::FETCH_OBJ)){
?>

<?php echo $obj->text; ?>

<?php
    }
?>

I personally don't like the way that this looks though.


Classical inheritance in JavaScript I've been playing around with inheritance in JavaScript and I'm wondering if there are any drawbacks to this method which tries to emulate classical inheritance from C-based languages.

Function.prototype.extends = function (parentClass) {
    function temp () { this.constructor = this; };
    temp.prototype = new parentClass();

    this.prototype = new temp();
};

var Animal = function() {
    function Animal() {};

    Animal.prototype.walk = function () {
        return this.name + ' walks';
    };

    return Animal;
}();

var Cat = function () {
    function Cat (name) {
        this.name = name;
    };

    Cat.extends(Animal);

    Cat.prototype.meow = function() {
        return this.name + ' meows';
    };

    return Cat;
}();

var Chester = new Cat('Chester');

console.log(Chester.meow()); // Chester meows
console.log(Chester.walk()); // Chester walks

This is a crude example, but can anyone point out any glaring problems?

Hang on a tick... there is a problem here that, weirdly nobody has noticed. I will admit that this is hardly ever used, but still:
In a normal JS prototypal inheritance scenario, you would be able to pull stunts like these:

var Animal = (function()
{
    function Animal()
    {
        this.name = this.constructor.name;
    };
    Animal.prototype.walk = function()
    {
        return this.name + ' is walking';
    };
    return Animal;
}()):
var Cat = (function(parent)
{
    function Cat(name)
    {
        this.name = name || this.constructor.name;
    };
    Cat.prototype = new Animal;
    Cat.prototype.constructor = Cat;
    Cat.prototype.sleep = function()
    {
        return this.name + ' is sleeping';
    };
    return Cat;
}(Animal));
var garfield = new Cat('Garfield');
//code
var newSameClass = new garfield.constructor();

Whereas the extends method you have written causes infinite constructor recursion: you're declaring a new function, that uses the this keyword... but because it's being used with the new keyword, it creates a new instance of itself (temp).
In the constructor of temp, all it does is equate its prototype to its own instance... you create an object that is its own prototype and therefore effectively is not part of any chain (it keeps on pointing to itself) and for some bizarre reason, you expect this to keep track of the constructor? I think what you ought to write is this:

Function.prototype.extends = function(parentClass)
{
    var temp = this.prototype.constructor;
    this.prototype = new ParentClass;
    this.prototype.constructor = temp;
};

Which works just fine, and doesn't require some temp constructor/object at any point.
Just try it with this simple example:

function Animal(){};
function Cow(){};
Cow.extends(Animal);
function Bird(){};
function Chicken(){};
Bird.extends(Animal);
Chicken.extends(Bird);
var dinner = new Chicken();
console.log(dinner instanceof Animal);//true -- sorry vegetarians
console.log(dinner instanceof Cow);//false -- no beef tonight
console.log(dinner instanceof Bird);//true -- mmm, poultry
console.log(dinner instanceof Chicken);//true -- roasted, probably

Which is, I take it, what you wanted.
PS: I would strongly advise you not to augment prototypes you don't own. By that I mean Object, Array, Function, Date and the like... Save for a few cases where the String prototype poses X-browser issues (String.trim wasn't implemented in IE8, for example).
You might encounter issues with other toolkits/libs, future updates, issues depending on the implementation (IE, FF and V8 deal with native prototypes differently in some cases)

All in all, it's best to leave them be... I see no reason why you shouldn't simply write a function, and not attach it to the prototype. Or, if needs must, do what ECMA is doing: all these createObject-like thingies go into one big bin: Object as in Object.getOwnPropertyNames() and Object.getPrototypeOf(). What's so difficult about:

function extend(child, parent)
{
    var prto, Object.getPrototypeOf(child),
        tmp = prto.constructor;
    prto = new parent;
    prto.constructor = tmp;
    return child;//<-- this might be handy
}
//usage:
var garfield = new (extends(Cat, Animal))('garfield');


Fetching player data I would like to select some information I have stored in my MySQL DB.

I have 2 tables named gameTable and playerTable. Now I want to get the gameTable and get the names and levels of these players.

This is what I got so far, it works but I'm not sure this is the most efficient way. Do I really need to create a join for every referred row?

<?php

include ('connect.php');

 $result = mysqli_query($connect,
        "SELECT gameTable.id,
            playerTable1.name AS name1, playerTable1.level AS level1,
            playerTable2.name AS name2, playerTable2.level AS level2,
            playerTable3.name AS name3, playerTable3.level AS level3,
            playerTable4.name AS name4, playerTable4.level AS level4
         FROM gameTable
            JOIN playerTable AS playerTable1 ON gameTable.P1_id = playerTable1.id
            JOIN playerTable AS playerTable2 ON gameTable.P2_id = playerTable2.id
            JOIN playerTable AS playerTable3 ON gameTable.P3_id = playerTable3.id
            JOIN playerTable AS playerTable4 ON gameTable.P4_id = playerTable4.id
        ");

    while ($record = mysqli_fetch_array($result))
    {
        echo $record['id']  . " - "
            . $record['name1'] . " - " . $record['level1'] . " - "
            . $record['name2'] . " - " . $record['level2'] . " - "
            . $record['name3'] . " - " . $record['level3'] . " - "
            . $record['name4'] . " - " . $record['level4'] . "<br>";
    }

?>

As a secondary question, my database is set to MyISAM. Would it be helpful to change it  to InnoDB? Or is this only useful for update delete etc. In your database and not for fetching data?

Well, as far as I see it, I can't see another way of listing what you're using. But as you're using JOIN, I would make sure that your columns have the right indexes there, or it'll be very slow when your database grow up.

About INNODB: It depends on your database, and what you're doing with the data, but the good thing about INNODB is that if you have that table engine you can make use of Transactions, and will be easier to make a backup of your live databases if that is the engine selected. And also, if your engine is INNODB, you can use foreign keys to keep your data synchronized  (for example, you can restrict the delete of a user if the id is in another table, or not allowing insert a player id if that id doesn't exist).

If you're not planning using transactions neither foreign keys, and your tables are going to perform way more read operation than modify operations (create/update) usually MyISAM is faster, but if you have to modify the rows and read at the same time, is going to be better INNODB because when reading/modifying it only locks the row that is using, while MyISAM locks the whole table. When you'll have heavy load and read/write at the same time, MyISAM will be slower. Anyway, don't trust me: Test it by yourself performing load testing on your application, that's the way to go.

And lately, about your code: Don't mix connection, SQL and show the presentation on the same file, or it'll be hard to you later make the code maintenance. I would suggest you divide that code into classes, so you can retrieve the data from a model class, and show them in a template. Read From flat PHP to Symfony 2, which is quite good and may help you a lot with the concept. Although is Symfony2, the main point is grab the idea and try to separate the code concepts as much as possible, trying to reuse it.


urllib proxy access function Several of my Python 2.7 programs need to access server tasks via our server Apache instance.
The 'client' programs will run on Windows in various environments. They may be run on systems that do not use a proxy, or on systems that use a proxy with no authentication, or enterprise systems that use an authenticated proxy server.

The function below is meant to address all three proxy situations.

My main question is whether I have coverage for requirement for all scenarios - i.e. for all possible proxy configurations.  I am not familiar with, for example, the kind of proxy authentication that might be used on Windows Active Directory systems - is it different than what my code can handle?

I'm also interested in suggestions for general improvement of the code.  The exception handlers are incomplete - I'll work on them once I'm sure the code is ok.

def openUrl(url, data=None):
    """ General purpose URL opener
    Handles no proxy - un-authenticated proxy and authenticated proxy

    urlencoding of the data argument to be performed prior to invoking this function

    :param url: The url to access
    :param data: Post data to accompany the url request
    :return: an open urllib object
    """
    # case 1: Proxy not required as no PROXY_URL found in environment.  Fall through to urlopen
    if os.getenv('PROXY_URL') is None:
        pass

    # case 2: un-authenticated proxy, just load the proxy handler and then fall through to urlopen
    elif os.getenv('PROXY_USER') is None:
        proxy_support = urllib2.ProxyHandler({})
        opener = urllib2.build_opener(proxy_support)
        urllib2.install_opener(opener)

    # case 3:  Authenticated proxy.  Load password manager and proxyHandler, the fall through to urlopen
    else:
        password_mgr = urllib2.HTTPPasswordMgrWithDefaultRealm()
        password_mgr.add_password(None, os.getenv('PROXY_URL'), os.getenv('PROXY_USER'), os.getenv('PROXY_PASSWORD'))
        auth_handler = urllib2.ProxyBasicAuthHandler(password_mgr)
        opener = urllib2.build_opener(auth_handler)
        urllib2.install_opener(opener)

    # Access url using proxy defined above (or no proxy at all)
    try:
        connection = urllib2.urlopen(url, data)
    except urllib2.URLError, err:
        print(repr(err))
        exit()
    except urllib2.HTTPError, err:
        print(repr(err))
        exit()

    return connection

The function should be called open_url according to Python coding
conventions.
The if/pass combination isn't so nice, it's just more to read.
I'd nest the conditions instead.
I don't get the exception handler.  Why exit hard?  Maybe there's a
short network outage and then the program just shuts down?  Let other
parts of the program handle those scenarios.  They need to deal with
urllib2 exceptions anyway.
PROXY_PASSWORD isn't verified, does None work as a value?

Otherwise looks fine; I'd suggest caching those handlers and only
installing them once, e.g.:

def install_open_url_handlers():
    # case 1: Proxy not required as no PROXY_URL found in environment.  Fall through to urlopen,
    # otherwise:
    proxy_url = os.getenv('PROXY_URL')
    if proxy_url is not None:
        proxy_user = os.getenv('PROXY_USER')

        # case 2: un-authenticated proxy, just load the proxy handler and then fall through to urlopen
        if proxy_user is None:
            handler = urllib2.ProxyHandler({})
        # case 3: Authenticated proxy.  Load password manager and proxyHandler, the fall through to urlopen
        else:
            password_mgr = urllib2.HTTPPasswordMgrWithDefaultRealm()
            password_mgr.add_password(None, proxy_url, proxy_user, os.getenv('PROXY_PASSWORD'))
            handler = urllib2.ProxyBasicAuthHandler(password_mgr)

        urllib2.install_opener(urllib2.build_opener(handler))

HANDLER_INSTALLED = False

def open_url(url, data=None):
    """ General purpose URL opener
    Handles no proxy - un-authenticated proxy and authenticated proxy

    urlencoding of the data argument to be performed prior to invoking this function

    :param url: The url to access
    :param data: Post data to accompany the url request
    :return: an open urllib object
    """
    global HANDLER_INSTALLED
    if not HANDLER_INSTALLED:
        HANDLER_INSTALLED = False
        install_open_url_handlers()

    # Access url using proxy defined above (or no proxy at all)
    return urllib2.urlopen(url, data)


Find common factor of numbers in a list This program calculates common factor for given numbers. The original use case is to find a possible resolution for pixel games that scale well with new screen resolutions (that's why I don't have any check and optimizations for big numbers and use simple brute force).

(define (fun1 l d)
    (if (apply = (map (lambda (x) (remainder x d)) l))
        (print d)))

(define (fun2 l)
    (do ((i 1 (+ i 1)))
        ((= i (apply min l)) i)
        (fun1 l i)))

(fun2 `(1080 720))

I'm learning lisp and scheme and want to know if there's any way to make this code more functional and lispy.

Also, I'm wondering if (apply = (1 2 3) is the right way to do it. I haven't found another way to force feed list of numbers to the = function or break numbers out of the list.

Scheme has a built-in function called gcd, which computes the greatest common divisor. You don't need to write your own version. (gcd 1080 720) returns 360.

One other thing I should mention is that, as a matter of style, you should prefer to use quote rather than backquote if you have nothing to unquote.


Small football table builder in Ruby Please review this small program I wrote to create a World Cup Group Table. You feed the program match info, and it builds a table based on that info.

  def table_for(matches)
    # Defaults that each team in the table should have
    defaults = { goals_for: 0, points: 0 }

    # Using the matches, let's create a hash for each team involved in a match
    teams = matches.each_with_object([]) do |match, array|
      array.push({ name: match[:home_team] }.merge!(defaults))
      array.push({ name: match[:away_team] }.merge!(defaults))
    end

    # We have 6 matches; each match involves 2 teams; we have 12 teams in our array.
    # Each team plays 3 matches, so we have 4 unique.
    teams.uniq

    # Loop through matches to build the table.
    matches.each do |match|

      # We only want to calculate data for matches that were played
      if match[:home_score].present? && match[:away_score].present?

        # Grab the teams for this particular match from the list of teams
        home_team = teams.detect { |team| team[:name] == match[:home_team] }
        away_team = teams.detect { |team| team[:name] == match[:away_team] }

        # Compute goals_for
        home_team[:goals_for] += match[:home_score]
        away_team[:goals_for] += match[:away_score]

        # Home team wins
        if match[:home_score] > match[:away_score]
          home_team[:points] += 3

        # Away team wins
        elsif match[:home_score] < match[:away_score]
          away_team[:points] += 3

        # It's a draw
        else
          home_team[:points] += 1
          away_team[:points] += 1
        end
      end
    end

    teams
  end

The rules are a win earns three points, a draw one point, a loss zero points.

For example, if we feed the method this match data:

[
  { id: 1, home_team: "Honduras", away_team: "Chile", home_score: 0, away_score: 1 },
  { id: 2, home_team: "Spain", away_team: "Switzerland", home_score: 0, away_score: 1 },
  { id: 3, home_team: "Chile", away_team: "Switzerland", home_score: 1, away_score: 0 },
  { id: 4, home_team: "Spain", away_team: "Honduras", home_score: 2, away_score: 0 },
  { id: 5, home_team: "Chile", away_team: "Spain", home_score: 1, away_score: 2 },
  { id: 6, home_team: "Honduras", away_team: "Switzerland", home_score: 0, away_score: 0 }
]

We should see this table:

| TEAM        | GF | POINTS |
| Spain       | 4  | 6      |
| Chile       | 3  | 6      |
| Switzerland | 1  | 4      |
| Honduras    | 0  | 0      |

How can I improve this? If there a better way to get the unique teams from the match info? That parts looks convoluted. I also feel that the method is too large.

Note that uniq is not in-place, uniq! is. Otherwise, I guess your code is pretty good if you like imperative style (maybe some unnecessary comments could be removed, though). Personally, I prefer a functional approach: less statements, more expressions. If you open a console and play with the input data, applying transformations, you'll see it's not as difficult as it would seem at first. In this case I'd write:

goals_cmp_to_points = {-1 => 0, 0 => 1, 1 => 3}
team_results = match_results.flat_map do |result|
  [[:home, :away], [:away, :home]].map do |side1, side2|
    goals_cmp = result[:"#{side1}_score"] <=> result[:"#{side2}_score"]
    {
      :team => result[:"#{side1}_team"],
      :goals => result[:"#{side1}_score"],
      :points => goals_cmp_to_points[goals_cmp],
    }
  end
end

#[{:team=>"Honduras", :goals=>0, :points=>0},
# {:team=>"Chile", :goals=>1, :points=>3},
# [...]
# {:team=>"Switzerland", :goals=>0, :points=>1}]

table = team_results.group_by { |r| r[:team] }.map do |team, results_for_team|
  {
    :team => team,
    :total_goals => results_for_team.map { |r| r[:goals] }.reduce(0, :+),
    :total_points => results_for_team.map { |r| r[:points] }.reduce(0, :+),
  }
end
sorted_table = table.sort_by { |tr| tr.values_at(:total_points, :total_goals) }.reverse

#[{:team=>"Spain", :total_goals=>4, :total_points=>6},
# {:team=>"Chile", :total_goals=>3, :total_points=>6},
# {:team=>"Switzerland", :total_goals=>1, :total_points=>4},
# {:team=>"Honduras", :total_goals=>0, :total_points=>1}]


Simple tkinter GUI that uses a popup menu class I'm new to OOP and not sure if I'm importing and using my class correctly.

I'm writing a simple app called MyTestApp that uses a tkinter GUI.
I have a textbox widget and I created a popup menu that is bound to that widget.

But my code started to look a little messy so I decided to move all the popup related methods and variables to its own class. And also decided to write it that I can reuse my popup class for other projects in the future that will require a popup menu.

I instantiate my popup class like this: self.popup = p(parent)

Then I assign the self.popup.textbox_text instance variable in the popup class to the self.data_textbox instance variable in my MyTestApp class like so:

self.popup.textbox_text = self.data_textbox

Everything works correctly, but I'm not sure if this is the correct way of doing it. I initially was thinking that I should pass self.data_textbox as a parameter when instantiating the class but was having a hard time to get it too work like that.

Any tips or advice I can get will be greatly appreciated. I would prefer to do things the correct way from the start.

main.py:

import tkinter as tk
from tkinter import Tk, ttk, Frame, FALSE, Text, Button, \
    Scrollbar, Entry, END, INSERT
from popup import Popup as p

class MyTestApp(Frame):

    def __init__(self, parent, *args, **kwargs):
        Frame.__init__(self, parent, *args, **kwargs)
        self.root = parent
        self.root.title('MY Test App')
        self.grid(column=0, row=0, sticky='nsew', padx=12, pady=5)

        self.data_textbox = Text(self, borderwidth=2, relief='sunken')
        self.data_textbox.config(height=30, width=80)
        self.data_textbox.grid(row=1, column=0, sticky="new")
        self.exit_btn = ttk.Button(self, text='Exit', command=self.on_exit)
        self.exit_btn.grid(row=2, column=0, sticky='W', pady=15)

        ## Not sure if im doing the below part correct
        ## Instantiate my popup class
        self.popup = p(parent)
        ## Assign self.popup.textbox_text to self.data_textbox
        self.popup.textbox_text = self.data_textbox
        # Add imported popup menu and bind to textbox widget
        self.data_textbox.bind("<Button-3>", self.popup.text_popup)

    # Exit the program. Linked to the Exit Button
    def on_exit(self):
        self.root.destroy()

def main():
    root = Tk()
    MyTestApp(root)
    root.mainloop()

if __name__ == '__main__':
    main()

popup.py:

import tkinter as tk
from tkinter import Frame, Entry, END, INSERT

class Popup(Frame):
    def __init__(self, parent, *args, **kwargs):
        Frame.__init__(self, parent, *args, **kwargs)

        ### Not sure if this below part is correct
        ### Must it rather be set from the init parameter?
        self.textbox_text = ''

        self.text_widget = tk.Menu(self, tearoff=0, relief='sunken')
        self.text_widget.add_command(label="Copy", command=self.text_copy)
        self.text_widget.add_separator()
        self.text_widget.add_command(label="Paste", command=self.text_paste)
        self.text_widget.add_separator()
        self.text_widget.add_command(label="Clear", command=self.text_clear)

    def text_popup(self, event):
        self.text_widget.post(event.x_root, event.y_root)

    def text_copy(self, event=None):
        self.clipboard_clear()
        text = self.textbox_text.get("sel.first", "sel.last")
        self.clipboard_append(text)

    def text_paste(self):
        self.textbox_text.insert(INSERT, self.clipboard_get())

    def text_clear(self):
        self.textbox_text.delete(1.0, END)

Fixing the bug

First of all, I would like to address an issue you did not ask about. It is related to the situation where the user tries to copy without selecting any text. That is exactly the scenario I faced. This situation triggers an exception:

Exception in Tkinter callback
Traceback (most recent call last):
  File "/usr/lib/python3.5/tkinter/__init__.py", line 1553, in __call__
    return self.func(*args)
  File "/home/begueradj/Desktop/popupkopy.py", line 26, in text_copy
    text = self.textbox_text.get("sel.first", "sel.last")
  File "/usr/lib/python3.5/tkinter/__init__.py", line 3095, in get
    return self.tk.call(self._w, 'get', index1, index2)
_tkinter.TclError: text doesn't contain any characters tagged with "sel"

To fix this issue, simply check if the user selected a text and then allow him to perform the copy action. This is something you can express within the body of text_copy() function as follows:

def text_copy(self, event=None):
    if self.textbox_text.tag_ranges("sel"):
        self.clipboard_clear()
        text = self.textbox_text.get("sel.first", "sel.last")
        self.clipboard_append(text)

Good decision?

  But my code started to look a little messy so I decided to move all
  the popup related methods and variables to its own class. And also
  decided to write it that I can reuse my popup class for other projects
  in the future that will require a popup menu.

A part of the software design, consists in identifying entities (objects/classes) of the problem you are trying to resolve. Thinking of the popup men as a class in itself is a right decision.

Discussing the implementation from the OOP perspective

Inside the Popup() class, you can get rid of this declaration self.textbox_text = '' as it is useless (never used, unlike what you may think) and a bit confusing. If you remove that variable, your program will still function correctly.
I say self.textbox_text = '' is useless because what you actually are using in all Popup() functions is rather the attribute you added to the instance of Popup() inside MyTestApp() when you coded: self.popup.textbox_text = self.data_textbox. That is also why it is confusing.
The remark 2 above means also that MyTestApp() and Popup() classes are not compliant with the OOP philosophy because the first one must not be responsible of "expanding" the list of attributes of the second one. Objects have rather a state (Python attributes, in our case) and a behavior defined through an interface (not the interface in the strict OOP concepts) consisting of a set of methods and functions. The communication between objects occurs through their interfaces (functions/methods). And that is exactly why I am telling you this line: self.popup.textbox_text = self.data_textbox does not comply with the OOP philosophy.
How to fix the issue highlighted via the remark 3 above? Well, by just respecting the principles: a MyTestApp() instance must communicate with a Popup() instance (self.popup, in your case) through methods. I mean, you must find a way to communicate self.data_textbox to self.popup object through a suitable method. This is too easy and clean as you will see in the improved version below.

Few other remarks

The previous section, especially the remark 4 above, responds fully to your questions. However, I would like to add few improvements suggestions to your code which I think you need to take in consideration as they are the widely respected standard. I will list some of them briefly:

Avoid wildcard imports. This is not what I recommend, but what PEP 8 asks us to do:

  Wildcard imports (from  import *) should be avoided, as they
  make it unclear which names are present in the namespace, confusing
  both readers and many automated tools. There is one defensible use
  case for a wildcard import, which is to republish an internal
  interface as part of a public API (for example, overwriting a pure
  Python implementation of an interface with the definitions from an
  optional accelerator module and exactly which definitions will be
  overwritten isn't known in advance).

  When republishing names this way, the guidelines below regarding
  public and internal interfaces still apply.

What does this mean in practice? It simply means you should remove these 2 lines: from tkinter import Tk, ttk, Frame, FALSE, Text, Button, \
    Scrollbar, Entry, END, INSERT and from tkinter import Frame, Entry, END, INSERT and be content with import tkinter as tk. This way you avoid useless imports you never used, but also you avoid repetition of the imports and, more importantly, in case you need to create a custom class/widget having the same name of one of the tkinter's classes, you will not face names clashes issues.

Choose the right names: while this rule sounds easy to understand, not lot of us respect it even if it is too important for the development process. In my solution below, I renamed several of your variables.
Last but not the least, follow Tkinter best practices.

Improved version

Taking in consideration all what is mentioned through the above sections, I came up with this improved version of your code (which surely can be subject to further improvements):

main.py:

from tkinter import ttk
import tkinter as tk
from popup import RightClickMenu

class MyTestApp(tk.Frame):
   def __init__(self, parent):
       self.master = parent
       tk.Frame.__init__(self, self.master)
       self.configure_gui()
       self.create_widgets()
       self.bind_right_click_menu_to_typing_area()

   def configure_gui(self):
       self.master.title('MY Test App')

   def create_widgets(self):
       self.create_text_area()
       self.create_exit_button()

   def create_text_area(self):
       self.text_area = tk.Text(self.master, borderwidth=2, relief='sunken')
       self.text_area.config(height=30, width=80)
       self.text_area.grid(row=0, column=0, sticky="new")

   def create_exit_button(self):
       self.exit_btn = ttk.Button(self.master, text='Exit', command=self.exit_application)
       self.exit_btn.grid(row=1, column=0, sticky='W', pady=15)

   def bind_right_click_menu_to_typing_area(self):
       self.popup = RightClickMenu(self.master, self.text_area)
       self.text_area.bind("<Button-3>", self.popup.popup_text)

   def exit_application(self):
       self.master.destroy()

def main():
   root = tk.Tk()
   MyTestApp(root)
   root.mainloop()

if __name__ == '__main__':
   main()

popup.py:

import tkinter as tk

class RightClickMenu(tk.Frame):
   def __init__(self, parent, text_area):
       self.master = parent
       tk.Frame.__init__(self, self.master)
       self.text_area = text_area
       self.create_widgets()

   def create_widgets(self):
       self.create_right_click_menu()

   def create_right_click_menu(self):
       self.right_click_menu = tk.Menu(self.master, tearoff=0, relief='sunken')
       self.right_click_menu.add_command(label="Copy", command=self.copy_text)
       self.right_click_menu.add_separator()
       self.right_click_menu.add_command(label="Paste", command=self.paste_text)
       self.right_click_menu.add_separator()
       self.right_click_menu.add_command(label="Clear", command=self.clear_text)

   def popup_text(self, event):
       self.right_click_menu.post(event.x_root, event.y_root)

   def copy_text(self):
       if self.text_area.tag_ranges("sel"):
           text = self.text_area.get("sel.first", "sel.last")
           self.clipboard_append(text)

   def paste_text(self):
       self.text_area.insert(tk.INSERT, self.clipboard_get())

   def clear_text(self):
       self.text_area.delete(1.0, tk.END)


First SO chat message in Haskell followup I have wrote an application which asks for the user id, and returns a link to the first available chat message:

Output

  Enter user id:
  322395
  Fetching data...
  http://chat.stackoverflow.com/transcript/17?m=231895#231895

Source code

module Main where

import Network.HTTP (simpleHTTP, getRequest, getResponseBody)
import Text.HTML.TagSoup
import Data.List (isInfixOf, isPrefixOf)

type Link   = String
type UserID = String

hostname = "http://chat.stackoverflow.com"

getDivText :: String -> [Tag String] -> String
getDivText m = innerText
             . takeWhile (~/= "
")
             . dropWhile (~/= m)

getURLContent :: Link -> IO String
getURLContent lnk = simpleHTTP (getRequest lnk) >>= getResponseBody

getMessages :: UserID -> IO Integer
getMessages uid = do

    contents <- getURLContent $ concat [ hostname
                                       , "/users/"
                                       , uid ]

    let count = getDivText "
"
              $ (parseTags contents)

    return (read count)

getFirstMessagePage :: UserID -> IO String
getFirstMessagePage uid = searchLast "" . ceiling . (/100)
                      =<< (fmap fromIntegral $ getMessages uid) where

    searchLast body pg = do

        let lnk = concat [ hostname
                         , "/users/"
                         , uid
                         , "/?tab=recent&pagesize=100&page="
                         , show pg ]

        body' <- getURLContent lnk

        if "monologue" `isInfixOf` body'
            then return body'
            else searchLast body (pg - 1)

getFirstMessage :: UserID -> IO Link
getFirstMessage ui = do

    page <- getFirstMessagePage ui

    let tags = filter step $ parseTags  page
        lnk  = fromAttrib "href" $ last tags

    return $ concat [ hostname, lnk ]

    where step t = isTagOpen t
                 && "/transcript" `isPrefixOf` fromAttrib "href" t

main :: IO ()
main = do

    putStrLn "Enter user id:"
    uid <- getLine

    putStrLn "Fetching data..."
    lnk <- getFirstMessage uid

    putStrLn lnk

I tried to keep the HTTP requests to a minimum.

If the user has removed messages, than there will be a difference between the pages available and the number of messages in the profile.

To solve this, the app recursively lowers the page number till it finds the last message.

body is unused in searchLast.

I don't like explicit recursion as it is used in searchLast. MaybeT has the correct behavior for what you want.

ala X turns a function like foldMap into a function like fold by applying the newtype constructor X to it and unwrapping the result of the resulting function so that it looks like the type wrapped in the newtype has all the instances defined on the newtype.

alaf X works the same, except that it turns a function like foldMap into a function like foldMap, so that further ala-functions can be applied.

Control.Lens.Wrapped (ala)

Alt is the newtype wrapper that gives Alternatives like Maybe a Monoid instance, therefore ala Alt foldMap would be asum, and alaf Alt foldMap is something like "asumMap", being to asum as foldMap is to fold.

(ala MaybeT . alaf Alt) foldMap = ala MaybeT (alaf Alt foldMap) = ala MaybeT asumMap (except that asumMap doesn't exist) takes [m (Maybe a)] and combines it into m (Maybe a) using MaybeT's Alternative instance.

getFirstMessagePage :: UserID -> IO String
getFirstMessagePage uid = (ala MaybeT . alaf Alt) foldMap
    . map fetchMonologue
    . reverse . enumFromTo 0 . ceiling . (/100)
    =<< (fmap fromIntegral $ getMessages uid) where

    fetchMonologue :: Int -> IO (Maybe String)
    fetchMonologue pg = do

        let lnk = concat [ hostname
                         , "/users/"
                         , uid
                         , "/?tab=recent&pagesize=100&page="
                         , show pg ]

        body' <- getURLContent lnk

        return $ mfilter (isInfixOf "monologue") $ Just body'