Deserialize JSON Strings Directly into Models? I have Ruby models which are populated from the responses of API calls in the following way:

JSON.parse converts the response to a Hash
the Hash is passed into the initialize method of a class
the initialize method converts camelCase hash keys and assigns underscore_case instance variables
controller code works with these instances and converts back to json to send to the browser

This works fine, but some of these response objects are large.  Others are arrays of large objects.

Profiling shows that this process consumes a lot of CPU (and memory, but that is less of a concern) -- which makes sense given that I create hashes in order to create objects, and the back and forth between camelCase and underscore_case happens A LOT -- so what libraries or techniques have you come across which solve this problem?

Here is an oversimplified example:

JSON response from a third party API (unlikely to change):

"{\"abcDef\": 123, \"ghiJkl\": 456, \"mnoPqr\": 789}"

Class definition (attributes unlikely to change):

class Data
  attr_accessor :abc_def, :ghi_jkl, :mno_pqr

  def initialize(attributes = {})
    attributes.each do |key, val|
      send "#{key.underscore}=".to_sym, val
    end
  end

  def as_json
    instance_variables.reduce({}) do |hash, iv|
      iv_name = iv.to_s[1..-1]
      v = send(iv_name) if self.respond_to?(iv_name)
      hash[iv_name.camelize(:lower)] = (v.as_json(options) if v.respond_to?(:as_json)) || v
      hash
    end
  end
end

Controller:

get '/' do
  d = Data.new JSON.parse(api.get)
  # ... do some work ...
  content_type 'application/json'
  d.to_json
end

TheJSON schema and data class attributes are unlikely to change. Consider implementing the class to encapsulate the JSON hash, adapting it to the ruby idioms and whatever other custom operations you've added. This should greatly decrease the cost and complexity of deserialization.


Multi-Threaded Memory Preallocation I need to figure out the best way to deal with memory pre-allocation

Below is pseudo-code for what I am doing now, and it seems to be working fine.

I am sure there is a better way to do this, I would like to see if anyone has any good idea.

In Thread A I need to allocate 300 MB of memory and zero it out:

Thread A:

char* myMemory = new char[10*30*1024*1024];

In Thread B I incrementally get 10 sets of data 30 MB each. As I get this data it must be written to memory

Thread B:

int index1 = 0;
char* newData = getData(...); // get a pointer to 30 MB of data
memcpy(&myMemory[index1*30*1024*1024], &newData[0], 30*1024*1024];
SetEvent(...) // tell Thread C that I have written new data

// use index i as circular Buffer
if (index1 < 10)
 index1++;
else
 index1 = 0;

In Thread C, when new data is written, I need to get it and process it

Thread C:

int index2 = 0;
char* dataToProcess[] = new char[30*1024*1024];
if (event Fires) // if event in Thread B is set
 memcpy(&dataToProcess[0], &myMemory[index2*30*1024*1024], 30*1024*1024];
 processData(dataToProcess)

// use index i as circular Buffer
if (index2 < 10)
 index2++;
else
 index2 = 0;

Another approach I can think of could be this. Get rid of myMemory altogether. Once you receive data in thread B, allocate memory and copy that data (as you're doing now). Send thread C a message that data is received. Once thread C copies the data into its internal buffer, thread C can send message back to thread B that the data has been copied. Thread B then deallocate the buffer.

Your design seems fine to me. Yet another approach could be avoiding the data copy in thread C.

int index2 = 0;
char* dataToProcess;
if (event Fires) // if event in Thread B is set
 dataToProcess = &myMemory[index2*30*1024*1024];
 processData(dataToProcess)
// Rest of the code


OpenCL approximation of Pi using vectorized implementation Yesterday I started following the hands-on OpenCL course. I now got to the point where we are requested to reimplement an approximation algorithm for Pi in OpenCL (in steps, up to a vectorized implementation). I have done so and wanted to get a review for it coupled with some questions I had.
My kernel:
__kernel void pi(const long steps_total, const long steps_per_kernel,
const double step, __global double *global_sums)
{
    const int global_id  = get_global_id(0);
    const int local_id   = get_local_id(0);
    const int local_size = get_local_size(0);

    const long vector_size = 16;
    const double16 deltas  = (0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15);

    const long work_dx_start     = global_id * steps_per_kernel;
    const long work_dx_end       = min(work_dx_start + steps_per_kernel, steps_total);
    const long vectorized_dx_end = work_dx_end / vector_size * vector_size;

    double work_sum = 0.0;
    for (long dx = work_dx_start; dx < vectorized_dx_end; dx += vector_size)
    {
        const double16 mid_points   = (dx - 0.5 + deltas) * step;
        const double16 partial_sums = (4.0 / (1.0 + mid_points * mid_points));

        work_sum += partial_sums.s0 + partial_sums.s1 + partial_sums.s2 + partial_sums.s3 +
                    partial_sums.s4 + partial_sums.s5 + partial_sums.s6 + partial_sums.s7 +
                    partial_sums.s8 + partial_sums.s9 + partial_sums.sa + partial_sums.sb +
                    partial_sums.sc + partial_sums.sd + partial_sums.se + partial_sums.sf;
    }
    for (long dx = vectorized_dx_end; dx < work_dx_end; dx++)
    {
        const double mid_point = (dx - 0.5) * step;
        work_sum += 4.0 / (1.0 + mid_point * mid_point);
    }

    const double group_sum = work_group_reduce_add(work_sum);
    if (local_id == 0)
    {
        global_sums[global_id / local_size] = group_sum;
    }
}

I create max_compute_units * preferred_work_group_size work-items, with preferred_work_group_size as the number of items in a group (and so each kernel executes around steps/global_work_size iterations). On the host side the global sums array is finally added again.
Any comments are greatly appreciated (style, optimizations,...), I also have some questions:

It there a nicer way to remove the extra loop at the end in case the iterations are not a multiple of the vector size?
Is there a way to sum all elements of an OpenCL vector faster than this (the elements of partial_sums)? The best I could find was IIRC calculating the dot product (which only seems to exist for vectors up to size 4, and even then, it was slower on my GPU). Also stepwise reducing to vectors of half the size by using addition between lower and upper has no apparent effect. I guess it may not even be possible to optimize further?
On my GPU, using vectors of size 16 performs the best. I take it this can change depending on the device? Is there some way to try to detect/heuristically calculate this up front (statically) and dynamically load a specific kernel? (or other techniques?)

Addressing question #2, you can move the parallel sum out of the for loop, and only do it once.
Use another double16 variable (initialized to all zeros), accumulate the sum into it during the loop body. Then, once the loop is done, do the parallel sum to initialize work_sum before the last loop that handles that last few elements.


Checking day number in a year I'm supposed to make a program that will tell me the number of the day of a certain date. So I input the date, and it will tell me which number day it is. (i.e: 1.1.2014 would be day 1, and 30.4.2012 would be 121) The program in its current state works, but it looks a little sloppy (or I'm just too self-conscious). I'm somewhat new to programming and was wondering if I can improve it, or if I can make it less sloppy.

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

