A basic, modern, idiomatic GNU makefile I have prepared a small makefile for a small project, but I am trying to find what would be the best practices; up to now I have gathered these:

relatively idiomatic (typical structures, flows, var names for Least Astonishment)
define the products in terms of the inputs (if I have .c files and want an exec, why should I think about intermediate files?)
up-to-date with GNU make's and gcc's capabilities (auto-dependency tracking)
silent by default
but can be verbose (for Eclipse integration for example)
keeps object files in directories separate from sources
avoids implicit rules (explicit is clearer and anyway I wanted to mute the output)

Also a couple of make-quirk-addressing details like immediate assignment :=, order-only prerequisites…

So here's the makefile:

WARNINGS := -Wall -Wextra
CFLAGS ?= -std=gnu99 -g $(WARNINGS)

OBJDIR := obj
SRCDIR := src

# silent by default
ifeq ($(VERBOSE),1)
    SILENCER :=
else
    SILENCER := @
endif

# pass this environment variable to the C source
ifeq ($(DEBUG_BUILD),1)
    CFLAGS +=-DDEBUG_BUILD
endif

# typical way to list files and build full paths
# list the sources, not the object files (nor includes)
_SRCS := uthreads.c main.c
SRCS := $(patsubst %,$(SRCDIR)/%,$(_SRCS))
OBJS := $(patsubst %,$(OBJDIR)/%,$(_SRCS:c=o))

# generate phony deps during compilation
CFLAGS += -MMD -MP
DEPS := $(patsubst %,$(OBJDIR)/%,$(_SRCS:c=d))
# can't include the deps before the first, default target has appeared!

# "all" is the classical default target
all: main

createdir:
        $(SILENCER)mkdir -p $(OBJDIR)

main: $(OBJS)
        #@echo " LINK $^"
        $(SILENCER)$(CC) $(CFLAGS) -o $@ $^

# put object and dependency files away from the sources
# create the dir before building into it
$(OBJDIR)/%.o: $(SRCDIR)/%.c | createdir
        #@echo " CC $<"
        $(SILENCER)$(CC) $(CFLAGS) -c -o $@ $<

clean:
        $(SILENCER)$(RM) -f *~ core main
        $(SILENCER)$(RM) -r $(OBJDIR)

.PHONY: clean all

# dependencies won't turn into a default target here
-include $(DEPS)

I'll be grateful for any criticism or addition to either the list of "best practices" or to the makefile itself.

Thanks!

You should be able to simplify the definition of DEPS to

DEPS := $(OBJS:.o=.d)

For small projects that don't require complicated configuration schemes, it's often times easiest to get the list of source files by wildcard instead of manually maintaining an explicit list:

SRCS := $(wildcard $(SRCDIR)/*.c)
OBJS := $(patsubst $(SRCDIR)/%.c,$(OBJDIR)/%.o,$(SRCS))
DEPS := $(OBJS:.o=.d)

That way, you won't have to change the makefile when you add, delete, or rename source files.

Since all of your object files are going into the same output folder, you only need to create the output folder when it does not exist.  You are currently calling mkdir for every C file that gets compiled.  This won't break your build, but it's unnecessary overhead and doesn't scale well.  You should be able to simplify to something like this:

$(OBJDIR)/%.o: $(SRCDIR)/%.c
    #@echo " CC $<"
    $(SILENCER)$(CC) $(CFLAGS) -c -o $@ $<

$(OBJS): | $(OBJDIR)
$(OBJDIR):
    $(SILENCER)mkdir -p $(OBJDIR)

The second line of your recipe for "clean" should add the "-f" flag to avoid error messages when $(OBJDIR) doesn't exist.


Simplify this document.referrer check Can this be simplified in any way?

if ($('.content').length && (document.referrer.search('page1.html') > 0) ||
    $('.content').length && (document.referrer.search('page2.html') > 0) ||
    $('.content').length && (document.referrer.search('page3.html') > 0) ||
    $('.content').length && (document.referrer.search('page4.html') > 0) ||
    $('.content').length && (document.referrer.search('page5.html') > 0) ||
    $('.content').length && (document.referrer.search('page6.html') > 0) ||
    $('.content').length && (document.referrer.search('page7.html') > 0) ||
    $('.content').length && (document.referrer.search('search/?') > 0)) {
        window.location.href = document.referrer;
};

Easily! You can use a regular expression to match page[1-7].html and also search/?:

if ($('.content').length && document.referrer.match(/page[1-7]\.html|search\/\?/)) {
        window.location.href = document.referrer;
};

And never write conditions in such wasteful way:

if (A && B || A && C || A && D) {

when it can be easily simplified to:

if (A && (B || C || D)) {


Allocation and reallocation of memory I am programming a parallel tree algorithm where I have to send data to other processors. The amount of data is not constant. Therefore I have to work with dynamic arrays using malloc() and realloc(). Since I am new to C I wondered if I am doing it right. I do not get any error. But I think that is not proof enough, that it works all the time. How could I really test this code? Do I have memory leaks? Do I occupy to much memory with this method?

Here I have an array containing 10 double values. Then I want add 3 new double values to that array. Finally I want to delete the whole array.

Here is my code:

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

int main(){
    int n = 10;
    double *p;

    p = malloc(sizeof(*p)*n); // similar to int array[n]
    if(p==NULL){
        printf("Error! Memory not allocated.");
        return 1;
    }
    for(int i=0; i<n; i++)
        printf("%lf\n", p[i]);
    printf("\n");

    int add = 3;
    double *temp;
    temp = realloc(p,(n+add)*sizeof(*temp));
    if(temp != NULL){
        p = temp;
    }else{
        free(p);
        printf("Error! Memory not reallocated\n");
        return 1;
    }
    for(int i=0; i<n+add; i++)
        printf("%lf\n", p[i]);

    return 0;
    free(p);
}

I tried to check the size of the arrays before and after reallocation. But it seems to me that this is not possible in C.

If you swap the last two lines, so you actually free the memory before returning, then the code looks to be leak-free in the face of errors - well done!  You do read uninitialized values from the allocated memory; don't do that even in a test program (Valgrind complains a lot).

Some notes:

This reads easier if you re-order the multiplication:

p = malloc(sizeof(*p)*n); // yours

p = malloc(n * sizeof *p); // mine

(BTW, it's good that you are using sizeof *p rather than sizeof (double), as this means no risk of the type becoming out of step with the size allocated.  Definitely a good practice to be encouraged!).

Testing pointers against NULL is more idiomatic if you use the default conversion to boolean:

if(p==NULL){ // yours

if (!p) { // mine

Error messages should go to standard error, not standard output, and should end with a newline:

    printf("Error! Memory not allocated."); // yours

    fprintf(stderr, "Error! Memory not allocated.\n"); // mine

The reallocation can be simplified in a similar manner to the initial allocation; I'd declare and allocate in a single line like this:

double *temp; // yours
temp = realloc(p,(n+add)*sizeof(*temp)); // yours

double *temp = realloc(p, (n+add) * sizeof *temp); // mine

Complete program

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

int main() {
    int n = 10;

    double *p = malloc(n * sizeof *p);
    if (!p) {
        fprintf(stderr, "Error! Memory not allocated.\n");
        return 1;
    }

    /* initialize these values */
    for (int i = 0;  i < n;  ++i)
        p[i] = i;

    for (int i = 0;  i < n;  ++i)
        printf("%lf\n", p[i]);
    printf("\n");

    int add = 3;
    double *temp = realloc(p, (n+add) * sizeof *temp);
    if (temp) {
        p = temp;
    } else {
        free(p);
        fprintf(stderr, "Error! Memory not reallocated.\n");
        return 1;
    }

    /* initialize the new values */
    for (int i = n;  i < n+add;  ++i)
        p[i] = 100 + i;

    for (int i = 0;  i < n+add;  ++i)
        printf("%lf\n", p[i]);

    free(p);
    return 0;
}

If we build this with my usual Makefile and run it in Valgrind, we see no leaks:

gcc -std=c11 -fPIC -g -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds 166874.c -o 166874
valgrind --leak-check=full ./166874

==30432== Memcheck, a memory error detector
==30432== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==30432== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==30432== Command: ./166874
==30432==
0.000000
1.000000
2.000000
3.000000
4.000000
5.000000
6.000000
7.000000
8.000000
9.000000

0.000000
1.000000
2.000000
3.000000
4.000000
5.000000
6.000000
7.000000
8.000000
9.000000
110.000000
111.000000
112.000000
==30432==
==30432== HEAP SUMMARY:
==30432==     in use at exit: 0 bytes in 0 blocks
==30432==   total heap usage: 3 allocs, 3 frees, 1,208 bytes allocated
==30432==
==30432== All heap blocks were freed -- no leaks are possible
==30432==
==30432== For counts of detected and suppressed errors, rerun with: -v
==30432== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)


Does Java need an Observable object with generics? This is the follow-up question from here.

This is an Observable class similar to java.util.Observable. The difference is that it uses generics to avoid casts.

There are two questions:

Is it really worth the effort? or it's just making things more complex than needed?
Should this class be declared final, or is it ok to be subclassed? java.util.Observable is not final.

package cc.koosha.silkroad.lang;
import java.util.ArrayList;
import java.util.Collection;

/**
 * like java.util.Observable, But uses generics to avoid need for a cast.
 * For any un-documented variable, parameter or method, see java.util.Observable
 */
public class Observable<T> {
    public interface Observer<T> {
        public void update(Observable observable, T args);
    }

    /**
     * Holds registered observers.
     * @TODO if class is not final can we make this protected? (should we?).
     */
    private final Collection<Observer<? super T>> registry;
    private boolean changed = false;

    /**
     * @TODO should this be private and the class final?.
     */
    protected Observable(final Collection<Observer<? super T>> registry) {
        this.registry = registry;
    }

    /**
     * Static constructor.
     */
    public static <F> Observable<F> getInstance() {
        ArrayList<Observer<? super F>> container = new ArrayList<Observer<? super F>>();
        return new Observable<F>(container);
    }

    // @TODO Should o be checked for null?
    public void addObserver(final Observer<? super T> o) {
        synchronized (this.registry) {
            if (!this.registry.contains(o))
                this.registry.add(o);
        }
    }

    protected void update(final T args) {
        ArrayList<Observer<? super T>> toNotify;

        synchronized (this.registry) {
            if (!changed)
                return;

            toNotify = new ArrayList<Observer<? super T>>(this.registry);
            this.changed = false;
        }
        // Look at OpenJDK to see why a new Collection is used.
        for (Observer<? super T> o : toNotify)
            o.update(this, args);
    }

    public void notifyObservers(final T args) {
        this.update(args);
    }

Methods deleteObserver, deleteObservers, setChanged, clearChanged, hasChanged, countObservers, notifyObservers are removed for brevity.

The rest of the code is at GitHub.

A few observations.

Your choice of Collection as type for registry and all other things point at it that you want this class to be extended. If it weren't so a few desing choices would have been different.

I still want to review the code as if it were a closed class. Because if it weren't I'd suggest you throw that all away and think about why able things very often are interfaces. Don't make the same design mistakes...

Your registry is overly Generic. Not in choice of generics, but in choice of Interface...

I'd have expected at least a Set instead of Collection. Also your code could greatly benefit from eagerly initializing it. For one it allows you to add a no-argument constructor easily, and secondly it's more conventional.

Let's look at getInstance for a moment.
You create an empty collection to be set as your registry. This is boilerplate code when extending your Observable.
Additionally it makes the behavior of your observable concerning references somewhat unconventional. I'd have expected:

