Initializing an EGL display I'm somewhat new to writing real-world projects in C, so I'm not familiar with best practices and standards. I'm writing code dealing with EGL and OpenGL, and some setup calls might fail for reasons outside my control. I want to write an initialization function that performs all necessary setup for my application, but exits early and returns false if any call fails (essentially replicating simple exception handling behavior).

Given various philosophies regarding multiple returns, GOTOs, and so forth, I've  written my function as follows:

int init_gl(EGLDisplay* outDisplay, EGLContext* outContext, EGLSurface* outSurface) {
    int success = 1;

    EGLConfig config;
    EGLint num_config;

    EGLDisplay display;
    EGLContext context;
    EGLSurface surface;

    static const EGLint attribute_list[] =
    {
        EGL_RED_SIZE, 8,
        EGL_GREEN_SIZE, 8,
        EGL_BLUE_SIZE, 8,
        EGL_ALPHA_SIZE, 8,
        EGL_SURFACE_TYPE, EGL_WINDOW_BIT,
        EGL_NONE
    };

    static const EGLint context_attributes[] =
    {
        EGL_CONTEXT_CLIENT_VERSION, 2,
        EGL_NONE
    };

    bcm_host_init();

    success = success && (display = eglGetDisplay(EGL_DEFAULT_DISPLAY)) != EGL_NO_DISPLAY;
    check();

    success = success && eglInitialize(display, NULL, NULL) != EGL_FALSE;
    check();

    success = success && eglChooseConfig(display, attribute_list, &config, 1, &num_config) != EGL_FALSE;
    check();

    success = success && eglBindAPI(EGL_OPENGL_ES_API) != EGL_FALSE;
    check();

    success = success && (context = eglCreateContext(display, config, EGL_NO_CONTEXT, context_attributes)) != EGL_NO_CONTEXT;
    check();

    success = success && (surface = eglCreateWindowSurface(display, config, NULL, NULL)) != EGL_NO_SURFACE;
    check();

    success = success && eglMakeCurrent(display, surface, surface, context) != EGL_FALSE;
    check();

    if(success) {
        glClearColor(0.0f, 0.0f, 0.0f, 1.0f);
        glClear(GL_COLOR_BUFFER_BIT);
        check();

        *outDisplay = display;
        *outContext = context;
        *outSurface = surface;
    }

    return success;
}

Is this awful? As I understand it, this is not an appropriate place to use assert() for postconditions as the success of these functions is outside of my control. Would it be better to write this as a series of if(!function()) { return 0; } or leave it as is?
Don't worry about the check(); statements, this is just a debug macro for assert(glGetError() == 0).

Certainly found the alternative (2nd version) immediately understandable.  Not so with the first (both simplified below).  The 2nd maps directly to the stated requirement "exits early and returns false if any call fails".  This alone gives it a strong preference.

int success = 1;
....
success = success && func1();
success = success && func2();
success = success && func3();
....

versus

if (!func1()) {
  return 0;
}
if (!func2()) {
  return 0;
}
if (!func3()) {
  return 0;
}
....

Minor: Why isn't the first success = success && func1(); vs. int success = func1();.  Just to maintain symmetry?

Why use int instead of bool?  Portability?  If C99 or later available as the minimum standard, better to use bool.

The success approach may have merit in select circumstances yet do not see that called for here.  I would opt for the if () style as it matches the scant coding requirements.


Short analogue of Riffle[] accepting two blocks I have an array of some elements that I need to loop through (i.e. execute some block A on each element) but between them I have also to "clean up" (block B) from the previous iteration and that I don't need to clean after the last iteration because B is slow and it is done somewhere further in more efficient way like process exit or whatever. If the array is [1,2,3] the flow should be like:

[1, :IN]
[1, :OUT]
[2, :IN]
[2, :OUT]
[3, :IN]

It is similar to Mathematica Riffle[] function. I already implemented it in Ruby three years ago but that implementation is complex and there is a place only for B, not for A. So I rewrote it for my current case:

riffle = lambda do |enumerable, &block|
  prev = nil
  enumerable.each_with_index do |e, i|
    block.call prev unless i.zero?
    prev = e
  end
end

riffle.call( Enumerator.new do |e|
  [1,2,3].each do |i|
    e << i

    # start block A
    p [i, :IN]
    # end

  end
end ) do |i|

  # start block B
  p [i, :OUT]
  # end

end

Is there a way to make less code and maybe not use Enumerator.new? I tried to use .to_enum and .lazy.map(&A) but had no success.

There's not too much to say about your implementation, though I'm not sure declaring riffle as a lambda provides any benefit instead of just defining it as a standard function.