valid(int day, int month, int year)
{
    int st[12]={31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
    int copy[12]={31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
    if(year % 4 != 0)
        {
        if(day < 1 ||  day > st[month-1] || month < 1 || month > 12 || year < 0)
            return 0;
        else
        return 1;
    }
    else
        if(day < 1 ||  day > copy[month-1] || month < 1 || month > 12 || year < 0)
            return 0;
        else
            return 1;

}

int main()
{
    int day, month, year, sum;
    scanf("%d", &day);
    scanf("%d", &month);
    scanf("%d", &year);
    printf("Date is %d.%d.%d", day, month, year);
    sum = day;
    int st[12]={31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
    int copy[12]={31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
    if(valid(day, month, year) == 1)
        {
        if(year % 4 == 0)
            for(int i = 0; i < month; i++)
                sum = sum + copy[i-1];
        else
            for(int i = 0; i < month; i++)
                sum = sum + st[i-1];
        printf("\nDay number is %d", sum);
        }
    else
        printf("\nInvalid date");

}

The function valid checks whether my date is valid, so that I cannot input a wrong date. I also check for leap years inside my main program. I have an array which has the number of days in every month, and another one for leap years.

The compiler warns you

The gcc compiler doubles as a code analysis tool. Running on your code it found the following potential problems:

year.c:4:1: warning: return type defaults to ‘int’
 valid(int day, int month, int year)
 ^
year.c: In function ‘main’:
year.c:26:5: warning: ignoring return value of ‘scanf’, declared with attribute warn_unused_result [-Wunused-result]
     scanf("%d", &day);
     ^
year.c:27:5: warning: ignoring return value of ‘scanf’, declared with attribute warn_unused_result [-Wunused-result]
     scanf("%d", &month);
     ^
year.c:28:5: warning: ignoring return value of ‘scanf’, declared with attribute warn_unused_result [-Wunused-result]
     scanf("%d", &year);
     ^

The last 3 warnings are no big deal in a toy like this, but the first is important: you forgot saying that the functions returns an int, greatly confusing the future reader of your code as he may not know by heart that unspecified return type defaults to int.

The (not)-boolean return antipattern

    if(day < 1 ||  day > st[month-1] || month < 1 || month > 12 || year < 0)
        return 0;
    else
    return 1;

Means that if day < 1 ||  day > st[month-1] || month < 1 || month > 12 || year < 0 is true you return 0 (false) otherwise (if it is false) you return true.

expression_value = true -> return_value = false
expression_value = false -> return_value = true

Just return the negation: !

return ! (day < 1 ||  day > st[month-1] || month < 1 || month > 12 || year < 0);

The same in general applies when there is no negation, booleans shall be returned direclty, not after an if - else

Use a negative check to reduce nesting

If a pre-condition is not met, just abort early:

if(! valid(day, month, year)) {
    puts("\nInvalid date\n");
    return 1; // non-zero return code signals error.
}

puts

printf has a lot of power, when dealing with simple strings puts is preferred (see the example above).


Fixed size double linked list in Scala I have written a double linked list in Scala. The list should be fixed in size and items are always added to the head. if the list reaches max size then the tail is dropped.

This is my code. I want to review this for brevity and functional approach.

sealed trait List {
    var prev : List
    var next : List
}

case object Empty extends List {
    var prev : List = Empty
    var next : List = Empty
}
case class Node(v: Int, p: List, n: List) extends List {
    var prev = p
    var next = n
}

class DoubleLinkedList(maxSize: Int) {
    private var head : List = Empty
    private var tail : List = Empty
    private var currentSize = 0
    def print() : Unit = {
        var temp = head
        while(temp != Empty) {
            temp match {
                case Empty => println("Empty")
                case Node(v, _, _) => println(v)
            }
            temp = temp.next
        }
    }
    def add(i: Int) : List = {
        if (head == Empty || tail == Empty) {
            val n = new Node(i: Int, Empty, Empty)
            head = n
            tail = n
            currentSize += 1
            Empty
        } else {
            val n = new Node(i, null, head)
            head.prev = n
            head = n
            currentSize += 1
            if (currentSize > maxSize) {
                val retVal = tail
                tail = tail.prev
                tail.next = Empty
                currentSize -= 1
                retVal
            } else Empty
        }
    }
}

A few things I've noticed.

while(temp != Empty) {
  temp match {
    case Empty => println("Empty")
    . . .

That will never print "Empty".

case object Empty extends List {
  var prev : List = Empty
  var next : List = Empty
}

That looks like a reasonable definition of an empty end-point, but the member elements are both mutable variables which means that some code later on could modify the Empty object to have real prev and/or next values.

What you're after is a set of references to prev and next nodes if the node exists, otherwise it should be a meaningless, or "empty", reference. To me that suggests Option[Node].

You say you seek "brevity and functional approach," and there's room for improvement on both fronts, but the basic design, class instances that maintain mutable state, goes against the fundamental FP concept of referential transparency.

What's more, the add() method returns a List instance that can allow code outside of the DoubleLinkedList definition to modify its state and introduce hard-to-find bugs.

val dll = new DoubleLinkedList(3)
dll.print()
dll.add(9)
dll.add(8)
dll.add(7)
dll.add(6).prev = Empty
dll.print()  // dll has been corrupted

Here's an implementation of your basic design that is a bit more concise and needs fewer vars. It still exposes mutable nodes to the outside world, but that seems to be an inescapable aspect of your design.

class DoubleLinkedList[A](maxSize: Int) {
  class Node(val v :A, var prev :Option[Node], var next :Option[Node]) {
    def print() :Unit = {
      Predef.print(s"$v <-> ")
      next.fold(println("End"))(_.print())
    }
  }
  private var head :Node = _
  private var tail :Node = _
  private var currentSize = 0
  def print() :Unit = Option(head).fold(println("Empty"))(_.print())
  def add(i :A) :Option[Node] = {
    head = new Node(i, None, Option(head))
    head.next.fold(tail = head)(_.prev = Some(head))
    if (currentSize < maxSize) {
      currentSize += 1
      None
    } else {
      val retVal = tail
      tail = tail.prev.get
      tail.next = None
      Option(retVal)
    }
  }
}

usage:

val dll = new DoubleLinkedList[Int](3)
dll.print()  // "Empty"
dll.add(9)
dll.add(8)
dll.add(7)
dll.add(6)
dll.print()  // "6 <-> 7 <-> 8 <-> End"


Python function to generate Spanish conjugations I tried to make a Python function to generate present tense Spanish conjugations.

from unidecode import unidecode
def solveSpanishPresent(pronoun,verb):
  if unidecode(verb).endswith("ar"):
    arDict = {"yo": "o", "tú": "as", "usted": "a", "él": "a", "ella": "a", "nosotros": "amos", "vosotros": "áis", "ustedes": "an", "ellos": "an", "ellas": "an"}
    verb = verb[:-2] + arDict[pronoun]
    return verb
  if unidecode(verb).endswith("er") or unidecode(verb).endswith("ir"):
    erDict = {"yo":"o", "tú":"es", "usted":"e", "él":"e", "ella":"e", "nosotros":"emos", "vosotros":"éis", "ustedes":"en", "ellos":"en", "ellas":"en"}
    irDict = {"nosotros":"imos", "vosotros":"ís"}
    if (pronoun == "nosotros" or pronoun == "vosotros") and verb.endswith("ir"):
        verb = verb[:-2] + irDict[pronoun]
    else:
        verb = verb[:-2] + erDict[pronoun]
    return verb

I think that there are too many special cases in the code.  A data-driven approach would be easier to read and maintain than hard-coded logic.  The entire function could be done using just dictionary lookups.

Furthermore, I think that you are missing a translation layer.  You can classify the three pronouns "usted", "él", and "ella" as "third person singular", and it will be clear that they all lead to the same conjugation.  It would be more worthwhile to "compress" your conjugation tables that way than to try to override a few entries of the erDict with irDict.

I would also format the dictionaries to look more like the tables that would typically be presented in grammar books.  The line containing your arDict stretches to column 163, which is too wide to be readable.

from unidecode import unidecode

PRONOUN_CLASSIFICATION = {
    'yo': '1s',     'nosotros': '1p',
    'tú': '2s',     'vosotros': '2p',
    'usted': '3s',  'ustedes': '3p',
    'él': '3s',     'ellos': '3p',
    'ella': '3s',   'ellas': '3p',
}

PRESENT_CONJUGATION_ENDINGS = {
    'ar': {
        '1s': 'o',      '1p': 'amos',
        '2s': 'as',     '2p': 'áis',
        '3s': 'a',      '3p': 'an',
    },
    'er': {
        '1s': 'o',      '1p': 'emos',
        '2s': 'es',     '2p': 'éis',
        '3s': 'e',      '3p': 'en',
    },
    'ir': {
        '1s': 'o',      '1p': 'imos',
        '2s': 'es',     '2p': 'ís',
        '3s': 'e',      '3p': 'en',
    },
}

def conjugate_spanish_present(pronoun, infinitive):
    person = PRONOUN_CLASSIFICATION[pronoun]
    base, ending = infinitive[:-2], unidecode(infinitive)[-2:]
    try:
        return base + PRESENT_CONJUGATION_ENDINGS[ending][person]
    except KeyError:
        return infinitive


Test four int of commonality in the lower four bits The four parameters a, b, c and d can be -1 (meaning it's not set) or a random one of {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}. If they are different from -1, they are guaranteed to be distinct.

The code looks at the four least significant bits of the four ints and tests if they all have a bit in common:

1101
1110
0100
0101

have the second left bit 1.

1001
1010
0000
0001

all have the second left bit 0.

1000
0110
0101
1101

have no common bit.

The following is possible:

a = -1, b = -1, c = 15, d =  4; //sim(a, b, c, d) == 0, because at least one is -1
a = -1, b = -1, c = -1, d = -1; //sim(a, b, c, d) == 0, because all are -1
a =  9, b =  1, c =  3, d =  5; //sim(a, b, c, d) == 1, because all values have a bit in common (a & 1, b & 1, c & 1 and d & 1 are all 1)
a =  8, b =  1, c =  2, d =  4; //sim(a, b, c, d) == 0, because there is no bit that is equal for a, b, c and d

The following will never happen:

a = 1, b = 1, c = 2, d = 5; // a == b, which is guaranteed to never happen because a == b

The code is meant to be as fast as possible on x86, readability would be a plus but is not necessary.

The code:

int sim(int a, int b, int c, int d)
{
    return ((a != -1) && (b != -1) && (c != -1) && (d != -1)) &&
    ((((a & 8) == (b & 8)) && ((a & 8) == (c & 8)) && ((a & 8) == (d & 8))) ||
     (((a & 4) == (b & 4)) && ((a & 4) == (c & 4)) && ((a & 4) == (d & 4))) ||
     (((a & 2) == (b & 2)) && ((a & 2) == (c & 2)) && ((a & 2) == (d & 2))) ||
     (((a & 1) == (b & 1)) && ((a & 1) == (c & 1)) && ((a & 1) == (d & 1))));
}

Are there obvious ways to speed up the code?

The code isn't bad, but it's a little more verbose than it needs to be.  Consider that we don't really need to check one bit at a time; we can check four simultaneously.

The key here is that we're looking for bits that are the same in all four numbers.  If we wanted to look for ones that were shared, we could do this:

a & b & c & d & 0xf

If we want to look for zeroes, we can simply invert:

~a & ~b & ~c & ~d & 0xf

If we put those together with the -1 part, it might look like this:

return a != -1 && b != -1 && c != -1 && d != -1 &&
    ((a & b & c & d) || (~a & ~b & ~c & ~d & 0xf));

However, the problem with this is that even though it's parallel, it still requires more operations than might be required.

If we consider the exclusive or function, it effectively return a 1 whenever the bits differ.  So if there were only two numbers, we could do:

return (a ^ b) ^ 0xf;

The expression would only be false if all of the bits were different.  We can use a similar strategy for three numbers.

return ((a ^ b) | (b ^ c)) ^ 0xf;

For this one, (a ^ b) returns 1 for every bit that is not the same between a and b, and the expression (b ^ c) returns 1 for every bit that is not the same between b and c.  When we do a bitwise or of those quantities, only the the bits that are the same for all three quanties remain zeroes.  When we do ^ 0xf we invert the bottom 4 bits so that only bits that are the same are ones.

Extrapolating this to four quantities,

return ((a ^ b) | (b ^ c) | (c ^ d)) ^ 0xf;

This is good but not sufficient since we still need to deal with the possible -1 quantities that may be among the inputs.  One obvious way to do this is this:

return a != -1 && b != -1 && c != -1 && d != -1 &&
    (((a ^ b) | (b ^ c) | (c ^ d)) ^ 0xf);

To compare this routine which I'll call "Edward" to the one above, which I'll call "naive" to the original and the other three proposals so far, I used this code:

testcode.c

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

bool sim(int a, int b, int c, int d)
{
    return ((a != -1) && (b != -1) && (c != -1) && (d != -1)) &&
    ((((a & 8) == (b & 8)) && ((a & 8) == (c & 8)) && ((a & 8) == (d & 8))) ||
     (((a & 4) == (b & 4)) && ((a & 4) == (c & 4)) && ((a & 4) == (d & 4))) ||
     (((a & 2) == (b & 2)) && ((a & 2) == (c & 2)) && ((a & 2) == (d & 2))) ||
     (((a & 1) == (b & 1)) && ((a & 1) == (c & 1)) && ((a & 1) == (d & 1))));
}

bool naive(int a, int b, int c, int d)
{
    return a != -1 && b != -1 && c != -1 && d != -1 &&
        ((a & b & c & d) || (~a & ~b & ~c & ~d & 0xf));
}

bool edward(int a, int b, int c, int d)
{
    return a != -1 && b != -1 && c != -1 && d != -1 &&
        (((a ^ b) | (b ^ c) | (c ^ d)) ^ 0xf);
}

bool BitsInCommon(int a, int b, int c, int d)
{
    if (a == -1 || b == -1 || c == -1 || d == -1)
        return false;

    int rslt = 0;
    rslt = (a & 15) & (b & 15) & (c & 15) & (d & 15);
    if (rslt != 0)
        return true;

    //check for zero bits in common???  if needed?
    rslt = ((a ^ -1) & 15) & ((b ^ -1) & 15) & ((c ^ -1) & 15) & ((d ^ -1) & 15);

    return rslt != 0;
}

bool jan_korous(int a, int b, int c, int d) {

    if( a == -1 || b == -1 || c == -1 || d == -1 ) { return 0; }

    const unsigned int diff = (a ^ b) | (c ^ d) | (a ^ c);

    return
        ( (diff & 1) == 0 ) ||
        ( (diff & 2) == 0 ) ||
        ( (diff & 4) == 0 ) ||
        ( (diff & 8) == 0 );

}

bool scottbb(int a, int b, int c, int d) {
    if (a == -1 || b == -1 || c == -1 || d == -1) {
        return 0;
    }
    else {
        unsigned int all_1 = (unsigned int)a & (unsigned int)b &
                             (unsigned int)c & (unsigned int)d;
        unsigned int all_0 = ~((unsigned int)a | (unsigned int)b |
                               (unsigned int)c | (unsigned int)d);
        return (all_1 | (all_0 & 0xF)) ? 1 : 0;
    }
}

int main()
{
    bool troubleshoot = false;
    struct {
        const char *name;
        bool (*func)(int, int, int, int);
        double elapsed;
        bool isCorrect;
    } tests[] = {
        { "original", sim, 0, true },
        { "naive", naive, 0, true },
        { "Edward", edward, 0, true },
        { "dbasnett", BitsInCommon, 0, true },
        { "Jan Korous", jan_korous, 0, true },
        { "scottbb", scottbb, 0, true},
        { NULL, NULL, 0, false}
    };
    // see if they're all correct
    for (int a = -1; a < 16; ++a) {
        for (int b = -1; b < 16; ++b) {
            for (int c = -1; c < 16; ++c) {
                for (int d = -1; d < 16; ++d) {
                    for (size_t i = 1; tests[i].func; ++i) {
                        if (tests[i].func(a,b,c,d) != tests[0].func(a,b,c,d)) {
                            if (troubleshoot) {
                                printf("%s failed! [%d, %d, %d, %d] => %d, should have been %d\n",
                                    tests[i].name, a, b, c, d,
                                    tests[i].func(a, b, c, d),
                                    tests[0].func(a, b, c, d)
                                );
                            }
                            tests[i].isCorrect = false;
                            if (troubleshoot)
                                return -1;
                        }
                    }
                }
            }
        }
    }

    puts("All functions checked for accuracy; checking timing...");
    for (int iterations = 100; iterations; --iterations) {
        for (size_t i = 0; tests[i].func; ++i) {
            if (tests[i].isCorrect) {
                for (int a = -1; a < 16; ++a) {
                    for (int b = -1; b < 16; ++b) {
                        for (int c = -1; c < 16; ++c) {
                            for (int d = -1; d < 16; ++d) {
                                clock_t start = clock();
                                tests[i].func(a,b,c,d);
                                tests[i].elapsed += clock() - start;
                            }
                        }
                    }
                }
            }
        }
    }
    // print results
    for (size_t i = 0; tests[i].func; ++i) {
        if (tests[i].isCorrect) {
            printf("%12s\t%.10f\t%f%% %s than %s\n", tests[i].name, tests[i].elapsed,
                100.0*fabs(tests[i].elapsed-tests[0].elapsed)/tests[0].elapsed,
                (tests[i].elapsed > tests[0].elapsed ? "slower" : "faster"),
                tests[0].name
            );
        } else {
            printf("%12s\twas not correct; no time recorded\n", tests[i].name);
        }
    }
}

Results:

Note: I've updated the results to include everybody's correct versions and to include the two solutions proposed by @js1 although I didn't bother duplicating the source code for those solutions in the code above.

All functions checked for accuracy; checking timing...
    original    1052389.0000000000  0.000000% faster than original
       naive    1034792.0000000000  1.672100% faster than original
      Edward    1024637.0000000000  2.637048% faster than original
    dbasnett    1032151.0000000000  1.923053% faster than original
  Jan Korous    1028349.0000000000  2.284326% faster than original
     scottbb    1026372.0000000000  2.472185% faster than original
         JS1    1035062.0000000000  1.646444% faster than original
  JS1 Lookup    1023049.0000000000  2.787942% faster than original

So in my tests, on a quad-core x86_64 machine, the Edward and Jan Korous routines are nearly identical in time and both are a small improvement (approximately 2%) over the original.  There was a slight but persistent speed advantage to the "JS1 Lookup" version on this machine.

Compile command (Linux) was:

  gcc -O2 -std=c99 testcode.c -o testcode && ./testcode

On a quad core ARM7, I got this result with the same code:

All functions checked for accuracy; checking timing...
    original    17302157.0000000000 0.000000% faster than original
       naive    17211229.0000000000 0.525530% faster than original
      Edward    17231692.0000000000 0.407261% faster than original
    dbasnett    17255411.0000000000 0.270174% faster than original
  Jan Korous    17115011.0000000000 1.081634% faster than original
     scottbb    17120390.0000000000 1.050545% faster than original
         JS1    17267606.0000000000 0.199692% faster than original
  JS1 Lookup    17118642.0000000000 1.060648% faster than original

I also tested on Windows under Cygwin, but found that the variability in timing on that platform was so large as to render the test results meaningless.  As an example, here are four successive runs on that platform:

All functions checked for accuracy; checking timing...
    original    1902.0000000000 0.000000% faster than original
       naive    2124.0000000000 11.671924% slower than original
      Edward    2182.0000000000 14.721346% slower than original
    dbasnett    2091.0000000000 9.936909% slower than original
  Jan Korous    1980.0000000000 4.100946% slower than original
     scottbb    2144.0000000000 12.723449% slower than original
         JS1    2276.0000000000 19.663512% slower than original
  JS1 Lookup    2075.0000000000 9.095689% slower than original
All functions checked for accuracy; checking timing...
    original    2174.0000000000 0.000000% faster than original
       naive    2121.0000000000 2.437902% faster than original
      Edward    2088.0000000000 3.955842% faster than original
    dbasnett    2055.0000000000 5.473781% faster than original
  Jan Korous    2136.0000000000 1.747930% faster than original
     scottbb    2109.0000000000 2.989880% faster than original
         JS1    1855.0000000000 14.673413% faster than original
  JS1 Lookup    2149.0000000000 1.149954% faster than original
All functions checked for accuracy; checking timing...
    original    2073.0000000000 0.000000% faster than original
       naive    2154.0000000000 3.907381% slower than original
      Edward    1960.0000000000 5.451037% faster than original
    dbasnett    2288.0000000000 10.371442% slower than original
  Jan Korous    2032.0000000000 1.977810% faster than original
     scottbb    2091.0000000000 0.868307% slower than original
         JS1    2245.0000000000 8.297154% slower than original
  JS1 Lookup    2138.0000000000 3.135552% slower than original
All functions checked for accuracy; checking timing...
    original    2158.0000000000 0.000000% faster than original
       naive    2277.0000000000 5.514365% slower than original
      Edward    1840.0000000000 14.735867% faster than original
    dbasnett    2081.0000000000 3.568119% faster than original
  Jan Korous    2135.0000000000 1.065802% faster than original
     scottbb    2228.0000000000 3.243744% slower than original
         JS1    1969.0000000000 8.758109% faster than original
  JS1 Lookup    2010.0000000000 6.858202% faster than original


controller design of a REST API in PHP I'm building an example REST API in PHP in my local environment to learn how they work.  The starting point was an online tutorial.  The example is working fine; however, I have a hunch that there is room for improvement in the logic that handles the endpoints which is stored in a script called RestController.php.

Currently, I have the example working both via a UI and, separately, using Postman.  The pertinent lines in .htaccess file are:

RewriteRule ^mobile/?$ controllers/RestController.php [nc,L]
RewriteRule ^mobile/([0-9]+)/?$ controllers/RestController.php?id=$1 [nc,qsa,L]

Here is an example of a Postman test for the GET one mobile API endpoint:

The code that follows is RestController.php.  I'm wondering if there is a way to refactor this into a class but still have the ability to use Postman for testing the endpoints.  If so, how would one go about doing that?

<?php
require_once("../classes/MobileRestHandler.php");

# read raw data from request body and stuff it into $_POST
$_POST = json_decode(file_get_contents('php://input'), true);

# get parameter values according to the method
$method = $_SERVER['REQUEST_METHOD'];

# initialize and populate $parameters_* variables
if (in_array($method, ['GET', 'PUT', 'DELETE'])) {
    $id = isset($_GET["id"]) ? $_GET["id"] : "";
    $parameters_id_only = ['id' => $id];
}

if (in_array($method, ['POST', 'PUT'])) {
    $name = isset($_POST["name"]) ? $_POST["name"] : "";
    $model = isset($_POST["model"]) ? $_POST["model"] : "";
    $color = isset($_POST["color"]) ? $_POST["color"] : "";
    $parameters_no_id = ['name' => $name, 'model' => $model, 'color' => $color];
}

if ($method == 'PUT') {
    $parameters_all = array_merge($parameters_id_only, $parameters_no_id);
}

# this logic controls the RESTful services URL mapping
$mobileRestHandler = new MobileRestHandler();

if (!in_array($method, ['GET', 'POST', 'PUT', 'DELETE'])) {
    $statusCode = 404;
    $statusMessage = $mobileRestHandler->getHttpStatusMessage($statusCode);
    exit($statusCode . ' - ' . $statusMessage);
} else {
    if ($method == 'GET' && strlen($id) == 0) {
        $result = $mobileRestHandler->getAllMobiles();                      // handles method GET + REST URL /mobile/
    } else if ($method == 'GET') {
        $result = $mobileRestHandler->getMobile($parameters_id_only);       // handles method GET + REST URL /mobile/<id>/
    } else if ($method == 'POST') {
        $result = $mobileRestHandler->addMobile($parameters_no_id);         // handles method POST + REST URL /mobile/
    } else if ($method == 'PUT') {
        $result = $mobileRestHandler->editMobile($parameters_all);          // handles method PUT + REST URL /mobile/<id>/
    } else if ($method == 'DELETE') {
        $result = $mobileRestHandler->deleteMobile($parameters_id_only);    // handles method DELETE + REST URL /mobile/<id>/
    }

    echo $result;

}
?>

Unless I am missing something in your requirements, I don't think there is a terrific need for the overhead of a class. (I could be wrong)

I do spy some opportunities to ratchet up your code though.  Consider these points and if you decide to further develop your script into a class, then this should move you in that direction with some single responsibility thinking.

Don't extract data until you are sure that you will use it.
Don't Repeat Yourself (DRY). Don't make the server repeat any operations that provide results that its already provided.
switch() blocks are verbose, but they are appropriately used when you are performing multiple evaluations on the same variable.

require_once("../classes/MobileRestHandler.php");

function getId() {
    return ['id' => $_GET["id"] ?? ''];
}

function getPost() {
    $defaults = array_fill_keys(['name', 'model', 'color'], '');
    $posted = json_decode(file_get_contents('php://input'), true);
    return array_replace($defaults, array_intersect_key($posted, $defaults));
}

function getIdAndPost() {
    return array_merge(getId(), getPost());
}

switch ($_SERVER['REQUEST_METHOD']) {
    case 'GET':
        $result = empty($_GET['id'])
            ? $mobileRestHandler->getAllMobiles();
            : $mobileRestHandler->getMobile(getId());
        break;
    case 'POST':
        $result = $mobileRestHandler->addMobile(getPost());
        break;
    case 'PUT':
        $result = $mobileRestHandler->editMobile(getIdAndPost());
        break;
    case 'DELETE':
        $result = $mobileRestHandler->deleteMobile(getId());
        break;
    default:
        $result = '404 - ' . $mobileRestHandler->getHttpStatusMessage(404);
}

exit($result);

Admittedly, my untested script doesn't validate, sanitize, or throw any errors, but your script didn't seem to mind these relevant topics.

p.s. For the record, I don't endorse the technique of deleting a record based on $_GET data -- if a crawler would hapen to come upon your script and some valid ids, it could vanquish records during the simple act of crawling.  When manipulating data stores, always use $_POST.


Triggering approach for an input box I am setting a value to input box and trigger to hear the change. I would also like to always hear the input change, and for that I made this function.

Can any expert please verify the way I did is correct? Or is it further tune-able?

var page = function () {
    return {
        init : function () {
            var input = $("#pNo");
            $(document).on("pageChange", this.onPageChange.bind(this));
            $(input).val(1).trigger({type:'pageChange',element:input});
            $(input).bind("change paste keyup",function() { $.event.trigger({type: "pageChange", element:input})});
        },

        onPageChange : function (e) {
            console.log($(e.element).val());
        }
    }
}();

page.init();

Live Demo

My suggestion is to reduce the code:

var page = {
  init: function(selector) {
    $(selector).on('change paste keyup', page.onPageChange);
  },
  onPageChange: function(e) {
    console.log($(e.target).val());
  }
};

page.init('#pNo');


Saving posts, then tweeting them Here's a part of my controller and it's getting quite lengthy (the code works).

Would this code slow down the performance of my website? Can it be cleaned up and be written more efficiently?

def create
  @post = current_user.posts.build(params[:post])

  if @post.save
    if @post.verify
      UserMailer.verify_email(@user).deliver
      redirect_to root_path
    else
      flash[:success] = "Posted"

      if @post.content.to_s.length > 140 and @post.image.present?
        @twitter = Twitter::Client.new
        @twitter.update(@post.content.truncate(120)+"... " +@post.image.path.to_s)
        redirect_to root_path
      elsif @post.content.to_s.length > 140
        @twitter = Twitter::Client.new
        @twitter.update(@post.content.truncate(140))
        redirect_to root_path
      elsif @post.content.to_s.length <= 140 and @post.image.present?
        @twitter = Twitter::Client.new
        @twitter.update(@post.content.truncate(120)+""+@post.image.path.to_s)
        redirect_to root_path
      else @post.content.to_s.length <= 140
        @twitter = Twitter::Client.new
        @twitter.update(@post.content)
        redirect_to root_path
      end
    end
  else
    @feed_items = []
    render 'static_pages/home'
  end
end

Roughly:

twitter = Twitter::Client.new

img, max = @post.image ? [@post.image.path.to_s, 120] : ["", 140]
twitter.update("#{@post.content.truncate(max)}#{img}")

redirect_to root_path

All the Twitter stuff shouldn't be in the controller, rather in a utility class. This makes testing easier, makes plugging in implementations easier, makes extension easier, and so on.

Also, truncate should be already adding the "..." if truncation was necessary.

Personally, I prefer Amit's answer. IMO it's easier to read and more obvious than mine.

I would probably tighten it up a bit after an initial review to something like this:

s = @post.image ? "#{@post.content.truncate(120)}... #{@post.image.path.to_s}"
                :    @post.content.truncate(140)
twitter.update(s)

It's formatted to highlight the similarities between the two, another option is to use string interpolation again, but it seems redundant in this particular case.

(Obviously the string creation would go into a method in some sort of support decorator etc.)

All the above is untested, but probably close.


Resizing and recoloring a logo using PHP GD Library I have the following code, that works very well. However I'm about to launch it to 5000 users and will no doubt take a beating early on. I want to ensure that if there are any opportunities to streamline so the code executes better or faster, that I do not miss them.

I've only pasted the PHP GD portion of the code, as the rest of my page is fine (some basic HTML)

$position = strpos($size, 'x');
$width = substr($size,0,$position);
$height = substr($size,$position+1);

$logo_image = imagecreatefrompng($image);
imagealphablending( $logo_image, false );
imagesavealpha( $logo_image, true );

$logo_width = imagesx($logo_image);
$logo_height = imagesy($logo_image);

if(($width>=601)&&($width<=1080)) {
    $percent = $width/$logo_width;
} elseif($width>=1081) {
    $percent = 1.2;
    $font_size = 28;
}

$logo_new_width = $logo_width*$percent;
$logo_new_height = $logo_height*$percent;

$base_image = imagecreatetruecolor($width, $height);

$logo_x = (($width/2)-($logo_new_width/2));
$logo_y = (($height/2)-($logo_new_height/2));

$text_box = imagettfbbox($font_size,$angle,$font,$text);
$text_width = $text_box[2]-$text_box[0];
$text_height = $text_box[3]-$text_box[1];

$x = ($width/2) - ($text_width/2);
if($height==768) {
    $y = (($height/2)+($logo_new_height/2));
} else {
    $y = (($height/2)+($logo_new_height));
}

//set overall background color here based on user selection
if($theme=='plum') {
    $fill_color = imagecolorallocate($base_image, 93, 62, 93);
} elseif($theme=='black') {
    $fill_color = imagecolorallocate($base_image, 0, 0, 0);
} elseif($theme=='white') {
    $fill_color = imagecolorallocate($base_image,255,255,255);
} elseif($theme=='gray') {
    $fill_color = imagecolorallocate($base_image,149,150,153);
} elseif($theme=='charcoal') {
    $fill_color = imagecolorallocate($base_image,78,80,84);
} elseif($theme=='warm') {
    $fill_color = imagecolorallocate($base_image,255,79,31);
}

//Set the Text Color
if($textC=='black') {
    $text_color = imagecolorallocate($base_image,0,0,0);
} elseif($textC=='red') {
    $text_color= imagecolorallocate($base_image,255,79,31);
} elseif($textC=='white') {
    $text_color = imagecolorallocate($base_image,255,255,255);
} elseif($textC=='plum') {
    $text_color = imagecolorallocate($base_image, 93, 62, 93);
} elseif($textC=='gray') {
    $text_color = imagecolorallocate($base_image,78,80,84);
}

imagefill($base_image, 0, 0, $fill_color);
imagesavealpha($base_image, TRUE);

//set text color here
imagettftext($base_image,$font_size+1,0,$x,$y,$text_color,$font,$text );

imagecopyresampled($base_image, $logo_image, $logo_x, $logo_y, 0, 0,$logo_new_width, $logo_new_height, $logo_width, $logo_height);

$num_str = "";
$charsRand = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";
for($i=0;$i<6;$i++)
    $num_str.=substr($charsRand,rand(0,strlen($charsRand)),1);

$filename_create = substr($text,0,strpos($text, ".")).'_'.$num_str.'.png';
imagepng($base_image,'images_created/'.$filename_create,1);
$file_size = filesize('images_created/'.$filename_create);

Did I do anything that will cause me grief? Any tips for me to optimize this portion of the code ?

Look forward to any and all comments. This was my first go around with GD.

If you have ImageMagick available, I would strongly consider moving towards that library - not necessarily for any performance/quality gains (I think this can vary between libraries based on use case) - but because it presents an object oriented interface.  Right now, your code is very much procedural in nature and you might find it harder to maintain and reuse over time.

From a performance standpoint, your best best is to actually load test your application to see if it is going to meet your needs.  I am guessing when you say 5000 users, you don't mean 500 concurrent users.  How many users do you truly expect executing this script concurrently? How much memory does this take on typical execution?  How does that compare against your server hardware?  Is the user experience on your hardware (i.e. the time that this takes to execute) appropriate?  Does that time to complete change under concurrent load?

Really your performance is going to have more to do with the questions I just asked than your code.

You need to DRY (Don't Repeat Yourself) up your code.  You can do this through some configuration and use of variables:

For example:

$background_colors = [
    'plum' => ['red' => 93, 'green' => 62, 'blue' => 93],
    ...
];
$text_colors = [
    'black' => ['red' => 0, 'green' => 0, 'blue' => 0],
    ...
];

$fill_color = imagecolorallocate(
    $base_image,
    $background_colors[$theme]['red'],
    $background_colors[$theme]['green'],
    $background_colors[$theme]['blue'],
);

$text_color = imagecolorallocate(
    $base_image,
    $background_colors[$textC]['red'],
    $background_colors[$textC]['green'],
    $background_colors[$textC]['blue'],
);

This rids you of all those if-else conditionals.  And also gives you the ability to lookup a provided background/text color string to determine if it exists in configuration.  This could be useful in setting defaults.  For example:

if(!empty($background_colors[$theme])) {
    $theme = $default_theme;
}

You have a lot of hard-coded "configuration" values (the colors as noted before, image size thresholds, font size, file name string length, etc.) that are simply hard-coded into your script. Personally, I would look to do all of this in a class context, so you have means to store default /overridden configuration as well as to get your main logic out of the main script path and provide you greater re-use down the line (like when you want to do this same thing on another site).

By the way, your $font_size variable only seems to be defined on one half of a conditional which could be problematic (though perhaps it is doe that is not shown).

You should actively try to design away else constructs.  When you get in the habit of doing this, you will find that in most cases these are not really necessary.

Why should you do this?  Because the more code paths you have, the more fragile and harder to maintain/debug your code will tend to be (the term cyclomatic complexity is used to describe this).

So when you see yourself with a conditional like this:

if(($width>=601)&&($width<=1080)) {
    $percent = $width/$logo_width;
} elseif($width>=1081) {
    $percent = 1.2;
    $font_size = 28;
}

Consider changing it to something like:

$percent = 1.2;
$font_size = 28;
if(($width>=601)&&($width<=1080)) {
    $percent = $width/$logo_width;
}

This is easier to read and reduces your potential code paths by one.  This is also at the heart of my early comment to get rid of all those if-else conditions.  They simply are not needed in this case and can be designed away.

Similarly,

if($height==768) {
    $y = (($height/2)+($logo_new_height/2));
} else {
    $y = (($height/2)+($logo_new_height));
}

Could become:

  $y = (($height/2)+($logo_new_height));
  if($height==768) {
    $y = (($height/2)+($logo_new_height/2));
  }

if($height==768) {

Do you really have a use case for this specific height?  Should this be > or < comparison?

You should consider using exact comparisons (=== and !==) as your default means for comparison as opposed to the loose comparisons you are doing.  Loose comparisons tend to introduce bugs into code around unexpected truthy/falsey conditions.  I find myself using them in only very specific cases where a truly loose comparison is warranted.

Some stylistic thoughts:

Use space around your comparison operators for better readability.
Be consistent in using curly braces around flow-control structures (conditionals, loops, etc.).
Try to limit the length of your lines to less than 80 characters to make the code easier to read. Your imagecopyresampled() call is pretty long and could easily be broken across multiple lines for better readability.


Performance of an OLX crawler I'm learning concurrency and asynchronous programming in C#. For a better grasp on the matter I'm trying to create an OLX Crawler. My issue here is the performance: It takes roughly 159 minutes to crawl 17.901 links.

The method which crawls each item is this:

public async Task FetchItemsData()
    {
        var itemsTask = new List<Task<string>>();
        string itemUrl = string.Empty;
        var semaphore = new SemaphoreSlim(150);

        while (this.queue.TryDequeue(out itemUrl))
        {
            await semaphore.WaitAsync();
            try
            {
                var source =  Task.Run( () => this.GetSource(itemUrl) );

                itemsTask.Add(source);

                await Task.Delay(1);
            }
            finally
            {
                if (itemsTask.Count == 150)
                {
                    await Task.WhenAll(itemsTask);
                    ItemSourceParser itemParser = new ItemSourceParser();

                    foreach (var task in itemsTask)
                    {
                        await task.ContinueWith(async _ =>
                        {
                            itemParser.InitializeNewItem(_.Result);
                            itemParser.TryToParse();
                            Console.WriteLine(itemParser.data["Id"]);
                            this.database.InsertItem(itemParser.data);
                            itemParser.RefreshData();
                            await Task.Delay(1);
                        });
                    }

                    itemsTask.Clear();
                }
                semaphore.Release();
            }
        }

       /* FURTHER PROCESSING */
    }

The queue is a ConcurrentQueue which holds the item string. In the finally block I'm trying to await 150 concurrent tasks to not overload the server.

The GetSource method is here:

public async Task<string> GetSource(string url)
    {
        HttpResponseMessage responseMessage = new HttpResponseMessage();
        try
        {
            this.ProgressUpdater.ProgressBarUpdater.Report(1);
            responseMessage.Version = HttpVersion.Version10;
            responseMessage = await this.client.GetAsync(url);
        }
        catch (Exception e)
        {
            this.ProgressUpdater.LogUpdater.Report("Connection timeout on: " + url + ". Retrying after 0.5 seconds");
            await Task.Delay(1500);
            this.ProgressUpdater.ProgressBarUpdater.Report(-1);
            return await this.GetSource(url);
        }
        return await responseMessage.Content.ReadAsStringAsync();
    }

I'm guessing that something is not right. Is there any redundant code that needs to be removed or something that I should have done so that the crawler has a better performance?

You don't have to use a SemaphoreSlim because a Task is not equivalent to a Thread. It is a more abstract kind of "working item" that will be spreaded to real threads by the TPL. If you start 1000 tasks it does not mean that you create 1000 threads. When starting a task like you did (without option TaskCreationOptions.LongRunning), it will be executed on threads from the TreadPool. Therfore, the Threadpool is managing the number of threads for you and you can just start 17.901 tasks at once.

What is await Task.Delay(1) for?

If there is no reason for it, it just produces unnecessary overhead

var source =  Task.Run( () => this.GetSource(itemUrl) );

You don't need to wrap the returning taks in a new task. The following has less overhead and is functional equivalent:

var source =  this.GetSource(itemUrl);

Move the code from the finally block to the call of GetSourceAsync. There is no need to attach it via ContinueWith in the finally block.

All the points above applied yields to something like:

async Task FetchItemsDataAsync()
{
    var itemsTask = new List<Task<string>>();
    var itemParser = new ItemSourceParser();

    while (this.queue.TryDequeue(out itemUrl))
    {
        var task = Task.Run(async() =>
        {
            var result = await this.GetSourceAsync(itemUrl);
            itemParser.InitializeNewItem(result);
            itemParser.TryToParse();
            Console.WriteLine(itemParser.data["Id"]);
            this.database.InsertItem(itemParser.data);
            itemParser.RefreshData();
        }
        itemsTask.Add(task);
    }

    return Task.WhenAll(itemsTask);
}


Generating a table based on an array of objects I have a React component that generates a table based on an array of objects, which gets populated with data in an object.

I'm trying to learn as much as I can about React.js / JavaScript. Could anyone with more experience in this area show me a way to simplify the code that I have here? Or if there are any other 'fancy' ways of doing what is being done in the code?

App.js

import React, { Component } from 'react';
import Table from './Table';

//Columns defines table headings and properties to be placed into the body
let columns = [
  {
    heading: 'Name',
    property: 'name'
  },
  {
    heading: 'Age',
    property: 'age'
  },
  {
    heading: 'Sex',
    property: 'sex'
  },
  {
    heading: 'Breed',
    property: 'breed'
  },
]

//Data is the array of objects to be placed into the table
let data = [
  {
    name: 'Sabrina',
    age: '6',
    sex: 'Female',
    breed: 'Staffordshire'
  },
  {
    name: 'Max',
    age: '2',
    sex: 'Male',
    breed: 'Boxer'
  }
]

class App extends Component {

  render() {
    return (
      <>

      </>
    );
  }
}

export default App;

Table.js

import React, { Component } from 'react';

class Table extends Component {

  buildTable = (columns, data, key) => {
    let headerRow = [];
    let dataRows = [];

    //Build the table header
    columns.forEach (col => {
      headerRow.push(
        <th key={`header-${col.heading}`}>{col.heading}
      );
    });

    //Build the rows
    data.forEach(item => {
      let dataCells = [];

      //Build cells for this row
      columns.forEach (col => {
        dataCells.push(
          <td key={`${item[key]}-${col.property}`}>{item[col.property]}
        );
      });

      //Push out row
      dataRows.push(
        <tr key={`${item[key]}-row`}>{dataCells}

      )
    });

    return(
      <>

{headerRow}

          {dataRows}

      </>
    );
  };

  render() {
    const {
      columns,
      data,
      propertyAsKey
    } = this.props;

    return (

        {this.buildTable(columns, data, propertyAsKey)}

    );
  }
}

export default Table;

TL;DR Fully functionnal example :

//Columns defines table headings and properties to be placed into the body
const columns = [{ heading: 'Name', property: 'name' }, { heading: 'Age', property: 'age' }, { heading: 'Sex', property: 'sex' }, { heading: 'Breed', property: 'breed' },]

//Data is the array of objects to be placed into the table
const data = [{ name: 'Sabrina', age: '6', sex: 'Female', breed: 'Staffordshire' }, { name: 'Max', age: '2', sex: 'Male', breed: 'Boxer' }]

const App = props =>

const Table = ({ columns, data, propertyAsKey }) =>

{columns.map(col => <th key={`header-${col.heading}`}>{col.heading}	)}

            {data.map(item =>
                <tr key={`${item[propertyAsKey]}-row`}>
                    {columns.map(col => <td key={`${item[propertyAsKey]}-${col.property}`}>{item[col.property]}	)}

            )}

ReactDOM.render(<App/>, document.getElementById('root'))

The main thing that would highly reduce this code is the map function. This function can be applied on an array, execute a callback on every item in the array and returns a new array.

By using it you can reduce the following code :

let headerRow = [];

columns.forEach(col => {
    headerRow.push(
        <th key={`header-${col.heading}`}>{col.heading}
    );
});

Can be reduced to this with the exact same result :

const headerRow = columns.map(col => <th key={`header-${col.heading}`}>{col.heading}	)

You can now include this function directly into your JSX :

{columns.map(col => <th key={`header-${col.heading}`}>{col.heading}	)}

And the same thing with nested mapping for the body :

    {data.map(item =>
        <tr key={`${item[key]}-row`}>
            {columns.map(col => <td key={`${item[key]}-${col.property}`}>{item[col.property]}	)}

    )}

Your full buildTable function is now reduced to the following :

buildTable = (columns, data, key) => {
    return (
        <>

{columns.map(col => <th key={`header-${col.heading}`}>{col.heading}	)}

                {data.map(item =>
                    <tr key={`${item[key]}-row`}>
                        {columns.map(col => <td key={`${item[key]}-${col.property}`}>{item[col.property]}	)}

                )}

        </>
    );
};

If you want to go a little further you can also delete this function and embed this code in your render function JSX. I will also convert your component into a stateless function, since you are not using any state value :

const Table = ({ columns, data, propertyAsKey }) => //Deconstructs your props

{columns.map(col => <th key={`header-${col.heading}`}>{col.heading}	)}

            {data.map(item =>
                <tr key={`${item[propertyAsKey]}-row`}>
                    {columns.map(col => <td key={`${item[propertyAsKey]}-${col.property}`}>{item[col.property]}	)}

            )}

(You may add brackets and a return statement after the arrow sign, its up to you)

It is also not required to have a fragment as a parent element in your render, as long as there is a single element, it is fine :

class App extends React.Component {
  render() {
    return (

    );
  }
}

Your App component could also be converted to a stateless component:

const App = props =>


Questions & Responses: Let me tell you about you Please review this code.

import java.util.Scanner;
import java.util.* ;
public class LMComplex {
Scanner name  = new Scanner(System.in);
Scanner age  = new Scanner(System.in);
Scanner gender  = new Scanner(System.in);
Scanner height  = new Scanner(System.in);
String usrname;
String usrage;
String usrgender;
Double usrheight;
public void main(){
System.out.println("Enter your name: ");
usrname = name.nextLine();
System.out.println("Enter your age(Numbers): ");
usrage = age.nextLine();
System.out.println("Enter your gender(male/female): ");
usrgender = gender.nextLine();
System.out.println("Enter your height(feet.inches . 5 feet 5 inches = 5.5): ");
usrheight = height.nextDouble();
Double rusrheight = Math.ceil(usrheight);
if(rusrheight > usrheight)
{
rusrheight = rusrheight - 1;
}
else
{

}
Double rusrheightininches = usrheight - rusrheight;
if(rusrheightininches >= 0.10 && rusrheightininches < 0.12)
{
rusrheightininches = rusrheightininches * 100;
} else {
rusrheightininches = rusrheightininches * 10;
}
Double rusrheightinincheschecker = rusrheightininches;
rusrheightininches = Math.ceil(rusrheightininches);
if(rusrheightininches > rusrheightinincheschecker)
{
rusrheightininches = rusrheightininches - 1;
}
else
{

}
System.out.println("Hii " + usrname + ". I am Cartic, a basic textual virtual personal assistant. I do know that you are " + usrage +" years old. You are a " + usrgender + " and are " + rusrheight + " feet " + rusrheightininches + " inch(es) tall");
}
}

Indenting is the first part of common practice that you should consider when sharing your code with anyone. Here is your code, which I have put though the 'standard' indenting function of Eclipse:

import java.util.Scanner;

public class LMComplex {
    Scanner name = new Scanner(System.in);
    Scanner age = new Scanner(System.in);
    Scanner gender = new Scanner(System.in);
    Scanner height = new Scanner(System.in);
    String usrname;
    String usrage;
    String usrgender;
    Double usrheight;

    public void main() {
        System.out.println("Enter your name: ");
        usrname = name.nextLine();
        System.out.println("Enter your age(Numbers): ");
        usrage = age.nextLine();
        System.out.println("Enter your gender(male/female): ");
        usrgender = gender.nextLine();
        System.out.println("Enter your height(feet.inches . 5 feet 5 inches = 5.5): ");
        usrheight = height.nextDouble();
        Double rusrheight = Math.ceil(usrheight);
        if (rusrheight > usrheight)
        {
            rusrheight = rusrheight - 1;
        }
        else
        {

        }
        Double rusrheightininches = usrheight - rusrheight;
        if (rusrheightininches >= 0.10 && rusrheightininches < 0.12)
        {
            rusrheightininches = rusrheightininches * 100;
        } else {
            rusrheightininches = rusrheightininches * 10;
        }
        Double rusrheightinincheschecker = rusrheightininches;
        rusrheightininches = Math.ceil(rusrheightininches);
        if (rusrheightininches > rusrheightinincheschecker)
        {
            rusrheightininches = rusrheightininches - 1;
        }
        else
        {

        }
        System.out.println("Hii " + usrname + ". I am Cartic, a basic textual virtual personal assistant. I do know that you are " + usrage
                + " years old. You are a " + usrgender + " and are " + rusrheight + " feet " + rusrheightininches + " inch(es) tall");
    }
}

Now, it is much easier to see the structure.

Java code-style guidelines put the { opening brace on the same line as the control statement, so, for example, the code:

        if (rusrheightininches >= 0.10 && rusrheightininches < 0.12)
        {
            rusrheightininches = rusrheightininches * 100;
        } else {
            rusrheightininches = rusrheightininches * 10;
        }

should be:

        if (rusrheightininches >= 0.10 && rusrheightininches < 0.12) {
            rusrheightininches = rusrheightininches * 100;
        } else {
            rusrheightininches = rusrheightininches * 10;
        }

Also, in the previous if statement you  have an empty else block.... you can just remove it:

        if (rusrheight > usrheight)
        {
            rusrheight = rusrheight - 1;
        }
        else
        {

        }

becomes:

        if (rusrheight > usrheight) {
            rusrheight = rusrheight - 1;
        }

As for your variables, they are nice descriptive names, but, in Java, it is common to use what is called 'camelCase', where the first letter is lower-case, and the subsequent first-word-letters are Capitalized.

Also, while it is often convenient to shorten long parts of variables, the abbreviation of 'user' to 'usr' is not saving you much.....

So, for example, your variables :

String usrname;
String usrage;
String usrgender;
Double usrheight;

would be

String userName;
String userAge;
String userGender;
Double userHeight;

I would recommend that you use an IDE (Eclipse/IntelliJ/Netbeans) to help you get these things right. There is some debate about whether new programmers should use an IDE or not, because they make some things really easy and you may miss understanding some of the basic requirements of the language.... but, I believe that the IDE's allow you to focus on the stuff that is more important, like the code content.


New way of doing the A* Algorithm I wrote a Python script for implementing an A* algorithm. Map, Start and Goal are given.

The code works well as far as I tested it but I want to get feedback from the best out there too. So I am sharing the code here. Map is given to you as pickle file.

In the code M is:

class Map:
    def __init__(self, G):
        self._graph = G
        self.intersections = networkx.get_node_attributes(G, "pos")
        self.roads = [list(G[node]) for node in G.nodes()]

Graph = pickle.load(pickleFile)
M = Map(Graph)

Start/Goal are two integers representing nodes.

def shortest_path(M, start, goal):
    explored = set([start])
    frontier = dict([(i,[start]) for i in M.roads[start]]) if start!=goal else {start:[]}
    while frontier:
        explore = g_h(frontier,goal,M)
        for i in [i for i in M.roads[explore] if i not in frontier.keys()|explored]:
            frontier[i]=frontier[explore]+[explore]
        frontier = cleanse_frontier(frontier,M)
        if explore==goal:return frontier[explore]+[explore]#break when goal is explored.
        explored.add(explore)
        del frontier[explore]#once explored remove it from frontier.

def g_h(frontier,goal,M):
    g_h = dict([(path_cost(frontier[node]+[node],M)+heuristic(node,goal,M),node) for node in frontier])
    return g_h[min(g_h)]

def heuristic(p1,p2,M):#Euclidean Heuristic from the node to the goal node
    M=M.intersections
    return ((M[p1][0]-M[p2][0])**2+(M[p1][1]-M[p2][1])**2)**0.5

def path_cost(path,M,cost=0):#Euclidean distance for the path
    M=M.intersections
    for i in range(len(path)-1):
        cost += ((M[path[i]][0]-M[path[i+1]][0])**2+(M[path[i]][1]-M[path[i+1]][1])**2)**0.5
    return cost

def cleanse_frontier(frontier,M):
    """If any node can be removed from the frontier if that can be reached
    through a shorter path from another node of the frontier, remove it
    from frontier"""
    for node in list(frontier):
        for i in [i for i in frontier if i!=node]:
            if node not in frontier:continue
            if frontier[i]==frontier[node]:continue
            if i not in M.roads[node]:continue
            if path_cost(frontier[node]+[node]+[i],M)<path_cost(frontier[i]+[i],M):
                del frontier[i]
    return frontier

A suggestion I have is to use a heap data structure to store the costs, so that min can be calculated in logarithmic time.


Merge Sort Implementation: Space Usage I have made a merge sort algorithm but am unsure of the 'Space Usage' of the algorithm.

public class Sorting {

    public static void mergeSort(int[] arr) {
        if (arr.length == 1) {
            return;
        }
        int[] newArrLeft = new int[arr.length / 2];
        int[] newArrRight = new int[arr.length - (arr.length / 2)];
        int currentRight = 0;
        for (int i = 0; i < arr.length; i++) {
            if (i < arr.length / 2) {
                newArrLeft[i] = arr[i];
            } else {
                newArrRight[currentRight++] = arr[i];
            }
        }
        mergeSort(newArrLeft);
        mergeSort(newArrRight);
        merge(newArrLeft, newArrRight, arr);

    }

    private static void merge(int[] arrLeft, int[] arrRight,
            int[] sortedValuesArr) {
        int currentLeft = 0;
        int currentRight = 0;
        int currentSorted = 0;
        while (currentLeft < arrLeft.length && currentRight < arrRight.length) {
            if (arrLeft[currentLeft] < arrRight[currentRight]) {
                sortedValuesArr[currentSorted++] = arrLeft[currentLeft++];
            } else {
                sortedValuesArr[currentSorted++] = arrRight[currentRight++];
            }
        }

        while (currentLeft < arrLeft.length) {
            sortedValuesArr[currentSorted++] = arrLeft[currentLeft++];
        }
        while (currentRight < arrRight.length) {
            sortedValuesArr[currentSorted++] = arrRight[currentRight++];
        }

    }
}

I am specifically inquiring about:

int[] newArrLeft = new int[arr.length / 2];
int[] newArrRight = new int[arr.length - (arr.length / 2)];
int currentRight = 0;
for (int i = 0; i < arr.length; i++) {
    if (i < arr.length / 2) {
        newArrLeft[i] = arr[i];
    } else {
        newArrRight[currentRight++] = arr[i];
    }
}

Is the above code wasting space? Is there a better implementation?

Is the above code wasting space?

Yes. As @cHao pointed out in a comment, you are using \$O(N log N)\$ space. You can do mergesort in \$O(N)\$ space.

  Is there a better implementation?

Yes. The biggest problem wrt both time and space efficiency is that you are unnecessarily allocating and copying auxiliary arrays.

You can instead create 1 auxiliary array and pass the same array around together with the interval to be sorted, or merged.

Your methods would look like these, some implementation left exercise :

public static void mergeSort(int[] arr) {
    mergeSortBetween(arr, new int[arr.length], 0, arr.length -1);
}

private static void mergeSortBetween(int[] arr, int[] aux,
    int startIndex, int endIndex) {
    if (...) {
        return;
    }
    //...
    mergeSortBetween(arr, aux, startIndexLeft, endIndexLeft);
    mergeSortBetween(arr, aux, startIndexRight, endIndexRight);
    merge(arr, aux,
        startIndexLeft, endIndexLeft,
        startIndexRight, endIndexRight);
}

private static void mergeBetween(int[] arr, int[] aux,
    int startIndexLeft, int endIndexLeft,
    int startIndexRight, int endIndexRight) {
    // only need to merge consecutive chunks
    assert startIndexRight = endIndexLeft + 1;
    //merge into aux
    while (currentLeft < startIndexLeft && currentRight < endIndexRight) {
        if (...)
            aux[...] = arr[...]
        else
            aux[...] = arr[...]
    }
    while (currentLeft < endIndexLeft) {
        aux[...] = arr[...]
    }
    while (currentRight < endIndexRight) {
        aux[...] = arr[...]
    }

    //copy merged values back into arr
    System.arraycopy(aux, ..., arr, ..., ...);
}


Randomly selecting an adjective and noun, combining them into a message This is my first Rust program.  I have previously written a lot of Python, and some C.  I would like some advice on whether I am doing things idiomatically, and any improvements I could make!  I also appreciate any suggestions with regards to style.

The code takes two text files as cmd line args.  One file contains an adjective on each line, the other file contains a noun on each line.  The code reads the files and randomly chooses an entry from each.  It then combines them to give a message in this format, which is printed:

"You are a adjective noun"

For example:

"You are a beautiful avocado"

Here is the code:

extern crate rand;

use rand::Rng;
use std::env;
use std::error::Error;
use std::fs::File;
use std::io::prelude::*;
use std::path::Path;

/// Takes file path, returns a vector of strings, each string a line in file.
///
/// # Arguments
/// * `file_path: &Path` - The path to the file to read
///
/// # Return value
/// Returns `Vec<String>` where each string is a line in the file
fn read_lines(file_path: &Path) -> Vec<String> {
    let content = read_file(file_path);
    split_lines(content)
}

/// Takes file path, returns corresponding `File` object.
///
/// # Arguments
/// * `file_path: &Path` - The path to file to read
///
/// # Return value
/// Returns `File` corresponding to `file_path`
fn open_file(file_path: &Path) -> File {
    let display = file_path.display();
    let file = match File::open(file_path) {
        Err(why) => panic!("Couldn't open file {}: {}", display,
                           Error::description(&why)),
        Ok(file) => file,
    };
    file
}

/// Takes `file path, returns string containing the file's content.
///
/// # Arguments
/// * `file_path: &Path` - The path of file to read
///
/// # Return value
/// Returns `String` containing contents of file
fn read_file(file_path: &Path) -> String {
    let mut file = open_file(file_path);
    let display = file_path.display();
    let mut content = String::new();
    match file.read_to_string(&mut content) {
        Err(why) => panic!("Couldn't read file {}: {}", display,
                           Error::description(&why)),
        Ok(content) => content,
    };
    content
}

/// Split a string on newlines and return vector of resulting strings.
///
/// # Arguments
/// * `string: String` - string to be split
///
/// # Return value
/// Returns `Vec<String>`
fn split_lines(string: String) -> Vec<String> {
    string
        .lines()
        .map(ToOwned::to_owned)
        .collect()
}

/// Chooses random adjective and noun from file, combines into message.
///
/// This function does not return the message, but prints it to stdout.
/// The message is in the format "You are a <adjective> <noun>."
///
/// # Arguments
/// * `adj: Vec<String>` - vector of adjectives
/// * `nouns: Vec<String` - vector of nouns
///
/// # Return value
/// ()
fn selfcare(adj: Vec<String>, nouns: Vec<String>) {
    let adjective = rand::thread_rng().choose(&adj).unwrap();
    let noun = rand::thread_rng().choose(&nouns).unwrap();
    println!("You are a{} {}", adjective, noun)
}

/// Print random inspiring message in format "You are a <adjective> <noun>".
///
/// Takes command line args `adj_file` and `noun_file`, respectively the paths
/// to the file containing the adjectives and the containing the nouns.
fn main() {
    let mut args: Vec<String> = env::args().collect();
    if args.len() == 1 {
        args.push("adjectives.txt".to_string());
        args.push("nouns.txt".to_string());
    }
    let adj_file = Path::new(&args[1]);
    let noun_file = Path::new(&args[2]);
    let adj = read_lines(adj_file);
    let nouns = read_lines(noun_file);
    selfcare(adj, nouns)
}

Everything looks pretty straight-forward, there are just some idiomatic changes to be made.

The function documentation, especially the argument and return value documentation, is mostly useless. In a statically typed language, the types provide a large amount of implicit documentation. Variable and method names tell another large chunk. Don't repeat all that compiler-checked documentation in your prose. Maybe even think about introducing new types for documentation purposes!
The type is also encoded in the name in certain places, which risks being redundant. — file_path: &Path.
Instead of requiring &Path, use AsRef<Path> to accept more types and improve the ergonomics at the call site.
It's inefficient to evaluate the path's display before it's needed. Since it's only used for error cases, only evaluate it in error cases. This is another way of saying to scope variables as tightly as possible.
There's no reason to create a variable to return it in the next statement; just return it directly — match {...} instead of let foo = match {...}; foo.
Call trait methods directly when possible, instead of using the Universal Function Call Syntax (UFCS) — foo.description() instead of Error::description(&foo).
read_to_string doesn't return an Ok(content), it returns the number of bytes read. This variable name is misleading.
If only one branch of a match statement is used, it's better to use an if let statement.
Functions shouldn't take a String if they just reading from it, it's better to take a &str as it is more accepting.
Functions shouldn't take a Vec<T> if they just reading from it, it's better to take a &[T] as it is more accepting.
Use expect instead on unwrap as it will help you find logic errors much easier when they happen. They may also help end users figure out problems on their own.
There's no need to specify the item type when collecting into a collection like a vector. You can use the placeholder _ instead — Vec<_>.
selfcare is confusingly named.
It's better to get the random number generator just once and reuse it.
When writing the sentence to the output, the code doesn't care if it's a string, just that it's printable. Accept a generic type that is printable instead.

extern crate rand;

use rand::Rng;
use std::env;
use std::error::Error;
use std::fs::File;
use std::io::prelude::*;
use std::path::Path;

fn read_lines<P>(file_path: P) -> Vec<String>
    where P: AsRef<Path>
{
    let content = read_file(file_path);
    split_lines(&content)
}

fn open_file<P>(file_path: P) -> File
    where P: AsRef<Path>
{
    let file_path = file_path.as_ref();
    match File::open(file_path) {
        Err(why) => panic!("Couldn't open file {}: {}", file_path.display(), why.description()),
        Ok(file) => file,
    }
}

fn read_file<P>(file_path: P) -> String
    where P: AsRef<Path>
{
    let file_path = file_path.as_ref();
    let mut file = open_file(file_path);
    let mut content = String::new();
    if let Err(why) = file.read_to_string(&mut content) {
        panic!("Couldn't read file {}: {}", file_path.display(), why.description());
    }
    content
}

fn split_lines(string: &str) -> Vec<String> {
    string
        .lines()
        .map(ToOwned::to_owned) // Nice
        .collect()
}

/// Chooses random adjective and noun from file, combines into message.
///
/// This function does not return the message, but prints it to stdout.
/// The message is in the format "You are a <adjective> <noun>."
fn selfcare<S>(adj: &[S], nouns: &[S])
    where S: std::fmt::Display
{
    let mut rng = rand::thread_rng();
    let adjective = rng.choose(adj).expect("No adjective found");
    let noun = rng.choose(nouns).expect("No noun found");
    println!("You are a {} {}", adjective, noun)
}

/// Print random inspiring message in format "You are a <adjective> <noun>".
///
/// Takes command line args `adj_file` and `noun_file`, respectively the paths
/// to the file containing the adjectives and the containing the nouns.
fn main() {
    let mut args: Vec<_> = env::args().collect();
    if args.len() == 1 {
        args.push("adjectives.txt".to_string());
        args.push("nouns.txt".to_string());
    }
    let adj = read_lines(&args[1]);
    let nouns = read_lines(&args[2]);
    selfcare(&adj, &nouns);
}

I'd also probably roll all the file stuff into one method:

fn read_lines<P>(file_path: P) -> Vec<String>
    where P: AsRef<Path>
{
    let file_path = file_path.as_ref();

    let file = match File::open(file_path) {
        Err(why) => panic!("Couldn't open file {}: {}", file_path.display(), why.description()),
        Ok(file) => file,
    };

    let buf = BufReader::new(file);

    buf.lines().map(|line| {
        match line {
            Ok(l) => l,
            Err(why) => panic!("Couldn't read file {}: {}", file_path.display(), why.description()),
        }
    }).collect()
}

Note that this also allows me to use BufRead::lines, which is probably more efficient than reading the entire file into memory and then splitting it.

I'm also not the biggest fan of burying print statements in my code. You even have comments warning the caller about that! Instead, check out this exciting change:

/// Chooses a random adjective and noun.
fn selfcare<A, N, F, T>(adj: &[A], nouns: &[N], f: F) -> T
    where F: FnOnce(&A, &N) -> T
{
    let mut rng = rand::thread_rng();
    let adjective = rng.choose(adj).expect("No adjective found");
    let noun = rng.choose(nouns).expect("No noun found");
    f(adjective, noun)
}

// ...

selfcare(&adj, &nouns, |adjective, noun| println!("You are a {} {}", adjective, noun));

This pushes the printing to the outermost layers of your program, where it belongs :-) It also opens a new name for selfcare - random_adjective_and_noun, thus allowing you to delete the comment. Note that the requirement for being able to print the value disappears, and I took the opportunity to allow the adjective and noun to be different types (which is a bit of YAGNI).

Thinking about your main method, it seems a little strange to push into the args vector to provide default values. Instead, I might combine Vec::get and Option::map_or to provide defaults:

let args: Vec<_> = env::args().collect();
let adj_filename = args.get(1).map_or("adjectives.txt", |f| &**f);
let noun_filename = args.get(2).map_or("nouns.txt", |f| &**f);
let adj = read_lines(adj_filename);
let nouns = read_lines(noun_filename);

This has the benefit of not needing to allocate the fallback filenames, and it handles the case of providing the first filename and not the second. In fact, you don't have to even collect the arguments anymore, you could just use the args iterator directly. However, putting them into a vector is pretty standard.


Rewriting clumsy code that gets the value of a nested property in JS object I have an API that returns JSON, where the value that I need is stored in either one of the two object properties:

data.d.GetContextWebInformation.FormDigestValue

or

data.value.GetContextWebInformation.FormDigestValue

The code that I'm using is pretty clumsy. At first, I used something like this:

if (data && data.d && data.d.GetContextWebInformation && data.d.GetContextWebInformation.FormDigestValue) {
    this.options.token = data.d.GetContextWebInformation.FormDigestValue;
} else {
// check for second one

Now I've rewritten it using try/catch, and it looks a bit cleaner, but still clumsy:

    var token;
    try {
        // If Verbosity was odata=verbose:
        token = data.d.GetContextWebInformation.FormDigestValue;
    } catch (ex) {
        try {
            // If Verbosity was odata=nometadata or odata=minimalmetadata:
            token = data.value.GetContextWebInformation.FormDigestValue;
        } catch (ex) {
            throw 'Unable to obtain authorization token.';
        }
    } finally {
        if (token) { this.options.token = token; }
    }

What would be a better way to write the same code for a cleaner, nicer look? If that matters, I need to support IE9 so probably can't use newer language features.

If you're certain the token exists in either of the objects you can do:

First check for the existence of the value in data.

var value = data.d || data.value;

then access the value to get the token

var token = value && value.GetContextWebInformation.FormDigestValue;

var value = data.d || data.value;

var token = value && value.GetContextWebInformation.FormDigestValue;

if (token) {
  this.options.token = token;
}

Otherwise, you can use getIn function:

function getIn(pathArray, obj) {
  var tmp = obj;
  for (var i = 0; i < pathArray.length; i++) {
    if (tmp.hasOwnProperty(pathArray[i])) {
      tmp = tmp[pathArray[i]];
      continue;
    }
    return undefined;
  }
  return tmp;
}

var token = getIn(['d', 'GetContextWebInformation', 'FormDigestValue'], data) ||
            getIn(['value', 'GetContextWebInformation', 'FormDigestValue'], data);

if (token) {
  this.options.token = token;
}


Producing queries based on user interface selection I have written a module in my application that makes extensive use of dynamic LINQ to produce Linq/SQL queries based on user interface selection.
Currently, the Linq-SQL translation is resulting in very unoptimised query results, so I am looking at ways to get LINQ to improve the SQL that it is producing.

The following is a typical scenario:

Firstly, the user interface controls are selected, resulting in a LINQ statement of:

Company_contacts.Any(Cust_order_header.Any(Order_time >= @0))

The table relations are as following:

Companies 1-*
Contacts 1-*
Cust_order_header

The Cust_order_header contains the datetime field Order_time.

Don't worry about @0 - that is a dynamic Linq parameter that is passed in containing the datetime object.

The query translates to:

  Give me all companies that have placed orders within the last (insert days based on datetime) days

This results in a query that takes 5 minutes to complete, and kicks the hell out of the server.

The SQL it has created (which I have retrieved using ((ObjectQuery)groupQuery).ToTraceString()) is:

SELECT *
FROM [dbo].[Companies] AS [Extent1]
WHERE  EXISTS (SELECT
    1 AS [C1]
    FROM ( SELECT
        [Extent2].[Company_cont_key] AS [Company_cont_key]
        FROM [dbo].[Company_contacts] AS [Extent2]
        WHERE [Extent1].[Company_reference] = [Extent2].[Company_reference]
    )  AS [Project1]
    WHERE  EXISTS (SELECT
        1 AS [C1]
        FROM [dbo].[Cust_order_header] AS [Extent3]
        WHERE ([Project1].[Company_cont_key] = [Extent3].[Company_cont_key]) AND ([Extent3].[Order_time] >= convert(datetime, '2012-05-20 00:00:00.000', 121))
    )
)

Now, firstly - there are no joins in there, which leads me to think that the SQL it is producing is significantly unoptimised.

AS, if I do to the following in SQL Management Studio manually:

SELECT *
FROM [dbo].[Companies] co
join  [dbo].[Company_contacts] cc on co.Company_reference = cc.Company_reference
join  [Cust_order_header] coh on coh.Company_cont_key = cc.Company_cont_key
WHERE  [Order_time] >= convert(datetime, '2012-05-20 00:00:00.000', 121)

Then it's < 1s.

So, I have spent 3 weeks putting together my Dynamic-LINQ based user controls/solution; getting rid of it isn't an option.

I should also mention I am using an SQL2000 database. There is nothing I can do about this by the way.

Whilst playing around with the SQL to see if I can get improvements, I have noticed that bizarrely if I change the operator from '>' as it should be to '<', which takes the set of rows I was to ignore, it returns 18k rows (instead of the intended 300) but only takes 2 seconds instead of 4 minutes for the smaller query.

I'm wondering what the options are, or specifically how I should go about configuring LINQ to do things a bit better. I'm even starting to wonder if it's my dynamic LINQ extension code that is unoptimised (downloaded from here).

Company_contacts.Any(x => x.Company_contacts.Cust_order_header.Any(y => y.Order_time >= @0))

That should put the joins in for you. Right now it is not referencing the objects you use in the query through their relationships so it is writing the SQL out as though they were subqueries.


A not-so-simple time calculation I'm struggling with some code that should be a very simple computation of a duration. The purpose of the code is to generate a random delay, constrained by an upper and lower limit. The reason I'm looking for advise here is that what should be a simple task, has turned into a behemoth of type conversions:

package main

import (
        "math/rand"
        "time"
)

func main() {
        // parameters, hardcoded for illustration purposes only
        tMin := float32(1.2)
        tMax := float32(2.7)

        // intention is approximately this:
        // delay := tMin + rand.Float32() * (tMax - tMin)

        delay := time.Duration((tMin + rand.Float32() * (tMax - tMin)) * float32(time.Second))

        time.Sleep(delay)
}

Questions:

Apart from rounding errors (which I'm happy to ignore here), does this code have any actual bugs that I missed?
I know that elegance is in the eye of the beholder, but is there any way to write this more elegantly? In particular, I'd like to reduce the number of required casts, preferably down to zero.

Encapsulate implementation details and any ugliness in a function. Readability is paramount.

For example, a more elegant solution,

package main

import (
    "math/rand"
    "time"
)

// delay returns a random delay, in nanoseconds,
// between min and max seconds.
func delay(min, max float64) time.Duration {
    // time doesn't go backwards
    if min < 0 || min > max {
        return 0
    }
    // delay in seconds
    d := min + rand.Float64()*(max-min)
    // delay in nanoseconds
    return time.Duration(d * float64(time.Second))
}

func main() {
    dMin, dMax := 1.2, 2.7

    time.Sleep(delay(dMin, dMax))
}

By design, Go is type safe. Conversions between diffent types must be explicit.

Unless you have a very large number of occurrences of a floating-point variable and you can tolerate the loss of precision, use type float64, not float32.

  behemoth : something of monstrous size. Merriam-Webster

  a behemoth of type conversions. uli

I only found a few type conversions in the code you posted.


python script to download multiple reports from odoo We have an odoo server running (http://www.odoo.com/documentation/9.0/api_integration.html), which allows us to programmatically query and download invoice reports as PDF's

We're running this on python 2.7.3.  I'm looking for tips on DRYness, future maintainability and help on blind spots in the code where something might go wrong, and the ability to output helpful data to the person running the script.

#!/usr/bin/python
# -*- coding: utf-8 -*-

import sys
import xmlrpclib
import os

class UniReportPrinter():
    def __init__(self, mode):

        self.mode = mode

        from apiv9connector import s51OpenERPConnector_apiv9

        # during dev only get vals from prod that are needed
        self.odooconnection1 = s51OpenERPConnector_apiv9()
        self.odooconnection1.login()
        print "logged in"

        odooconnection1 = ''
        self.partners = []

    @property
    def partners(self):
        return self.partners

    @partners.setter
    def partners(self, value):
        self.partners = value

    def get_reports(self):
        nextmonth = self.__get_next_month()

        print('searching for invoices, filtered by:')
        print('state: open')
        print('date: ' + nextmonth.strftime("%Y-%m-%d"))
        print('reference: like SB%')

        invoice_ids = self.odooconnection1.models.execute_kw(
            self.odooconnection1.db, self.odooconnection1.uid, self.odooconnection1.password, 'account.invoice', 'search',
            [[('state', '=', 'open'), ('date_invoice', '=', nextmonth.strftime("%Y-%m-%d")),('reference', 'like', 'SB%')]])

        self.__get_partner_details(invoice_ids)
        self.__download_report(invoice_ids)

    def __get_next_month(self):
        from datetime import date
        from dateutil.relativedelta import *
        today = date.today()
        nextmonth = today+relativedelta(months=+1)
        nextmonth = nextmonth+relativedelta(day=1)

        return nextmonth

    def __get_partner_details(self, invoice_ids):
        partners = self.odooconnection1.models.execute_kw(
            self.odooconnection1.db, self.odooconnection1.uid, self.odooconnection1.password, 'account.invoice', 'search_read',
            [[['id', 'in', invoice_ids]]],
            {'fields': ['reference', 'id', 'partner_id']})

        self.partners = partners

    def __download_report(self, invoice_ids):
        for invoice_id in invoice_ids:
            report = xmlrpclib.ServerProxy('{}/xmlrpc/2/report'.format('https://odoo.example.com'))
            result = report.render_report(
                self.odooconnection1.db, self.odooconnection1.uid, self.odooconnection1.password, 'account.report_invoice', [invoice_id])
            filename = self.__get_file_name(invoice_id)
            sys.stdout.write("\r[%s]" % filename )
            sys.stdout.flush()
            self.__save_report(result, filename)

    def __get_file_name(self, invoice_id):
        partner = ([p for p in self.partners if p["id"] == invoice_id])[0]
        return str(partner['partner_id'][0])+'-'+partner['reference'] + '.pdf'

    def __save_report(self, report_data, filename):
        nextmonth = self.__get_next_month()
        year = nextmonth.year
        month = nextmonth.month
        path = str(year) + '/' + str(month) + '/'

        try:
            os.makedirs(path)
        except OSError:
            if not os.path.isdir(path):
                raise

        fileNamePath = os.path.join(path, filename)
        my_file = open(fileNamePath, 'w')
        my_file.write(report_data['result'].decode('base64'))
        my_file.close()

###############main#############

if __name__ == "__main__":

    unireportprinter = UniReportPrinter(mode='live')
    unireportprinter.get_reports()

Wasted looping

Let's do a pop quiz on this code:

partner = ([p for p in self.partners if p["id"] == invoice_id])[0]

If self.partner contains a million values,
and the first value has id equal to invoice_id,
how many values will be visited?

       All of them :(

That's unfortunate.
It would be better to spell out a loop:

for p in self.partners:
    if p["id"] == invoice_id:
        partner = p
        break
else:
    partner = None

Sure, this is longer, but it avoids a performance issue waiting to happen,
and it also handles the case gracefully when there is no matching value.

A compact alternative is possible using generator expressions, which are very cool:

partner = next(p for p in self.partners if p["id"] == invoice_id)

However, in this case, you would still need to handle the case when there is no matching value.
You would do that by expecting StopIteration to be raised.

Avoid wildcard imports

It's good to avoid wildcard imports.
It makes it easier to understand where symbols come from.
When you import *,
it can easily become confusing to tell where some names come from.

In this code the wildcard import seems pointless:

from dateutil.relativedelta import *
today = date.today()
nextmonth = today+relativedelta(months=+1)
nextmonth = nextmonth+relativedelta(day=1)

return nextmonth

It seems the only thing it imports is the relativedelta,
so you could write just that instead of using a *.

Also, I think that the last 3 statements are actually easier to read inlined:

return today + relativedelta(months=1) + relativedelta(day=1)

Import at the top of the file

It's recommended to import packages at the top of the file, not inside functions.

The above link is part of PEP8, a collection of recommended guidelines that should help you achieve the goals such as maintainability outlined in your question.

Drop pointless statement

In the constructor of UniReportPrinter there is this line:

odooconnection1 = ''

I'm wondering if you might be confusing odooconnection1 with self.odooconnection1.
They are different variables.
The first is a local variable,
the second is a field of the object being created.

If you want to unset self.odooconnection1, assign None to it instead of an empty string.

Getters should return something

I would expect functions with "get" in the name to return something.
The __get_partner_details function doesn't return anything.
I suggest to rename the method. Perhaps replace "get" with "fetch".

Be consistent

In some places I see print("string"), in others print "string".

In some places I see '{}/foo'.format(bar), in others '%s/foo' % bar, or bar + '/foo'.

It's good to be consistent.
And to consistently use the right techniques.
In each of these examples the right technique is the first.

Use '...'.format(...)

Especially when formatting complex strings like here,
definitely use '...'.format(...) instead:

return str(partner['partner_id'][0])+'-'+partner['reference'] + '.pdf'


Is this good login process in CodeIgniter? I'm creating my first login process in CodeIgniter. I'm using the simpleloginsecure library for actual session management but I wrote the controller and model myself and I was hoping if you could see any flaws in it.

My User_model Class

<?php if ( ! defined('BASEPATH')) exit('No direct script access allowed');

class User_model extends CI_Model {
    public function __construct() {
        parent::__construct();
    }

    function login($email, $password) {
        if($this->simpleloginsecure->login($email, $password)) {
            return true;
        }
        return false;
    }
}

?>

My User Controller class

<?php if(!defined('BASEPATH')) exit('No Direct script access allowed');

Class User extends CI_Controller {

    public function __construct() {
        parent::__construct();
        $this->load->model('user_model');
    }

    public function index() {
        if($this->session->userdata('logged_in')) {
            redirect('/user/dashboard/', 'location');
        } else {
            $data['message'] = '
You need to be logged in to view the administration area
';
            $this->load->view('user/login', $data);
        }
    }

    public function dashboard() {
        if($this->session->userdata('logged_in')) {
            $data['title'] = 'Welcome';
            $this->load->view('user/dashboard', $data);
        } else {
            $data['message'] = '
You need to be logged in to view the administration area
';
            redirect('/user/login/', 'location');
        }

    }

    public function login() {

        if($this->session->userdata('logged_in')) {
            redirect('/user/dashboard/', 'location');
        }

        $this->form_validation->set_rules('email', 'E-mail', 'trim|required|valid_email');
        $this->form_validation->set_rules('password', 'Wachtwoord', 'trim|required|min_length[4]|max_length[32]');

        if($this->form_validation->run() == FALSE) {
            $this->index();
        } else {
            if($this->user_model->login($this->input->post('email'), $this->input->post('password'))) {
                redirect('/user/dashboard/', 'location');
            } else {
                $this->index();
            }
        }
    }

    public function logout() {
        $this->simpleloginsecure->logout();
        redirect('/user/login/', 'location');
    }

}

Simply use return $this->simpleloginsecure->login($email, $password) in the login method.
redirect('/user/xxx') is enough, since location is the default redirect type.
Since you're doing a redirect(), $data['message']is probably "dead code" here. Use CodeIgniter's Flashdata instead.
<p class="error"> should be handled in the view, the controller only needs to say there's an error. For example: $data['message'] = array('error' => 'This is my error text');
Trimming passwords is dangerous: what if my password starts with a space? Also, a space will look like a real character in an HTML password form, it makes no sense to trim it.
When the validation fails, redirect to the login form and use flashdata to explain what got wrong.


Simple decorator for creating and managing a database cursor I'm working on a simple decorator that creates and manages a database cursor.  The goal is to 'inject' the cursor as the first argument of the function, but not expose it to the caller.

Usage:

import psycopg2

@needsNewCursor
def findItem(cursor, item_id):
    cursor.execute('SELECT name FROM items WHERE id = %s', [item_id])
    item_result = cursor.fetchone()
    # ...
    return item

# caller only needs to know about the ID
findItem(1234)

Decorator:

def needsNewCursor(func):
    def decorator(*args, **kargs):
        print 'Creating new cursor'
        # This is slightly different in the final code, but I'm only
        # concerned about how I'm dealing with decorators, at the moment
        cursor = createDefaultCursor()
        if cursor:
            with cursor:
                # Pass the cursor in as the 1st param
                return func(cursor, *args, **kargs)

        # Give the function a chance to deal with fails
        return func(None, *args, **kargs)

    # QUESTION: Is this enough?
    decorator.__name__ = func.__name__
    return decorator

Is this a good approach?

No, I don't think this is a good approach. Anyone who sees the declaration def findItem(cursor, item_id): will call the function with two arguments and scratch one's head to no end when it does not work.

You could just as well have cursor = createDefaultCursor() as the first line of each function that uses a cursor. The number of lines does not increase as you can omit the @needsNewCursor line.

Even better might be to create a context manager to deal with opening and closing a cursor and start each function with a statement like with new_cursor() as cursor:. A context manager also has an opportunity to deal with exceptions raised in the with block.

Anyway, when writing decorators, instead of assigning yourself decorator.__name__ = func.__name__ you can apply the functools.wraps decorator that also copies the docstring.

One more thing: in your example needsNewCursor is a decorator. You should choose a different name for the inner function.