 ... Observable (Collection <Observer <? super T>> registry) {
    this.registry.addAll(registry);
}

And here's already my next problem... why is your constructor private? I think it would be beneficial to allow me as programmer to instantiate Observables myself ;)

And last but not least toNotify in your update method is never written to again. I suggest you use a unmodifiableCollection instead of copying your registry into an ArrayList to clearly express what you actually want


How can I Shorten and Simplify Ajax callback processes? I've reusing this code for multiple areas and will like to make it more short and/or portable. What suggestions or changes could I make to simplify shorten it's callback processes?

        $("#add_question_member").submit(function(){
            $.ajax({
                    type:'post',
                    url: '/posts_in.php',
                    dataType: "json",
                    data: $(this).serialize(),
                    success: function(data){
                        var response = data[0];
                        $("#entry_area").fadeOut(function(){
                            if(response==1){
                                $("#msg0").stop().fadeIn("slow").delay(8000).fadeOut("slow",function(){
                                    $("#entry_area").stop().fadeIn("slow");
                                });

                            }else{
                                $("#msg1").stop().fadeIn("slow").delay(8000).fadeOut("slow",function(){
                                    $("#entry_area").stop().fadeIn("slow");
                                });
                            }
                        });

                    }
                });
            return false;
        });

Here is a jsfiddle where I cleaned it up for you

I did not actually test it so there might be a syntax error here and there but its how you would do it overall.
It might not be 100% but it should get you started as far as the techniques you should look into such as:

Module pattern to create reusable code
Callback or options to support the ability to pass in a simple callback or a more complex object
Defaults with $.extend
Functions returning callbacks

That's a lot of stuff and a bit overkill on this example but I wanted to showcase the techniques for you. Feel free to ask any questions.

A couple additional notes

You generally want to use the expression context parameter to limit your selectors
You probably want to avoid using ids and use classes instead since things get wierd if you accidentally end up with 2 of the same id on the page. This is something that happens more often as you start to make your views more composable. This is something that using the expression context can help with.
submit is only available on forms anyways, you could denote that explicitly $('form#add_question_member')
Do you really want to be returning false?
Consider using the form's action and method properties instead of hardcoding them - $(this).prop('action'), $(this).prop('method') at the same level as you serialize the form

//****This part can be in a seperate script file **********//
(function() { //use an immediately executing function to limit scope
    var submitFormDefaults = { //set up all the defaults to be used the function
        type: 'post',
        url: '/posts_in.php',
        dataType: "json",
        myAppUnWrapData: true //a custom option to unrwap your returned data by default
    };

var standardSubmitForm = function(successFnOrOptions) {
    // This allows your input to be just the success function or a richer object
    var options = $.isFunction(successFnOrOptions) ? {
        success: successFnOrOptions
    } : successFnOrOptions;

    // return the actual submit handler
    return function() {
        var op = $.extend(true, //deep clone
          submitFormDefaults, //use defaults unless overridden
          { data: $(this).serialize() }, //not in the defaults cause it needs access to 'this'
          options //any overrides specified
        );
        if (op.myAppUnWrapData && op.success) { //if unwrap data custom option is truthy and there is a success callback
            var _oldSuccess = op.success;
            op.success = function(data) {
                _oldSuccess(data[0]);
            }
        }
        $.ajax(op);
        return false;
    }
}

//Export the local function to the gloabl namespace using Module pattern
window.MyApp = window.MyApp || {}; //create a global MyApp namespace
$.extend(window.MyApp, { //add some global functions. I prefer to use $.extend rather than directly assigning it - I have no good reason for this
    submitForm: standardSubmitForm //export our function to the global namespace
});

window.MyApp.submitForm.defaults = submitFormDefaults //export the defaults as a global property on this function

})(); //Invoke the above immediately invoking function

//******************** Elsewhere ********************************//
$("#add_question_member").submit(window.MyApp.submitForm(function(response) { //this will return a function handler
    if (response !== 1) {
        $("#msg0").stop().fadeIn("slow").delay(8000).fadeOut("slow", function() {
            $("#entry_area").stop().fadeIn("slow");
        });
    } else {
        $("#msg1").stop().fadeIn("slow").delay(8000).fadeOut("slow", function() {
            $("#entry_area").stop().fadeIn("slow");
        });
    }
}));

// or...
$('form.blah').submit(window.MyApp.submitForm({
    url: '/somewhere/else',
    success: function() {
        console.log('ok', this, arguments);
    }
});

// or...
window.MyApp.submitForm.defaults.action = 'GET'; //changes it globally
$('form.blah').submit(window.MyApp.submitForm({
    url: '/somewhere/else',
    myAppUnWrapData: false,   //keep it from doing the data[0] thing
    success: function(data) {
        console.log('ok', this, arguments);
    }

});


Inserting Data in The Database using LINQ TO SQL Can this Code be any shorter? because it seems it is very long with just simple insertion of data using LINQ TO SQL

 public void addPatientInformation() {

            using(DbClassesDataContext myDb = new DbClassesDataContext()){

                PatientInfo patientInfo = new PatientInfo();

                patientInfo.Phy_ID = physcianID;
                patientInfo.Pat_First_Name = txtFirstName.Text;
                patientInfo.Pat_Middle_Init = txtMiddleName.Text;
                patientInfo.Pat_Last_Name = txtLastName.Text;
                patientInfo.Pat_Gender = cmbGender.Text;
                patientInfo.Pat_Marital_Status = cmbMaritalStatus.Text;
                patientInfo.Pat_Date_Of_Birth = dtpDOB.Value;
                patientInfo.Pat_Home_Add = txtHomeAdd.Text;
                patientInfo.Pat_Home_Num = txtPhone.Text;
                patientInfo.Pat_Work_Add = txtWorkAdd.Text;
                patientInfo.Pat_Work_Num = txtWorkPhone.Text;
                patientInfo.Pat_Prim_Physician = txtPrimPhysician.Text;
                patientInfo.Pat_Ref_Physician = txtRefePhysician.Text;

                myDb.PatientInfos.InsertOnSubmit(patientInfo);
                myDb.SubmitChanges();
        }

Yes. You can use object initializer syntax and bring the amount of text down a bit:

     public void addPatientInformation() {
        using(DbClassesDataContext myDb = new DbClassesDataContext()) {
            myDb.PatientInfos.InsertOnSubmit(new PatientInfo {
                Phy_ID = physcianID,
                Pat_First_Name = txtFirstName.Text,
                Pat_Middle_Init = txtMiddleName.Text,
                Pat_Last_Name = txtLastName.Text,
                Pat_Gender = cmbGender.Text,
                Pat_Marital_Status = cmbMaritalStatus.Text,
                Pat_Date_Of_Birth = dtpDOB.Value,
                Pat_Home_Add = txtHomeAdd.Text,
                Pat_Home_Num = txtPhone.Text,
                Pat_Work_Add = txtWorkAdd.Text,
                Pat_Work_Num = txtWorkPhone.Text,
                Pat_Prim_Physician = txtPrimPhysician.Text,
                Pat_Ref_Physician = txtRefePhysician.Text,
            });
            myDb.SubmitChanges();
        }
    }


Prefer simplicity over testability? Currently I try to create some unit tests for a project, which provides access to some webservice methods.

It's interface is rather simple. I have a class "WebService" which offers methods like "CreatePDF".
Here's a little example:

The Model:
The model contains a number of properties, which each represent a URL parameter of the webservice. The models provide a method "GetParameterDictionary", which delivers a dictionary with key == url parameter name and value == url parameter value.

public class CreatePDFParameters
{
        public List<string> Names { get; set; }
        public List<string> Colors { get; set; }

        public Dictionary<string, string> GetParameterDictionary()
        {
            [...] //returns Key == url parameter name and Value == ";" separated list of values
        }
}

So here's a example for the Interface method:

public byte[] CreatePDF(CreatePDFParameters parameters)
{
    return MakeByteRequest(GetQuery(WebServiceMethods.CreatePDF, parameters.GetParameterDictionary()));
}

/// <summary>
///     Creates a query out of a dictionary. The dictionary must have the paramname as key and the value as value!
/// </summary>
private string GetQuery(string methodName, Dictionary<string, string> parameters)
{
    string parameterString = parameters.Where(parameter => !String.IsNullOrEmpty(parameter.Value))
    .Aggregate(String.Empty, (current, parameter) => String.Format(String.IsNullOrEmpty(current) ? "{0}?{1}={2}" : "{0}&{1}={2}", current, parameter.Key, parameter.Value));
    return methodName + parameterString;
}

As you can see, you only need to call the CreatePDF method with a parameters object. The method itself calls a methods, which creates the query and calls a method, which makes the actual request (It calls directly the ByteRequest method, there are methods like StringRequest (delivers a string as return value) too).

I would like to write a UnitTest for the GetQuery method. But I don't want to make it public or internal in the actual state. I could make the method public If I would remove the "CreatePDF" method and let the user make direkt calls to MakeByteRequest / GetQuery and so on - which would require the users to have some knowledge about the webservice itself (knowledge about return types, web method names and so on).

Would you prefer a more simple "interface" over unit tests in this case?

The WebService interface with a CreatePDF method is good like that.
The private GetQuery method is an implementation detail that should not be exposed.

But there's something else you can do to test GetQuery.
You can move that logic outside to a different class,
whose main responsibility will be to build query strings.
Then the method will be public, naturally, and you can implement unit tests for it.

The query string builder class can become a collaborator of WebService:
implementations of WebService can call it from their GetQuery methods to build query strings correctly,
with the comforting thought that the utility class is properly unit tested.


Sorting algorithms with utility functions and iterators My implementation of insertion, merge, and quick sort, with some utility testing functions. I just wanted to know how I can improve the efficiency and general cleanliness of my code.

(note: I know C++ has partition, but I want to do most of the implementation myself)

In particular, my main algorithms take iterators [first, last], but STL container's v.begin() and v.end() go 1 over the last element, so I have wrappers that call the main algorithm with first and end - 1.

How can I restructure this into the main algorithm (if that would be better)?

#include <iterator>
#include <vector>
using namespace std;

// Insert sort O(n^2) time O(1) space
template <typename Iterator>
void ins_sort(Iterator first, Iterator end) {
    for (Iterator cur = first; cur < end; ++cur) {
        auto key = *cur;
        Iterator ins = cur - 1;
        for (; first <= ins && key < *ins; --ins)
            *(ins + 1) = *ins;    // move 1 to right until correct position's found
        *(ins + 1) = key;
    }
}

// Merge sort O(nlogn) time O(n) space
template <typename Iterator>
void merge(Iterator first, Iterator middle, Iterator last) {
    using T = typename iterator_traits<Iterator>::value_type;
    vector<T> left(first, middle + 1);
    vector<T> right(middle + 1, last + 1);
    left.push_back(0xFFFFFFF); // sentinel
    right.push_back(0xFFFFFFF);
    for (int i = 0, j = 0, k = 0; k <= last - first; ++k) {
        if (left[i] < right[j]) first[k] = left[i++];
        else first[k] = right[j++];
    }
}

template <typename Iterator>
void merge_helper(Iterator first, Iterator last) {
    if (first < last) {
        Iterator pivot = first + (last - first)/2;   // half way point rounded down
        merge_helper(first, pivot);
        merge_helper(pivot + 1, last);
        merge(first, pivot, last);
    }
}

template <typename Iterator>
void mer_sort(Iterator first, Iterator end) {
    merge_helper(first, end - 1);
}

// Quick sort O(nlogn) time O(n) space
template <typename Iterator>
void quick_sort(Iterator first, Iterator last) {
    if (last - first > 0) {
        auto pivot = *(first + (last - first) / 2);
        Iterator left {first};
        Iterator right {last};
        while (left <= right) {
            while (*left < pivot) ++left;
            while (pivot < *right) --right;
            if (left <= right) { swap(*left, *right); ++left; --right; }
        }
        quick_sort(first, right);
        quick_sort(left, last);
    }
}
template <typename Iterator>
void qck_sort(Iterator first, Iterator end) {
    quick_sort(first, end - 1);
}

Testing code

#include <random>
#include <vector>
#include <iostream>
using namespace std;

class Rand_int {
public:
    Rand_int(int low, int high) : distribution{low, high} {
    }
    int operator()() {
        return distribution(engine);
    }

private:
    default_random_engine engine;
    uniform_int_distribution<> distribution;
};

template <typename T>
void print(vector<T> v) {
    for (auto x : v)
        cout << x << ' ';
    cout << '\n';
}

vector<int> randgen(int max, int num) {
    static Rand_int die{0, max};
    vector<int> res(num);
    for (int i = 0; i < num; ++i)
        res[i] = die();
    return res;
}

There are few problems with merge:

There is no guarantee that T can be constructed from an integral constant (how about sorting strings?). Even if it could, 0xFFFFFFF (BTW, what is this number?) is not necessarily a largest possible value (e.g.T could be 64 bit wide, or 16 bit wide); besides this value may legitimately belong to user data. Bottom line it, using an artificial sentinel is wrong.

(left[i] < right[j]) condition breaks merge sort stability (which is very important reason of using merge sort in the first place).

Calculating pivot as  first + (last - first)/2 assumes a random access iterator, which rules out merge sorting linked lists (which is a second important reason for merge sort to be used). Take a look at std::distance and std::advance.
C++ has partition for a reason: it is very important algorithm of its own. I strongly recommend to factor it out as a function, and call it from quick_sort.
I don't see the reason for qck_sort and mer_sort to exist.

Take a look on how all std:: algorithms operate on semi-open ranges. One of the reasons for that is that last (in STL sense) serves as a natural sentinel. For example, merging may look along the lines of

void merge(I first1, I last1, I first2, I last2, O dst)
{
    while ((first1 < last1) && (first2 < last2)) {
        if (*first1 <= *first2) {
            *dst++ = *first1++;
        } else {
            *dst++ = *first2++;
        }
    }
    while (first1 < last1) *dst++ = *first1++;
    while (first2 < last2) *dst++ = *first2++;
}


Updating complex objects I'm mainly looking at usage of Dapper, but in general, any comments relating SQL -> C# object mapping best practices would be appreciated.

I've got two super simple objects I'm working with:

public class Subcategory
{
    public Guid Id { get; set; }
    public Guid CategoryId { get; set; }
    public string Name { get; set; }
}

public class Category
{
    public Category()
    {
        Subcategories = new List<Subcategory>();
    }

    public Guid Id { get; set; }
    public string Name { get; set; }
    public ICollection<Subcategory> Subcategories { get; set; }
}

Id for both of them is set by the db on insert via T-SQL's newsequentialid().

I'm writing repository classes for each of the objects that conform to the following interface (actually implemented via an abstract class):

public interface IMaintanceRepository<T> where T: class
{
    Guid Create(T toCreate);
    T Read(Guid id);
    T Update(T toUpdate);
    void Delete(Guid id);
}

Right now, I'm trying to write the Update method in the Category repository. I have the following written, but I'm not sure if it's the most performant Dapper code as written.

public override Category Update(Category toUpdate)
{
    var deleteRemovedSubcategories = @"
        DELETE FROM dbo.Subcategories
        WHERE CategoryId = @Id
            AND Id NOT IN @Ids";

    var addNewSubcategories = @"
        INSERT INTO dbo.Subcategories (CategoryId, Name)
        VALUES (@CategoryId, @Name)";

    var updateExistingSubcategories = @"
        CREATE TABLE #subcategories (
            Id uniqueidentifier NOT NULL,
            CategoryId uniqueidentifier NOT NULL,
            Name varchar(50) NOT NULL
        );

        INSERT INTO #subcategories VALUES (@Id, @CategoryId, @Name);

        UPDATE s1
        SET s1.CategoryId = s2.CategoryId, s1.Name = s2.Name
        FROM dbo.Subcategories s1
        INNER JOIN #subcategories s2
        ON s1.Id = s2.Id;

        DROP TABLE #subcategories;";

    var updateCategory = @"
        UPDATE dbo.Categories
        SET Name = @Name
        OUTPUT INSERTED.*
        WHERE Id = @Id;

        SELECT * FROM dbo.Subcategories WHERE CategoryId = @Id";

    using (SqlConnection connection = new SqlConnection(connectionString))
    {
        connection.Execute(deleteRemovedSubcategories, new { Id = toUpdate.Id, Ids = toUpdate.Subcategories.Select(s => s.Id) });
        connection.Execute(addNewSubcategories, toUpdate.Subcategories.Where(s => s.Id == Guid.Empty));
        connection.Execute(updateExistingSubcategories, toUpdate.Subcategories);
        using (var multi = connection.QueryMultiple(updateCategory, toUpdate))
        {
            var category = multi.ReadSingle<Category>();
            category.Subcategories = multi.Read<Subcategory>().ToList();
            return category;
        }
    }
}

Is there any suggested improvements to doing something like this?

I was looking at what you had coded here

var deleteRemovedSubcategories = @"
    DELETE FROM dbo.Subcategories
    WHERE CategoryId = @Id
        AND Id NOT IN @Ids";

and it took me a minute to figure out exactly what the Where clause was filtering, what finally gave it away was the name of the C# variable deleteRemovedSubcategories. If it were possible I would change some of these column names and/or T-SQL variables so that it is more clear what you are saying here.


A linked-list implementation I am new to C, and this is my first attempt at creating a linked list.

If anyone has a minute and could point out any errors or bad practices in this small linked list program, it would be really helpful.

note:
I'm using the get_choice function to run a loop to either delete, insert, locate, print-out links, or exit.

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

struct Link_Data {

    int value;

    struct Link_Data *prev;

    struct Link_Data *next;
};

int main(int argc, char *argv[]) {

    char declare[] = "Begin...";
    printf ("%s\n", declare);

    struct Link_Data *head = malloc( sizeof *head );
    head->prev = NULL;
    head->next = NULL;
    head->value = 0;
    get_choice(head);

    return 0;
}

int get_choice(struct Link_Data *head) {

    int choice;
    int value;

    while ( true ) {

        printf ("Enter 1 to delete, 2 to insert, 3 to locate, 4 to print_all, or 0 to quit: ");
        scanf ("%d", &choice);

        switch (choice) {

            case(1):
                printf ("Enter value: ");
                scanf ("%d", &value);
                delete_node(head, value);
                break;

            case(2):
                printf ("Enter value: ");
                scanf ("%d", &value);
                insert_node(head, value);
                break;

            case(3):
                printf ("Enter value: ");
                scanf ("%d", &value);
                locate_node(head, value);
                break;

            case(4):
                printf ("Printing list...\n");
                print_links(head);
                break;

            case(0):
                return 0;
        }
    }
}

int delete_node(struct Link_Data *node, int value) {

    if (node->next == NULL && node->prev == NULL) {

        printf ("List is empty!\n");
        return 0;
    }

    else if (node->next == NULL && node->value != value) {

        printf ("Value %d not in list!\n", value);
        return 0;
    }

    else {
        if (node->value == value && node->prev != NULL) {

            printf ("Deleting %d...\n", value);
            struct Link_Data *prev = node->prev;
            prev->next = node->next;
            free(node);
        }
        else {
            delete_node(node->next, value);
        }
    }
    return 0;
}

int insert_node(struct Link_Data *node, int value) {

    if (node->next == NULL) {

        printf ("Inserting %d...\n", value);
        struct Link_Data *new_link = malloc( sizeof *new_link );

        new_link->value = value;
        new_link->prev = node;
        new_link->next = NULL;

        node->next = new_link;
    }

    else {

        insert_node(node->next, value);
    }

    return 0;
}

int locate_node(struct Link_Data *node, int value) {

    if (node->next == NULL && node->value != value) {

        printf ("Value %d not in list!\n", value);
    }
    else {
        if (node->value == value) {

            printf ("Locating %d...\n", value);
            printf ("Value %d in list!\n", value);
        }
        else {

            locate_node(node->next, value);
        }
    }

    return 0;
}

int print_links(struct Link_Data *node) {

    if (node->prev == NULL && node->next == NULL) {

        printf ("List is empty!\n");
        return 0;
    }
    else {
        if (node->prev != NULL) {

            printf ("Value: %d\n", node->value);
        }
        if (node->next == NULL) {

            return 0;
        }
        else {

            print_links(node->next);
        }
    }

    return 0;
}

I was never a fan of a "fake first element"-approach. And here is why:

Begin...
Enter 1 to delete, 2 to insert, 3 to locate, 4 to print_all, or 0 to quit: 2
Enter value: 17
Inserting 17...
Enter 1 to delete, 2 to insert, 3 to locate, 4 to print_all, or 0 to quit: 3
Enter value: 0
Locating 0...
Value 0 in list!

So, if you're going to do that approach, be careful not to "locate" it or do anything else with it.

My second complaint is that you traverse your list using recursions. This is both slow and memory consuming on large lists. Just imagine a list with 1000 elements. Your insert_node will have 1000 recursive calls, each with its variables node and value on stack, which takes time to create there, and - when done - remove from there. I suggest you do all the list traversing using for or while loops. For example:

int locate_node(struct Link_Data *node, int value) {

    node = node->next; /* skipping the fake first node */

    while (node && node->value != value) node = node->next;

    if (node) {
        printf ("Value %d in the list!\n", value);
        return 1;
    } else {
        printf ("Value %d not in the list!\n", value);
        return 0;
    }
}

Another thing I have added here is the return value. You should either have void functions or make their return values meaningful. I've used 1 as "there is an element in the list" and 0 as "there is no such element in the list".

I'd also suggest removing output from such functions. They should get the job done (delete, insert, locate) and return a meaningful value, while any user output should be done outside of them.

Last, you create a doubly-linked list, but you don't seem to use prev anywhere except when deleting (which can be done without it), and even there you do not use it properly:

Begin...
Enter 1 to delete, 2 to insert, 3 to locate, 4 to print_all, or 0 to quit: 2
Enter value: 17
Inserting 17...
Enter 1 to delete, 2 to insert, 3 to locate, 4 to print_all, or 0 to quit: 2
Enter value: 19
Inserting 19...
Enter 1 to delete, 2 to insert, 3 to locate, 4 to print_all, or 0 to quit: 2
Enter value: 23
Inserting 23...
Enter 1 to delete, 2 to insert, 3 to locate, 4 to print_all, or 0 to quit: 1
Enter value: 19
Deleting 19...
Enter 1 to delete, 2 to insert, 3 to locate, 4 to print_all, or 0 to quit: 1
Enter value: 23
Deleting 23...
Enter 1 to delete, 2 to insert, 3 to locate, 4 to print_all, or 0 to quit: 3
Segmentation fault

The problem is that you do not fix your prev field of a node after the deleted one, so when you delete two consecutive nodes, the second delete works with a prev that was already freed.

I suggest you make a completely new, recursion-less program, using an ordinary singly-linked list, and without a fake header, and then open a new review request. This seems like a decent tutorial for the lists in C.

One final comment: a print_list function (and the option) would really make it easier for you to test your program.


Function to find factors of a number, optionally including 1 and the number itself I have a function for finding factors of a number in Python

def factors(n, one=True, itself=False):
    def factors_functional():
        # factors below sqrt(n)
        a = filter(lambda x: n % x == 0,
                   xrange(2, int(n**0.5)+1))
        result = a + map(lambda x: n / x, reversed(a))
        return result

factors(28) gives [2, 4, 7, 14]. Now i want this function to also include 1 in the beginning of a list, if one is true, and 28 in the end, if itself is true.

([1] if one else []) + a + map(lambda x: n / x, reversed(a)) + ([n] if itself else [])

This is "clever" but inelegant.

if one:
    result = [1] + result
if itself:
    result = result + [n]

This is straight-forward but verbose.

Is it possible to implement optional one and itself in the output with the most concise yet readable code possible? Or maybe the whole idea should be done some other way - like the function factors can be written differently and still add optional 1 and itself?

What about this?

[1] * one + result + [n] * itself


SICP exercise 1.28 - miller-rabin primality test part II This is a follow-up to SICP exercise 1.28 - miller-rabin primality test.

  Exercise 1.28:

  One variant of the Fermat test that cannot be fooled is
  called the Miller-Rabin test (Miller 1976; Rabin 1980). This starts
  from an alternate form of Fermat’s Little Theorem, which states that
  if \$n\$ is a prime number and \$a\$ is any positive integer less than \$n\$, then
  \$a\$ raised to the \$n-1\$-st power is congruent to \$1 \mod n\$.

  To test the
  primality of a number \$n\$ by the Miller-Rabin test, we pick a random
  number \$a < n\$ and raise \$a\$ to the \$n-1\$-st power \$\mod n\$ using the expmod
  procedure. However, whenever we perform the squaring step in expmod,
  we check to see if we have discovered a “nontrivial square root of \$1
\mod n\$,” that is, a number not equal to \$1\$ or \$n-1\$ whose square is
  equal to \$1 \mod n\$.

  It is possible to prove that if such a nontrivial
  square root of \$1\$ exists, then \$n\$ is not prime. It is also possible to
  prove that if \$n\$ is an odd number that is not prime, then, for at least
  half the numbers \$a < n\$, computing \$a^{n-1}\$ in this way will reveal a
  nontrivial square root of \$1 \mod n\$. (This is why the Miller-Rabin
  test cannot be fooled.)

  Modify the expmod procedure to signal if it
  discovers a nontrivial square root of \$1\$, and use this to implement the
  Miller-Rabin test with a procedure analogous to fermat-test. Check
  your procedure by testing various known primes and non-primes.

  Hint:
  One convenient way to make expmod signal is to have it return \$0\$.

Changes:

Avoid redundant modulo computations
Corrected use of base modulo n in expmod to avoid computing extremely large numbers
for any number greater than 2, prevent use of 1 as random base in the expmod because I think it will always return true when the random chosen number is 1. Is this needed?

While fixing my previous code, I already saw a procedure with recursive process that fixed all my problems above. I can't really call it my own, so to keep the spirit of the exercise, I rewrote my horrible recursive process code and turned it into a procedure that generates an iterative process.

Please review my code.

(define (square x) (* x x))

(define (expmod base exp m)
  (define (expmod-iter a base exp)
    (define squareMod (remainder (square base) m))
    (cond ((= exp 0) a)
          ((and (not (= base (- m 1)))
                (not (= base 1))
                (= squareMod 1))
           0)
          ((even? exp)
           (expmod-iter
            a
            squareMod
            (/ exp 2)))
          (else
           (expmod-iter
            (remainder (* a base) m)
            base
            (- exp 1)))))
  (expmod-iter
   1
   base
   exp))

(define (miller-rabin-test n)
  (define (try-it a)
    (if (and (= a 1)
             (> n 2))
        (miller-rabin-test n)
        (= (expmod a (- n 1) n) 1)))
  (try-it (+ 1 (random (- n 1)))))

(define (fast-prime? n times)
  (cond ((= times 0) true)
        ((miller-rabin-test n)
         (fast-prime? n (- times 1)))
        (else false)))

Did I improve on my previous code? Did I follow all the requirements in the exercise this time? How can I improve this code and make it faster?

Stylistically, I'd write it this way:

;; expmode b e m = b^e modulo m
(define (expmod b e m)
  (define (expmod-iter acc b e)      ; iterative accumulation
    (define bsq (remainder (* b b) m))
    (cond ((= e 0) acc)
          ((and (not (= b (- m 1)))
                (not (= b 1))
                (= bsq 1))
           0)
          ((even? exp)
           (expmod-iter acc bsq (/ e 2)))
          (else
           (expmod-iter (remainder (* acc b) m)
                        b
                        (- e 1)))))
  (expmod-iter 1 b e))               ; initial accumulator = 1

(define (miller-rabin-test n)
  (define (try-it b)
    (if (and (= b 1)
             (> n 2))
        (miller-rabin-test n)
        (= (expmod b (- n 1) n)
           1)))
  (try-it (random 1 n)))

(define (fast-prime? n times)
  (or (= times 0)
      (and (> times 0)
           (miller-rabin-test n)
           (fast-prime? n (- times 1)))))

You've corrected the error of not taking the remainder of the square.

There's no need to define a function square just to save one call to *.

You've converted your recursive expmod into iterative code; although I wouldn't call the former "horrible" since its complexity is only logarithmic, so recursion depth is kept in check.

Assuming you're using Racket, (+ 1 (random (- n 1))) can be written simply (random 1 n).

You've kept the awkward encoding for fast-prime? which is more naturally coded directly with the logical connectives.

Will think over some more with regards to its correctness, that you ask about.


Unit testing extension for equality testing I have created an extension for unit-testing my own equality implementations.
Is it a good approach in general to have such extensions for unit testing?
What unit-testing extensions do you use?

public static class EqualityExtensions
    {
        /// <summary>
        /// Checks that all variations Equals method conforms to equality by properties
        /// </summary>
        /// <typeparam name="T"></typeparam>
        /// <param name="variations"></param>
        /// <param name="propertyAccessors">Property accessors specifying which properties contribute to equality</param>
        /// <returns>True, if all instances from a set conform to property equality</returns>
        /// <example>
        /// Assert.IsTrue(from name in new[] {"foo","bar"} from count in new[] {1,2} select new MyClass{Name=name,Count=count}).ConformToPropertyEquality(x=>x.Name,x=>x.Count))
        /// </example>
        public static bool ConformToPropertyEquality<T>(this IEnumerable<T> variations, params Func<T, object>[] propertyAccessors)
        {
            var propertyComparisions =
                from lhs in variations
                from rhs in variations
                select
                    Equals(
                        (from comparator in propertyAccessors select Equals(comparator(lhs), comparator(rhs))).All(c => c),
                        Equals(lhs, rhs)
                    );

            return propertyComparisions.All(p => p);
        }
    }

It's perfectly OK. As an example SharpTestEx is a whole library with such an extensions.


Creating an Employee Punch Clock I'm trying to create a way to clock in and clock out at work using python and tkinter. There are several features to be implemented.

Change the Employee Name
Clock In button
Clock Out button
Export file with the information

I have all the components and they work when I run the different features individually. When I try to combine to one script, the program fails.

import tkinter as tk
import time

class StopWatch(Frame):
    """ Implements a stop watch frame widget. """
    def __init__(self, parent=None, **kw):
       Frame.__init__(self, parent, kw)
    self._start = 0.0
    self._elapsedtime = 0.0
    self._running = 0
    self.timestr = StringVar()
    self.makeWidgets()

    def makeWidgets(self):
        """ Make the time label. """
        l = Label(self, textvariable=self.timestr)
        self._setTime(self._elapsedtime)
        l.pack(fill=X, expand=NO, pady=2, padx=2)

    def _update(self):
        """ Update the label with elapsed time. """
        self._elapsedtime = time.time() - self._start
        self._setTime(self._elapsedtime)
        self._timer = self.after(50, self._update)

    def _setTime(self, elap):
        """ Set the time string to Minutes:Seconds:Hundreths """
        minutes = int(elap/60)
        seconds = int(elap - minutes*60.0)
        hseconds = int((elap - minutes*60.0 - seconds)*100)
        self.timestr.set('%02d:%02d:%02d' % (minutes, seconds, hseconds))

    def Start(self):
        """ Start the stopwatch, ignore if running. """
        if not self._running:
            self._start = time.time() - self._elapsedtime
            self._update()
            self._running = 1

    def Stop(self):
        """ Stop the stopwatch, ignore if stopped. """
        if self._running:
            self.after_cancel(self._timer)
            self._elapsedtime = time.time() - self._start
            self._setTime(self._elapsedtime)
            self._running = 0

    def Reset(self):
        """ Reset the stopwatch. """
        self._start = time.time()
        self._elapsedtime = 0.0
        self._setTime(self._elapsedtime)

    def clock_in(self):
        global e1
        data = e1.get()
        ClockIn_time = datetime.datetime.now()
        ClockIn_date = datetime.datetime.now().strftime('%Y-%m-%d')
        totalinput = [ data, ClockIn_time, ClockIn_date]
        with open(r"C:\Users\Desktop\ClockIn.csv", "a") as savedb:
            w = csv.writer(savedb)
            w.writerow(totalinput)

    def clock_out(self):
        global e1
        data = e1.get()
        ClockOut_time = datetime.datetime.now()
        ClockOut_date = datetime.datetime.now().strftime('%Y-%m-%d')
        totalinput = [ data, ClockOut_time , ClockOut_date ]
        with open(r"C:\Users\Desktop\ClockIn.csv", "a") as savedb:
            w = csv.writer(savedb)
            w.writerow(totalinput)

def main():
    root = Tk()
    sw = StopWatch(root)
    sw.pack(side=TOP)
    root.geometry("400x400")

    ####
    label = tk.Label(root, text="Employee Name: ")
    label.pack(side="top")

    new = StringVar()
    e1 = Entry(root, textvariable=new)
    e1.pack()
    #######

    Button(root, text='Clock In', command=lambda :[sw.Start(), sw.clock_in()]).pack(side=LEFT)
    Button(root, text='Clock Out', command=lambda :[sw.Start(), sw.clock_out()]).pack(side=LEFT)
    Button(root, text='Reset', command=sw.Reset).pack(side=LEFT)
    Button(root, text='Quit', command=root.quit).pack(side=LEFT)

    root.mainloop()

if __name__ == '__main__':
    main()

I'm having trouble get() the employees name and saving to a file. I've created a stand alone tkinter app, I'm able to do this, when I implement multiple features I cannot get them all to marinate.

Incorrect indention

All of the methods in a class need to be indented such that they are inside the class definition. (this is based on the original version of the code you posted.

Instead of this:

 class StopWatch(Frame):
   """ Implements a stop watch frame widget. """
   def __init__(self, parent=None, **kw): ...

def makeWidgets(self): ...
def _update(self): ...
def _setTime(self, elap): ...
def Start(self): ...
def Stop(self): ...
def Reset(self): ...
def clock_in(self): ...
def clock_out(self): ...

You need to do this:

class StopWatch(Frame):
   """ Implements a stop watch frame widget. """
    def __init__(self, parent=None, **kw): ...

    def makeWidgets(self): ...
    def _update(self): ...
    def _setTime(self, elap): ...
    def Start(self): ...
    def Stop(self): ...
    def Reset(self): ...
    def clock_in(self): ...
    def clock_out(self): ...

Follow PEP8 Guidelines

PEP8 gives guidelines that every python program should follow. Specifically in your case, function names need to begin with a lowercase character.

StringVar is unnecessary overhead

In most cases, using StringVar adds overhead with no real value. While this is more of a personal preference, I see no reason to use it. StringVar objects are good if you want the exact same value displayed in two or more widgets, or if you're wanting to trace changes. Otherwise they are superfluous.

Instead of self.timestr, you can directly alter the widget value. So, instead of saving a reference to the StringVar, save a reference to the widget:

def makeWidgets(self):
    ...
    self.time_label = Label(self, textvariable=self.timestr)
    ...

def _setTime(self, elap):
    ...
    self.time_label.configure(text='%02d:%02d:%02d' % (minutes, seconds, hseconds))
    ...

Be consistent with private methods

By convention, private methods (methods that are only used internally) are named with a leading underscore. You do this with _update and _setTime, but you didn't do it with makeWidgets. You should try to be consistent: if a function is only used internally, name it with a leading underscore.

Don't hard-code the filename

Your program would be a bit more portable if you didn't hard-code the filename. Right now you have the filename hard-coded in the clock_in and clock_out functions.

Instead, make it a parameter when you instantiate the class, or make it global.

For example:

def clock_in(self):
    ...
    with open(self.filename, "a") as savedb:
    ...

def clock_out(self):
    ...
    with open(self.filename, "a") as savedb:
    ...
sw = StopWatch(root, filename=r"C:\Users\Desktop\ClockIn.csv")

Separate widget creation from widget layout

In my experience, separating widget creation makes the code easier to visualize and easier to maintain.

Instead of this:

Button(root, text='Clock In', command=lambda :[sw.Start(), sw.clock_in()]).pack(side=LEFT)
Button(root, text='Clock Out', command=lambda :[sw.Start(), sw.clock_out()]).pack(side=LEFT)
Button(root, text='Reset', command=sw.Reset).pack(side=LEFT)
Button(root, text='Quit', command=root.quit).pack(side=LEFT)

... do this:

clock_in_button = Button(...)
clock_out_button = Button(...)
reset_button = Button(...)
quit_button = Button(...)

clock_in_button.pack(side=LEFT)
clock_out_button.pack(side=LEFT)
reset_button.pack(side=LEFT)
quit_button.pack(side=LEFT)

This makes it much more clear to see at a glance that all for buttons are aligned to the left. When it's all blocked together it's much harder to see.

Incorrect use of tkinter

You're importing tkinter with import tkinter as tk but then trying to use tk classes without the tk. prefix.

The import is good, but you need to fix all of the code that is creating widgets. For example, tk.Button(...) instead of just `Button(...).


Distance calculation implementation What I'm asking for is the review of a particular implementation of the haversine formula, found under:
http://blog.julien.cayzac.name/2008/10/arc-and-distance-between-two-points-on.html
This site is linked under the wikipedia article: http://en.wikipedia.org/wiki/Haversine_formula

Problem: I get the wrong distance for the points:

lat/lon, lat2/lon2

36.987814/-122.107887,   38.989185/-122.116728

I get a distance of 4336622.8812906 (meters)
however according to http://www.movable-type.co.uk/scripts/latlong.html it should be about 222.5 km.

If somebody could point out the error in this implementation, that would be great:

inline double getDist(double *gp) {
    /// @brief The usual PI/180 constant
    static const double DEG_TO_RAD = 0.017453292519943295769236907684886;
    /// @brief Earth's quatratic mean radius for WGS-84
    static const double EARTH_RADIUS_IN_METERS = 6372797.560856;

    double latitudeArc  = (gp[0] - gp[2]) * DEG_TO_RAD;
    double longitudeArc = (gp[1] - gp[3]) * DEG_TO_RAD;
    double latitudeH = sin(latitudeArc * 0.5);
    latitudeH *= latitudeH;
    double lontitudeH = sin(longitudeArc * 0.5);
    lontitudeH *= lontitudeH;
    double tmp = cos(gp[0]*DEG_TO_RAD) * cos(gp[2]*DEG_TO_RAD);
    return EARTH_RADIUS_IN_METERS*2.0 * asin(sqrt(latitudeH + tmp*lontitudeH));
}

There's nothing wrong with your implementation. There must therefore be something wrong with how you're declaring or populating gp.
Doing it like this:

double gp[4] = {36.987814, -122.107887, 38.989185, -122.116728};
printf("%.6f\n", getDist(gp));

gives the expected answer of 222606.439993


Hotkey detection for UI elements I have several UI elements that can be opened using hotkeys for example the settings menu's key is Escape. The problem is that I have the Keycode stored in Dictionary along with an Action or a Button.ButtonClickedEvent here's how the two dictionaries are declared:

    public static readonly Dictionary<KeyCode, Button.ButtonClickedEvent> KeysUIElementsWithButtonEvents = new Dictionary<KeyCode, Button.ButtonClickedEvent>
    {
        { KeyCode.P, SpellbookButton.onClick },
        { KeyCode.Escape, SettingsMenuButton.onClick },
    };

    public static readonly Dictionary<KeyCode, Action> KeysUIElementsWithActions = new Dictionary<KeyCode, Action>
    {
        { KeyCode.LeftAlt, SwitchCursorState},
    };

And here's how I used to invoke them :

        foreach (var item in Settings.Settings.KeysUIElementsWithButtonEvents)
        {
            if(Input.GetKeyDown(item.Key))
            {
                if(Cursor.lockState == CursorLockMode.Locked)
                {
                    Settings.Settings.SwitchCursorState();
                }
                item.Value.Invoke();
            }
        }

        foreach (var item in Settings.Settings.KeysUIElementsWithActions)
        {
            if (Input.GetKeyDown(item.Key))
            {
                if (Cursor.lockState == CursorLockMode.Locked)
                {
                    Settings.Settings.SwitchCursorState();
                }
                item.Value.Invoke();
            }
        }

I didn't like the repetitive code in here so I decided to create a function which solves the problem:

    CheckUIKeysCollection(Settings.Settings.KeysUIElementsWithButtonEvents);
    CheckUIKeysCollection(Settings.Settings.KeysUIElementsWithActions);

    private void CheckUIKeysCollection<T>(Dictionary<KeyCode, T> collection)
    {
        foreach (var item in collection)
        {
            if (Input.GetKeyDown(item.Key))
            {
                if (Cursor.lockState == CursorLockMode.Locked)
                {
                    Settings.Settings.SwitchCursorState();
                }
                if (item.Value is Action)
                {
                    (item.Value as Action).Invoke();
                    continue;
                }
                (item.Value as Button.ButtonClickedEvent).Invoke();
            }
        }
    }

However I'm not sure if the method is efficient enough it involves some casting here and there and it's also being called in the Update function (every frame) and if I have big collections it might hurt the performance. What can I do to improve the method?

The easiest way to solve this with one dictionary is to simply create an action for the Button.ButtonClickedEvent calls:

public static readonly Dictionary<KeyCode, Button.ButtonClickedEvent> KeysUIElementsWithButtonEvents = new Dictionary<KeyCode, Button.ButtonClickedEvent>
{
    { KeyCode.P, SpellbookButton.onClick },
    { KeyCode.Escape, SettingsMenuButton.onClick },
};

public static readonly Dictionary<KeyCode, Action> KeysUIElementsWithActions = new Dictionary<KeyCode, Action>
{
    { KeyCode.LeftAlt, SwitchCursorState},
};

Can be one Dictionary<KeyCode, Action>:

public static readonly Dictionary<KeyCode, Action> KeysUIElementsWithActions = new Dictionary<KeyCode, Action>
{
    { KeyCode.P, () => SpellbookButton.onClick.Invoke() },
    { KeyCode.Escape, () => SettingsMenuButton.onClick.Invoke() },
    { KeyCode.LeftAlt, SwitchCursorState }
}

Then this:

private void CheckUIKeysCollection<T>(Dictionary<KeyCode, T> collection)
{
    foreach (var item in collection)
    {
        if (Input.GetKeyDown(item.Key))
        {
            if (Cursor.lockState == CursorLockMode.Locked)
            {
                Settings.Settings.SwitchCursorState();
            }
            if (item.Value is Action)
            {
                (item.Value as Action).Invoke();
                continue;
            }
            (item.Value as Button.ButtonClickedEvent).Invoke();
        }
    }
}

Becomes this:

private void CheckUIKeysCollection(Dictionary<KeyCode, Action> collection)
{
    foreach (var item in collection)
    {
        if (Input.GetKeyDown(item.Key))
        {
            if (Cursor.lockState == CursorLockMode.Locked)
            {
                Settings.Settings.SwitchCursorState();
            }
            item.Value.Invoke();
        }
    }
}

No more generics, no more casting, no more inferences. Simply create an action that calls the ButtonClickedEvent.Invoke() method for the appropriate Button.

You could, also, create a method that does that for you:

public void ClickButton(Button button)
{
    button.onClick.Invoke();
}

Then make your dictionary:

public static readonly Dictionary<KeyCode, Action> KeysUIElementsWithActions = new Dictionary<KeyCode, Action>
{
    { KeyCode.P, ClickButton(SpellbookButton) },
    { KeyCode.Escape, ClickButton(SettingsMenuButton) },
    { KeyCode.LeftAlt, SwitchCursorState }
}

This also means it's simpler to click buttons from anywhere in your code: simply call ClickButton(Button), instead of Button.onClick.Invoke().

Apart from that:

Using is then as is not recommended, simply use as then check if it's null:

var temp = value as string;

if (temp != null)
{
    Console.WriteLine(temp.Length);
}


Stack implemented with a linked list I have made a basic stack class utilizing a linked list for data storage and \$O(1)\$ push and pop and just want some basic feedback on the class to see if there is anything basic I missed. It seems to be working perfectly and I know there are no memory leaks. I'm just being cautious and wondering if there is one or two big things I am missing or doing wrong.

template <typename T>
class Stack {
public:
  struct Node {
    T data;
    Node *next = nullptr;
    Node(T data) : data(data) {}
    Node(T data, Node *next) : data(data), next(next) {}
  };
  Node *top;
  int size_;

  Stack() : size_(0), top(nullptr) {}

  bool empty() {
    return top == 0;
  }

  void push(T data) {
    if(empty()) {
      top = new Node(data);
      size_++;
    } else {
      top = new Node(data, top);
      size_++;
    }
  }

  T pop() {
    if(!empty()) {
      T ret = top->data;
      Node *tmp = top->next;
      delete top;
      top = tmp;
      size_--;
      return ret;
    }

    return -1;
  }

  int size() {
    return size_;
  }

  ~Stack() {
    if(empty()) return;

    if(size_ == 1) {
      delete top;
      return;
    }

    Node *current = top;
    Node *next;
    while( current != 0 ) {
        next = current->next;
        delete current;
        current = next;
    }
    top = nullptr;

  }

};

Your code is easy to read, formatting is pretty consistent, and does indeed do what it is supposed to as far as I can tell, so that's a good. I just have a few suggestions.

class Stack {
public:

Well if everything is public, use a struct instead, it will save you a few keystrokes. But the size_ and top members probably shouldn't be public - they're not meant to be inspected (much less changed) from outside your class.

bool empty() { return top == 0; }
while (current != 0) {

Prefer nullptr instead of 0 (you already did that in other places).

void push(T data) {
  if(empty()) {
    top = new Node(data);
    size_++;
  } else {
    top = new Node(data, top);
    size_++;
  }
}

You can simplify this: if the stack is empty, then top is null, and new Node(data) does the same thing as new Node(data, nullptr). So you don't need the conditional, you can simplify it to:

void push(T data) {
  top = new Node(data, top);
  size_++;
}

The pop function will fail to compile for e.g. Stack<std::string>: you can't construct a string from -1. Your only real option here is to throw an exception if pop is called on an empty stack. (You should look at the standard container's interface: they have separate top() and pop() members. See this answer for example as for why it is done that way.)

And your destructor is a bit too complex, you don't need the two special cases as they'll be handled by the main loop just fine: if empty(), top is null so the while loop simply won't run. If there's just one element, the while loop will run just once. So you can cut it down to:

~Stack()
{
  while (top != nullptr) {
    Node *next = top->next;
    delete top;
    top = next;
  }
}

One last observation - what happens if someone writes this:

Stack<int> s1;
s1.push(1);
Stack<int> s2 = s1;
s1.pop();
s2.pop();

Unfortunately, this will compile but produces undefined behavior - the second pop will access free'd memory. You should either implement the rule of three (sometimes now called the "rule of 5" taking into account move constructor and move assignment), or simply not allow copying of your stacks:

Stack(Stack const&) = delete;


Simplifying minimum search of two dimensional vector with complex struct Is there a way to simplify this function? I tried to use the std::min_element function inside the lambda, but i don't get it to work with *var.

const auto miniFunc = [](const std::vector<std::vector<KEntryContact>> & entryContact, double KEntryContact::*var) -> double
{
    double min = DBL_MAX;
    for (const std::vector<KEntryContact> &entryvec : entryContact) {
        for (const KEntryContact &entry : entryvec) {
            min = std::min(min, entry.*var);
        }
    }

    return min;
};

The function can then be used like this:

double rAMin = miniFunc(tPOC->getEntryContact(), &KEntryContact::r1);

Simplifying isn't enough. I mean, your function is already quite simple, and complexity isn't what's questionable about it. To improve it, I'd suggest to:

handle empty input better

The way you handle empty input is questionable.

First, if you want to initialize min with the highest value for a double, use std::numeric_limits<double>::max() in <limits> instead of a macro.

Second, what if entryContact is empty, or if all its members are empty? You'll return DBL_MAX. And what if the smallest element inside is equal to DBL_MAX? You'll return DBL_MAX. Your return value is ambiguous.

At that point, you need to decide if an empty input is meaningless or meaningful. If it's meaningless, you might handle it by throwing an exception; if not, your return type should represent the fact that there are no smallest element if your input vector is empty. std::optional is a good candidate if c++17 is available to you.

Use the standard library more

Beyond std::optional, I would recommend to use algorithms from the standard library over raw loops. std::min_element will fit the bill for the inner vectors, but not for the outer one. You could use std::accumulate (std::reduce would be better but is only available in clang as for now).

Improve your code readability

I would advocate that using the keyword auto makes for a more readable code sometimes, and gives more flexibility, especially for lambdas because they are defined locally.

So here's how I would write it:

#include <algorithm>
#include <numeric>
#include <optional>
#include <limits>

const auto miniFunc = [](const auto& entry_vectors, auto member_var) {
    auto value  = [=] (auto&& entry)            { return entry.*member_var; };
    auto cmp    = [=] (auto&& lhs, auto&& rhs)  { return value(lhs) < value(rhs); };
    auto min    = [=] (auto&& init, auto&& vec)   {
        auto vec_min = std::min_element(vec.begin(), vec.end(), cmp);
        if (vec_min == vec.end()) return init;
        auto candidate = std::make_optional(value(*vec_min));
        return init ? std::min(init, candidate) : candidate;
    };
    return std::reduce(entry_vectors.begin(), entry_vectors.end(), std::optional<double>{}, min);
};

Edit: cf @TobySpeight's comment


Minesweeper, Bombcount method I have a method which checks all of its surrounding squares and returns the number of bombs around it. It uses a long list of if statements, which is pretty ugly and probably inefficient.

       public int countArround(){
    int bombNum = 0;
    //x=0 y=1 works
    if(((SmartSquare)board.getSquareAt(xLocation, yLocation+1)!=null)&&
        (((SmartSquare)board.getSquareAt(xLocation, yLocation+1)).getBomb())){
        bombNum++;
    }
    //x=1 y=1
    if(((SmartSquare)board.getSquareAt(xLocation+1, yLocation+1)!=null)&&
        ((SmartSquare)board.getSquareAt(xLocation +1, yLocation+1)).getBomb()){
        bombNum++;
    }
    //x=1 y=0 works
    if(((SmartSquare)board.getSquareAt(xLocation+1, yLocation)!=null)&&
        ((SmartSquare)board.getSquareAt(xLocation+1, yLocation)).getBomb()){
        bombNum++;
    }
    //x=1, y=-1 works
    if(((SmartSquare)board.getSquareAt(xLocation+1, yLocation-1)!=null)&&
        ((SmartSquare)board.getSquareAt(xLocation+1, yLocation-1)).getBomb()){
        bombNum++;
    }
    //x=0 y=-1
    if(((SmartSquare)board.getSquareAt(xLocation, yLocation-1)!=null)&&
        ((SmartSquare)board.getSquareAt(xLocation, yLocation-1)).getBomb()){
        bombNum++;
    }
    //x=-1, y=-1 works
    if(((SmartSquare)board.getSquareAt(xLocation-1, yLocation-1)!=null)&&
        ((SmartSquare)board.getSquareAt(xLocation-1, yLocation-1)).getBomb()){
        bombNum++;
    }
    //x=-1, y=0 works
    if(((SmartSquare)board.getSquareAt(xLocation-1, yLocation)!=null)&&
        ((SmartSquare)board.getSquareAt(xLocation-1, yLocation)).getBomb()){
        bombNum++;
    }
    //x=-1 y=1 works
    if(((SmartSquare)board.getSquareAt(xLocation-1, yLocation+1)!=null)&&
        ((SmartSquare)board.getSquareAt(xLocation-1, yLocation+1)).getBomb()){
        bombNum++;
    }
    return bombNum;

I was wondering how this can be cleaned up. I cannot seem to get it into an if/while/for statement.

A double-for-loop over x and y seems appropriate.

Also, it seems like a good idea to assign (SmartSquare)board.getSquareAt(...) to a variable.

int bombNum = 0;
for    (int xOffset = -1; xOffset <= 1; xOffset++)
   for (int yOffset = -1; yOffset <= 1; yOffset++)
   {
      if (xOffset == 0 && yOffset == 0)
      {
         continue;
      }
      SmartSquare neighbour = (SmartSquare)board.getSquareAt(xLocation+xOffset, yLocation+yOffset);
      if (neighbour != null && neighbour.getBomb())
      {
         bombNum++;
      }
   }
return bombNum;


Deserializer in ruby Given a serialized hash of any depth in string format, convert it into a hash object.

I want to know if my approach is correct, I am capturing states and storing the results in a stack and generating the hash from the stack. Also, is there a name for this kind of parsing approach?

def deserialize(input)
  h = {}
  l = 0
  stack = []
  curr_string = ""
  while l < input.length do
    case input[l]
    when "{" #state = DICT_START
      curr_string = ""
    when ":" #state =  VALUE_START
      stack.push(curr_string)
      curr_string = ""
    when "}", "," #state = DICT_END
      if curr_string != "" then
        stack.push(curr_string)
      end
      if stack.length > 0 then
        val = stack.pop
        key = stack.pop
        stack.push({key => val})
      end
      curr_string = ""
    else
      curr_string+=input[l]
    end
    l+=1
  end
  stack.each {|n| h.merge!(n)}
  h
end

p deserialize("{a:test,b:test1,c:test3}")
p deserialize("{a:{b:test1}}")
p deserialize("{a:{b:{c:test1}}}")
p deserialize("{a:{b:{c:test1}},d:123}")

The name for this type of parsing is called the shunting yard algorithm.

Your code itself seems fine, as long as you don't intend to support strings with curly braces, e.g. deserialize("{a:{b:'{{{{{{{{'}}").  The biggest issue I see is that it's not very ruby-like in the naming or formatting.  I've made brief attempt at cleaning it up a little below.

def deserialize(input)
  hash = {}                 # avoid one letter variable names
  stack = []
  curr_string = ''          # use single quotes for uninterpolated strings

  input.chars.each do |c|   # This is a better was to loop through the characters
    case c
      when '{'
        curr_string = ''
                            # whitespace makes it easier to read the different cases
      when ':'
        stack.push(curr_string)
        curr_string = ''

      when '}', ','
        stack.push(curr_string) unless curr_string.empty?  # unless is the same as "if not"
                                                           # also use one line statements when the logic is simple
        if stack.length > 0
          val = stack.pop
          key = stack.pop
          stack.push({key => val})
        end
        curr_string = ''

      else
        curr_string += c
    end
  end
  stack.each { |n| hash.merge!(n)}
  hash
end


Navigating to a view I'm navigating to a view. I have an EventDateTime property which I want to set to DateTime.Today unless the navigation is coming from MyCustomView

This is what I've come with so far. It's working, but I find this really ugly:

public void OnNavigatedTo(NavigationParameters parameters)
{
    if (parameters != null)
    {
        if (!parameters.ContainsKey(PARAM_NAVIGATEDFROM)) // If there is parameter PARAM_NAVIGATEDFROM, then the navigation doesn't come from MyCustomView
        {
            EventDateTime = DateTime.Today;
        }
        else if (parameters[PARAM_NAVIGATEDFROM] != typeof(MyCustomViewModel)) // If there is a parameter PARAM_NAVIGATEDFROM but not with the specific value, the navigation doesn't come from MyCustomView
        {
            EventDateTime = DateTime.Today;
        }
    }
    else // If there are no parameters at all, the navigation is not coming from MyCustomView either
    {
        EventDateTime = DateTime.Today;
    }
}

Any ideas how I can improve this Code? Maybe even with another approach instead of putting the Code into the OnNavigatedTo() method?

I think Bethan has the best approach, but you could take it one step further and use a generic Type:

public void OnNavigatedTo(NavigationParameters parameters)
{
    if (!IsNavigationFrom<MyCustomView>(parameters))
    {
        EventDateTime = DateTime.Today;
    }
}

private bool IsNavigationFrom<T>(NavigationParameters parameters)
{
    return parameters != null
        && parameters.ContainsKey(PARAM_NAVIGATEDFROM)
        && parameters[PARAM_NAVIGATEDFROM] == typeof(T);
}

This makes the method applicable to multiple view models.


Dependency Inversion / Injection? - Networking code in model classes @implementation User

- (void)refreshProperties
{
    assert(!self.isSyncing);

    NetworkOperation *op = [[NetworkOperation alloc] initWithEndPoint:GetUserDetails parameters:nil];
    [self setupCompletionForRefresh:op];

    [self setNetworkOperation:op];
    [self setSyncing:YES];

    [[NetworkManager sharedManager] enqueueOperation:op];
}

The above code is used by a UIViewController subclass to update the properties of a User instance (i.e. the view controller calls [aUser refreshProperties] in pull-to-refresh).

Concerted efforts have been made to make sure there are clear lines defining the separation of concerns:

Network Operation class only deals with getting data from the network
Network Manager class deals with enqueuing operations and watching
reachability
User class houses the strings, numbers, bools, etc that
make up a User instance

However, there are some problems with the above method. First and foremost is it's  hard to test. There isn't any way to mock a network operation or network manager and get them inside the method call without swizzling. Also, it couples the User class with the NetworkOperation and NetworkManager classes.

While the above code works, I would like to refactor it.

Looking through GoF, it looks like there are some ways to remove the dependency between User and Network-Operation/Manager: adapter, dependency inversion (using a protocol), etc. From what I can tell, that code would look something like:

[aUser setNetworkOperation:<object conforming to protocol>];
[aUser setNetworkManager:<object conforming to protocol>];
[aUser refreshProperties];

But... isn't this just shifting the dependency? The above calls would happen in the view controller, and now it has to know about and use three classes instead of just the main one it was originally concerned with - User.

Thoughts, discussion, resources, or code examples are greatly appreciated!

In GitHub for Mac, we use a combination of AFNetworking and our own Mantle and ReactiveCocoa frameworks for API requests.

AFNetworking makes it trivial to create your own "network manager" type object as a subclass of AFHTTPClient. We enqueue requests on it, ask them to hand back a certain kind of MTLModel subclass, and then subscribe to the resulting signal.

The resulting code looks something like this in the caller:

GHGitHubClient *client = [GHGitHubClient clientForUser:loggedInUser];
[[[client enqueueRequestWithMethod:@"GET" path:@"user" parameters:nil resultClass:GHGitHubUser.class]
    deliverOn:RACScheduler.mainThreadScheduler]
    subscribeNext:^(GHGitHubUser *user) {
        // Do something with the fetched user here.
    } error:^(NSError *error) {
        // Handle errors here.
    }];

There are several advantages to this:

You're building on the strength of AFNetworking. There's really no reason to write networking code from scratch anymore – this framework is validated in many, many applications, and can do almost anything you'd need.
MTLModel provides a way to parse model objects knowing only the desired class. GHGitHubClient can drive the parsing without being coupled to the specific model types.
Signals in ReactiveCocoa are composable, so this code can be easily modified to manipulate and transform the GHGitHubUser, or even perform other fetches and combine the results, before delivering to the main thread.
Your domain model isn't doing network activity. I firmly believe that domain models should care only about data, not the mechanics of persisting or fetching it.


Run a command for all files in a directory I have a program that needs to run an command on all the files in a folder:

import glob
import os

dirName = "../pack"

if not os.path.exists(dirName):
    os.mkdir(dirName)

os.chdir("xml/")

files = []
for file in glob.glob("*.xml"):
    files.append(file)

os.chdir("..")

filearchiver_cmdline = "archiver.exe"

for myFile in files:
    filearchiver_cmdline += " xml\\" + myFile

os.system( filearchiver_cmdline )

The command should look like this:

Arhiver.exe file1.xml

What should I change / optimize for this code? Would a class be needed? Should there be logical def-s? Are the variables name good?

Looping over glob.glob to build files is unneeded.
You shouldn't need to chdir if you change the glob to xml/*.xml
filearchiver_cmdline can be changed to use str.join and a comprehension.

Note: untested

import glob
import os

dir_name = "../pack"

if not os.path.exists(dir_name):
    os.mkdir(dir_name)

os.system(" ".join(
    ["archiver.exe"]
    + ["xml\\" + f for f in glob.glob("xml/*.xml")]
))


Best place to locate filters in MVC n-layer web application and should I use services/repositories in them? I am a little confused over which layers my filters should be contained in with an n-layer web application.

I've got 4 layers:

Presentation layer
Application layer
Business layer
Infrastructure layer

The Presentation layer can only talk to the Application layer and the Infrastructure layer. The Application layer can only talk to the Presentation layer, Business layer and the Infrastructure layer. The Business layer can only talk to the Application layer and the Infrastructure layer.

All my models, views and controllers are stored in the Presentation layer.

I have the following code:

public class AuthoriseAttribute : AuthorizeAttribute
{
    public override void OnAuthorization(AuthorizationContext filterContext)
    {
        if (filterContext == null)
        {
            throw new ArgumentNullException("filterContext");
        }

        if (SkipAuthorisation(filterContext))
        {
            return;
        }

        // This line...
        var evalReportProxy = new EvalReportProxy(WebHelper.PcwAliasName);
        if (evalReportProxy.EvalReport.SubscribedToModule("Workspace Only"))
        {
            filterContext.Result = new RedirectToRouteResult(
            new RouteValueDictionary
            {
                { "controller", "Redirect" },
                { "action", "Workspace" }
            });
        }

        if (string.IsNullOrEmpty(WebHelper.Username) || WebHelper.IsGuest)
        {
            filterContext.Result = new RedirectToRouteResult(
            new RouteValueDictionary
            {
                { "controller", "Account" },
                { "action", "LogIn" }
            });
        }
    }

    private static bool SkipAuthorisation(AuthorizationContext filterContext)
    {
        return filterContext.ActionDescriptor.IsDefined(typeof(AllowAnonymousAttribute), inherit: true)
            || filterContext.ActionDescriptor.ControllerDescriptor.IsDefined(typeof(AllowAnonymousAttribute), inherit: true);
    }
}

I've highlighted a line of code using a comment which shows where I am making use of a concrete type in the infrastructure layer to check to see if a user is subscribed to a module. By using a concrete type I've made it impossible to use mock objects which I want to avoid, but...

I'm a little confused because in the line highlighted using a comment I guess I should really be using a repository or a service although I'm not really sure if I should be or not. Also, if I use a repository then the filters can't be in the presentation layer, but I'm not sure if services are purely for controllers or whether I can use services elsewhere.

Can anyone please advise me on whether I should be putting filters in the presentation layer or not, and whether or not I should be using a service, repository, or keep the code how it is.

Also, I'm open to code improvements if you think any can be made.

Here's my take on this problem.

Your use of the AttributeFilter in the presentation layer is fine. You are correct about avoiding the use of a Repository in the filter, it does not belong there. However you can call upon the business layer (which in turn calls your repository) to check if they are subscribed. Keep your business logic where it belongs.

Dependencies in Filters can be mocked, you simply need to create a constructor which accepts the IBusinessService of your choice. Most dependency injection frameworks will create Filters and pass in the correct dependency.

If you don't want to go down that route, change your Filter like so.

class AuthoriseAttribute {

  // Use for testing
  public AuthoriseAttribute(IMockedBusinessService) {
  }

  // Production
  public AuthoriseAttribute():this(new ConcreteBusinessService()) {
  }
}

As always with these things, "it depends". If you have a high performance site, say > 200 connections per second and you get considerable performance gains by having the repository in directly in the presentation layer, then I would bend the rules and leave a nice comment in the code on why you have data access in the presentation layer.

Almost always I prefer code readability and separation of concerns much higher than anything else. Just be sure to take measurements on the performance gains by doing X. If your not measuring, your guessing.


Finding tuples of numbers subject to constraints I just watched the video from "Think Like a Programmer, Episode 2: Puzzles & Problems".

The problem is:

  Assign number 1-7 to three departments in a city: Fire, Police,
  Sanitation.

  Requirement:

  Each department should have a unique number
  Police should be an even number
  Fire + Police + Sanitation = 12

  Generate a list of permutations for these department numbers.

Here is my Python attempt:

def valid_department_number(f, p, s):
    if (f != p != s) and (f+p+s == 12) and p % 2 == 0:
        return True
    else:
        return False

numbers = range(1, 8)

for f in numbers:
    for p in numbers:
        for s in numbers:
            if valid_department_number(f, p, s):
                print(f, p, s)

I searched the python documentation and found that I could use the permutations function from itertools:

import itertools

all_permutations = list(itertools.permutations(numbers, 3))

for each_p in all_permutations:
    f, p, s = each_p[0], each_p[1], each_p[2]
    if valid_department_number(f, p, s):
        print(each_p)

The permutation function gets rid of three levels of nested for loops. I'm wondering whether there's a better way to deconstruct the tuples from permutation generated list of tuples.

f != p != s does not prevent f and s from being equal

You can improve your algorithmic approach, using your constraints:

def iter_permutations():
    total = 12
    for f in range(1, 8):        # Fire must be between 1 and 7
        for p in range(2, 8, 2): # Police must be even
            s = total - p - f    # The sum must be 12
            if 0 < s < 8 and f != p != s != f: # They must be all different
                 yield f, p, s   # Use an iterator, because it's elegant!

You can simply use it with : for f, p, s in iter_permutations():

I think using an iterator is nicer than a list (you can easily get a list with list(iter_permutations()) if needed)


Start task as soon as multiple tasks are successful I write an hobbyist application that collects soccer-data from different sources around the www and aggregates it for a graphical representation and sends it out as a tweet. Now I want to fully automate this process in a more or less robust way (more or less because 1. this is a hobby and 2. depends on scraped 3rd party websites).

I have problems to wrap my head around how this full automation can be organized. The following tasks need to be done:

check once daily if and when there are soccer matches I am interested in
start collecting data of these matches from all sources as soon as match is
over
retry collecting data every x minutes if it wasn't available yet (the sources have very different speed in releasing the data and it may take 24hrs. sometimes)
as soon as all sources have been collected successfully launch aggregation and tweet.

My question:
I don't know how to implement the connection between 3. and 4. in a good way. I think I need some wrapper which is called by the scheduler as soon as a match is over, but this doesn't seem to be a robust way:

class MatchCollector:

    def __init__(self,teams,sources=settings.sources):
        self.teams=teams
        self.sources = sources

    def execute(self):
        finished = False
        counter = 0
        while not finished:
            finished = True
            for source in self.sources:
                s = SourceCollector(source,self.teams)
                successful = s.execute()
                if not successful:
                    finished = False
            time.sleep(600)
            counter += counter
            if counter == 10000 # arbitrary chosen, have to make calc, when would be a good point to give up
                 raise IterationException('Tried for x hours without success. Somethings broken')
        d = DataAggregator(sources)
        d.execute()

Am I missing something problematic, by doing this?

I arrange daily scheduling (1., 2.) via cron and python-crontab in a scheduling script. It is started once daily and reads the daily matches and writes again into cron the script to handle the single matches with arguments passed via sys.argv.

Well, whatever you do, you should only re-check those sources which have not finished, so you should remember which have finished already, or equivalently, only those which have not.

class MatchCollector:
    def __init__(self, teams, sources=settings.sources):
        self.teams = teams
        self.sources = sources
        self.unfinished = {SourceCollector(source, teams) for source in sources}
        self.timeout = 600  # s
        self.iterations = 1000 # arbitrarily chosen, have to make calc, when would be a good point to give up

    def execute(self):
        counter = 0
        while self.unfinished:
            self.unfinished = {source for source in self.unfinished if not s.execute()}
            time.sleep(self.timeout)
            counter += 1
            if counter >= self.iterations
                 raise IterationException('Tried for x hours without success. Somethings broken')
        d = DataAggregator(self.sources)
        d.execute()

This assumes that SourceCollector.execute can be run repeatedly without needing to re-initialize it. If this is not the case, just do self.unfinished = {source for source in self.unfinished if not SourceCollector(source, self.teams).execute()}.

Note that doing counter += counter is probably not what you want. Since you initialize it to 0, it does 0 + 0 = 0, so nothing. If you had initialized it to a non-zero value, it would also not simply count up, but double every iteration.

I also made the timeout between retries and the number of iterations members of the class so they are no longer magic numbers.

You should also follow Python's official style-guide, PEP8, which recommends using spaces around = in assignments and after commas in argument lists of functions and methods.


Web Scraping with Python + asyncio I've been working at speeding up my web scraping with the asyncio library. I have a working solution, but am unsure as to how pythonic it is or if I am properly using the library. Any input would be appreciated.

import aiohttp
import asyncio
import requests
from lxml import etree

@asyncio.coroutine
def get(*args, **kwargs):
    """
    A wrapper method for aiohttp's get method. Taken from Georges Dubus' article at
    http://compiletoi.net/fast-scraping-in-python-with-asyncio.html
    """
    response = yield from aiohttp.request('GET', *args, **kwargs)
    return (yield from response.read_and_close())

@asyncio.coroutine
def extract_text(url):
    """
    Given the url for a chapter, extract the relevant text from it
    :param url: the url for the chapter to scrape
    :return: a string containing the chapter's text
    """
    sem = asyncio.Semaphore(5)
    with (yield from sem):
        page = yield from get(url)

    tree = etree.HTML(page)
    paragraphs = tree.findall('.//*/div[@class="entry-content"]/p')[1: -1]
    return b'\n'.join(etree.tostring(paragraph) for paragraph in paragraphs)

def generate_links():
    """
    Generate the links to each of the chapters
    :return: A list of strings containing every url to visit
    """
    start_url = 'https://twigserial.wordpress.com/'
    base_url = 'https://twigserial.wordpress.com/category/story/'
    tree = etree.HTML(requests.get(start_url).text)
    xpath = './/*/option[@class="level-2"]/text()'
    return [base_url + suffix.strip() for suffix in tree.xpath(xpath)]

@asyncio.coroutine
def run():
    links = generate_links()
    chapters = []

    for f in asyncio.as_completed([extract_text(link) for link in links]):
        result = yield from f
        chapters.append(result)

    return chapters

def main():
    loop = asyncio.get_event_loop()
    chapters = loop.run_until_complete(run())
    print(len(chapters))

if __name__ == '__main__':
    main()

Looks ... great?  Not a lot to complain about really.

The semaphore doesn't do anything though used like this, it should be
passed in from the top to protect the get/aiohttp.request.  You can
see that if you print something right before the HTTP request.

Also, the result of asyncio.as_completed will be in random order, so
be sure to sort the resulting chapters somehow, e.g. by returning both
the URL and the collected text from extract_text.

A couple of small things as well:

List comprehensions are okay, but with just a single argument it can
be shorter and equally performant just to use map.
The URL constants should ideally be defined on the top level; at least
base_url can also be defined by concatenating with start_url.
Alternatively they could be passed in to generate_links.  Then
again, it's unlikely that another blog has the exact same layout?
The manual append in run seems unnecessary, I'd rewrite it into a
list of generators and use a list comprehension instead.
At the moment generate_links is called from run; I think it makes
more sense to call it from the main function: it doesn't need to run
concurrently and you could think of a situation where you'd pass in
the result of a different function to be fetched and collected.

All in all, I'd maybe change things to the code below.  Of course if you
were to add things to it, I'd recommend looking into command line
arguments and configuration files, ...

import aiohttp
import asyncio
import requests
from lxml import etree

@asyncio.coroutine
def get(*args, **kwargs):
    """
    A wrapper method for aiohttp's get method. Taken from Georges Dubus' article at
    http://compiletoi.net/fast-scraping-in-python-with-asyncio.html
    """
    response = yield from aiohttp.request('GET', *args, **kwargs)
    return (yield from response.read_and_close())

@asyncio.coroutine
def extract_text(url, sem):
    """
    Given the url for a chapter, extract the relevant text from it
    :param url: the url for the chapter to scrape
    :return: a string containing the chapter's text
    """
    with (yield from sem):
        page = yield from get(url)

    tree = etree.HTML(page)
    paragraphs = tree.findall('.//*/div[@class="entry-content"]/p')[1:-1]
    return url, b'\n'.join(map(etree.tostring, paragraphs))

def generate_links():
    """
    Generate the links to each of the chapters
    :return: A list of strings containing every url to visit
    """
    start_url = 'https://twigserial.wordpress.com/'
    base_url = start_url + 'category/story/'
    tree = etree.HTML(requests.get(start_url).text)
    xpath = './/*/option[@class="level-2"]/text()'
    return [base_url + suffix.strip() for suffix in tree.xpath(xpath)]

@asyncio.coroutine
def run(links):
    sem = asyncio.Semaphore(5)
    fetchers = [extract_text(link, sem) for link in links]
    return [(yield from f) for f in asyncio.as_completed(fetchers)]

def main():
    loop = asyncio.get_event_loop()
    chapters = loop.run_until_complete(run(generate_links()))
    print(len(chapters))

if __name__ == '__main__':
    main()


Validate if the user is associated with the organization then it must have any valid timezone def validate_time_zone
  if time_zone.present? && org.present? && !org.country.states.map(&:time_zone).uniq.include?(time_zone)
    errors.add(:time_zone, :invalid)
  end
end

I have a user model with the following relations:

User can optionally belong to Org
Org belongs to a Country
A Country has some timezones (not unique as two states may have the same timezone).

So I validate that if the user is associated with an organization then it must have a valid timezone. Valid timezone means any time zone that comes under organization country. Ex Organization is ABC, Country is USA and Timezones are CET, Pacific, and Other US Time Zone. Then the user must use a timezone within the above three only.

I have filtered the condition but I am not sure breaking down will improve quality or further performance.

Can we used scope here too in country or organization or user module as it help in DRY not just validation.

In case if you wanna improve only the validate_time_zone method:

Performance wise I don't see anything sinister in your implementation.

I assume the states table has a direct time_zone field. If so, then we can use the pluck method to avoid instantiation ActiveRecord objects.

Array.uniq returns a new array by removing duplicate values. The idea is nobel but it means it will use more memory and will loop over the time zones array which will happen anyway in case of include?

On the other hand nowadays in software development the real cost is more in maintenance rather than in CPU cycles.

With this in mind, how about the following refactor?

class User
  validates_presence_of :time_zone

  validate :validate_time_zone

  def validate_time_zone
    # using guard clause and let the built in validation do the presence validation
    return unless org.present? && time_zone.present?

    # A temp variable is a code smell, but here we have an underlying concept
    # that is better to be named
    allowed_time_zones = org.country.states.pluck(:time_zone)

    # tip: you can use the .exclude? API if you don't want to a use guard clause
    return if allowed_time_zones.include?(time_zone)

    errors.add(:time_zone, :invalid)
  end
end


Inserting data into database I started learning PHP and MySQL, was using MySQL_ functions. Then I learned they are deprecated, had to choose between PDO and MySQL. I chose MySQL and started using it.

As I learn OOP PHP, I thought I should change my code to OOP style, since it's cleaner. I was wondering whether this block of my code is secure or not. I use mysqli_real_escape_string() and htmlspecialchars() above.

Old code

$result = "INSERT INTO book(book_original_name, book_turkish_name, book_publication_date, book_synopsis) VALUES (?,?,?,?)";
$stmt = mysqli_prepare($sqli, $result);
mysqli_stmt_bind_param($stmt, "ssss", $_POST['book_original_name'], $_POST['book_tr_name'], $_POST['book_year'], $_POST['book_synopsis']);
mysqli_stmt_execute($stmt);
mysqli_stmt_close($stmt);

New code ($sqli is my connection variable.)

$stmt = $sqli->prepare("INSERT INTO book(book_original_name, book_turkish_name, book_publication_date, book_synopsis) VALUES (?,?,?,?)");
$stmt->bind_param("ssss", $_POST['book_original_name'], $_POST['book_tr_name'], $_POST['book_year'], $_POST['book_synopsis']);
$stmt->execute();
$stmt->close();
echo "Mission has been completed, sir!";

There doesn't seem much wrong with your code, but it is difficult to judge a small piece of code in isolation.

One thing I must urge you to do is to check the $_POST parameters before you put them into the database. Do not rely solely on the checks you do before the form is submitted. For instance, make sure you've actually got some content, and that the year is really four digits long. This way you can stop rougue data getting into your database.

In some cases you will have to try to prevent robots from posting to your database. This is especially true for registration forms.

See: http://www.ultramegatech.com/2009/08/5-basic-php-security-tips

Or: http://www.dreamhost.com/dreamscape/2013/05/22/php-security-user-validation-and-sanitization-for-the-beginner