Instead of storing the previous element of the iteration and using it to call the block, you should check only call the block if you are not on the last element of the enumerable (if possible, of course, there are some cases when it isn't).

While you can use a mixture of a custom enumerator and a block, it's not the nicest way to do things. I'd recommend instead passing in two lambdas (or a lambda and a block) so that it is immediately clear that the first block follows the second.

def riffle(enumerable, a, b)
  enumerable.each_with_index  do |x, i|
    a.call x
    b.call x unless i + 1 == enumerable.size
  end
end

riffle [1, 2, 3],
  -> i { p [i, :IN] },
  -> i { p [i, :OUT] }

This gives the output:

[1, :IN]
[1, :OUT]
[2, :IN]
[2, :OUT]
[3, :IN]
=> [1, 2, 3]


Complete factorization in > Maybe a somewhat odd choice of language, but here is a program in ><> I have tried
to optimize the execution time of. (Not golfing). The efficiency is measured in the number of required instructions on an ideal ><>-machine, which uses the same amount of time to execute each instruction.

The program does a complete factorization of a number on the stack, and prints the result. The method used is trial division of odd numbers, up to the square root of the original number. The 2-factors are calculated first. The core of this program is the third line, which wraps around, and has a minimum of 2D-program flow commands. (I have also not used p, so the source code is static, and does not modify itself.)

An online interpreter for ><> is available here.

:?!;:2%?v2n2,30.
        >:3:02.
::*$}(?v::{:@%?!v2+
;n;?=1:<
,:r~:n:02.      >:@

Here is a commented version:

:?!;                   //Checks if n is 0. Sends the next part into an infinite loop if not dealt with

    :2%?v2n2,30.           //Loop that gets all factors of 2

        >:3:02.                //Escape condition executed when the number is no longer divisible by 2, go to main loop

::*$}(?v               //Main loop, the first condition checks if the candidate for trial division is > sqrt(n)

        ::{:@%?!v2+            //Checks divisibility by modulo, if not, increase candidate by 2

;n;?=1:<               //Prints the remainder as the last factor, except when it is 1

,:r~:n:02.      >:@    //When a factor is found, it is printed, n is divided, and the program goes back to the main loop

The program outputs the numbers directly, so for testing purposes, it may be convenient to separate them with commas:

:?!;:2%?v2n2,4b*o30.
        >:3:02.
::*$}(?v::{:@%?!v2+
;n;?=1:<
,:r~:n4b*o:02.  >:@

The main loop in detail:

The stack starts off with n,n,i ,where n is the number we are trying to factorize, and i is the iterator.

::*  //The iterator is copied two times, and mad into a square. n,n,i,s
$}   //The iterator is sent back to the bottom of the stack. i,n,n,s
(?v  //The square is compared with n. (both values are popped). If the square is larger or equal, the instructor goes down one line. i,n
::{: //n is copied two times, the iterator called to the top, and is copied too. n,n,n,i,i
@    //The three topmost values of the stack are rotated. n,n,i,n,i
%?!v //Takes the modulo of n and the iterator, if >0, the instructor jumps over the arrow down to the next line. n,n,i
2+   //Increments the iterator by two. The line wraps back to the beginning of the loop.

I would like to know if there are optimization opportunities I have missed, and if the code is fairly readable by ><> standards. Again, this is not about golfing or obfuscating the code.

If you are testing the code with the online interpretor, there is an easy way to put a number in the initial stack (Otherwise, you get the standard ><>-error "Something smells fishy...").

First off, I'd like to say that you're asking two questions which are at odds with each other. Having readable ><> code usually requires a decent amount of whitespace, but minimising the number of ticks requires reducing the amount of whitespace and a bit of golfing. I'll assume that you're primarily aiming for the latter, while keeping the code as readable as possible under this constraint.

General comments

Less than

A minor one about your explanation - ( is second < top, so by having n, s at the top of the stack you're checking whether square > n, not square >= n. If it were the latter, your code would fail for numbers like 9! (no factorial)

Floats

Just a note of warning: both the online interpreter and the official Python interpreter use floats for division, so this program would most likely fail for large numbers. Unfortunately, I don't know of any good way to perform integer division as efficiently.

Wrapping and reading direction

Wrapping the last line doesn't affect efficiency, so it's more readable to have the , follow the @. Also, if we wanted to save every tick, we could have this part on the second-last line instead:

:?!;:2%?v2n2,30.
        >:3:02.
::*$}(?v::{:@%?!v2+
       >:1=?;n; >:@,:r~:n:02.

Note that I've also flipped the :1=?;n; for readability since it fits going left-to-right.

Print if not 1

Speaking of :1=?;n;, this reads as "halt immediately if the number is 1, else print the number then halt". This could be more clearly expressed as

:1=?!n;

to "print the number if it's not 1, then halt". Alternatively, you can use -? as an idiom for "if not equals", and do

:1-?n;

to save a tick.

Division by 2 loop

It's a bit more logical to separate the initial zero check from the rest of the first line, which divides out factors of 2. This is also a bit more efficient, since we save on the 30. jump.

:?!;v
2n2,>:2%?v

Main loop efficiency

To get from the stack state [n, n, i] to [i, n, n, s], you currently do

::*}$

Instead, you can do

:}:*

Putting all of the above notes together, we get the slightly more efficient version:

:?!;v
2n2,>:2%?v
         >:3:03.
:}:*(?v::{:@%?!v2+
      >:1-?n;  >:@,:r~:n:03.

Outputting characters

This only concerns your testing code, but outputting single characters should be done with quotes for readability, i.e. ','o rather than 4b*o to output a comma.

Would-be readability comments

In terms of general ><> readability, I wouldn't call this particularly readable, but under the constraint of efficiency it's pretty good. However, if the goal were strictly readability, here are a few comments:

Whitespace should be used to group logical components, e.g. :2%?v 2n 2, 30..
It makes more sense logically to enter the main loop starting with [n, i] rather than [n, n, i].
Because 02. starts execution of the main loop from the second : of ::*, you have an extra : to compensate before both instances of 02.. To cut down on repetition, the main loop should be indented. (You could also move the + from the end to the start so as to not lose efficiency, but that comes at a cost to readability).

Extra: using l as a counter

For extra efficiency and sheer stupidity, we can use l (which pushes the length of the stack) as a counter to save on stack manipulation. Granted, this would run into memory problems for large numbers and is terrible for readability, but I'm sure that's the least of our worries if we're factorising in ><>!

:?!;v
2n2,>:2%?v
         >:f3.
:l:*(?v:l%?!v::
;n?-1:<     >:lnl,$~f3.


Status display module I've developed a template to modularize JavaScript code I write for client work. The example below is a simple class with 1 attribute and 3 functions.

Rationale:

Standardized constructor to add or modify settings/ options
Functions as variables used later to return only "public" functions, all else are private
Using local "call" variable to easily replace functions in unit testing

Are there any improvements you can recommend or general best practices to look for in terms of improvement?

var dataStatus = function( constructorOptions )
{
    "use strict";

    var options = {
        displayId: 'dataStatus',
        statii: {
            'settingUp': {
                tooltip: 'Setting Up...',
                color: '808080'
            },
            'loading': {
                tooltip: 'Data Loading...',
                color: '99ee90'
            },
            'loaded': {
                tooltip: 'Data Loaded',
                color: '006400'
            },
            'changed': {
                tooltip: 'Data Changed',
                color: '8b0000'
            },
            'saving': {
                tooltip: 'Saving...',
                color: 'ffd700'
            }
        },
        defaultStatus: 'settingUp'
    };

    var local = {
        status: false
    };

    var init = function( optionsToSet )
    {
        jQuery.extend( options, optionsToSet);

        call.setStatus( options.defaultStatus );
        return this;
    };

    var inject = function (functionToReplace, injectedFunction)
    {
        call[functionToReplace] = injectedFunction;
        return injectedFunction;
    };

    var getStatus = function()
    {
        return local.status;
    };

    var setStatus = function( status, signal )
    {
        local.status = status;
        if (typeof signal == 'undefined' || signal != false){
            call.signal( call.getStatus() );
        }
    };

    var signal = function ( status ){
        if (status in options.statii){
            jQuery( '#' + options.displayId )
                .html( '<i class="fa fa-circle" title="' + options.statii[status].tooltip + '"></i>')
                .css( 'color', '#' + options.statii[status].color )
                .css( 'background-color', '#ffffff');
        }
    };

    var call = {
        init: init,
        options: options,
        inject: inject,
        getStatus: getStatus,
        setStatus: setStatus,
        signal: signal
    };

    if(typeof myPublicTestNamespace == "undefined"){//http://stackoverflow.com/a/9172377/123594
        return {
            init: init,
            getStatus: getStatus,
            setStatus: setStatus,
            signal: signal
        };
    }else{
        return call;
    };

    init( constructorOptions );
};

Your code has somewhat inconsistent styling which can inhibit readability and prevent the code from looking crisp and professional. For example, some of your function definitions have braces on the next line others don't. Some of your function definitions and calls put spaces between parenthesis var signal = function ( status ){, and some don't var inject = function (functionToReplace, injectedFunction).

Also, I don't believe that the last line init( constructorOptions ); is executing because you return from the constructor in both the if and else blocks right above it.

That said, using closures to keep private functions private is a great idea.

As for the modularity, while returning a separate object for for testing is a truly fascinating idea, my gut instinct goes against it. I find it somewhat troublesome because it violates cohesion with your constructor really doing two things: creating a dataStatus object and creating a dataStatus test object. This introduces room for bugs if future functions aren't properly assigned to both the real and test object. To catch these bugs, you'd want to write a test case that ensures the test object shares all the same functions as the real object. (i.e. call could have extra functions but it must also have each one that a normal dataStatus would)

I'd recommend eliminating call and if needed to use this instead. Although it might be best to just call the functions directly.

var init = function () {
    jQuery.extend(options, optionsToSet);

    setStatus(options.defaultStatus); // just call setStatus directly
    return this;
}

You can still do your testing by dissecting the methods you want from the real object.

var realObj = new dataStatus (options);
var testObj = {
    setStatus : realObj.setStatus,
    getStatus : function () {},
    signal : function () {}
};

Private functions should be tested through the testing of public functions, but in your case the only private function is for changing functions when testing.

UPDATE:

If you're set on testing your private functions--and I can fully understand how it could be worth it--you ought to to take care of a few things.

First, make sure the test object and the real object have the same functions. You could write a test case that checks that their functions are the same, but it may be best to just write the private functions directly as properties of the call object:

var call = {};

call.init = function () {
    // ...
}
call.inject = function () {
    // ...
}
call.getStatus = function () {
    // ...
}

// ...

if (/* determine if your unit testing each function */) {
    return call;
}
else {
    return {
        init: call.init,
        getStatus: call.getStatus,
        setStatus: call.setStatus,
        signal: call.signal
    };
}

Secondly, you will want to also test the real object. You could have millions of test cases for your test object, but since your real object isn't your test object, the real object might still break.


Follow button like on Twitter I want to create an like button like on Twitter. I used Twitter bootstrap and this code:

$(document).ready(function(){
  $('.btn-follow').on('mouseover', function(e){
    if ($(this).hasClass('following')){
      $(this).removeClass('btn-primary').addClass('btn-danger').text('UnFollow');
    }
  })
  $('.btn-follow').on('mouseleave',function(e){
    if ($(this).hasClass('following')){
      $(this).text('Following').removeClass('btn-danger').addClass('btn-primary');
    }
  })
  $('.btn-follow').on('click',function(e){
    $(this).toggleClass('following follow')
    if ($(this).hasClass('follow')){
      alert('unfollow')
      $(this).text('Follow').removeClass('btn-danger')
    }else{
      alert('follow')
      $(this).text('Following')
    }
  })
});

And in HTML I have this:

  <button class="btn btn-primary btn-follow following">Following</button>

  <button class="btn btn-follow follow">Follow</button>

But I think it's very dirty code and has some bugs. How can I summarize this code?

This code has a bug. When I click on click of the follow button, the text is changed to "Following", but the color of the button is not changed while the mouse leave from the button area. How I can fix it?

I reworked the code a bit. Check out the comments for explanation.

$(document).ready(function() {
  //Don't copy and paste the same string.
  //It is also helpful when the CSS or text changes. There is just one part
  //of the code to look at.
  var followBtn = $('.btn-follow'),
      followCls = 'following',
      dangerCls = 'btn-danger',
      primaryCls = 'btn-primary',
      unfollowText = 'UnFollow',
      followText = 'Follow'

  //No need to keep making a jQuery "query" to get a the button every time.
  //I am not sure about the internals of jQuery, but I assume it is reading the
  //DOM on every $('.btn-follow') call.

  //Don't pass argument to method if it is not being used.
  followBtn.on('mouseover', function(){
    //same thing inside the handler no need for $(this).
    if (followBtn.hasClass(followCls)){
      followBtn.removeClass(primaryCls).addClass(dangerCls).text(unfollowText);
    }
  });

  followBtn.on('mouseleave',function(){
    if (followBtn.hasClass(followCls)){
      //Keep the same order as before, just be consistent.
      followBtn.removeClass(dangerCls).addClass(primaryCls).text(followText);
    }
  });
  //The rest
});

If you post the CSS I can help you with that. I suspect that a few of the removeClass calls can be removed in favor of more specific selectors.


Constructors and inheritance in Groovy I am new to Groovy and I am having a little problem with constructors of subclasses. Basically, I have a base abstract class like

class BaseClass {
  def BaseClass(Map options) {
    // Does something with options,
    // mainly initialization.
  }
  // More methods
}

and a bunch of derived classes like

class AnotherClass extends BaseClass {
  // Does stuff
}

I would like to be able to create instances like

def someObject = new AnotherClass(foo: 1, bar: 2)

Unfortunately, Groovy creates automatically all sorts of constructors for AnotherClass, with various signatures - based on the properties of AnotherClass - but will not allow me to just reuse the constructor in BaseClass. I have to manually create one like

class AnotherClass extends BaseClass {
  def AnotherClass(options) {
    super(options)
  }
  // Does stuff
}

It feels repetitive doing so for each subclass, and it is probably the wrong way.

What would be the Groovy way to share some logic in the constructor?

@InheritConstructors is probably what you are looking for:

@InheritConstructors
class AnotherClass extends BaseClass {}

will create the constructors corresponding to the superclass constructors for you.


Best way to refactor this loop of animations with delay? I have some animations happening after some delay calls the code to animate.  It works fine, but the code to call the functions is not nice.

    double delayInSeconds = 0.2;
    dispatch_time_t popTime = dispatch_time(DISPATCH_TIME_NOW, delayInSeconds + 0.1 * NSEC_PER_SEC);
    dispatch_time_t popTime2 = dispatch_time(DISPATCH_TIME_NOW, delayInSeconds + 0.2 * NSEC_PER_SEC);
    dispatch_time_t popTime3 = dispatch_time(DISPATCH_TIME_NOW, delayInSeconds + 0.3 * NSEC_PER_SEC);
    dispatch_time_t popTime4 = dispatch_time(DISPATCH_TIME_NOW, delayInSeconds + 0.4 * NSEC_PER_SEC);
    dispatch_time_t popTime5 = dispatch_time(DISPATCH_TIME_NOW, delayInSeconds + 0.5 * NSEC_PER_SEC);
    dispatch_time_t popTime6 = dispatch_time(DISPATCH_TIME_NOW, delayInSeconds + 0.6 * NSEC_PER_SEC);
    dispatch_time_t popTime7 = dispatch_time(DISPATCH_TIME_NOW, delayInSeconds + 0.7 * NSEC_PER_SEC);
    dispatch_time_t popTime8 = dispatch_time(DISPATCH_TIME_NOW, delayInSeconds + 0.8 * NSEC_PER_SEC);

    if (self.menuType == kRacingMenu) {

        self.menuAnimationSequence = @[@0, @1, @3, @2, @4]; //animation sequence!

        dispatch_after(popTime, dispatch_get_main_queue(), ^(void){
            [self animateWithCoreAnimaGenerallyWithIndex:0];
        });
        dispatch_after(popTime2, dispatch_get_main_queue(), ^(void){
        [self animateWithCoreAnimaGenerallyWithIndex:1];
        });
        dispatch_after(popTime3, dispatch_get_main_queue(), ^(void){
        [self animateWithCoreAnimaGenerallyWithIndex:2];
        });
        dispatch_after(popTime4, dispatch_get_main_queue(), ^(void){
        [self animateWithCoreAnimaGenerallyWithIndex:4];
        });
        dispatch_after(popTime5, dispatch_get_main_queue(), ^(void){
        [self animateWithCoreAnimaGenerallyWithIndex:3];
        });

    }else if (self.menuType == kLoginMenu) {
        self.menuAnimationSequence = @[@0, @1, @3, @2]; //animation sequence!
        dispatch_after(popTime, dispatch_get_main_queue(), ^(void){
            [self animateWithCoreAnimaGenerallyWithIndex:0];
        });
        dispatch_after(popTime2, dispatch_get_main_queue(), ^(void){
            [self animateWithCoreAnimaGenerallyWithIndex:1];
        });
        dispatch_after(popTime3, dispatch_get_main_queue(), ^(void){
            [self animateWithCoreAnimaGenerallyWithIndex:2];
        });
        dispatch_after(popTime4, dispatch_get_main_queue(), ^(void){
            [self animateWithCoreAnimaGenerallyWithIndex:3];
        });

    } else {
        [self animateWithCoreAnimaGenerallyWithIndex:1];
        dispatch_after(popTime, dispatch_get_main_queue(), ^(void){
            [self animateWithCoreAnimaGenerallyWithIndex:0];
        });
        dispatch_after(popTime2, dispatch_get_main_queue(), ^(void){
            [self animateWithCoreAnimaGenerallyWithIndex:2];
        });
        dispatch_after(popTime3, dispatch_get_main_queue(), ^(void){
            [self animateWithCoreAnimaGenerallyWithIndex:4];
        });
        dispatch_after(popTime4, dispatch_get_main_queue(), ^(void){
            [self animateWithCoreAnimaGenerallyWithIndex:3];
        });
        dispatch_after(popTime5, dispatch_get_main_queue(), ^(void){
            [self animateWithCoreAnimaGenerallyWithIndex:5];
        });
        dispatch_after(popTime6, dispatch_get_main_queue(), ^(void){
            [self animateWithCoreAnimaGenerallyWithIndex:7];
        });
        dispatch_after(popTime7, dispatch_get_main_queue(), ^(void){
            [self animateWithCoreAnimaGenerallyWithIndex:6];
        });
        dispatch_after(popTime8, dispatch_get_main_queue(), ^(void){
            [self animateWithCoreAnimaGenerallyWithIndex:8];
        });

    }

How best to abstract this timer?  And the calls to start the animation?

Anything that you're using as a constant should be marked as such.

double const delayInSeconds = 0.2f;

Now then, we know in Objective-C, we have NSArray and its mutable cousin, and they can only hold objects.  But also remember, Objective-C is a superset of C.  We can use a C-Style array.

int const POPTIME_COUNT = 8;

dispatch_time_t poptime[POPTIME_COUNT];

And then fill this an array with a loop...

for (int i = 0; i < POPTIME_COUNT; ++i) {
    double totalDelay = delayInSeconds + ((i + 1) * 0.1 * NSEC_PER_SEC)
    poptime[i] = dispatch_time(DISPATCH_TIME_NOW, totalDelay);
}

Firing off the animations is done just as easily in a loop:

for (int i = 0; i < NUM_ANIMATIONS; ++i) {
    dispatch_after(poptime[i], dispatch_get_main_queue()), ^(void){
        [self animateWithCoreAnimaGenerallyWithIndex:i];
    }];
}

Where NUM_ANIMATIONS is the number of animations you need to fire off (it's different in each branch).


Markov chain-based word salad generator I like Markov chains. Last time I used one, I made one that generates words. This time, I made one that generates sentences, given a set of words and valid connections. This time, it's not weighted and it's reading from a JSON file, so the code is a bit simpler:

require 'json'

data = JSON[ARGV[0]]
CONNECTIONS = data['connections']
WORDS = data['words']

sentence = []
current_pos = 'start'
until current_pos == 'end'
  current_pos = CONNECTIONS[current_pos].sample
  sentence << WORDS[current_pos].sample
end
puts sentence.join(' ')

If we use this as the input data:

{
  "connections": {
    "start": ["article"],
    "article": ["adjective", "noun"],
    "adjective": ["adjective", "noun"],
    "noun": ["verb", "end"],
    "verb": ["article"],
    "end": []
  },
  "words": {
    "article": ["the"],
    "noun": ["dog", "cat", "penny", "truck"],
    "verb": ["ate", "stole", "helped", "cuddled"],
    "adjective": ["red", "big", "fluffy", "honorable"],
    "end": [""]
  }
}

which gives some fantastic word salad like:

the fluffy penny ate the truck stole the penny cuddled the cat stole the big cat
the red red dog helped the honorable honorable fluffy big big red big cat stole the truck

And, my personal favorite so far:

the fluffy honorable cat stole the dog

I'm looking for tips on:

Efficiency -- I think this is about as good as it'll get, but any suggestions are great.
Usability -- I like word salad. I want to share it with everyone. To that end, I want to make this as easy and simple-to-use as possible.
Readbility -- Because I like word salad (it's very nutritious) I want people to easily understand how to make it. Again, I think this is about as it's gonna get, but any suggestions are great.

There are various methods to generate a random n-word text. Markov chain is one of them.

Basic idea how to do this is described in Centennial of Markov Chains (by Oleksandr Pavlyk) under "Text Generation" - you generate n-grams carry info,  encode transitions and define a function to walk the graph (in a non-brute force manner).

Result will heavily depend on word corpus and their connections rather then algorithm implementation itself.

The real problem is actually to generate the word corpus and connections from random text. In order to do this well you need to work out the grammatical structure of sentences. Because, you do not want to be doing this manually.

For example, consider the text:

  The strongest rain ever recorded in India shut down the financial hub
  of Mumbai, snapped communication lines, closed airports and forced
  thousands of people to sleep in their offices or walk home during the
  night, officials said today.

You want to tag word grammatical structure

  The/DT strongest/JJS rain/NN ever/RB recorded/VBN in/IN India/NNP
  shut/VBD down/RP the/DT financial/JJ hub/NN of/IN Mumbai/NNP ,/,
  snapped/VBD communication/NN lines/NNS ,/, closed/VBD airports/NNS
  and/CC forced/VBD thousands/NNS of/IN people/NNS to/TO sleep/VB in/IN
  their/PRP$ offices/NNS or/CC walk/VB home/NN during/IN the/DT night/NN
  ,/, officials/NNS said/VBD today/NN ./.

To do this you can use The Stanford Parser (ruby wrapper).


Getting last and this monday as Unix timestamp in JavaScript This is how I achieved it:

<script type="text/javascript">

// Get this monday
var dif, d = new Date();
dif = (d.getDay() + 6) % 7;
d = new Date(d.getTime() - dif * 24*60*60*1000);
d.setHours(0,0,0);
var thisMondayUnixtimestamp = Math.round((d.getTime() / 1000));

console.log('thisMondayUnixtimestamp', thisMondayUnixtimestamp);

// Get last monday
var beforeOneWeek = new Date(new Date().getTime() - 60 * 60 * 24 * 7 * 1000)
  , day = beforeOneWeek.getDay()
  , diffToMonday = beforeOneWeek.getDate() - day + (day === 0 ? -6 : 1)
  , lastMonday = new Date(beforeOneWeek.setDate(diffToMonday));
lastMonday.setHours(0,0,0);
var lastMondayUnixtimestamp = Math.round(lastMonday.getTime() / 1000);

console.log('lastMondayUnixtimestamp = ', lastMondayUnixtimestamp);

</script>

This works. Today is 24th of Jan 2016 and the timestamp values parsed returns the following:

last Monday:
string '11/01/2016 01:00:00' (length=19)

this Monday:
string '18/01/2016 01:00:00' (length=19)

Although this works, I am uncertain if it's correct and reliable as I have put the above together from snippets found via googling.

Is there a more elegant way to do this?

I would suggest you use a date library like moment.js for this. That way, you don't have to do this all over again. With that library, it's just a few subtraction calls.

<script type="text/javascript">

By default, the browser executes JS. type is not required. The only time type is every used is if you want to tell the browser that it contains something not JS, preventing it from executing as JS.

24*60*60*1000

60 * 60 * 24 * 7 * 1000

These are constants. I suggest you use const with the convention of uppercase and underscores. If you can't use const, var will have to do but the naming convention stays.

new Date().getTime()

Use Date.now(). It's the same as new Date().getTime() without the Date object that it generates. Also, your code uses a lot of Date objects when the operation can be done with millisecond manipulation. Try avoiding the creation of objects when necessary.

setHours mutates the date object hours using local time. This means the hours you set are midnight local time. Unless this is something you intended or something UI only, this may cause issues with timekeeping.

In some regions, weeks start on a Sunday. "This Monday" may refer to the Monday that comes after that Sunday. "Last Monday" may also refer to this week's Monday that already passed. This makes the phrase "This Monday" and "Last Monday" vague.

Anyways, here's my take on it. Not much of a departure, but there's some notable changes.

(function() {
  'use strict'

  const WEEK_IN_MS = 604800000;
  const DAY_IN_MS = 86400000;
  const DAY_MONDAY = 1;

  function getDateThisMonday() {

    // start with today's midnight
    let today = new Date();
    today.setHours(0, 0, 0);

    // We start the week with Monday, we assign 7 when its a Sunday (0)
    let dayToday = today.getDay() || 7;

    // Calculate milliseconds elapsed since Monday
    let msSinceMonday = ((dayToday - DAY_MONDAY) * DAY_IN_MS);

    // Subtract today midnight with the diff to get midnight of the Monday
    let mondayTime = today.getTime() - msSinceMonday;

    return mondayTime;
  }

  // This is pretty much the same except we subtract an additional week.
  function getDateLastMonday() {
    return getDateThisMonday() - WEEK_IN_MS;
  }

  document.write('Last Monday: ' + new Date(getDateLastMonday()));
  document.write('This Monday: ' + new Date(getDateThisMonday()));

}());


Using cURL to retrieve remote image I am using the following code to retrieve the URL of a random image in a remote folder that I own.

As a newbie and although this is working I would like to know if this is done properly, if it could be improved and if there is any security issue with this approach.

I am using json_encode and json_decode because I need to have an array as response. Also I am passing $params to remote through cURL because I need the returned image already with it's URL.

Thank you.

if (function_exists('curl_init')){
    $url    = 'http://example.com/images';
    $params = 'path=' . $url;

    $ch = curl_init();
    curl_setopt($ch, CURLOPT_URL, $url . '/my_remote_script.php');
    curl_setopt($ch, CURLOPT_POST, true);
    curl_setopt($ch, CURLOPT_POSTFIELDS, $params);
    curl_setopt($ch, CURLOPT_HEADER, false);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);

    $images = json_decode(curl_exec($ch));

    curl_close($ch);
}

my_remote_script.php

if (!$_POST['path']){
    die;
}

function getImages($path){
    if ($img_dir = opendir(dirname(__FILE__))){
        $images = array();

        while (($img_file = readdir($img_dir)) !== false){
            if (preg_match('/(\.gif|\.jpg|\.jpeg|\.png|\.GIF|\.JPG|\.JPEG|\.PNG)$/', $img_file)){
                $images[] = $path . '/' . $img_file;
            }
        }

        closedir($img_dir);
        return $images;
    }
}

function randomImage($list){
    $num   = array_rand($list);
    $image = $list[$num];

    return $image;
}

$images = getImages($_POST['path']);
$image  = randomImage($images);

print_r(json_encode($image));

Here are my thoughts:

Why is this a POST operation at all?  Why not GET, since you are not changing ANY data whatsoever? The "parameter" you pass is not meaningful (it is just the base URL of the remote service) and it is not really even used in the server side script other than to append to the image name.  If that is the case why not just remove it altogether since client already know the base URL of the site anyway.
Both the client-side cURL code and server-side code very heavily assume that everything is working correctly (the so called "happy path").  Some examples of that:

On server side, don't just do something like die() if the the request is mal-formed.  Good services will reply with an HTTP 400-series error if the request is bad. That allows the client-side code to potentially better understand what went wrong, rather just receiving an empty HTTP 200 response.
If you were actually passing a meaningful parameter, you might consider stronger server-side validation before working with it.  What happens if empty string is passed?  What happens if an extraordinarily long string (i.e. buffer overflow attack) is passed?
In your getImages function you actually start to handle the edge case by having a conditional around being able to read the directory.  Where is the else to handle a guess where directory is not readable?  You would likely need to send a 500 series response to client to let them know there is internal server problem that is preventing the request from being fulfilled.
On client side, what happens if the server is not able to get a good response?

You are kind of relying on undocumented JSON behavior that certain libraries allow when you are just JSON-encoded a string primitive.  Technically, the JSON specification requires that the only allowed root structures are objects or arrays.  Passing an encoded primitive is not reliable behavior, if you ever have other clients connecting to this service.  You should consider explicitly creating and passing an array like $image = array(randomImage($images))
You should just be echoing out json_encode($image) not print_r


Exception Handling Helper This is some code that I found in a project I work on. Please tell me that I am crazy and that this code is good. Please do.

public class ExceptionHandlingHelper
{
    static IExeptionHandlerProvider _exeptionHandlerProvider = null;
    static IExeptionHandlerProvider ExeptionHandlerProvider
    {
        get
        {
            if (_exeptionHandlerProvider == null)
            {
                Assembly clientLib = Assembly.Load("Bla.BlaDiBla");
                Type type = clientLib.GetType("Bla.BlaDiBla.Controls.ExeptionHandlerProvider");
                _exeptionHandlerProvider = (IExeptionHandlerProvider)Activator.CreateInstance(type);
            }
            return _exeptionHandlerProvider;
        }
    }

    public static void HandleException(FaultException<DefaultFaultContract> fault)
    {
        var message = new StringBuilder();
        var detail = new StringBuilder();

        ExeptionHandlerProvider.GetMessage(fault, message, detail);

        if (!string.IsNullOrWhiteSpace(fault.Detail.FullStack))
        {
            detail.AppendLine();
            detail.AppendLine(fault.Detail.FullStack);
        }

        if (!string.IsNullOrWhiteSpace(fault.Detail.InnerExceptionStack))
        {
            detail.AppendLine();
            detail.AppendLine("Inner Exception Stack");
            detail.AppendLine(fault.Detail.InnerExceptionMessage);
            detail.AppendLine(fault.Detail.InnerExceptionSource);
            detail.AppendLine(fault.Detail.InnerExceptionStack);
        }

        Handle(message.ToString(), detail.ToString());
    }

    ...
}

Used like this:

        string serverName = GetServerName();
        using (var factory = new ChannelFactory<T>(GetBinding(TransportType), GetEndPointAddressForClient(typeof(T), TransportType, serverName)))
        {
            var client = GetChannel(factory);
            try
            {
                using (var op = new OperationContextScope(client as IContextChannel))
                {
                    OperationContext.Current.OutgoingMessageHeaders.Add(GetCachedHeader());
                    action(client);
                    ((IClientChannel)client).Close();
                }
            }
            catch (FaultException<DefaultFaultContract> ex)
            {
                ((IClientChannel)client).Abort();
                factory.Abort();
                ExceptionHandlingHelper.HandleException(ex);
            }
            catch (FaultException ex)
            {
                ((IClientChannel)client).Abort();
                factory.Abort();
                ExceptionHandlingHelper.HandleException(ex);
            }
            catch (Exception ex)
            {
                ((IClientChannel)client).Abort();
                factory.Abort();
                ExceptionHandlingHelper.HandleException(ex);
            }
        }

One thing which I noticed is this little spelling error in the code

static IExeptionHandlerProvider _exeptionHandlerProvider = null;

Both the name of the interface and the name of the field should be fixed by adding a c -> IExceptionHandlerProvider _exceptionHandlerProvider = null;

public static void HandleException(FaultException<DefaultFaultContract> fault)

You should initialze the StringBuilder's with a starting capacity, because the default capacity of a StringBuilder is 16 characters, and if it is reached the capacity will be doubled which involves copying the current buffer to the newly created buffer.

By choosing the initial capacity to be e.g 256 you will maybe waste some memory but you avoid the internal increasing of the buffer.


Codility AbsDistinct: Why is this solution not O(n)? This is part of a series of algorithmic lessons and tests by Codility.

For copyright reasons, I cannot reproduce the question, which asks to compute the number of distinct absolute values in a given sorted array.

The question requires a response in \$O(n)\$ time complexity. My response (below) was detected as \$O(n*log(n))\$ by Codility, but I have no idea why. It looks to me like we're only going to go over each item of the array once. Am I wrong or is this \$O(n)\$?

public int solution(int[] A) {
    // write your code in Java SE 8
    int result = 0;
    int start = 0;
    int end = A.length - 1;

    while (start <= end) {
        result++;
        int startValue = Math.abs(A[start]);
        int endValue = Math.abs(A[end]);
        int currentValueFound = Math.max(startValue, endValue);
        while (start <= end && Math.abs(A[start]) == currentValueFound) {
            start++;
        }
        while (start <= end && Math.abs(A[end]) == currentValueFound) {
            end--;
        }
    }
    return result;
}

In addition, if anyone has an idea what could have triggered the error in the arth_overflow test, I am interested.

I don't see any reason why your solution would not be O(N).

However, it would fail if the array contained Integer.MIN_VALUE, since its absolute value won't fit within an int.


Defining of new, temporary, variables or usage of already known ones? I want to check if the length of phone number is appropriate for specified country (let's consider that only some countries have restriction, another countries accept phone number with various length). I have a Map, where the correct pairs are defined so this map can be used as a reference in the condition:

public static ErrCode checkStatePhoneLen(final String state, final String phoneNo)
{
    String  stateTmp = state.trim();
    String  phoneTmp = phoneNo.trim();
    Integer phoneLen = new Integer(phoneTmp.length());

    if ( statePhoneNoMap.containsKey(stateTmp) && !phoneLen.equals(statePhoneNoMap.get(stateTmp)))
    {
        return ERROR;
    }
    return SUCCESS;
}

My questions are:

Is it better to use temporary variables or directly usage of already existed object? I can just use "state.trim()" instead of creating the variable state_tmp and so on. I think that advantages of the solution with temporary variables are better readability and debugging but disadvantages are the effort to create new variable by runtime (or is it optimized someway by compiler?) and more rows of code (but I prefer readability factor more than number of rows factor).
is it better to check if map contains the key and then compare, or to get value for given key and then check if it is not null and compare them? As following example:

Integer definedLen = (Integer) statePhoneNoMap.get(stateTmp);

if (definedLen != null && !definedLen.equals(phoneLen)){

In this code sample, there is needed one more variable, but the condition is clearer. And, there is just one operation upon map (get()) instead of two in previous code (containsKey(), get())

What is better solution? How would you modify this function?

public static ErrCode checkStatePhoneLen(final String state, final String phoneNo)
{
    String  stateTmp = state.trim();
    String  phoneTmp = phoneNo.trim();

The issue is that stateTmp is less readable than state.trim(), so if you want to create a new variable, make sure that the name carries your intent. You can go for "normalizedState", but since you're only trimming (and are not normalizing capitalization for example) then a variable is useless.

    Integer phoneLen = new Integer(phoneTmp.length());

This one is useful! phoneLen is clearer than phoneNo.trim().length(). This answers your first question: it depends! Use new variable names when they do make thing clearer, but never use dummy names like phoneTmp, phone1, and so on. By the way, I may be mistaken, but are you certain Integer is necessary? Java should do autoboxing. I would simply write int phoneLen = phoneNo.trim().length().

    if (statePhoneNoMap.containsKey(stateTmp) && !phoneLen.equals(statePhoneNoMap.get(stateTmp)))
    {
        return ERROR;
    }
    return SUCCESS;

To answer your second question, you don't need to explicitely check for null: it's simpler to compare phoneLen and statePhoneNoMap.get(stateTmp) directly. If the latter is null, then the comparison will return false. If phoneLen is null, you don't want to return a value but throw an exception anyway, and this is what happens with your current code because null.equals(...) throws.
It makes more sense to check for success and return ERROR if something went wrong.
If you have an ErrCode type instead of a boolean, you have to return more explicit codes! Otherwise just return the condition directly.

The code becomes:

public static ErrCode checkStatePhoneLen(final String state, final String phoneNo)
{
    int phoneLen = phoneNo.trim().length();
    int stateTrimmed = state.trim();

    if (statePhoneNoMap.containsKey(stateTrimmed)
        && phoneLen == statePhoneNoMap.get(stateTrimmed))
    {
        return SUCCESS;
    }
    return ERROR;
}


Array manipulation exercise While trying to learn more about arrays in C, I tried to write some code that did the following:

Read a stream of numbers from the stdin and store them in an array
Print the array in the order the numbers have been stored (i.e. print the original array)
Print the array in reversed order
Sort and print the array
Given an array, search whether an entered value is present in an array. If it is present, print the index otherwise print that the value doesn't exist.

It will be great if somebody could review it and let me know how to improve it.

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

static void printArray(int array[],int startIndex,int endIndex){
    if(startIndex < endIndex){
        while(startIndex <= endIndex){
            printf(" %i ",array[startIndex++]);
        }
    }else{
        while(startIndex >= endIndex){
            printf(" %i ",array[startIndex--]);
        }
    }
    printf("\n");
}

static int cmpfunc(const void * a,const void * b)
{
    if(*(int*)a > *(int*)b) return 1; else return -1;
}

static void sortedArray(int* originalArray){
    qsort((void*)originalArray,(sizeof(originalArray)/sizeof(originalArray[0])),sizeof(originalArray[0]),cmpfunc);
    return;
}

static int getIndex(int value,int array[],int size){
    int i;
    for(i = 0;i < size;i++){
        if(array[i] == value){
            return i;
        }
    }
    return -1;
}

static void identifyTheIndices(int *arbitraryArray,int size){
    char buf[10];
    printf("Enter the value to search for..enter q to exit\n");
    while(fgets(buf,sizeof buf,stdin) != NULL){
                if(buf[0] == 'q'){
            break;
        }
        char *end;
        int value = (int) strtol(buf,&end,0);
        if(end != buf){
            int currentIndex = getIndex(value,arbitraryArray,size);
            if(currentIndex > -1){
                printf("Found the entered value %i at index %i\n",value,currentIndex);
            }else{
                printf("Entered value %i doesn't exist\n",value);
            }
        }
    printf("Enter the value to search for..enter q to exit\n");
    }
}

int main(int argc,char **argv)
{
    int counter = 0;
    if(argc > 1){
        int originalArray[argc-1];
        while(counter < (argc - 1)){
            int currentValue = atoi(argv[counter+1]);
            printf("Reading input value %i into array \n",currentValue);
            originalArray[counter] = currentValue;
            counter++;
        }
        int size = sizeof(originalArray)/sizeof(originalArray[0]);
        printf("Printing out the original array\n");
        printArray(originalArray,0,size - 1);
        printf("Printing out the array in reverse\n");
        printArray(originalArray,size - 1,0);
        printf("Sorting the array in ascending order\n");
        qsort((void*)originalArray,size,sizeof(originalArray[0]),cmpfunc);
        printf("Printing out the sorted array\n");
        printArray(originalArray,0,size-1);
        int arr[] = { 47, 71, 5, 58, 95, 22, 61, 0, 47 };
        identifyTheIndices(arr,sizeof(arr)/sizeof(arr[0]));
    }
    return 0;
}

@sc_ray pretty good effort if you are new to C programming.

A few points in your code which you can improve:

Taking array values from command-line is not good. Its better you use scanf and proper data-type. Below are logs when I pass a very long int value as cmd-line argument. It fails to store the value and gives garbage.

./a.out 7 999999999999 0 123
Reading input value 7 into array
Reading input value 2147483647 into array
Reading input value 0 into array
Reading input value 123 into array
Printing out the original array
7  2147483647  0  123
.....

Hope this is a code pasting mistake. Remove this arr array not needed.

int arr[] = { 47, 71, 5, 58, 95, 22, 61, 0, 47 };
identifyTheIndices(arr,sizeof(arr)/sizeof(arr[0]));

The searching of elements in array can be improved using Binary Search Algo rather than sequential search.

Rest all looks good.


jQuery iterate or two-step? I have an array of values of the form:

configs = ["26W", "27W", "28W"...]

They correspond to some of the a elements in a list:

...

...
...

When the data in the list does correspond to an existing a, I need to set the parent li class to "available" and not "unavailable". If there is an a with a value not in the list, I need to set the class to "unavailable" and not "available". (Edit: I realize that having both classnames is redundant, but I don't have the option to change that right now.)

I can think of two ways to do this:

Iterate

var $allSwatchItems = $("#value-list > li");
$allSwatchItems.each(function () {
  var $this = $(this);
  var $a = $this.children("a");
  if (configs.indexOf($a.attr("data-value")) === -1) {
    $this.addClass("unavailable").removeClass("available");
  } else {
    $this.addClass("available").removeClass("unavailable");
  }
});

Two-step

var $allSwatchItems = $("#value-list > li");
// Set everything to unavailable at first.
$allSwatchItems.removeClass("available").addClass("unavailable");
// Build the selector for the swatch anchors.
var swatchSelector = configs.map(function (val, idx, arr) {
  return "a[data-value='" + val + "']";
}).join(", ");
// Set the matching list items to available.
var $availSwatchItems = $allSwatchItems.children(swatchSelector).parent();
$availSwatchItems.removeClass("unavailable").addClass("available");

I know there are pros and cons to both approaches, but is one significantly better than the other (and how), or is there a third approach I should consider?

Update: Here's how I finally did it:

function checkAvailable() {
    if (-1 !== $.inArray($(this).children("a").attr("data-value"), configs)) {
        return "available";
    } else {
        return "unavailable";
    };
}
$("ul#value-list > li").removeClass("unavailable available").addClass(checkAvailable);

A good general performance rule of thumb is to avoid hitting the DOM (the HTML-manipulating stuff) more than necessary. Most DOM properties aren't just static properties that get set from somewhere else as things change. Invoking them tends to involve buildup and tear-down and examining of internal state, etc. Anything that actually alters HTML is 99.99% likely to cause reflow calculations.

Why have two classes? Just let the lack of an 'available' when checked for with .hasClass('available') represent 'unavailable'. That way you only hit the DOM when you add 'available' and do the attribute checks with both of those className changes setting off reflow calculations.

A note on the second approach. It looks more concise but it's doing a lot of looping and DOM access under the hood. It's important to understand what JQuery does when invoking it. In this line for instance,

$allSwatchItems.removeClass("available").addClass("unavailable");

JQ is going to have to run down the collection of DOM elements with an internal loop access the className property, do a replace operation on it, set the className property, then access the className property again match for 'unavailable' and add it on if it isn't there.

People tend to shy away from using the class attribute as a place to tie things to data but I'm of the opinion that you're always binding to something in the HTML. Why not use the most efficient thing that's easiest to share with other concerns? And no, I do not consider HTML an exclusive property of the view in MVC and similar patterns. It also represents content and document structure. Leave HTML out of your JavaScript modeling conventions. It has too much overlap with other concerns and other domains. Think of DOM API stuff as low-level and view modeling as higher-level stuff that buries the DOM stuff.

Here's how I'd do it - I'll let you sort out what's worth haggling over with your team/tools where control over HTML/CSS is concerned but by baking things into your HTML structure the right way, it can save  a lot of work.

HTML like this with unavailable as default sharing with class with data_val classes if needed (classes can have multiple properties separated by spaces):

...

...
...

build your available selector:

var swatchSelector = '.data_val_' + config.join(', .data_val_'),
//#value-list step to reduce brutal IE8 DOM tomfoolery
$availableSwatchItems = $('#value-list').find(swatchSelector);

$availableSwatchItems.addClass('available'); //.remove('unavailable') if you must

It's not a minor thing to not have control over your HTML and CSS. By structuring to your advantage, you can drastically reduce code and improve performance. Any tool or back-end dev who makes this impossible is like a stick in the eye for a client-side dev. That's your domain. You should have control over it. As for custom attributes, when browsers have access methods as fast as className and ID for other attributes I don't really care what people use for attributes in HTML5 but for now, it's a pointless performance-hamperer, IMO.


Encapsulation of client side logic in web page Is this the best way of doing this.
The sample code is a simple lookup of postnr (read zipcode).
I'm trying to encapsulate all the client side logic inside one "class".

Is this the way of doing callbacks? The this/that stuff sort of creeps me out a little
Should the constructor just be a place where you declare variables? I have a init function call here that sets things up. But it is public and I wonder if there is some best practice around this?

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">

  <title>Untitled Page</title>

  Postnr <input type="text" id="postnr" />
  Poststed <input type="text" id="poststed" />

Update: To answer your specific questions (which I just now realized I completely neglected):

The this/that stuff is one way of doing "context resolution" (you used the word "callback", but that's when you pass a function to another function). There are other ways, such as Function.bind/jQuery.proxy, but in your case assigning a local that variable is a valid and customary way of doing it.
Sure, you can have the constructor do nothing except declare and assign instance variables. But there are no written or unwritten rules regarding a construtor's responsibilities, so it's up to you what it should do. As you say the init method ends up being public, which isn't terribly useful - in fact, init is only useful once in the object's lifetime, so it should probably be in the constructor. And if it's in the constructor, you don't need to assign instance variables; they work fine as local variables and closures. And if they're closures, you don't need the this/that either...

... and so you don't actually need a "class" anymore, which is how I arrived at the code below.

It depends on your specific needs, but with the example you give, it seems like overkill to have a "class" (constructor + prototype) to handle that bit of logic. You can make do with:

$(function () { // shorthand for $(document).ready(...)
  var postNrCtrl   = $("#postnr"),
      postStedCtrl = $("#poststed");

  postNrCtrl.on("change", function () {
    var postnr = jQuery.trim(this.value);

    // if the field is empty, clear the postStedCtrl and stop
    if( !postnr ) {
      postStedCtrl.val("");
      return;
    }

    // Return if the value isn't 4 digits (i.e.
    // if it isn't a valid Norwegian postal code)
    if( !/^\d{4}$/.test(postnr) ) {
      // perhaps you want to alert the user here before returning
      return;
    }

    // Make an explicit JSONP request to get around same-origin policy
    // jQuery takes care of encoding the params and adding the callback param
    $.ajax({
      type:     "GET",
      dataType: "jsonp",
      url:      "http://fraktguide.bring.no/fraktguide/api/postalCode.json",
      data: {
        country: "no",
        pnr:     postnr
      }
    }).done(function (data) {
      if( data && data.valid ) {       // check the response a little
        postStedCtrl.val(data.result); // postStedCtrl is available via closure
      }
    });
  });
});

Here's a demo

On a more general note: For things like controller-logic, I find "classes" too cumbersome. You only need 1 instance of a controller, so coding a prototype and construtor only to instantiate it once, seems like overkill in a language like JavaScript that has object literals and closures.


Finding all prime numbers within a range I am trying to find all primes less than 2,000,000 and sum them together. My code currently takes 1'36" to run. Is there a faster way to get my solution?

The logic I am following in this code is make a list of all primes I have found. Then check that every odd number less than 2,000,000 is not divisible by any of the primes in my list. If that is the case I am calling the number prime.

package main

import ("fmt"
"time"
)

func find_primes(max int) []int {
    // list of all primes that we find
    var primes []int
    primes = append(primes, 2)

    var is_prime = true
    for i:=3; i<=max; i++ {
        is_prime = false
        // only odds can be prime
        if i%2 != 0 {
            is_prime = true
            // should not be divisible by any previous
            // prime numbers
            for _, x := range primes {
                if i%x == 0 {
                    is_prime = false
                    break
                }
            }
        }
        if is_prime {
            primes = append(primes, i)
        }
    }
    return primes
}

// just sume up all of the primes
func sum_primes(primes []int) int {
    var total int = 0
    for _, x := range primes {
        total = total + x
    }
    return total
}

func main() {
    start := time.Now()
    // find all primes less than 2,000,000
    primes := find_primes(2000000)
    sum := sum_primes(primes)
    fmt.Println(sum)
    t := time.Now()
    elapsed := t.Sub(start)
    fmt.Println(elapsed)
}

I am trying to find all primes less than 2,000,000 and sum them
  together. My code currently takes 1'36" to run. Is there a faster way
  to get my solution?

Yes. For example,

142913828922
3.860761ms

versus your

142913828922
1m35.090248409s

prime.go:

package main

import (
    "fmt"
    "time"
)

const (
    prime    = 0x00
    notprime = 0xFF
)

func oddPrimes(n uint64) (sieve []uint8) {
    sieve = make([]uint8, (n+1)/2)
    sieve[0] = notprime
    p := uint64(3)
    for i := p * p; i <= n; i = p * p {
        for j := i; j <= n; j += 2 * p {
            sieve[j/2] = notprime
        }
        for p += 2; sieve[p/2] == notprime; p += 2 {
        }
    }
    return sieve
}

func sumPrimes(n uint64) uint64 {
    sum := uint64(0)
    if n >= 2 {
        sum += 2
    }
    for i, p := range oddPrimes(n) {
        if p == prime {
            sum += 2*uint64(i) + 1
        }
    }
    return sum
}

func main() {
    start := time.Now()

    var n uint64 = 2000000 - 1
    sum := sumPrimes(n)
    fmt.Println(sum)

    fmt.Println(time.Since(start))
}

Reference: Sieve of Eratosthenes - Wikipedia

Go was designed for Google scale. Therefore, Go package testing includes benchmarking tools. For example,

$ go test -bench=. -benchmem
BenchmarkPeterSO-8     500        3805125 ns/op    1007621 B/op     1 allocs/op
BenchmarkNightman-8      1    95703259026 ns/op    5866752 B/op    31 allocs/op
$

  Comment: You have presented an alternative solution, but haven't
  reviewed the code. Please explain your reasoning (how your solution
  works and why it is better than the original) so that the author and
  other readers can learn from your thought process. – Martin
  R

The thought process is simple, obvious, and well-known.

The prime number problem is well-known.

Therefore, Standing on the shoulders of giants - Wikipdia.

"If I have seen further it is by standing on the sholders [sic] of Giants." Isaac Newton

For example, Sieve of Eratosthenes - Wikipedia.

The algorithm given in the question is much slower than Eratosthenes' well-known algorithm, approximately 25,000 times slower.

In real-world code reviews, code should be correct, maintainable, robust, reasonably efficient, and, most importantly, readable. The code in the question is not reasonably efficient.


Implement stack using a linked list I am trying to implement Stack using a linked list. Looking for code-review, best practices and optimizations.

public class StackUsingLinkedList<T> {

    private Node<T> top;
    int size;

    public StackUsingLinkedList() {
    }

    private static class Node<T> {
        Node<T> next;
        T item;

        public Node (T item, Node<T> next) {
            this.item = item;
            this.next = next;
        }
    }

    /**
     * Push the item into the stack
     *
     * @param item  the item to be pushed.
     */
    public void push(T item) {
        final Node<T> node = new Node<T>(item, top);
        top = node;
        size++;
    }

    /**
     * Pops the item from the stack
     *
     * @return  the popped item
     * @throws EmptyStackException if attempt is made to access/pop out of an empty stack.
     */
    public T pop() {
        if (top == null) {
            throw new EmptyStackException();
        }

        T item = top.item;
        top = top.next;
        return item;
    }

    /**
     * Returns the item at top of the stack.
     *
     * @return  the item at top of stack.
     * @throws EmptyStackException if attempt is made to access empty stack
     */
    public T peek () {
        if (top == null) {
            throw new EmptyStackException();
        }
        return top.item;
    }

    public int size() {
        return size;
    }

    public boolean isEmpty() {
        return top == null;
    }

    public static void main(String[] args) {
        StackUsingLinkedList<Integer> s = new StackUsingLinkedList<Integer>();
        s.push(10);
        s.push(20);

        // expected 20, 10.
        while (!s.isEmpty()) {
            System.out.println(s.pop());
        }
    }

This is generally good code, with standards-conforming names and style.

It appears that size is an afterthought variable. It is not scoped as being 'private', and it is not decreased in the pop() method, so it is buggy in two ways.

Additionally, even in the inner private Node class, I would recommend declaring the item and next as private, and also adding getters/setters for them.

Otherwise, this is good code.


Error handling for function calls to parse a GIF file I'm debating which version would be more suitable in parameters of code length, readability and maintenance. This is a portion of a set of functions which together parse a Gif file.

Version 1:

if ((errno = GifLoadHeaders(g, &bufTemp)) != GIF_SUCCESS) {
    GifDispose(g);
    *gPtr = NULL;

    return errno;
}

if ((errno = GifLoadGCT(g, &bufTemp)) != GIF_SUCCESS) {
    GifDispose(g);
    *gPtr = NULL;

    return errno;
}

if ((errno = GifLoadImages(g, &bufTemp)) != GIF_SUCCESS) {
    GifDispose(g);
    *gPtr = NULL;

    return errno;
}

Version 2:

if ((errno = GifLoadHeaders(g, &bufTemp)) != GIF_SUCCESS
    || (errno = GifLoadGCT(g, &bufTemp)) != GIF_SUCCESS
    || (errno = GifLoadImages(g, &bufTemp)) != GIF_SUCCESS) {

    GifDispose(g);
    *gPtr = NULL;

    return errno;
}

I generally prefer the latter version. Maybe there's a more optimal third solution to my issue. Any help?

EDIT:
I'm sorry I did not include the entire routine. The code I've included is used in the function GifParse which initializes a given pointer to a Gif structure, which is named gPtr.

Without knowing the context, I'd write it as:

if( (errno = GifLoad( g, &bufTemp )) == GIF_SUCCESS ) {
  // Process the correctly loaded image as necessary.
  GifProcess( g );
}

// Release memory once processing is complete.
GifDispose( g );
*gPtr = NULL;

// Single exit point to the function.
return errno;

Then write a wrapper for GifLoad that calls the three other functions.

This might not be a suitable solution if, for example, the processing is part of an interactive program wherein the processing is indeterminate. If processing cannot be coded at this point in the source, then consider:

if( (errno = GifLoad( g, &bufTemp )) != GIF_SUCCESS ) {
  // Perform whatever is necessary to inform the user the GIF cannot be loaded.
  GifInvalid( g );

  // Release memory.
  GifDispose( g );
  *gPtr = NULL;
}

return errno;

In both cases consider abstracting the three function calls into a single function, which cleans up the code and expresses the intent of those three function calls more concisely.


Printing all the combination of indexes for 2 sum exact outcome Can somebody review the code and provide suggestions on where I can improve on the code like use of better data structure etc.

Input: {1,7,2,8,3,6}, 9

Output: [0, 3] [1, 2] [4, 5]

public class TwoSumExactAll {
    public static void main(String[] args) {
        int[] nums1 = {1,7,2,8,3,6};
        for (List<Integer> list : twoSumExactWithList(nums1,9)) {
            System.out.println(list);
        }
    }

    public static ArrayList<List<Integer>> twoSumExactWithList(int[] nums,int target) {
        HashMap<Integer,Integer> map = new HashMap<Integer, Integer>();
        ArrayList<List<Integer>> outerList = new ArrayList<List<Integer>>();
        ArrayList<Integer> innerList=null;
        int complement;
        for(int i=0;i<nums.length;i++) {
            complement = target-nums[i];
            if(map.containsKey(complement)) {
                innerList = new ArrayList<Integer>();
                innerList.add(map.get(complement));
                innerList.add(i);
                outerList.add(innerList);
            }else {
                map.put(nums[i], i);
            }
        }
        return outerList;
    }
}

I guess that the relevant piece of code is the twoSumExactWithList method and for now I will ignore the rest...

I think that depending on the context, e.g. the principles used in the project that you intent to use, this code you could either fully embrace collections or stick to arrays... but your code does a combo of both where the arguments are naked arrays and the returns uses collections... so please choose between the two.

If you choose arrays a possible return type is int[][] where one of the two dimensions is 2. representing first vs second index of the pair. In practice int[2][] is potentially more efficient than int[][2] but perhaps the Jit may make it irrelevant. Perhaps you could have a IntPair[] where IntPair is a pair of ints struct-like class.

If you choose collections... then the arguments should be List<Integer> nums, int target. The result is kind-of fine... but don't commit to return an ArrayList but rather a List; don't give any details on the implementation that are not necessary. To encode the index pair, consider the use of a custom class IntPair again... List is correct but seems a bit overkill considering that we know is always two and exactly two elements.

As for the method body... The varible types should be a general as they can be... so map's type should be Map<Integer, Integer> rather than HashMap<...>, outerList's should be List rather than ArrayList and so forth...

I would declare complement within the loop as it is not used outside of it and is rewritten at every iteration. innerList would go within the if within the for.

Whenever possible I would declare variables as final to give an extra hint to the compiler/optimizer that indeed these are not going to be changed... eg. final int[] nums, final int target, final Map<Integer, Integer> map, final int complement.

I would rename map to something a bit more informative as to what it contains...
so perhaps indexByNumValue?

I think your code would read and look much better with some additional spaces e.g.:

List<Integer> innerList=null; to List<Integer> innerList = null;.

for(int i=0;i<nums.length;i++) { to for (int i = 0; i < nums.length; i++) {.

}else { to } else {.

if( to if (.

nums,int to nums, int.

etc...


Basic template engine I wanted to know how I can improve it. I mainly want to look at speed and performance, but if there are any improvements please give a suggestion.

<?php
defined("SECURE") or exit('Please define SECURE keyword to continue.');
class template
{
    private $templateParams;

    public function __construct()
    {
        $templateParams = array();
    }

    public function loadTemplate($configClass)
    {
        $this->organiseParams($configClass);
        $this->displayContent($configClass);
    }

    private function organiseParams($configClass)
    {
        $values = $configClass->getAll();

        while(list($title, $value) = each($values))
            $this->addParameter('[config ' . $title . ']', $value);
    }

    private function displayContent($configClass)
    {
        if (strlen($_GET['page']) >= 1)
        {
            ob_start();

            if (file_exists(ROOT . $configClass->getConfig("style.folder") . '/' . $configClass->getConfig("style.name") . '/' . $_GET['page'] . '/public_html/content.php'))
                include ROOT . $configClass->getConfig("style.folder") . '/' . $configClass->getConfig("style.name") . '/' . $_GET['page'] . '/public_html/content.php';
            else
                include ROOT . $configClass->getConfig("style.folder") . '/' . $configClass->getConfig("style.name") . '/error/public_html/content.php';

            $htmlOutput = ob_get_contents();
            ob_end_clean();

            while (list($paramTitle, $paramValue) = each($this->templateParams)) {
                $htmlOutput = str_replace($paramTitle, $paramValue, $htmlOutput);
            }

            echo $htmlOutput;
        }
        else
        {
            header("location: " . $configClass->getConfig("website.url") . "/index");
        }
    }

    private function addParameter($title, $value)
    {
        $this->templateParams[$title] = $value;
    }
}
?>

Your first concern is performance, so let's start there. The way you're including and processing your content is going to slow you down. Because you're buffering it, then putting it to a variable, then doing a search replace, you're going to end up working through the content several times before you get your end result. If the content is small then don't even worry about it, but if you have embedded base64 images or just tons of content, then you're going to want a different approach.

From a code point a view I think you're on the right track with your object design. It's always good to break things out to make them easier to maintain. One thing you want to consider though is to take a more MVC style approach by separating processing from the visuals. This makes code easier to read and much easier to maintain. The long story short of this point is to change the echo of the output to a return so that your calling (visual) code can output it. It sounds minor but as your application becomes more complex, it can be a headache saver.

Back to your first point, how to improve getting of the content. My approach would be to make the included content an object file. The file could then include a get method of sorts that returned the html in a string with your titles and parameters already in place. That would allow you to have your content file be built just like a concatenated string (saving the find and replace work). Pass your values as parameters and you're good to go.

Here's how I would do this. I didn't include code for the content class as the code in this Template class should be enough to go on. Note that based on my example, all include files should have the class ContentObj with the method getOutput. I simplified your include path code to make things much easier to read. I also added an exit after your header redirect as there's no need for further processing (usually) after a redirect.

<?php
defined("SECURE") or exit('Please define SECURE keyword to continue.');
class Template
{
    private $configClass;
    private $templateParams;

    public function Template($configClass)
    {
        $this->configClass = $configClass;
        $this->templateParams = array();
    }

    public function loadTemplate()
    {
        $this->organiseParams();
        $this->displayContent();
    }

    private function organiseParams()
    {
        $values = $this->configClass->getAll();

        while(list($title, $value) = each($values))
            $this->addParameter('[config ' . $title . ']', $value);
    }

    private function displayContent()
    {
        if (strlen($_GET['page']) >= 1)
        {
            $contentPath = ROOT . $this->configClass->getConfig("style.folder") . '/' . $this->configClass->getConfig("style.name");

            if (file_exists($contentPath . '/' . $_GET['page'] . '/public_html/content.php'))
                include $contentPath . '/' . $_GET['page'] . '/public_html/content.php';
            else
                include $contentPath . '/error/public_html/content.php';

            $contentObj = new ContentObj($paramTitle, $paramValue, $htmlOutput);
            $htmlOutput = $contentObj->getOutput($this->templateParams);

            return $htmlOutput;
        }
        else
        {
            header("location: " . $configClass->getConfig("website.url") . "/index");
            exit;
        }
    }

    private function addParameter($title, $value)
    {
        $this->templateParams[$title] = $value;
    }
}
?>


Grouping consecutive numbers into ranges in Python 3.2 : New version for set def range_find(pageset,upperlimit=1000):

    pagerangelist=[]

    for page in range(0,upperlimit):

        if page in pageset and previouspage in pageset:pagerangelist[-1].append(page)
        elif page in pageset and not (previouspage in pageset):pagerangelist.append([page])
        else: pass

        previouspage=page

    pagerangestringlist=[]

    for pagerange in pagerangelist:

        pagerangestringlist.append(str(pagerange[0])+('-'+str(pagerange[-1]))*((len(pagerange)-1)>0))

    return(','.join(pagerangestringlist))

print(range_find({1,2,5,7,8,11,19,25,29,30,31,39,40,41,42,43,44,56,57,59,60,70,71,72,100,101,103,104,105}))

def range_find(pageset,upperlimit=1000):

    pagerangelist=[]

    for page in range(0,upperlimit):

There is no reason to include the 0. Why don't you calculate the upperlimit by max(pageset)?

        if page in pageset and previouspage in pageset:pagerangelist[-1].append(page)
        elif page in pageset and not (previouspage in pageset):pagerangelist.append([page])
        else: pass

Don't squish all that stuff onto one line. It makes it harder to read. Having an else: pass case is useless

        previouspage=page

Since you are counting, just use page - 1 instead of previouspage

    pagerangestringlist=[]

    for pagerange in pagerangelist:

        pagerangestringlist.append(str(pagerange[0])+('-'+str(pagerange[-1]))*((len(pagerange)-1)>0))

Too clever. Just use an if. This technique makes it harder to see what is going on. Also pointless extra parens.

    return(','.join(pagerangestringlist))

Don't put parens around your return statement expressions.

Algorithmwise this may be problematic because it will slower for books with many pages even if you don't use many pages from them. For example

range_find({1, 2, 1000000},100000000)

Will spend a long time churning through pages that aren't really of interest.


SQL Look up in a Stored Procedure across three tables The idea of the below code is that I feed back to a Gridview if the user has permission to view the property otherwise that property is not show.

The data is passed in via SessionParameters

USE [database]
GO

SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO

alter PROCEDURE [dbo].[spSafeguardingActionPropertyByPermission]

@RegionID bigint
,@EmployeeID varchar(10)
,@PropertyID varchar(10)

AS

BEGIN

SELECT  TblA.PropertyID as A_PropertyID,
        TblA.Propertyname as A_Propertyname ,
        TblB.FireSafety as FireSafety1,
        TblB.DisplayScreenEquipment as DSE
FROM    TbPropertyDetails as TblA
        INNER JOIN TbPropertyDetailsSafeguarding as TblB
            ON TblA.PropertyID = TblB.PropertyID
WHERE   TblA.RegionID > 0
AND     TblA.PropertyID LIKE '%' + @PropertyID + '%'
AND     EXISTS
        (   SELECT  1
            FROM    tblPropertyViewPermissions AS pvp
            WHERE   pvp.EmployeeID = @EmployeeID
            AND     pvp.PropertyID = TblA.PropertyID
        )

        END

Does your EmployeeID and PropertyID parameters really need to be varchar(max), I find it hard to believe that an employee would have an ID with 8 000 characters. The same goes for the property ID. Change this to how many characters they are allowed to have (e.g., if an employee can only have an ID of 8 characters then use that, if it's 100 characters use that, and if it indeed is 8 000 characters, well then I suppose varchar(max) is fine)

I'd also advocate to use clearer aliases for your column. Instead of as PId you should alias it as as PropertyID.

This line is a bit unclear to me WHERE   TblA.RegionID > 0. Can a Region have an ID of 0 or less-than 0?

I also think that the LIKE in this line TblA.PropertyID LIKE '%' + @PropertyID + '%' is used for the wrong purpose. It seems like what you really are doing is comparing the PropertyID of TblA.PropertyID to the parameter @PropertyID and in my opinion there should be no need of using wildcards here. Instead use the equals operator.


Analysis of call center employee performance This code basically calculates a few metrics based on numerators and denominators and then it bins the data into quintiles. This is a common operation that I perform for various data sets. My issue is that I always have to rename the metrics and columns.

I have a feeling that I could create functions to make this more general perhaps. The common operations I am doing are:

% of total numerator
delta from a target
attainment (actual divided by the target)
impact (the weighted delta from the target, how much a person is increasing or decreasing the total score)
The quintiles (I have them labeled so the end-user understands what they are. These files are used in Excel for pivoting how the top and bottom employees perform)

I would like to have it more general: input a numerator and denominator. Instead of global variables, perhaps they could be input into a function?

csat_target =  0.75
fcr_target = 0.75
nps_target = 0.25
aht_target = 5.25

csat_weight = 0.30
fcr_weight = 0.30
nps_weight = 0.40
aht_weight = 0.00

df['fcr'] = df['final_hits'] / df['total_hits']
df['csat'] = df['csat_score_weight'] / df['survey_count']
df['nps'] = (df['promoters'] - df['detractors']) / df['survey_count']
df['aht'] =  (df['total_time'] / df['total_calls'])
df['goal_attainment'] = ((df['nps'] / nps_target) * nps_weight) + ((df['csat'] / csat_target) * csat_weight) + ((df['fcr'] / fcr_target) * fcr_weight)

df['fcr_volume'] = df['total_hits'] / df['total_hits'].sum()
df['fcr_delta'] = df['fcr'] - fcr_target
df['fcr_impact'] = (df['fcr_delta'] * df['fcr_volume'])

df['survey_volume'] = df['survey_count'] / df['survey_count'].sum()
df['nps_delta'] = nps_target - df['nps']
df['nps_impact'] = df['nps_delta'] * df['survey_volume']
df['overall_nps_impact'] = np.where(df['survey_count'] > 0, df['nps_impact'], 0)

df['csat_delta'] = df['csat'] - csat_target
df['csat_impact'] = -(df['csat_delta'] * df['survey_volume'])
df['overall_csat_impact'] = np.where(df['survey_count'] > 0, df['csat_impact'], 0)

df.replace([np.inf, -np.inf], np.nan, inplace = True)

df['call_volume'] = df['total_calls'] / df['total_calls'].sum()
df['aht_delta'] = df['aht'] - aht_target
df['aht_impact'] = (df['aht_delta'] * df['call_volume'] )

df['fcr_weight'] = np.where(df['total_hits'] > 0, fcr_weight, 0)
df['nps_weight'] = np.where(df['survey_count'] > 0, nps_weight, 0)
df['csat_weight'] = np.where(df['survey_count'] > 0, csat_weight, 0)

df['fcr_dfile'] = pd.qcut(df['fcr_impact'], 5, labels = ["D1", "D2", "D3", "D4", "D5"])
df['nps_dfile'] =  pd.qcut(df['overall_nps_impact'] , 5, labels = ["D1", "D2", "D3", "D4", "D5"])
df['csat_dfile'] = pd.qcut(df['overall_csat_impact'] , 5, labels = ["D1", "D2", "D3", "D4", "D5"])
df['aht_impact_dfile'] = pd.qcut(df['aht_impact'], 5, labels = ["D1", "D2", "D3", "D4", "D5"])
df['aht_dfile'] = pd.qcut(df['aht'], 5, labels = ["D1", "D2", "D3", "D4", "D5"])

Yes, functions would be a splendid idea.  However a lot of the
calculations aren't repeated, so it's hard to see which parts could be
factored out usefully.

Apart from that I'd suggest putting the *_target and *_weight
variables into a dictionary or another data frame so that they can be
referred to by name, making it more easy to use them from functions:

parameters = pandas.DataFrame({
    'target': [0.75, 0.75, 0.25, 5.25],
    'weight': [0.30, 0.30, 0.40, 0.00]
}, index = ['csat', 'fcr', 'nps', 'aht']).transpose()

With the data frame above it's easy to select just the parameters for
"csat" for example, using parameters.csat.target or  .weight.

That also makes certain queries more generic:

df['goal_attainment'] = sum(df[name] / parameters[name].target * parameters[name].weight
                            for name in parameters if name not in {"aht"})

A couple of frequently used queries can also be reused,
e.g. df['survey_count'] should likely be cached, similarly for the
comparisons:

survey_count = df['survey_count']
positive_survey_count = df['survey_count'] > 0

(Maybe take a closer look to the Pandas API, e.g. how the data could be
structured with nested names and multi indexes.)


Finding the min and max of an array, using a pointer Modify the maxmin.c program so that the max min function uses a
  pointer instead of an integer to keep track of the current position in
  the array.

  maxmin.c

/* Finds the largesy and smallest elements in an array */

#include <stdio.h>
#define N 10

void max_min(int a[], int n, int *max, int *min);
int: main (void)
 {
  int b[N], i, big, small;
  printf (''Enter %d numbers: ", N) ;

  for (1 = 0: i < N: i++)
   scanf ( "%d'', &b[i] ) ;

  max_min (b, N. &big , &small);
  printf (" Largest : %d\n " , big) ;
  printf (" Smallest: %d\n", small);
  return 0;

  void max_min(lnt a[] , int n, int *max, int *min)
{
int i;
*max = *min = a[O];

   for (i = 1; i > n; i++) {
  if ([a[i] > *max)
  *max - a[i];

  else if (a[i] < *min)
  *min - a (i] ;
 }
 }

The code I have written:

#include <stdio.h>
#define N 10

void max_min(int a[], int n, int *max, int *min);
int main(void)

{
int b[N], *i, big, small;

printf("Enter %d numbers\t:",N);
for(i=&b[0];i<&b[N];i++)
    scanf("%d",i);

max_min(b, N, &big, &small);

printf("\nlargest:\t%d",big);
printf("\nsmallest:\t%d",small);

return 0;
}

void max_min(int a[], int n, int *max, int *min)
{
int *i;
*max = *min = a[0];
for (i=&a[0]; i<&a[n]; i++)

{
    if(*i>*max)
    *max=*i;
    else if (*i<*min)
    *min=*i;
}}

int b[N], *i, big, small;

printf("Enter %d numbers\t:",N);
for(i=&b[0];i<&b[N];i++)
    scanf("%d",i);

I would normally expect i to be an index variable.  If you want a one letter name for a pointer, I'd suggest p.  However, I would normally go ahead and name such a variable current.

    int b[N], big, small, *current, *end

    printf("Enter %d numbers\t:", N);
    for (current = b, end = &b[N]; current < end; current++)
    {
        scanf("%d", current);
    }

It's unnecessary to say &b[0].  Just b will do the same thing.  The compiler may optimize it out, but I find it more readable to use the variable end rather than calculate &b[N] each time.

It's generally preferred to use the block form for loops, even when the single statement form would work.  This makes it clearer what is in the loop.

It's not terribly important, but I'd consider naming b something more descriptive, even if it's just numbers.

void max_min(int a[], int n, int *max, int *min);

You wouldn't need a prototype here if you put the max_min function before main instead of after.

If you are not using an index variable, it would make more sense to pass a pointer into the function.

void max_min(int *start, int *end, int *max, int *min);

Which you can call

    max_min(b, end, &big, &small);

And then inside the function, you'd replace

int *i;
*max = *min = a[0];
for (i=&a[0]; i<&a[n]; i++)

{
    if(*i>*max)
    *max=*i;
    else if (*i<*min)
    *min=*i;
}

with

    int *current;
    *max = *min = *start;
    for (current = start + 1; current < end; current++)
    {
        if (*current > *max)
        {
            *max = *current;
        }
        else if (*current < *min)
        {
            *min = *current;
        }
    }

Note that this says start + 1 rather than start.  This is because you implicitly do the first iteration of the loop when you set *max and *min to *start.  You don't need to do it a second time.


Adding two different group of objects to a list of groups I have two lists of Strings and two lists of objects. I need to copy the list of strings to the correspondence list of objects with specific values.

public List<Name> retrieveCitiesAndCountries(){
  List<String> list1 = new ArrayList<String>(); //list of cities
  List<String> list2 = new ArrayList<String>(); //list of countries
  list1 = retrieveListOfCities();
  list2 = retrieveListOfCountries();

  List<Name> namesList = new ArrayList<Names>();
  return namesList;
}

Name class

class Name{
  String name;
  String attrib;
  ..
}

I need to add both lists to namesList and add the respective attribs. For example, all cities should have City as their attribs and all countries should have Country as their attribs.

 for(int i=0;i<list1.size();i++){
     Name name = new Name();
     name.setName(list1.get(i));
     name.setAttrib("City");
     namesList.add(name);
   }

  for(int i=0;i<list2.size();i++){
     Name name = new Name();
     name.setName(list2.get(i));
     name.setAttrib("Country");
     namesList.add(name);
   }

I am wondering if there is any better way to do it.

I use namesList to return in JSON format.

@RequestMapping(value = "/names")
public @ResponseBody List<Name> retrieveCitiesAndCountries(){
    List<Name> namesList = retrievalServ.retrieveCitiesAndCountries();
    return namesList;
}

The Name() class has what I would call a half-assed constructor: it creates an object in a not-quite-initialized state, making it temporarily useless until the setters are called.  If you had a Name(String name, String attrib) constructor, then the code would be a lot less cumbersome.

You can also avoid using indexed loops.

for (String city : list1) {
    namesList.add(new Name(city, "City"));
}
for (String country : list2) {
    namesList.add(new Name(country, "Country"));
}


Benchmarking inline vs normal functions in C++ I am currently learning C++ and have learnt about inline functions and how they can give a performance benefit.

How could this be improved?

#include <iostream>
#include <chrono>
#include <ctime>

long fibonacci(unsigned n) {
    if (n < 2) return n;
    return fibonacci(n - 1) + fibonacci(n - 2);
}

inline long fib(unsigned n) {
    if (n < 2) return n;
    return fibonacci(n - 1) + fibonacci(n - 2);
}

void timedFibonacci(unsigned n) {

    // Time vars
    std::chrono::time_point<std::chrono::system_clock> start, end;

    // Start time.
    start = std::chrono::system_clock::now();

    //std::cout << "Functional Fibonacci of " << n << " = " << fibonacci(n) << '\n';
    fibonacci(n);

    // End time.
    end = std::chrono::system_clock::now();

    // Time elapsed.
    std::chrono::duration<double> functionalTimeElapsed = end - start;

    // Start time.
    start = std::chrono::system_clock::now();

    //std::cout << "Inline Fibonacci of " << n << " = " << fib(n) << '\n';
    fib(n);

    // End time.
    end = std::chrono::system_clock::now();

    // Time elapsed.
    std::chrono::duration<double> inlineTimeElapsed = end - start;

    if (functionalTimeElapsed > inlineTimeElapsed) {
        std::cout << "n = " << n << "\tFunctional method was faster by " << ((functionalTimeElapsed - inlineTimeElapsed) / inlineTimeElapsed) * 100 << "%\n";
    } else if (functionalTimeElapsed < inlineTimeElapsed) {
        std::cout << "n = " << n << "\tInline method was faster by " << ((inlineTimeElapsed - functionalTimeElapsed) / functionalTimeElapsed) * 100 << "%\n";
    } else {
        std::cout << "n = " << n << "\tFunctional method and inline method took the same time.\n";
    }
}

int main() {

    for (int i = 5; i <=40 ; i++) {
        timedFibonacci(i);
    }

    cin.get();
}

Inlining

First of all, your inline function calls the non-inline function, so if the compiler is really paying attention to the inline specification, it's only affecting a single "layer" of invocation.

inline long fib(unsigned n) {
    if (n < 2) return n;
    return fibonacci(n - 1) + fibonacci(n - 2);
}

To stand a chance of getting some good out of the inline specification, you probably want to have this call itself instead of the other function:

inline long fib(unsigned n) {
    if (n < 2) return n;
    return fib(n - 1) + fib(n - 2);
}

In general, recursive functions are a problem for inlining in any case, so many compilers completely disable inline code generation for recursive functions.

Add in the fact that most reasonably current compilers basically ignore the inline specifier anyway, and you get a high likelihood that you won't see any difference between these functions at all.

Timing

I'd do the timing somewhat differently. The single responsibility principle applies here, just like everything else. That means the timer should deal only with timing. I usually use something on this general order:

template <typename F, typename ...Args>
auto timer(F f, std::string const &label, Args && ...args) {
    using namespace std::chrono;

    auto start = high_resolution_clock::now();
    auto holder = f(std::forward<Args>(args)...);
    auto stop = high_resolution_clock::now();
    std::cout << label << " time: " << duration_cast<microseconds>(stop - start).count() << "\n";

    return holder;
}

Arguably this already does more than it should (printing out results in addition to the actual timing) but at least it's quite a bit closer to a single responsibility.

With that, we invoke each Fibonacci generator separately, something on this order:

auto foo = timer(fib, "inline", max);

The result will then look something like this:

inline time: 1062

Just a minor note: this is a somewhat more general timing function that most people usually need. In particular, it doesn't require the function being timed to take a specific number of arguments--the number and types of arguments you pass after the label have to match those needed by the function you're timing, but as far as the timer itself cares, it's essentially wide open.

Efficiency

Ignoring, for the moment, the issue of inlining (which is probably pretty much a red herring anyway), I'd consider other ways of computing Fibonacci numbers. In the absence of memoization, a recursive function is horribly inefficient. An iterative function is many times faster. That can be written with a for loop, something on this general order:

unsigned long long fib(unsigned limit) {
    unsigned long long a = 1;
    unsigned long long b = 1;
    unsigned long long c;

    if (limit < 2)
        return 1;

    for (auto i=1ULL; i<limit; i++) {
        c = a + b;
        a = b;
        b = c;
    }
    return c;
}

This is somewhat longer, but the difference in speed is...fairly substantial. Computing fib(43) with each, I get timings like this:

out of line time: 2807407170
     inline time: 2794785991
  iterative time: 321
Sum (ignore): 1568397607

I had to switch to showing the time in nanoseconds to get a non-zero time for the iterative solution. Even so, for fib(43) and highest resolution-timing I can get, it still shows a time of 0 about one run out of every 3 or 4. The recursive versions are approximately 7 orders of magnitude slower (and although inline code generation did seem to help a tiny bit, it's still minuscule compared to the improvement from a better algorithm. Oh, and for what it's worth, it looks like the inline code generation really did make a difference. It is pretty small, but at least in my testing, the inline version does finish just a tiny bit faster every time.

Final Code

For what it's worth, here's the code I ran to get the timing comparison:

#include <iostream>
#include <string>
#include <chrono>

template <typename F, typename ...Args>
auto timer(F f, std::string const &label, Args && ...args) {
    using namespace std::chrono;

    auto start = high_resolution_clock::now();
    auto holder = f(std::forward<Args>(args)...);
    auto stop = high_resolution_clock::now();
    std::cout << label << " time: " << duration_cast<nanoseconds>(stop - start).count() << "\n";

    return holder;
}

long fibonacci(unsigned n) {
    if (n < 2) return n;
    return fibonacci(n - 1) + fibonacci(n - 2);
}

inline long fib(unsigned n) {
    if (n < 2) return n;
    return fib(n - 1) + fib(n - 2);
}

unsigned long long iter_fib(unsigned limit) {
    unsigned long long a = 1;
    unsigned long long b = 1;
    unsigned long long c;

    if (limit < 2)
        return 1;

    for (auto i = 1ULL; i < limit; i++) {
        c = a + b;
        a = b;
        b = c;
    }
    return c;
}

int main() {
    static const int max = 43;
    auto a = timer(fibonacci, "out of line", max);
    auto b = timer(fib,       "     inline", max);
    auto c = timer(iter_fib,  "  iterative", max);
    std::cout << "Sum (ignore): " << a + b + c;
}


Print first element of the last line of files when the second element does not match a pattern I am relatively new to Bash, regular expression and stuff.

In a directory, I need to read the last line of all the files that contain the word "fitness", and list (on one line and separated with comas) the first element of these last lines (where elements are defined by spaces), whenever the second element (or any element actually) is not "100000".

tail -n 1 *fitness* | grep -v '100000' | grep -v 'fitness' | awk '{print $1}' | grep -v '^$' | paste -d  "," - - - - - - - - - - - - - - - - -

In words:

  read all last lines | don't consider those that contain the number 100000 | remove the name of the files | take the first element | remove blank lines | paste the element together on one line separated by comas

Can you help me to improve my code?

That second grep (grep -v 'fitness') is there to filter out the filenames that tail spits out, right? That can be removed by using the -q option for tail.

Personally, I'd try to avoid that paste command, but that might just be because I'm not used to it. If you don't need spaces after the commas, tr works just fine.

tail -qn 1 *fitness* | grep -v '100000' | awk '{print $1}' | grep -v '^$' | tr '\n' ','

Now, since you're already using awk, why not do a bit more with it (actually, the whole thing could probably be made into an awk program with either more time or more experience than I have available)?

This uses a single awk statement to choose only lines that don't contain the word '100000' and also have a non-empty first field, and also to print that first field. What it does for readability depends on personal preference, but it certainly reduces the number of programs that need to be chained together:

tail -qn 1 *fitness* | awk '(! /\<100000\>/) && $1 !~ /^$/ {print $1}' | tr '\n' ','