Building a better Data Access Layer Class Since I am new to MVC and the Entity Framework, I have been struggling to grasp the concept of creating useful Data and Service layers. I have come across a scenario where I believe my code has become very redundant:

Data Access Layer:

public static class DeploymentOrderDataLayer
    {

     public static usr_OrderFulfillment GetScannedItem(string orderNumber, string itemNumber)
        {
            using (TPGContext context = new TPGContext())
            {
                return context.usr_OrderFulfillment.FirstOrDefault(x => x.SOPNUMBE == orderNumber && x.ItemNumber == itemNumber);
            }
        }

    public static void UpdateScannedItem(string orderNumber, string itemNumber, string user, string serialNumber)
        {
            using (TPGContext context = new TPGContext())
            {
                var result = context.usr_OrderFulfillment.FirstOrDefault(x => x.SOPNUMBE == orderNumber && x.ItemNumber == itemNumber);

                result.IsFulfilled = true;
                result.FulfilledBy = user;
                result.DateFulfilled = DateTime.Now;

                if (!string.IsNullOrEmpty(serialNumber))
                {
                    result.SerialNumber = serialNumber;
                }

                context.SaveChanges();
            }
        }
}

I call all of my Data Layers through a BusinessLayer, which handles my business logic:

public class DeploymentOrderBusinessLayer
    {

    public static void UpdateScannedItem(string orderNumber, string itemNumber, string user, string serialNumber = "")
            {

                    var itemInfo = DeploymentOrderDataLayer.GetScannedItem(orderNumber, itemNumber);

                    if (itemInfo == null) throw new BusinessException("Item number not found.");

                    if (itemInfo.IsFulfilled) throw new BusinessException(string.Format("Item already fulfilled by {0}", itemInfo.FulfilledBy));

                DeploymentOrderDataLayer.UpdateScannedItem(orderNumber, itemNumber, user, serialNumber);
            }

   }

The BusinessLayer then gets called from my DeploymentOrder Controller:

 try
        {

          DeploymentOrderBusinessLayer.UpdateScannedItem(orderNumber, itemNumber, User.Identity.Name);

        }
        catch (BusinessException ex)
        {
            ViewBag.AlertMessage = ex.Message;
        }

A couple things that bug me:

I'm calling GetScannedItem in my business layer so that I can do my validation first, before anything gets updated.  When I'm ready to update, I have to re-query the database again, so that I can update my context.  This extra hit to the database seems unnecessary to me, but I'm not sure how to fix it.
The DeploymentOrderDataLayer is fine for my DeploymentOrderController, but useless for any other controllers that might need to access similar data.  I would like my Data Layers to be more dynamic.

Any suggestions are appreciated.

This is going to be just a partial review, I don't have much time but I can't help it, I have to say something.

usr_OrderFulfillment might be how the table is called in your database, and SOPNUMBE might be how a column is called in that table. It doesn't mean your entities have to be as painful. Entity Framework is an object/relational mapper; this implies a mapping - you can easily map table usr_OrderFulfillment to entity OrderFulfillment and column SOPNUMBE to property OrderNumber.
Your DeploymentOrderDataLayer class (I wouldn't call a class a layer - call it a service if you will, but a layer is more for an assembly than for a type), is tightly coupled to TPGContext, and each new method requires a new instance of your context - even if 20 calls are made in the same request. You should inject the instance through the class' constructor and use a single instance per request. There's really no need to new it up and dispose it in every method. The class being static isn't helping either (it does explain the non-existing constructor though). Always prefer instances over static helpers.
This architecture feels like stored procedures written in C# code. It's bloated, over-engineered and makes you almost as good as me in shredding KISS into tiny little pieces. Have you tried using the EF DbContext directly in your controller to get the job done, and then refactor into a more structured approach?


Using bind instead of promises or stacked callbacks I've seen tutorials about node promises and such but not using bind. I feel like I can do lots of things this way and keep everything clean:

Updater.prototype.generateLocFieldForDocs = function(docs) {
  console.log('list obtained')
  async.eachLimit(docs, 10, this.generateLocFieldForDoc.bind(this), this.generateLocFieldForDocsDone.bind(this))
}

Updater.prototype.generateLocFieldForDoc = function(doc, done) {
  this.collection.update({_id: doc._id},
  {$set: {
    loc: {...}
  }}, function(err, result) {
    this.errorHandler(err)
    done()
  }.bind(this))
}

Updater.prototype.generateLocFieldForDocsDone = function(err) {
  this.errorHandler(err)
  this.generateLocFieldNextSection()
}

What do you think about using classes and bind like this in node.js?

What bind does is that it's sets the this in the function body to the this passed to bind therefore keeping the context you want.

I've realized there's no need for all these prototype functions and binds.
When using node.js, in a file, the normal named functions are private to the file, so it can be used as methods.

var self = {
  parallelLimit: 10
}

function generateLocFieldForDocs(docs) {
  log('list obtained')
  async.eachLimit(docs, self.parallelLimit, generateLocFieldForDoc, generateLocFieldForDocsDone)
}

function generateLocFieldForDoc(doc, done) {
  self.collection.update({_id: doc._id},
  {$set: {
    loc: {...}
  }}, function(err, result) {
    errorHandler(err)
    done()
  })
}

function generateLocFieldForDocsDone(err) {
  errorHandler(err)
  generateLocFieldNextSection()
}

Using module.export.myFunction = makes it public


Kotlin text validation I'm new to Kotlin, not so new to Java and Android development. But willing and trying to make a switch. I've wrote a simple validator for EditText
and was wondering if there is a way to write it more compact with lambdas.

textMTU.addTextChangedListener(object : EditTextValidator(textMTU) {
    override fun validate(textView: EditText, text: String) {
        val textToInt = if (text.isEmpty()) 0  else text.toInt()
        if (textToInt < 1280 || textToInt > 1500) {
            textView.error = getString(R.string.errorTroubleshoot)
        }
        else {
            textView.error = null
            val input = if (textMTU.rawText.isEmpty()) defaultSize else textMTU.rawText.toInt()
            if (input in 1281..1499) {
                prefs.mtuSize = input
            }
        }
    }

EditTextValidator here is simply extended TextWatcher without beforeTextChanged and onTextChanged methods

First of all, make attention - your code twice convert and check value. Suppose it's not efficient. Another tiny problem - 1280 and 1500 values is not change prefs.mtuSize…

My code:

class MainActivity : AppCompatActivity() {
    val TAG = MainActivity::class.simpleName.toString()

    private val defaultSize = 1500
    private var prefsMtuSize: Int = defaultSize

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(R.layout.activity_main)

        val textMTU = findViewById<EditText>(R.id.textMTU)

        // Extension function
        textMTU.addTextChangedListener { text ->
            val textToInt = try { text.toInt() } catch (e: NumberFormatException) { 0 }
            if (textToInt in 1280..1500) { // 1280 <= textToInt <= 1500
                textMTU.error = null
                prefsMtuSize = textToInt
            } else {
                textMTU.error = getString(R.string.errorTroubleshoot)
                prefsMtuSize = defaultSize
            }
            Log.i(TAG, "prefsMtuSize = $prefsMtuSize")
        }
    }
}

private fun EditText.addTextChangedListener(testFunction: (text: String) -> Unit) {
    addTextChangedListener(object : TextWatcher {
        override fun afterTextChanged(s: Editable?) {
            testFunction(s.toString())
        }
        override fun beforeTextChanged(s: CharSequence?, start: Int, count: Int, after: Int) { }
        override fun onTextChanged(s: CharSequence?, start: Int, before: Int, count: Int) { }
    })
}


Logger to syslog with error_log fallback Below is an implementation of a typical Logger interface that uses syslog() as the default means to store messages and falls back to error_log() if that does not work.  This is the first time I've used syslog and just want to ensure that my code is logical and there isn't something about syslog that I'm not aware of that could come back and bite me.  The code does work, messages are appearing in syslog as appropriate.

Project github repo
Source code for Logger interface
Source code for extended class

<?php

/**
 * @file
 * @brief Holds a class that implements error logging to an OS or system logger.
 */

namespace SprayFire\Logger;

/**
 * @brief A SprayFire.Logger.Log implementation that will attempt to log a message
 * to syslog and as a fallback will log the message to whatever option is set in
 * the error_log php.ini directive.
 *
 * @uses SprayFire.Logger.Log
 * @uses SprayFire.Core.CoreObject
 */
class SystemLogger extends \SprayFire\Core\CoreObject implements \SprayFire\Logger\Log {

    /**
     * @brief A flag to tell if fallback should be used.
     *
     * @property $syslogOpened
     */
    protected $syslogOpened = false;

    /**
     * @param $syslogIdent
     */
    public function __construct($syslogIdent = 'SprayFire') {
        $this->syslogOpened = \openlog($syslogIdent, \LOG_NDELAY, \LOG_USER);
    }

    /**
     * @param $syslogSeverity The timestamp for the \a $message being logged
     * @param $message The message to be logged
     * @return true if the message was logged, false if not
     */
    public function log($syslogSeverity, $message) {
        $loggedtoSyslog = $this->logToSyslog($syslogSeverity, $message);
        if (!$loggedtoSyslog) {
            return $this->logToErrorLog($message);
        }
        return $loggedToSyslog;
    }

    /**
     * @param $syslogSeverity The severity of the syslog message to store
     * @param $message The message to store in syslog
     * @return true if the message was logged into syslog, false if it wasn't
     * @see http://www.php.net/syslog
     */
    protected function logToSyslog($syslogSeverity, $message) {
        if ($this->syslogOpened) {
            return \syslog($syslogSeverity, $message);
        }
        return false;
    }

    /**
     * @param $message The message to log with error_log
     * @return true if logged, false if not
     * @see http://php.net/manual/en/function.error-log.php
     */
    protected function logToErrorLog($message) {
        $message = \date('M-d-Y H:i:s') . ' := ' . $message;
        return \error_log($message);
    }

    /**
     * @brief Ensures that the syslog is properly closed if it was opened.
     */
    public function __destruct() {
        if ($this->syslogOpened) {
            \closelog();
        }
    }

}

One question that I would have is how to unit test this?  How would I disable syslog to ensure the fallback is called?  Or is there some other way to do it?

You need a SysLog interface and a SysLogImpl implementation class.

public interface SysLog {
    public bool openlog(string $ident, int $option ,int $facility);
    public bool syslog (int $priority, string $message);
    public bool closelog (void);
}

SysLogImpl just delegate the calls to the openlog, syslog and closelog PHP functions.

public class SysLogImpl implements SysLog {
    public bool openlog(string $ident, int $option, int $facility) {
        return openlog($ident, $option, $facility);
    }

    public bool syslog(int $priority, string $message) {
        return syslog($priority, $message);
    }

    public bool closelog (void) {
        return closelog();
    }
}

It's important that this class should be just a thin facade without any logic since this class usually doesn't have any test or just has a few integration tests.

In production code pass a SysLogImpl instance to the constructor of your SystemLogger class and use this SysLogImpl instance instead of direct PHP calls.

In test code pass a mocked or a custom object which implements the SysLog interface to the SystemLogger class. The passed object should be able to store function calls and emulate responses (as mock objects do).

Maybe you'll need an ErrorLog interface and an ErrorLogImpl implementation class which logs with error_log PHP calls.

Another solution is a Test-Specific Subclass. You can extract out the openlog call from the constructor to a separate function and create a subclass of the SystemLogger which overrides this method and returns false.

A better and generic design would be creating a common Logger interface and a SyslogLogger and an ErrorLogLogger implementation classes. Both implements Logger. Then the SystemLogger class get one or more Logger instances and try calling their open functions and use that implementation whose open function returns true first.

If you use this the SystemLogger has only one responsibility: managing loggers, while SyslogLogger cares only about syslog and ErrorLogLogger cares only about error_log logging.


MaxCounters solution I am doing this Codility problem

  You are given N counters, initially set to 0, and you have two possible operations on them:

  increase(X) − counter X is increased by 1,
  max counter − all counters are set to the maximum value of any counter.

  A non-empty zero-indexed array A of M integers is given. This array represents consecutive operations:

  if A[K] = X, such that 1 ≤ X ≤ N, then operation K is increase(X),
  if A[K] = N + 1 then operation K is max counter.

I have ended up with this code -

def solution(N, array):
    counters = [0]*N
    maximum = 0
    for a in array: # O(m) as array has m integers
        if a >=1 and a <= N:
            counters[a-1] += 1
            maximum = max(counters[a-1], maximum)
        if a == N+1:
            counters = [maximum]*N
    return counters

I am failing two test cases because of timeout errors. I timed the function with array as [10]*100000 and \$N\$ as 9. It took 0.175681062359 seconds which is clearly not desirable. I do not understand where the time complexity increases. The for loop has \$O(M)\$ complexity because array has \$M\$ elements and even though max() has \$O(n)\$ complexity, that doesn't matter since I'm comparing just two elements. I looked at a solution by Andrei Simionescu and it looks awfully similar to mine -

def solution(n, arr):
    out = [0] * n
    m = 0
    last = 0
    for op in arr:
        op -= 1
        if op == n:
            last = m
            continue
        out[op] = max(out[op], last) + 1
        m = max(m, out[op])
    for i in xrange(n):
        out[i] = max(out[i], last)
    return out

I timed the above code and it took just 0.0276817503901 seconds on the same input. What is it that I'm doing wrong?

if a == N+1:
     counters = [maximum]*N #array initializer

Looks \$O(N)\$ to me, making your total time complexity \$O(Nm)\$ worst case. The other guy has 2 separate loops, one \$O(m)\$ the next \$O(n)\$, for a total time complexity of \$O(m+n)\$ or \$O(max(n,m))\$.

Take home message:

Ditch the \$O(N)\$ array initializer, it's not worth it. Use 2 separate passes, like the other guy did.


Passing restored packages as artefacts in GitLab Continuous Integration Introduction

I'm writing a script in YAML for building ASP.NET Core 2.2 project using GitLab Continuous Integration. In all YAML samples I could find (and there are not many) for building .NET Core based applications using GitLab CI I could see something like this:

before_script:
 - 'dotnet restore'

before_script is running a dependency restore before every job. It makes sense because if you use free GitLab runners (like I do) every job is executed on a different machine. There is no possibility to preserve state from previous jobs (with exception on cache and artifacts, but I'll get to that later). What that means is that on the next job, all previously restored packages will be gone, and they need to be restored again. And on the next job once again. And again on every job in the pipeline which needs the packages. I noticed a redundancy there. A redundancy that takes precious time, because a huge project with a lot of third-party packages takes a while for a full restore.

Using job artifacts

I found a way to preserve those packages and then pass them to the next job via GitLab artifacts:

restore:
 stage: restore
 script:
  - 'dotnet restore --packages .nuget/'
 artifacts:
  paths:
   - 'src/**/obj/*'
   - '.nuget/'

Let's break it down. With dotnet restore --packages .nuget/ I explicitly specify a custom directory for packages to be restored. Then I specify two paths which GitLab CI will be interested in when creating a job artifacts. dotnet restore creates a few files with metadata about packages inside a obj/ directory, so these will be needed as well. I include them in src/**/obj/*. Finally, I include the .nuget/ directory which after dotnet restore should contain all restored dependencies.

Note: A dependency restore saves the path where the packages will be kept inside <PROJECT_NAME>/obj/project.assets.json file. After that, there is no need for explicitly specifying where the restored packages are e.g. when building the project.

Eventually, in the next job I use previously created job artifacts by specifying a job dependecy. In that way, GitLab CI knows that it should download job artifacts from the dependant job.

build:
 stage: build
 script:
  - 'dotnet build --no-restore'
 dependencies:
  - restore

Whole YAML script:

image: microsoft/dotnet:2.2-sdk

variables:
 SOURCE_CODE_DIRECTORY: 'src'
 BINARIES_DIRECTORY: 'bin'
 OBJECTS_DIRECTORY: 'obj'
 NUGET_PACKAGES_DIRECTORY: '.nuget'

stages:
 - restore
 - build

restore:
 stage: restore
 script:
  - 'dotnet restore --packages="$NUGET_PACKAGES_DIRECTORY"'
 artifacts:
  paths:
   - '$SOURCE_CODE_DIRECTORY/**/$OBJECTS_DIRECTORY/*'
   - '$NUGET_PACKAGES_DIRECTORY/'

build:
 stage: build
 script:
  - 'dotnet build --no-restore'
 dependencies:
  - restore

Feedback

Please tell me what you think, any weaknesses of my approach, code smells, or maybe a better solution. All kind of constructive feedback appreciated.

I had a wrong concept about GitLab artifacts. After a good read on GitLab docs, especially the section distinguishing artifacts and cache, I deduced that I should use cache instead of artifacts as it was designed precisely for storing restored dependencies. Artifacts are meant for passing build output and binaries.

I also removed the restore stage, placing the dotnet restore command in a global before_script. Cache can fail and in such scenario the script should gracefully fallback to default 'download-from-internet' behaviour. With --no-restore option enabled it would not happen. Thus, I removed that option from dotnet build command. It won't make a noticeable difference with successfully download cache as a dependency restore with already downloaded packages will execute in next-to-no-time.

Finally, I added cache key, which will keep cache bundles separate for branches and stages.

Updated script:

image: microsoft/dotnet:2.2-sdk

variables:
 SOURCE_CODE_DIRECTORY: 'src'
 BINARIES_DIRECTORY: 'bin'
 OBJECTS_DIRECTORY: 'obj'
 NUGET_PACKAGES_DIRECTORY: '.nuget'

stages:
  - build

cache:
 key: '$CI_JOB_STAGE-$CI_COMMIT_REF_SLUG'
 paths:
  - '$SOURCE_CODE_DIRECTORY/*/$OBJECTS_DIRECTORY/project.assets.json'
  - '$SOURCE_CODE_DIRECTORY/*/$OBJECTS_DIRECTORY/*.csproj.nuget.*'
  - '$NUGET_PACKAGES_DIRECTORY'

before_script:
  - 'dotnet restore --packages $NUGET_PACKAGES_DIRECTORY'

build:
 stage: build
 script:
  - 'dotnet build --no-restore'


Using Pexpect for communication between multiple programs via I/O I have a Python script that runs three compiled programs and makes them communicate via standard I/O. This is my first time using pexpect. My program runs very slowly. Is there anything that I can do to speed up the program? If not, I can try to do it with C/C++ pipe/fork.

import pexpect
import os

Checker = pexpect.spawn("./checker") # a turn based game
Player = [0,1]
Player[1] = pexpect.spawn("./player1") # same game with computer player
Player[0] = pexpect.spawn("./player2") # same game with computer player

# tell each player wether it is 1st or 2nd player
Player[1].sendline("1") #
Player[1].expect("\n")  # delete input from before
Player[0].sendline("2")
Player[0].expect("\n")  # delete input from before

player = 1
while True:

    Player[player].expect("\n") # wait for current player's move(a line)
    move = Player[player].before.strip() # read current player's move
    print move

    if not Checker.isalive(): # this does not run in usual
        print "endgame checker died"
        break
    Checker.sendline(move)
    Checker.expect("\n") # delete input from before
    Checker.expect("\n") # wait checker's response
    check = Checker.before.strip() # read the response
    if check != "": # checker will send an empty line if move is valid and game didn't end
        print "endgame   ", check # otherwise response is other player's number
        Player[0].kill(0) # kill all
        Player[1].kill(0)
        Checker.kill(0)
        break

    player = (player+1) %2 # change current player
    if not Player[player].isalive(): # this does not run in usual
        print "endgame" + player + "died"
        break

    Player[player].sendline(move)
    Player[player].expect("\n")  # delete input from before

here are a few ways to cleanup the code and make it more readable. This does not really go over functionality too much as I am not super familiar with pexpect or your other program you are trying to interface with. There are probably some optimizations to do the While loop in your program especially around the end where you are changing the player you are looping on but since I am unfamiliar I will leave that alone.

Without seeing the code or having a better understanding of how the checker/player works, this will be the best I can give for now.

Unused imports aren't a terrible thing, but aren't really a good thing either, best to remove them.
It doesn't seem like any naming convention is really being strictly followed. For example you have Checker = pexpect.spawn("./checker") but then later are doing check = Checker.before.strip(). It is best to follow PEP8 and stick with the snakecase naming convention. So it would be checker = pexpect.spawn("./checker")
Similarly named variables can be confusing. You have Player and player but they are different things. Best to be more specific, player_list and current_player would be better.
Use list literals instead of creating a list with aribitrary values then indexing in. You could rewrite

Player = [0,1]
Player[1] = pexpect.spawn("./player1")
Player[0] = pexpect.spawn("./player2")

as

Player = [pexpect.spawn("./player2"),pexpect.spawn("./player1")]

but that looks a little backwards, so I am gonna change all the indexing so player 1 is in index 0.

player_list = [pexpect.spawn("./player1"), pexpect.spawn("./player2")]

Use string interpolation

print "endgame   ", check

Would become

print "endgame {}".format(check)

and

print "endgame" + player + "died"

would become

print "endgame {} died".format(player)

Use the natural feel of booleans instead of check != "" where you should have done if check. Python will check for None and "" for you.

Here is my version of the script that should function the same.

import pexpect

checker = pexpect.spawn("./checker")
player_list = [pexpect.spawn("./player1"), pexpect.spawn("./player2")]

player_list[0].sendline("1")
player_list[0].expect("\n")  # delete input from before
player_list[1].sendline("2")
player_list[1].expect("\n")  # delete input from before

current_player_idx = 0
while True:
    player = player_list[current_player_idx]
    player.expect("\n")
    move = player.before.strip()
    print move

    if not checker.isalive():
        print "endgame checker died"
        break
    checker.sendline(move)
    checker.expect("\n") # delete input from before
    checker.expect("\n")
    check = checker.before.strip()
    if check:
        # Really have no idea what you are trying to do in here, so I am leaving it alone.
        print "endgame {}".format(check)
        player_list[1].kill(0)
        player_list[1].kill(0)
        checker.kill(0)
        break

    # If I understand correctly, you are trying to see if the OTHER player is still alive.
    current_player_idx = (current_player_idx + 1) % 2
    if not player_list[current_player_idx].isalive():
        print "endgame {} died".format(current_player_idx)
        player_list[current_player_idx].sendline(move)
        player_list[current_player_idx].expect("\n")  # delete input from before
        break


React.js components for a golf app I've recently started to look into React. I feel like I'm understanding it, but I want to make sure I'm doing the best practices before I make too many bad habits.

I'm using the react-rails gem, and my data structure is as follows:

GolfCourse:

has_many :holes

Hole:

belongs_to :golf_course
has_many :yardages

Yardage:

belongs_to :tee_box

I load the component using:

<%= react_component('Scorecard', teeBoxes: TeeBox.all, holes: @course.holes, yardages: @yardages) %>

The idea behind the component is: The user has loaded the page to enter their recent golfing score, the page loads the scorecard for the current course they have selected. The user then changes the dropdown to select which tee box they played off.

The scorecard yardage/par/stroke_index will change depending on which tee colour they have chosen.

Scorecard Component

var Scorecard = React.createClass({

  getInitialState: function() {
    return {
      yardages: {}
    };
  },

  changeYardage: function(teeBoxId) {
    this.setState({
      yardages: _.filter(this.props.yardages, function(yardage){ return yardage.tee_box_id == teeBoxId; })
    });
  },

  renderHoles: function() {
    var state = this.state;
    return this.props.holes.map(function(option) {
      yardage = _.find(state.yardages, function(yardage) { return yardage.hole_id == option.id; });

      return (

          {option.number}
          {option.name}
          {typeof(yardage) !== "undefined" ? yardage.yards : ""}
          {typeof(yardage) !== "undefined" ? yardage.par : ""}
          {typeof(yardage) !== "undefined" ? yardage.stroke_index: ""}

      );
    });
  },

  render: function() {
    return (

        <TeeBoxSelect teeBoxes={this.props.teeBoxes} changeYardage={this.changeYardage} />

              Hole
              Yards
              Par
              Stroke Index
              Score

            {this.renderHoles()}

    );
  }
});

Tee box select component

var TeeBoxSelect = React.createClass({
  getInitialState: function() {
    return {
      selected: 0
    }
  },

  handleChange: function() {
    this.props.changeYardage(this.refs.teeBox.value);
  },

  optionNodes: function() {
    return this.props.teeBoxes.map(function(option) {
      return <option key={option.id} value={option.id}>{option.colour}</option>;
    });
  },

  render: function() {
    return (
      <select ref="teeBox" value={this.state.selected} onChange={this.handleChange}>
        <option value="0">-- Select your tee box --</option>
        {this.optionNodes()}
      </select>
    );
  }
});

How did I do? My main concerns are where I'm rendering the multiple elements (holes
 in scorecard component, tee box options in the teebox component.), and the way I handle state (yardages) in the Scorecard component.

Your component is pretty good. I even had to look at this guide to check when props and state should be used, and your code pretty much nailed it. There are a few things to change though.

First is to not rely on the fact that props and state get handed down to your component. It's best if you provide default props the same way you did with state. That way, you're always sure they have values. It also serves as pseudo-documentation, telling the developer that's lost and found his way into your component that this component expects the following props and state.

So... changeYardage selects a yardage, a single yardage. It makes no sense keeping it an array and end up using find later on. You can use find on changeYardage instead so that yardages stores an object instead of an array. If find returns undefined, just default it to a blank object. And since it's a single yardage object, we rename it to a singular yardage instead.

this.setState({
  yardage: this.props.yardages.find(yardage => yardage.hole_id === option.id) || {};
});

I notice you use React, so probably you're using Babel (if not, you should). This gives you access to a lot of shortcuts seen above. First, in ES6, arrays now have a find method so you can drop using lodash/underscore for that. Then there's the arrow functions to make callbacks a bit shorter. Not related to ES6 syntax, you should also use strict comparison, unless the comparison calls for loose comparison.

Now with regards to the yardages table, I'm not sure you want to actually show a blank table when yardage is not defined. You might want to actually just show an error message instead of a blank table.

And I notice you use BEM convention for CSS. Good. At least you are not using inline styles, because it has a very high specificity and makes it hard for stylesheets to override.


pass argument to jQuery .on() event handler callback In my web application, a user will click a button which will call a function that triggers an AJAX call.  That function accepts a single argument.  I would like to be able to put that function in place of the anonymous function in the jQuery event handler.  I suspect that this is a scoping issue...

Here is my working code:

(function () {
 'use strict';

  var postId;

  function userAddFlagToPost (userId) {
    return $.ajax({
      type: 'POST',
      timeout: 1000 * 3;,
      url: '/someApi/postId',
      data: {
        userId: userId
      },
    }).done(onFlagSubmitDone)
      .fail(onFlagSubmitDone);
  }

  function onLoggedIn() {
    var userId = getCookie('some_user_auth') || null;

    $(SELECTORS.POST_FLAG).on('click', function() {
      postId = $(this).data('postId');
    });

    $(SELECTORS.SUBMIT_BUTTON).on('click', function() {
      userAddFlagToPost(userId);
    });
  }

  function init() {
    eventQ.register(onLoggedIn);
  }

  init();
})();

I would like to be able to do:

$(SELECTORS.SUBMIT_BUTTON).on('click', userAddFlagToPost(userId));

I'd appreciate any info or advice on how to go about refactoring this code, thanks!

It seems that what you want is a partially-applied function. One way this can be achieved is using Function.prototype.bind().:

$(SELECTORS.SUBMIT_BUTTON).on('click', userAddFlagToPost.bind(null, userId));

Pass null as the first argument unless the context needs to be set (e.g. to this for the same context?) and then userId as the second argument will pass that as the first argument to the function. Then note that userAddFlagToPost() would then receive the event Event eventobject argument per the `.on() method.

With this approach, the function is not invoked (as it would be with your proposed syntax) but instead the partially applied function is invoked when the click handler is applied.

See this demonstrated in the snippet below:

(function() {
  'use strict';

  var postId;
  var SELECTORS = {
    SUBMIT_BUTTON: '#submitButton',
    DISPLAY_ELEMENT: '#displayElement'
  };

  function userAddFlagToPost(userId) {
    var text = 'userId passed to userAddFlagToPost: ' + userId + ' count of arguments: ' + arguments.length;
    Array.from(arguments).forEach(function(argument, index) {
      text += '
 typeof arg[' + index + ']: ' + (typeof argument);
    });
    $(SELECTORS.DISPLAY_ELEMENT).html(text);
  }

  function init() {
    var userId = 1337; //hard code for now
    $(SELECTORS.SUBMIT_BUTTON).on('click', userAddFlagToPost.bind(null, userId));
  }
  init();
})();
#displayElement {
  border: 1px solid #0f0;
  margin: 4px;
  padding: 10px;
}

<button id="submitButton">Click this to see the partially function called</button>

Note that there is a jQuery shortcut method .click() that could be used in place of .on('click'):

$(SELECTORS.SUBMIT_BUTTON).click(userAddFlagToPost.bind(null, userId));


Extension method to get results of type T from a cache Are there issues of any kind with this extension method?

using System;
using System.Web;

namespace AccountingBackupWeb.CacheExtensions
{
    public static class Extensions
    {
        public static T Get<T>(this System.Web.Caching.Cache cache, string key, Func<T> f) where T : class
        {
            T results = HttpContext.Current.Cache[key] as T;

            if (results == null)
            {
                results = f();
                HttpContext.Current.Cache[key] = results;
            }

            return results;
        }
    }
}

Logically, it makes sense to me that you would want to tie a key to a Func.  This method could then just take the string and then delegate the responsibility to decide how to fetch the value to another class.  Also, a few nitpicky points about what is actually there.  I consider using letters as variables poor practice since it gives no context as to how it will be used.  I would rename "f" to something like "retrieve" or "fetch".  Also, there is no guarantee that running "f" will actually give you a non null value.  In that case, I doubt you would want to store null in your cache.


Variable sized type allocation I have a type that receives serial data over a pipe:

struct Packet : Base { // Base is POD, too
  int foo;
  char data[];
};

In the context, where I instantiate Packet, I already know how large this particular packet will be, so I write:

size_t const size = sizeof(Packet) + dataLength;
std::auto_ptr<Base> p(new((void*)(new char[size])) Packet());
// note that it will be deleted via 'delete'

Is this a sensible way of doing this, or is there a more idiomatic way?

The main thing I see wrong is that you are not accounting for sizeof(foo).

The other problem is that auto_ptr<> will not de-allocate the memory correctly. It  calls delete X, but this is not legal as you did not allocate the memory with new. You need a custom destroy function (as you can provide to std::unique_ptr) that does the following:

Note: Original Question was:

std::auto_ptr<Base> p(malloc(size)) Packet());

Leading to my original answer:

// Assuming Packet*   packet = // stufff

packet->~Packet();   // Need to call destructor
free(packet);        // then free the memory allocated with malloc

Why not do this?

size_t const size = ...;
std::auto_ptr<Base> p = new Packet(size);

Then change packet too:

struct Packet : Base
{
  int foo;
  std::vector<char> data;
  Packet(size_t size): foo(0), data(size) {}
};

Edit:

Based on the comments and the updated question:

I posted the question here (to get more eyes on it): https://stackoverflow.com/q/9714359/14065

Which brought up two points:

You still can use std::auto_ptr<>

The memory allocated with new char[size] must be freed by delete []
This is not supported by std::auto_ptr<> though you may be able to use std::unique_ptr

Secondly the implementation is allowed to put a marker after the end of the object

A few other points need to be considered so please read the linked questiom.


Modified Z-Algorithm for string similarity with one char difference I have modified a Z-Algorithm for string similarity in order to match strings which differ in 0 or 1 char.

However, it seems much slower than original Z-Algorithm although from my point of view there is only a minor different and it also has a O(n) time complexity.

Input is a string in which a pattern is going to be found. So the goal is to find parts in string where pattern is present. The goal is also to find parts in string where the pattern is 'almost present'. So peep, leek and peek are all 'present' in string = "blahpeekblahpeepblahleek" for pattern = "peek". The output are indices in which matches are found. So for my example the output is "4 12 20".

modified Z-Algorithm:

static String stringSimilarity(String string, String pattern ) {
String s = pattern + "$" + string;
int len = s.length();
int patternLen = pattern .length();

int[] z = new int[len];
int l, r, i;
StringBuilder result = new StringBuilder();

l = r = patternLen;
i = patternLen + 1;

while (i < len) {
    //O(1)
    if (i <= r)
        z[i] = Math.min(z[i - l], r - i + 1);

    while (i + z[i] < len && z[i] < patternLen && s.charAt(z[i]) == s.charAt(i + z[i]))
        z[i] += 1;

    // if the previous while encountered mismatch we accept the mismatch as match and seek for matches once more
    if (i + z[i] < len && z[i] < patternLen && s.charAt(z[i]) != s.charAt(i + z[i])) {
        z[i] += 1;
        while (i + z[i] < len && z[i] < patternLen && s.charAt(z[i]) == s.charAt(i + z[i]))
            z[i] += 1;
    }

    if (z[i] >= patternLen)
        result.append(i - patternLen - 1).append(" ");

    if (i + z[i] - 1 > r) {
        l = i;
        r = i + z[i] - 1;
    }

    i += 1;
}

return result.toString();
}

Z-Algorithm body (code is not entirely mine but I adopted it while mine was not that readable):

while (i < len) {       //O(n + n) = O(n)
    //O(1)
    if (i <= r)
        z[i] = Math.min(z[i - l], r - i + 1);

    //loops only n times at maximum - O(n)
    while (i + z[i] < len && s.charAt(z[i]) == s.charAt(i + z[i]))
        z[i] += 1;

    if (z[i] >= patternLen)
        result.append(i - patternLen- 1).append(" ");

    //O(1)
    if (i + z[i] - 1 > r) {
        l = i;
        r = i + z[i] - 1;
    }

    i += 1;     //O(1)
}

Can you please check the complexity of my modified Z-Algorithm?

From my point of view it is the same, however, tests on Hackerank show timeout.

Why it is slow

In your modified Z-algorithm, you start computing the Z array from i = patternLen + 1.  This means that for the whole pattern, your Z array will have value 0.  This defeats the purpose of the Z-algorithm.  Even if you removed the part where you ignore one mismatched character, this will have changed your search from a \$O(n)\$ search to a \$O(n*m)\$ search, where \$m\$ is the length of the pattern.

For example, I did a test where I searched a string containing 1 million 'a' in a row for a pattern containing 1000 'a' in a row.  This took 4 seconds.  When I changed the code to start at the beginning of the pattern (i.e. the real Z-algorithm), it took 0.08 seconds.

But...

You can't just start at i=1 and expect to fix the problem.  Your "ignore one mismatch" code requires that you start over from the beginning every time because you can't use partial substring matches like you can with the Z-algorithm.  The reason is you may have skipped over a mismatch, so the value in the Z array doesn't necessary correspond to the substring that you matched.

So in other words, perhaps Z-algortithm is not well suited for this problem, or at least you will need to modify it in a different way.


Look through a string and return the most frequent character (Ruby) I want to determine which separator is used in a csv file. CSV.foreach will return something like this:

["something1;something2;something3"]

The code beneath does the trick, but something better must exist. I find it annoying to have the need for sep_count. Do you know of a method that returns the most frequent of the characters from SEPERATORS?

SEPERATORS = [";", ","]

CSV.foreach(@file, @config) do |header|
  sep_count = 0
  SEPERATORS.each do |seperator|
    if header.first.scan(/#{seperator}/).count > sep_count
      @config[:col_sep] = seperator
      sep_count = header.first.scan(/#{seperator}/).count
    end
  end
  break
end

EDIT:

Based on your awesome answers I got the 1-liner that I asked for:

@config[:col_sep] = %w(; ,).sort_by { |separator| File.open(@file).first(1).join.count(separator) }.last

I have also come up with this piece of code that determines both col_sep and row_sep:

first_line = ""
File.open(@file) do |file|
  file.each_char do |char|
    first_line << char
    if "\r\n".include?(char)
      @config[:row_sep] = first_line.scan(/\n$|\r$/).first
      break
    end
  end
end
@config[:col_sep] = %w(; ,).sort_by { |separator| first_line.count(separator) }.last

By using the full code we ensure that it is always the first line that gets used, and we also set the row_sep. Feel free to comment if you think anything could be improved further.

You can get the most common separator in header with a one-liner like this:

most_common = SEPARATORS.sort_by{|separator| header.count(separator)}.last

But as you have noticed CSV.foreach attempts to split up the rows, assuming by default that the separator is a comma.

You probably need to determine the separator in a preprocessing step before actually doing the CSV processing.

You could just do something like

contents = File.read(@file)
@config[:col_sep] = %w(; ,).sort_by{|separator| contents.count(separator)}.last

CSV.parse(contents, @config) do |row|
   ...
end

# or use the returned array of arrays
rows = CSV.parse(contents, @config)

This might be quite slow if your file is large because you have to read the whole thing into memory. In that case you might want to just look at the first line of the file, and guess the separator from that. To do this, assuming \n is your line separator:

first_line = File.open(@file) do |file|
  file.first
end

Note that you should use the block form to ensure that the file gets closed.

If you need to be line-separator agnostic, I don't think there's a built-in way to do so (although you can change the line separator, that assumes you know it in advance). You might try something like

first_line = ""
File.open(@file) do |file|
  file.each_char do |char|
    break if "\r\n".include?(char)
    first_line << char
  end
end


Greasemonkey script for filtering out results from experteer.com I've written a GM script to filter some results from Experteer.com results: specifically, many of the rows are "only for Premium members" and don't allow you to click through or see details, so I've written the script to hide those results from cluttering the display.

I'm very much a newbie to JavaScript and cobbled together this script after a lot of googling, so it's quite possibly horrific in several ways, please show me better ways in those cases.

The code follows with some notes on my reasoning:

// ==UserScript==
// @name        ExperteerNoPremium
// @namespace   abiteasier.in
// @description Removes the annoying "only for Premium members" rows from Experteer results
// @include     https://eu.experteer.com/goal/position/*
// @include     https://eu.experteer.com/search/jobs
// @version     0.3
// @grant       none
// ==/UserScript==

var remove_premium_rows = function () {
    var result_rows = document.body.querySelectorAll('.search-result-body');

    for (var i = 0; i < result_rows.length; i++) {
        var row = result_rows[i];

The two different pages included in @include above have the "only for premium" text in different columns, but they both have a td with class="search-result-job-action" which is in both cases two columns before the one we need, hence the selection and double nextElementSibling calls:

        var company_cell = row.querySelector('.search-result-job-action').nextElementSibling.nextElementSibling;

        var premium_only_re = /only\s+for\s+Premium\s+Members/i;
        if (premium_only_re.test(company_cell.textContent)) {
            row.style.display = 'none';
        }
    }
};

remove_premium_rows();

The results page also has a few Ajax controls which update the results table and I would like the pruning to happen on these new results as well. Since Mutation events seem to be discouraged by everyone, I went with a very hacky way of monkeypatching the function that does the Ajax updates itself, adding the remove_premium_rows as a callback:

unsafeWindow.ajaxUpdater = function () {
    new Ajax.Updater('result', '/desire/refine_matches',
            {asynchronous:true, evalScripts:true, method: 'get',
                parameters:Form.serialize($('matches')),
                onComplete: remove_premium_rows});
}

That's it, the code as above works for my purposes, but I would like to know in what it can be improved to adhere with standard practices.

If you want to maximize the speed, to iterate big arrays, use

for (var i = 0, l = array.length; i < l; i++)

instead of

for (var i = 0; i < array.length; i++)

Moreover, if you want to get the elements of a class, there's no need to use querySelectorAll, you can use the faster getElementsByClassName (see benchmark)


Font manager/loader class written in C++ (uses unordered_map) This font manager is part of a game engine. It's supposed to mostly load fonts at the start, so I'm more worried about reading performance than loading performance. If there's any way of improving loading performance as well I'm happy to hear it.

How would I optimize this font manager or managers in general, and what is the best practices for doing so?
Is there any way of improving the use of the unordered_map container?
I'm also trying to write more modern C++ code. Is there anything that could be improved on that front as well?

Header file:

class FontManager {
public:
    static FontManager &Instance() {
        static FontManager singleton;
        return singleton;
    }

    const Font *LoadFont(const std::string fileName);
    void BatchLoadFonts(const std::vector<std::string>& infos);

    const Font *GetFont(const std::string& fileName);
private:
    FontManager() { }
    ~FontManager();
private:
    std::unordered_map<std::string, const Font*> m_Fonts;
};

.cpp file:

const Font *FontManager::LoadFont(const std::string fileName) {
    if (m_Fonts.count(fileName))
        return m_Fonts.at(fileName);

    m_Fonts.emplace(fileName, new Font(FONT_FOLDER_PATH + fileName));
    return m_Fonts.at(fileName);
}

void FontManager::BatchLoadFonts(const std::vector<std::string>& infos) {
    for (auto info : infos) {
        LoadFont(info);
    }
}

const Font *FontManager::GetFont(const std::string &fileName) {
    if (m_Fonts.count(fileName)) {
        return m_Fonts.at(fileName);
    }

    LoadFont(fileName);
    return m_Fonts.at(fileName);
}

FontManager::~FontManager() {
    for (auto const& f : m_Fonts) {
        delete f.second;
    }
}

Relationship between batch and single-call api

Typically, when a manager class like like has both a single and a batch API call, the general function is the batch API, and the special case is the single-case. so it's the single-call API that should be calling the batch API, or the batch api should not exist at all.

The reasoning is that normally, the batch API provides some kind of advantage on top of just calling the single call in a loop. If there is no advantage to be gained, then there should only be the single resource call function, and let your user write a loop around it if they need to call it a bunch of time.

You want to keep your API surface as small as possible.

Smart pointers

Unless you are dealing with some very legacy system, you should have access to C++11. use std::unique_ptr<> instead of raw pointers, it'll massively reduce the amount of possible errors, as well as the amount of code you have to write.

Avoid double lookups:

if (m_Fonts.count(fileName))
    return m_Fonts.at(fileName);

// vs

auto found = m_Fonts.find(fileName);
if(found != m_Fonts.end())
    return found->second;

The second block of code only does a single lookup through the container.

similarly:

LoadFont(fileName);
return m_Fonts.at(fileName);

// vs

return LoadFont(fileName);

and

m_Fonts.emplace(fileName, new Font(FONT_FOLDER_PATH + fileName));
return m_Fonts.at(fileName);

// vs

auto emplaced = m_Fonts.emplace(fileName, new Font(FONT_FOLDER_PATH + fileName));
assert(emplaced.second);
return emplaced.first->second;

removes extra lookups.

avoid Singleton instantiation in the header file

This is a little subtle, but the definition of FontManager::instance belongs in the cpp file. This is important because if DLLs are ever involved, you are going to have a bad time otherwise.


Logging out using nested promises Is it OK to do that ?

for example, I have my log out service

  logout: function() {
    var defer = $q.defer();

    this.getCustomer().then(function(credentials) {
      $http.post(CONSTANT_VARS.BACKEND_URL + '/auth/logout',
      {username: credentials.username, customer: credentials.customer}
    ).success(function(data) {
        if (data.error) {
          defer.reject(data);
        }
        LocalForageFactory.remove(CONSTANT_VARS.LOCALFORAGE_CUSTOMER).then(function() {
          /*Removing LocalForage Items*/
          cleanLocalForage();
          defer.resolve(data);
        }, function(err) {
          console.log(err);
          defer.reject(data);
        });
      }).error(function(data) {
        cleanLocalForage();
        defer.reject(data);
      });
    }, function(err) {
      defer.reject(err);
    });
    return defer.promise;
  },

and then I have a function in a controller which returns an error once the session expires. When the session expires I need to logout the user and redirect him to the login path. So, this is what I have so far:

$scope.removeSlip = function(slip) {
  BetSlipFactory.removeSlip(slip).then(function() {
  }, function(err) {
    console.log(err);
    AuthFactory.logout();
    $location.path('/');
  });
};

or should I do something like this with the logout promise into the BetSlipFactory.removeSlip() promise ?

$scope.removeSlip = function(slip) {
  BetSlipFactory.removeSlip(slip).then(function() {
  }, function(err) {
    console.log(err);
    AuthFactory.logout().then(function() {
     $location.path('/');
    })
  });
};

there is my question: what is the proper way in this case ?

Well, Promises are designed so that you won't have to nest at all! You can think of Promises as try-catch-finally statements. And the nice thing about Promises is that if you return a promise inside then, the next chained then waits for that promise instead of the original. Here's a simplified version of your code (assuming I understood it correctly).

// Call the async function #1
return this.getCustomer().then(
  function(credentials) {
    // Success for async function #1

    // You can return promises. It will be listened to in the next .then()
    return $http.post(CONSTANT_VARS.BACKEND_URL + '/auth/logout', {
      username: credentials.username,
      customer: credentials.customer
    });

  }
).then(
  function(data) {
    // Success callback for async function #2 (the POST request)

    // We can also return a newly rejected promise to fail the next then
    // Or send back the promise of your remove operation
    // $.Deferred().reject().promise(), Deferred is read-write, Promise is read-only
    return data.error ? new Promise.reject() : LocalForageFactory.remove(CONSTANT_VARS.LOCALFORAGE_CUSTOMER);

  },
  function(error) {
    // Fail callback for async function #2 (the POST request)

    cleanLocalForage();
  }
).then(
  function() {
    // Success callback for async function #3 (the remove)

    cleanLocalForage();
  },
  function(err) {
    // Fail callback for async function #3 (the remove)

    console.log(err);
  }
);

The return value is either a resolved or rejected promise, depending on the result of the entire chain. If the entire thing resolves, the return is a resolved promise. If it rejects, then it returns a rejected promise. You can also recover from a reject by returning a promise. If that promise resolves, the next chain resolves and so on.

It's the main reason why it's called "then". It's like "Do this, then what?". The idea of the promise is to be some sort of linear flow-control mechanism, to avoid nested callbacks and proxy deferreds.

As for the other function, assuming logout is an async operation, you can do it like:

BetSlipFactory.removeSlip(slip).then(null, function(err) {
  console.log(err);
  return AuthFactory.logout();
}).then(function() {
  $location.path('/');
});


Python List Continuation with multiple lambdas applied I'm attempting to apply multiple filters and maps to a list and attempted to refactor into something i believe is more readable. In my code, I am reading a file to create a list of terms, then munging it into something I will feed into another script.

Please note my terminology in the title might be wrong, but please feel free to correct.

Originally, I had something that looked like this:

random_substring = lambda word: word[:(random.randrange(len(word) - 2) + 1)]

only_longer_words = lambda word: len(word) > 2

terms = []

#File manipulation that adds words to terms

terms = filter(only_longer_words, terms)
terms = map(random_substring, terms)
terms = filter(only_longer_words, terms)

#Save terms to file

After refactoring, I created a ChainList which inherits from list:

class ChainList(list):
    def filter(self, function):
        return ChainList(filter(function, self))

    def map(self, function):
        return ChainList(map(function, self))

    def uniquify(self):
        return ChainList(OrderedDict.fromkeys(self))

and use it as such:

terms = ChainList()

working_list = (
    terms.filter(only_longer_words)
         .map(random_substring)
         .filter(only_longer_words)
         .uniquify()
)

The intent is to make it easy to chain operations on lists. It works, but as a Python beginner, wanted a review.

The biggest problem with your code was the formatting issues (PEP8 violations), which you already fixed, with the exception of workingList which should be working_list.

Doc strings would be nice.

Instead of "uniquify", perhaps unique or even uniq sounds better.

In your first lambda you used word as the variable, in the second you used x. It would be more natural to unify.

This code doesn't make your ass look fat, I think.


BF interpreter in Python that uses recursion to handle loops I wrote a Brainfuck interpreter in Python and I'm wondering how to simplify it.

I handle separately loop commands and the others. A recursive function deals with loops.

import sys
array = [0] # byte array
ptr = 0 # data pointer

Read and execute commands except loop

def readNoLoop(char):
  global ptr
  # Increment/Decrement the byte at the data pointer
  if char=='+':
    array[ptr] += 1
  elif char=='-':
    array[ptr] -= 1
    if array[ptr] < 0:
      raise ValueError("Negative value in array")
  # Increment/Decrement the data pointer
  elif char=='>':
    ptr += 1
    while(ptr>=len(array)-1):
      array.append(0)
  elif char=='<':
    ptr -= 1
    if ptr < 0:
      raise ValueError("Negative value of pointer")
  # Output the byte at the data pointer
  elif char=='.':
    sys.stdout.write(chr(array[ptr]))
  # Store one byte of input in the byte at the data pointer
  elif char==',':
    array[ptr] = ord(sys.stdin.read(1))

Recursive function to deal with loops

def interpret(charChain):
  it = 0
  loopBegin = []
  while(it<len(charChain)):
    if charChain[it]=='[':
      loopBegin.append(it)
    elif charChain[it]==']':
      subChain = charChain[loopBegin[-1]+1:it]
      while(array[ptr]>0):
        interpret(subChain)
      loopBegin.pop()
    else:
      readNoLoop(charChain[it])
    it+=1

Main

if __name__ == "__main__":
  # Brainfuck program to print "Hello World!"
  code = '++++++++[>++++[>++>+++>+++>+<<<<-]>+>+>->>+[<]<-]>>.>---.+++++++..+++.>>.<-.<.+++.------.--------.>>+.>++.'
  try:
    interpret(code)
  except:
    raise

My main concern is to know if it's possible to have a simpler code without recursion.

As you didn't mention what are you looking for in a review, I will make some general comments regarding your code.

Formatting and variables

Try to read PEP8

indent your code using 4-spaces TAB
try removing redundant parentheses from your while loop loops
function names should be lower case (so try renaming your def readNoLoop(). Perhaps something like read_no_loop().
variable in functions should also be lowercase

Name conventions

Try renaming you variables based on their usage. For example: it - what is it ? What is its purpose ? Think about this when you are naming your methods / variables.

For example, instead of making comments right beside the variables, rename them and remove the comments:

array = [0]  # byte array
ptr = 0  # data pointer

Will become:

byte_array = [0]
data_pointer = 0

So far, we would have something like this:

import sys

byte_array = [0]
data_pointer = 0

def read_no_loop(char):
    global data_pointer
    # Increment/Decrement the byte at the data pointer
    if char == '+':
        byte_array[data_pointer] += 1
    elif char == '-':
        byte_array[data_pointer] -= 1
        if byte_array[data_pointer] < 0:
            raise ValueError("Negative value in byte_array")
    # Increment/Decrement the data pointer
    elif char == '>':
        data_pointer += 1
        while data_pointer >= len(byte_array) - 1:
            byte_array.append(0)
    elif char == '<':
        data_pointer -= 1
        if data_pointer < 0:
            raise ValueError("Negative value of pointer")
    # Output the byte at the data pointer
    elif char == '.':
        sys.stdout.write(chr(byte_array[data_pointer]))
    # Store one byte of input in the byte at the data pointer
    elif char == ',':
        byte_array[data_pointer] = ord(sys.stdin.read(1))

def interpret(char_chain):
    it = 0
    loop_begin = []
    while it < len(char_chain):
        if char_chain[it] == '[':
            loop_begin.append(it)
        elif char_chain[it] == ']':
            sub_chain = char_chain[loop_begin[-1] + 1:it]
            while byte_array[data_pointer] > 0:
                interpret(sub_chain)
            loop_begin.pop()
        else:
            read_no_loop(char_chain[it])
        it += 1

if __name__ == "__main__":
    # Brainfuck program to print "Hello World!"
    code = '++++++++[>++++[>++>+++>+++>+<<<<-]>+>+>->>+[<]<-]>>.>---.+++++++..+++.>>.<-.<.+++.------.--------.>>+.>++.'
    try:
        interpret(code)
    except:
        raise

Those elifs are bothering me, so you can try doing something like this:

instructions = {
  '+': increment,
  '-': decrement,
  # and so on
}

As per your code, I can't see much of improvement at the functionality level. Perhaps I will let others to do this.

More, I like how you used recursion in this code. In general, you have a nice structure.


Space and time complexity of operation on lists I'm new to programming and even more new to Haskell.  Below is a little tid-bit I wrote that operates on a bunch of lists. I am wondering if someone would be kind enough to walk through the function margProbs line-by-line, and enumerate their respective space/time complexities and/or just possibly suggest improvements.

import qualified Control.Monad as Mo
import qualified Data.Map as M
import qualified Data.List as L
import Data.Maybe

type PaIdx = Int

margProbs :: [[Bool]] -> PaIdx ->  [Double] -> [[Double]] -> Maybe [[Double]]
margProbs bs idx vpa vs = Mo.sequence $ fmap (\( _, ms ) -> collapseWith addV ms ) grouped
    where
        (p, q)   = ( vpa !! 0, vpa !! 1 )
        grouped  = reverse . sortAndGroup $ zip ( snd probBool ) scaled
        scaled   = zipWith (\p v -> scaleV (*) p v ) ( fst probBool ) vs
        probBool = foldr   (\b (probs, bools) -> let (prob,bool) = reducePa b in (prob:probs, bool:bools) ) ([],[]) bs
        reducePa bs = let split = fromJust $ pluckL idx bs in ( if fst split == True then p else q, snd split )

------------
-- Utils --
------------

addV :: ( Num a ) => [a] ->  [a] -> Maybe [a]
addV v1 v2 = if length v1 == length v2 then Just $ zipWith (+) v1 v2 else Nothing

scaleV :: (  Num a ) => ( a -> a -> a ) -> a -> [a] -> [a]
scaleV g num v = fmap (\e -> g num e ) v

collapseWith :: ( Num a ) => ( [a] -> [a] -> Maybe [a] ) -> [[a]] -> Maybe [a]
collapseWith g vs = foldr (\v1 v2 -> v2 >>= \w -> g v1 w ) ( return $ head vs ) ( tail vs )

sortAndGroup :: Ord k => [ (k, a) ] -> [ (k, [a]) ]
sortAndGroup ts = M.toList $ M.fromListWith (++) [(k, [v]) | (k, v) <- ts]

pluckL :: Int -> [a] -> Maybe ( a, [a] )
pluckL idx xs = case splitAt idx xs of
    ( _, [] )      -> Nothing
    ( hs, (t:ts) ) -> Just ( t, hs ++ ts )

Here are the params I used to test this snippet:

ret = margProbs bs 0 v m
bs = [[True,True],[True,False],[False,True],[False,False]]
v = [0.4,0.6]
m = [[0.95,0.05], [0.94,0.06], [0.29,0.71], [0.1, 0.9]]

ret should have value: Just [[0.554,0.446],[0.436,0.564]]

Update

I got rid of all dependencies to the hmatrix library.

scaleV

scaleV is very similar to fmap, it just has different precedence in its arguments, the following definition is simpler and highlights this similarity:

scaleV = (fmap .)

Guards

While they have the same effect of conditionals, they are preferred because they are extensible and nicer to read:

addV v1 v2 = if length v1 == length v2 then Just $ zipWith (+) v1 v2 else Nothing

Becomes:

addV v1 v2
  | length v1 == length v2 = Just $ zipWith (+) v1 v2
  | otherwise = Nothing


Manipulating each value in a 2D array This code is about manipulating each value in a 2D array. My method below is called based on how I want the values in the array to be manipulated (process).

For the first method in the switch statement - as an example, I am calling AllCaps() method to manipulate the value.

What I am trying to avoid is repeating statements when I can put it in a method instead and just call it when needed. With my code below, though, it seems like I am making the program work too hard by performing a check on each iteration - which could be in the thousands.

Is it effective to call a method for processing each 2D array value inside the nested for loop? Or call the method first and perform the nested for loop inside each method?

private void ProcessSelectedRange(string process)
{
    selectedRange = GetSelectedRange();

    // initialize and populate 2d array with values from selected range
    curValue = new object[,] { };
    curValue = selectedRange.Value;

    // possibly more statements

    // loop through curValue 2d array
    for (int i = 1; i <= curValue.GetLength(0); i++)
    {
        for (int j = 1; j <= curValue.GetLength(1); j++)
        {
            string str = string.Empty;

            // continue if curValue[i, j] is null or empty
            try
            {
                str = curValue[i, j].ToString();
                if (string.IsNullOrEmpty(str)) continue;
            }
            catch { continue; }

            //********************************************************************

            // call method depending on what we want to do with the value
            switch (process)
            {
                case "AllCaps": AllCaps(str); break;
                case "Process2": Process2(str); break;
                case "Process3": Process3(str); break;
                case "Process4": Process4(str); break;
                // possibly more methods
                default: Process5(str); break;
            }

            //********************************************************************
        }
    }
}

Removing the check on each iteration is definitely a good idea. My advice: have a separate method for each process. E.G.

SelectedRange = GetSelectedRange()

AllCapsRange(SelectedRange)
Process2Range(SelectedRange)

then, if you need to select a case, you only need to do it once, and not \$n^2\$ times.

As a general rule, the ONLY things that should go inside an iterative loop are things which could change during the iteration. Anything else should be declared/set beforehand.


Request review of sql validation trigger I have the following SQL structure (simplified for brevity):

TABLE [Posts]
     [Id]       INT NOT NULL IDENTITY PRIMARY KEY,
     [ParentId] INT NULL,
     [Type]     INT NOT NULL,
     FOREIGN KEY ( [ParentId] ) REFERENCES [Posts] ( [Id] )
     FOREIGN KEY ( [Type] ) REFERENCES [PostTypes] ( [Type] )

TABLE [PostTypes]
     [Id]   INT NOT NULL PRIMARY KEY,
     [Name] SYSNAME NOT NULL

TABLE [PostTypeValidParents]
     [ChildType]  INT NOT NULL,
     [ParentType] INT NOT NULL,
     PRIMARY KEY ( [ChildType], [ParentType] ),
     FOREIGN KEY ( [ChildType] ) REFERENCES [PostTypes] ( [Type] ),
     FOREIGN KEY ( [ParentType] ) REFERENCES [PostTypes] ( [Type] )

INSERT [PostTypes]
       ( [Type], [name] )
VALUES ( 0, 'Global' ),
       ( 1, 'Category' ),
       ( 2, 'Thread' ),
       ( 3, 'Reply' )

INSERT [PostTypeValidParents]
       ( [ChildType], [ParentType] )
VALUES ( 1, 0 ),
       ( 1, 1 ),
       ( 2, 1 ),
       ( 3, 2 )

Now, I want to create a trigger which validate [Posts] based on the rules defined in [PostTypeValidParents]. Here's what I have so far:

CREATE TRIGGER [ValidatePost]
ON [Posts]
AFTER INSERT, UPDATE
AS
BEGIN
    IF ( EXISTS ( SELECT * FROM [deleted] ) AND
         UPDATE ( [Type] ) )
        RAISERROR ( N'The [Posts].[Type] column cannot be modified.', 16, 1 );

    DECLARE @parents TABLE
      (
        [ChildType]  INT NOT NULL,
        [ParentType] INT NOT NULL
      );

    INSERT @parents
    SELECT c.[Type],
           CASE
             WHEN p.[Type] IS NULL THEN 0
             ELSE p.[Type]
           END
    FROM   [inserted] c
           LEFT JOIN [Forum].[Posts] p
                  ON c.[ParentId] = p.[Id];

    DECLARE @childType NVARCHAR(30),
            @parentType NVARCHAR(30);

    WITH [Conflicts] AS
      (
        SELECT [ChildType], [ParentType]
        FROM   @parents
        EXCEPT
        SELECT [ChildType], [ParentType]
        FROM   [PostTypeValidParents]
      )
    SELECT TOP 1
           @childType = c.[Name],
           @parentType = p.[Name]
    FROM   [Conflicts] x
           JOIN [PostTypes] c
             ON x.[ChildType] = c.[Type]
           JOIN [PostTypes] p
             ON x.[ParentType] = p.[Type];

    IF ( @childType IS NOT NULL )
        RAISERROR ( N'Posts of type ''%s'' may not have a parent of type ''%s''.', 16, 1, @childType, @parentType );
END

My basic pattern here is,

check for read-only fields and RAISERROR if modified
SELECT critical conditions into a table variable
find unsupported conditions with an EXCEPT inside a CTE, SELECT'ing into a variable and RAISERROR if not null.

It seems to work well enough (though I haven't thoroughly tested it yet). My problem is that this seems a bit verbose for what should be a relatively simple validation trigger. I'll have to write many of these in the near future, so I'm hoping to find a simpler pattern that accomplishes the same thing. Can anyone offer suggestions on how to reduce this code?

Your code is correct as long as you plan to validate business integrity using triggers.

But I would question whether business validation in SQL Server is a right choice:

You have no means (other than parsing return error string) to process errors on client side.
This kind of code may cause deadlocks under heavy load (I can't find them here but it doesn't mean there aren't any :))
Each insert causes at least 2 table searches

Alternative way (how most applications works) is to rely on a business logic inside client application to enforce business rules. It's much easier (and straightforward) to verify that your parent is of a valid type.

Also, based on your PostTypes table I think each type of "posts" should be different enough to store it in its own table. In that case the whole story of validating types is gone, and all the validation will be done by foreign keys.


Simple text diff function I've put together this extremely simple text diff function a while ago:

function diff($old, $new) {
    $old = trim($old);
    $new = trim($new);

    if($old == $new) {
        return $new;
    }

    $old    = explode(" ", $old);
    $new    = explode(" ", $new);
    $result = "";
    $length = max(count($old), count($new));

    for($i = 0; $i < $length; $i++) {
        if(!isset($new[$i])) {
            $result .= "{$old[$i]}";
            continue;
        }

        if(!isset($old[$i])) {
            $result .= "{$new[$i]} ";
            continue;
        }

        if($old[$i] != $new[$i]) {
            $result .= "{$old[$i]}{$new[$i]} ";
            continue;
        }

        $result .= "{$new[$i]} ";
    }

    $result = str_replace(array(" ", " "), " ", $result);

    return trim($result);
}

$string1 = "Hello World!! Please review my diff function. Be gentle.";
$string2 = "Hello world! Please critique my diff function. Be extra vigilant.";

echo diff($string1, $string2);
// Hello World!!world! Please reviewcritique my diff function. Be gentle.extra vigilant.

The html output is:

  Hello World!!world! Please reviewcritique my diff function. Be gentle.extra vigilant.

The purpose of the function wasn't something important, as I can't even remember why I built it (found in my sandbox directory, which is full of little experiments). Is it any good? I've always used PEAR Text_Diff for text diffs, but if I'm not doing anything inherently wrong in my simple function, I'd love to use it instead.

Any suggestions on an easy way to move html out of the function?

It looks fine. A few notes:

1, Use max instead of

$length = count($old) > count($new) ? count($old) : count($new);

2, Consider testing with newlines, tabs and maybe other whitespace characters.

3, http://en.wikipedia.org/wiki/Longest_common_subsequence could be useful if you want a better algorithm.

4, For removing HTML out of the function create a class which does it and delegate the calls to it. For example:

public interface Decorator {

    public function delete($input);

    public function insert($input);

    public function removeUnnecessaryMarkers($input);
}

public class HtmlDecorator {
    public function delete($input) {
        return "{$input}";
    }
    public function insert($input) {
        return "{$input}";
    }
    public function removeUnnecessaryMarkers($input) {
        return str_replace(array(" ", " "), " ", $input);
    }
}

...
if(!isset($new[$i])) {
    $result .= $decorator->delete($old[$i]);
    continue;
}
...

(I haven't tested that this is a valid PHP syntax or not. Feel free to fix it.)


Converting decimal to Roman numerals using MATLAB I have written a MATLAB function that converts decimals into Roman numerals.

function ans = dec2rom(z)
d = [ 1000, 900, 500, 400, 100, 90, 50, 40, 10, 9, 5, 4, 1];
c =  {'M', 'CM', 'D', 'CD', 'C', 'XC', 'L', 'XL', 'X', 'IX', 'V', 'IV', 'I'};
[];
for ii = 1:numel(d)
    if z >= d(ii)
        ans = [ans,repmat(c{ii},1,fix(z/d(ii)))];
        z = rem(z,d(ii));
    end
end

Your use of ans is very difficult to understand, it took me 5 minutes reading the lines to understand what []; really does and why it is necessary.

function r = dec2rom(z)
d = [ 1000, 900, 500, 400, 100, 90, 50, 40, 10, 9, 5, 4, 1];
c =  {'M', 'CM', 'D', 'CD', 'C', 'XC', 'L', 'XL', 'X', 'IX', 'V', 'IV', 'I'};
r='';
for ii = 1:numel(d)
    if z >= d(ii)
        r = [r,repmat(c{ii},1,fix(z/d(ii)))];
        z = rem(z,d(ii));
    end
end

/second version following the discussion to the other answer:

function r = dec2rom(z)

t={ 1000, 900, 500,  400, 100,   90,  50,   40,  10,    9,   5,    4,   1;
    'M', 'CM', 'D', 'CD', 'C', 'XC', 'L', 'XL', 'X', 'IX', 'V', 'IV', 'I'};
r='';
for ii = t
    num = ii{1};
    symb = ii{2};
    r = [r,repmat(symb,1,fix(z/num))];
    z = rem(z,num);
end

Now having the chars and numbers aligned in a data structure and using a simplified for loop.


Function that Loads a Random Image from an Array I'm new to PHP and had to put together a function that loads a random image from an array (it works fine). Here's what I came up with. Could it be improved? Any feedback would be appreciated.

<?php
function displayRandomPhotoArea() {
    $photoAreas = array("imageURL1", "imageURL2", "imageURL3", "imageURL4", "imageURL5");

    $randomNumber = rand(0, (count($photoAreas) - 1));

    echo '<img src="' . $photoAreas[$randomNumber] . '" width="725" height="194">';
}

// Display a random image here
displayRandomPhotoArea();
?>

Try to use array_rand function, code will look cleaner:

<?php
function displayRandomPhotoArea()
{
    $photoAreas = array("imageURL1", "imageURL2", "imageURL3", "imageURL4", "imageURL5");

    $randomNumber = array_rand($photoAreas);
    $randomImage = $photoAreas[$randomNumber];

    echo "<img src=\"$randomImage\" width=\"725\" height=\"194\">";
}

displayRandomPhotoArea();
?>


Emulate Java 8 predicate chaining in JavaScript I am trying to emulate Java 8's predicate chaining in JavaScript.
So far it works, but is there a more efficient way to write this? Furthermore, I had to create get and run methods to return the predicate so that is can be passed into a filter function.
Update: I added an _unwrap method to unwrap the internal predicate function so that you can pass a Predicate to another Predicate.

const records = [
  { name: 'Andrew' , age: 21, position: 'Developer' },
  { name: 'Bill'   , age: 26, position: 'Developer' },
  { name: 'Chris'  , age: 45, position: 'Manager'   },
  { name: 'Dave'   , age: 30, position: 'Developer' },
  { name: 'Eric'   , age: 32, position: 'Manager'   }
];

class Predicate {
  constructor(predicateFn) {
    this.predicateFn = this._unwrap(predicateFn);
  }
  /* @private */ _unwrap(predicateFn) {
    return predicateFn instanceof Predicate ? predicateFn.get() : predicateFn;
  }
  and(predicateFn) {
    return new Predicate((item) => this.predicateFn(item) && this._unwrap(predicateFn)(item));
  }
  or(predicateFn) {
    return new Predicate((item) => this.predicateFn(item) || this._unwrap(predicateFn)(item));
  }
  not() {
    return new Predicate((item) => !this.predicateFn(item));
  }
  get() {
    return this.predicateFn;
  }
  run(arr) {
    return arr.filter(this.predicateFn);
  }
}

const display = ({ name, age, position }) => `${name} (${age}) [${position}]`;

const ageOver21AndDeveloper = new Predicate(({ age }) => age > 21)
  .and(({ position }) => position === 'Developer');

console.log(ageOver21AndDeveloper.run(records).map(display).join(', '));

const billOrManager = new Predicate(({ name }) => name === 'Bill')
  .or(({ position }) => position === 'Manager');

console.log(billOrManager.run(records).map(display).join(', '));

const notUnder30 = new Predicate(({ age }) => age < 30).not();

console.log(notUnder30.run(records).map(display).join(', '));

// Pass predicate object to another predicate.
console.log(billOrManager.or(ageOver21AndDeveloper).run(records).map(display).join(', '));
.as-console-wrapper { top: 0; max-height: 100% !important; }

I believe you could retain the same level of flexibility without managing extra objects by using higher order functions. This also has the advantage of treating the predicates as functions and nothing more.

const records = [
  { name: 'Andrew' , age: 21, position: 'Developer' },
  { name: 'Bill'   , age: 26, position: 'Developer' },
  { name: 'Chris'  , age: 45, position: 'Manager'   },
  { name: 'Dave'   , age: 30, position: 'Developer' },
  { name: 'Eric'   , age: 32, position: 'Manager'   }
];

const display = ({ name, age, position }) =>
     `${name} (${age}) [${position}]`;

const not = condition => (...args) => !condition(...args);

const and = (...conditions) => (...args) =>
    conditions.reduce(
        (a,b)=>a&&b(...args),true
    )

const or = (...conditions) => (...args) =>
    conditions.reduce(
        (a,b)=>a||b(...args),false
    )

const ageOver21 = ({age}) => age > 21
const isDeveloper = ({position}) => position === 'Developer';
const ageOver21AndIsDeveloper = and(ageOver21,isDeveloper);

console.log(records.filter(ageOver21AndIsDeveloper).map(display).join(', '));

const billOrManager = or(
    ({name}) => name === 'Bill',
    ({position}) => position === 'Manager'
)

console.log(records.filter(billOrManager).map(display).join(', '));

const notUnder30 = not(({ age }) => age < 30)

console.log(records.filter(notUnder30).map(display).join(', '));

console.log(
    records.filter(
        or(billOrManager,ageOver21AndIsDeveloper)
    )
    .map(display).join(', ')
);


Edit distance between 2 strings I have written a program that calculates the "cost" of transforming one string into another. I would like you to review it.

public class EditDistance {

    public static void main(String[] args) {
        // TODO Auto-generated method stub
        String source="COTI",destination ="CAT";
        System.out.println(getEditDistance(source,destination));
    }

    /**
     * This function finds out the cost of transforming one string to
     * another
     * any change made to the string costs 1
     * @param sourceString
     * @param destinationString
     * @return
     */
    public static int getEditDistance(String sourceString, String destinationString) {

if (sourceString == null || destinationString == null){
            throw new IllegalArgumentException("String cannot be null");
        }

        int distance = 0;
        int sourceLength = sourceString.length() , destLength = destinationString.length();

        for ( int i=0 ;i < sourceLength && i < destLength ; i++){
            //how can I optimize loop condition, 3 conditions are checked on
            //every iteration
            if (sourceString.charAt(i) != destinationString.charAt(i)){
                //if characters are unequal and increment the distance
                distance++;
            }
        }
        if (destLength !=  sourceLength ){
            distance += Math.abs(sourceLength - destLength);
        }
        return distance;
    }
}

However, I would like to optimize the loop condition. Currently, 3 operations are done.

What can I do to speed it up or make it more elegant?

However, i would like to optimize the loop condition.
  Currently 3 operations are done. What can I do to speed it up?

Instead of checking for every position if it's within the bounds of both strings,
you could save the lowest bound using Math.min and use that as the loop condition.
That will cut down one comparison.

  Can anyone suggest a more elegant solution?

The last condition checking destLength !=  sourceLength can be omitted,
you can simply add the value of Math.abs because it will give 0 if the lengths are the same.
The result will be shorter code, which may be slightly more readable.

To make the code look more elegant,
it would be good to follow the common coding style.
For example instead of this:

    for ( int i=0 ;i < sourceLength && i < destLength ; i++){

Write like this:

    for (int i = 0; i < sourceLength && i < destLength; i++) {

I use my IDE to correct the formatting. It's a matter of a few key strokes.

It goes without saying that you should remove auto-generated lines like this:

    // TODO Auto-generated method stub

The revised implementation incorporating the above points (and a point from @user1016274 too):

public static int getEditDistance(String sourceString, String destinationString) {
    if (sourceString == null || destinationString == null){
        throw new IllegalArgumentException("String cannot be null");
    }

    int sourceLength = sourceString.length();
    int destLength = destinationString.length();
    int len = Math.min(sourceLength, destLength);

    int distance = Math.abs(sourceLength - destLength);
    for (int i = 0; i < len; ++i) {
        if (sourceString.charAt(i) != destinationString.charAt(i)) {
            ++distance;
        }
    }

    return distance;
}

I would also recommend to add some unit tests to verify the algorithm is working:

@Test
public void test_coti_cat() {
    assertEquals(2, EditDistance.getEditDistance("coti", "cat"));
}

@Test
public void test_alpha_beta() {
    assertEquals(5, EditDistance.getEditDistance("alpha", "beta"));
}

@Test
public void test_beta_pedal() {
    assertEquals(3, EditDistance.getEditDistance("beta", "pedal"));
}

@Test
public void test_empty() {
    assertEquals(3, EditDistance.getEditDistance("", "123"));
}


How to handle returned value if an exception happens in a library code There is a lib code, trying to parse an Element Tree object. If exception happens, it either returns an empty dict of dict or a partially constructed object of such type. In this case, caller needs to parse the results to see if parsing is correctly handled or not. Or in other words, the returned dict is not deterministic. How to solve this issue? Or if this is an issue?

def parse_ET(self, ETObj):
    if ETObj == None: return None
    dict_of_dict = collections.defaultdict(dict)
    try:
        for doc in ETObj.iter("result"):
            id = doc.attrib.get("id")
            for elem in doc.iter("attrib"):
                dict_of_dict[id].setdefault(elem.attrib.get("name"), elem.text)
    except Exception, ex:
        logging.exception("%s:%s" % (self.__class__, str(ex)))
    finally:
        return dict_of_docs

def parse_ET(self, ETObj):

Python style guide recommends lowercase_with_underscores for both function and parameter names

    if ETObj == None: return None

Use is None to check for None. However, consider whether you really want to support None as a parameter. Why would someone pass None to this function? Do they really want a None in return?

    dict_of_dict = collections.defaultdict(dict)

Avoid names that describe the datastructure. Choose names that describe what you are putting in it.

    try:
        for doc in ETObj.iter("result"):
            id = doc.attrib.get("id")
            for elem in doc.iter("attrib"):
                dict_of_dict[id].setdefault(elem.attrib.get("name"), elem.text)

Can there be duplicate "id" and "name"? If not you don't need to use defaultdict or setdefault

    except Exception, ex:

You should almost never catch generic exceptions. Only catch the actual exceptions you want.

        logging.exception("%s:%s" % (self.__class__, str(ex)))

Don't log errors and try to continue on as if nothing has happened. If your function fails, it should raise an exception. In this case, you probably shouldn't even catch the exception.

    finally:
        return dict_of_docs

finally is for cleanup tasks. Under no circumstances should you putting a return in it.


Update extremities losses when selected type changes The following code works, but is it a good idea to call another method in a setter?

public TypeSignalDTO SelectedTypeSignal
{
    get { return _selectedTypeSignal; }
    set
    {
        _selectedTypeSignal = value;
        RaisePropertyChanged(() => SelectedTypeSignal);
        UpdateExtremitiesLosses(); //This really bugs me.
    }
}

public void UpdateExtremitiesLosses()
{
    if (_completeLink.Any() && SelectedTypeSignal != null)
    {
        try
        {
            _lossService.SetLinkLosses(SelectedTypeSignal.Id_TSI, _completeLink);
            ShowLossColumn = true;
            TotalLoss = _completeLink.Last().LossAtCurrentPoint;
        }
        catch (ServiceException ex)
        {
            ShowLossColumn = false;
            TotalLoss = 0;
            _dialogService.ShowErrorMessage(ex.Message);
        }
        finally
        {
            Extremities = _completeLink.ToViewModel().ToList();
        }

    }
}

As you can see the setter calls another method: UpdateExtremitiesLosses which in turn changes other properties on the view model ShowLossColumn, Extremities and TotalLoss.

I know that properties are actually functions, but changing another property when calling a setter seems like an unintended side effect to me.

Being a beginner with WPF this really seems wrong to me. I would like to know if it is a better idea to subscribe to the ChangeEvent and call the method there, or just call the method in the setter.

The access to a property should be fast. If anything in a getter or setter of a property needs more time than usual to return it should better be a method.

So if UpdateExtremitiesLosses() is rather time consuming or if it should react on the changes made and there is an event which should handle changes like you mentioned, this call should better live inside this event.

public TypeSignalDTO SelectedTypeSignal
{
    get { return _selectedTypeSignal; }
    set
    {
        _selectedTypeSignal = value;
        RaisePropertyChanged(() => SelectedTypeSignal);
        UpdateExtremitiesLosses(); //This really bugs me.
    }
}

If you haven't done it already, you should override the =, != operators together with the Equals() and GetHashCode() methods and check before you call RaisePropertyChanged() if they are equal by any chance. If they are equal you shouldn't raise the event to prevent unneeded code execution.

RaisePropertyChanged(() => SelectedTypeSignal);

here you should simply use the _selectedTypeSignal instead of the property, because there is no need to call the getter of the property if you already has its value stored in a local variable.


Simple calculator in C I made a very simple calculator in C that allows the user to perform calculations like addition, subtraction, multiplication and division. This is my first C program that I've made outside of my University course. It would be great to get some feedback on this program so that I can avoid bad practices early on in my coding.

//Calculator program
#include <stdio.h>
#include <stdlib.h>

int main()
{
    int menu; //Variable for the number the user inputs
    float num1, num2, result; //Float variables for the user input and output, used floats in case the user enters e.g. 14.7

    printf("Enter a number from the list below\n\n");

    printf("1. Addition\n"); //Calculator menu, user must enter a value from 1 - 4 for the program to work
    printf("2. Subtraction\n");
    printf("3. Multiplication\n");
    printf("4. Division\n\n");

    printf("Enter number: "); //User input for the calculator menu
    scanf("%d", &menu);

    printf("\n");

    switch(menu) //switch statement for menu
    {
    case 1:
        printf("You entered Addition\n\n");

        printf("Enter first number: "); //User input for first number
        scanf("%f", &num1);

        printf("Enter second number: "); //User input for second number
        scanf("%f", &num2);

        printf("\n");

        result = num1 + num2; //Addition calculation

        printf("%.2f + %.2f = %.2f\n", num1, num2, result); //Addition output
        break;
    case 2:
        printf("You entered Subtraction\n\n");

        printf("Enter first number: "); //User input for first number
        scanf("%f", &num1);

        printf("Enter second number: "); //User input for second number
        scanf("%f", &num2);

        printf("\n");

        result = num1 - num2; //Subtraction calculation

        printf("%.2f - %.2f = %.2f\n", num1, num2, result); //Subtraction output
        break;
    case 3:
        printf("You entered Multiplication\n\n");

        printf("Enter first number: "); //User input for first number
        scanf("%f", &num1);

        printf("Enter second number: "); //User input for second number
        scanf("%f", &num2);

        printf("\n");

        result = num1 * num2; //Multiplication calculation

        printf("%.2f * %.2f = %.2f\n", num1, num2, result); //Multiplication Output
        break;
    case 4:
        printf("You entered Division\n\n");

        printf("Enter first number: "); //User input for first number
        scanf("%f", &num1);

        printf("Enter second number: "); //User input for first number
        scanf("%f", &num2);

        printf("\n");

        result = num1 / num2; //Division calculation

        printf("%.2f / %.2f = %.2f\n", num1, num2, result);
        break;
    default:
        printf("Enter correct number e.g 1 - 4\n"); //Outputted if the user enters a value other than 1 - 4
        break;
    }
    return 0;
}
//End of code

I must say, I respectively disagree with @ishyfishy's  example of the addition function. Although, I do agree with the point about splitting the arithmetic functions away from the main function.

I believe that the arithmetic functions should only handle performing the arithmetic; all the I/O should be handled by main function (I have been taught that this is a good practice, although I do not have a source right now).

So, the "1" (addition) section of the switch statement in main would look something like this:

scanf("%f", &num1);
scanf("%f", &num2);

result = addition(num1, num2);

printf("...", num1, num2, result);

And the addition function simply looks like this:

float addition(float num1, float num2) {
    return num1 + num2;
}

This doesn't make a much of a difference, but it speeds up your code a little bit.

Instead of storing the result of a calculation in result, it would be faster to just pass the return value an arithmetic functions into the statement/function that you will it next in.

For example, the end of the "1" (addition) section of the switch statement would look like this:

printf("%.2f + %.2f = %.2f\n", num1, num2, addition(num1, num2));

This is faster because it reduces memory interaction(however, not by much).

In this section:

printf("Enter a number from the list below\n\n");

printf("1. Addition\n"); //Calculator menu, user must enter a value from 1 - 4 for the program to work
printf("2. Subtraction\n");
printf("3. Multiplication\n");
printf("4. Division\n\n");

You make 5 calls to the printf function when you could easily just put all the strings into one and split them up by newline in a single printf call:

printf("Enter a number from the list below\n\n"
       "1. Addition\n"
       "2. Subtraction\n"
       "3. Multiplication\n"
       "4. Division\n");

This is also faster as you don't have to call a function 5 times.

Only use printf when you are need to use the formatting feature. Other than that, you are over-complicating things.

If you merely need to output a string with a newline here or there, use the built-in puts function; it automatically outputs a string with a newline on the end (and you can add more if needed).

For example, this line:

printf("You entered Addition\n\n");

Becomes:

puts("You entered Addition\n");


Quick Sort implementation in python Implemented the quick sort algorithm in python, It was just for learning it, now I want to port it to C.

Thoughts on the quality of my python implementation?

from random import randint, shuffle

def swap(array, a, b):
    array[a], array[b] = array[b], array[a]

def quick_sort(array, start=0, end=None):
    if end is None:
        end = len(array) - 1

    if end - start >= 1:
        pivot = array[end]
        a = start
        b = end - 1
        while not a > b:
            if not array[a] > pivot:
                a += 1
                continue
            else:
                if array[b] < pivot:
                    swap(array, a, b)
                    a += 1
                b -= 1
        swap(array, a, end)
        quick_sort(array, start, a - 1)
        quick_sort(array, a + 1, end)

arr = [randint(0, 30) for _ in range(20)]
print(arr)
quick_sort(arr)
print(arr)

Magic Numbers

arr = [randint(0, 30) for _ in range(20)]

What are 0, 30, and 20 supposed to represent? I would assign these to variables to make it more clear

Type Hints

This

def swap(array, a, b):

can be this

def swap(array: list, a: int, b: int) -> None:

These allow you to show what types of parameters are accepted, and what types are returned from the functions.

Main Guard

This

arr = [randint(0, 30) for _ in range(20)]
print(arr)
quick_sort(arr)
print(arr)

should be put in a main guard, like so

if __name__ == '__main__':
    arr = [randint(0, 30) for _ in range(20)]
    print(arr)
    quick_sort(arr)
    print(arr)

This prevents this code from running if you decide to import this module from another module/program.


Check if the string is palindrome The following code runs correctly:

#include <iostream>
#include <algorithm>
#include <string>

bool is_palindrom(const std::string& str)
{
    std::string temp(str);
    std::transform(temp.begin(), temp.end(), temp.begin(), ::tolower);
    return temp == std::string(temp.rbegin(), temp.rend());
}

int main()
{
    std::string str{ "KArak" };
    std::cout << is_palindrom(str) << std::endl;
    return 0;
}

Can I do this better? The constraint is to keep the declaration of the function the same and only change the implementation.

To the readers: I had to use ::tolower because the std:: version was causing compiler errors. I'm investigating this right now. The issue stems from std::tolower being overloaded. What is funny is that it is in header <locale>, which means that the standard library implementation includes it in some way. Rakete1111 proposed lambda version, which works great.

Yes.

Use more standard library:

The std::string provides reverse iterators, so there is no need to do any copy at all. In fact, it is possible to it with indices, but it won't look that pretty:

    return std::equal(str.cbegin(), str.cbegin() + str.size() / 2, str.crbegin(),
                      [](unsigned char lhs, unsigned char rhs) {
                          return std::tolower(lhs) == std::tolower(rhs);
                      });

The actual version was different, thanks to Rakete1111 for a new version. There is a note in the comments from Loki and Jerry: it seems like using functions in <cctype> are dangerous and will result in undefined behavior unless the input is cast into unsigned char first.

It is also possible to refactor the ugly + str.size() / 2 into reusable function:

#include <iterator>

//some namespace
template <typename Container>
constexpr auto mid(const Container& container)
{
    auto half_distance = std::distance(std::begin(container),
                                       std::end(container)) / 2;
    return std::next(container.begin(), half_distance);
}

Then it transforms into:

bool is_palindrome(const std::string& str) {
    return std::equal(str.cbegin(), mid(str), str.crbegin(),
                      [](unsigned char lhs, unsigned char rhs) {
                          return std::tolower(lhs) == std::tolower(rhs);
                      });
}

Other things that could be done better:

Avoid std::endl. It can cause big performance issues.
There is a typo in the word palindrome.
std::boolalpha could be used to provide more user friendly output.
more unit tests.
It seems like the code is missing <cctype> header as well.
putting everything into a namespace. Absence of it will force users to do unqualified call, which is prone to ADL, which is sometimes very evil.

Put together:

#include <iomanip>
#include <iostream>
#include <algorithm>
#include <string>
#include <cctype>
#include <iterator>

//some namespace
template <typename Container>
constexpr auto mid(const Container& container)
{
    auto half_distance = std::distance(std::begin(container),
                                       std::end(container)) / 2;
    return std::next(container.begin(), half_distance);
}

bool is_palindrome(const std::string& str) {
    return std::equal(str.cbegin(), mid(str), str.crbegin(),
                      [](unsigned char lhs, unsigned char rhs) {
                          return std::tolower(lhs) == std::tolower(rhs);
                      });
}

int main()
{
    std::string str{ "KArak" };
    std::cout << std::boolalpha;
    std::cout << "is \"KArak\" palindrome? " << is_palindrome(str) << '\n';
    std::cout << "is \"fuf\" palindrome? " << is_palindrome("fuf") << '\n';
    std::cout << "is empty string palindrome? " << is_palindrome("") << '\n';
    return 0;
}

Live demo.


Calculating Pi with Android I'm kind of new to Java but I am writing an Android app. Right now I'm working on an async task to calculate Pi but when I run it the memory usage increases alarmingly (+5MB per second). This one piece of code may need to run thousands of times so it is important that it be very optimized.

protected BigDecimal doInBackground(Object... params) {
        digits = (int)params[0];
        piView = (TextView)params[1];
        boolean add = true;
        BigDecimal pi = new BigDecimal(0.0);
        for(int i=1; pi.setScale(digits, BigDecimal.ROUND_DOWN).equals(realPi) == false; i++){
            if(add){
                pi = pi.add(new BigDecimal(4.0 / (-1 + (i*2))));
                add = false;
            }
            else {
                pi = pi.subtract(new BigDecimal(4.0 / (-1 + (i*2))));
                add = true;
            }
        }
        return pi;
    }

This one piece of code may need to run thousands of times so it is important that it be very optimized.

boolean add = true;
    for(int i=1; pi.setScale(digits, BigDecimal.ROUND_DOWN).equals(realPi) == false; i++){
        if(add){
            pi = pi.add(new BigDecimal(4.0 / (-1 + (i*2))));
            add = false;
        }
        else {
            pi = pi.subtract(new BigDecimal(4.0 / (-1 + (i*2))));
            add = true;
        }
    }

You don't need an add variable.  Consider

    int i = 1;
    while (!pi.setScale(scale, BigDecimal.ROUND_DOWN).equals(realPi)) {
        pi = pi.add(new BigDecimal(4.0 / i));
        i += 2;
        pi = pi.subtract(new BigDecimal(4.0 / i));
        i += 2;
    }

This may do one extra subtract operation, but it does half as many equals operations.  And it doesn't waste time on setting and checking a variable just to alternate between two operations.

We don't need the (-1 + (i*2)) part.  If we increment by 2 instead of 1, we get the same stream of values with strictly less math.  1, 3, 5, 7, 9, 11, ...

I changed the == false to the more idiomatic !.

I also changed digits to scale as I had trouble reading the original.

BigDecimal gives precise results, not quick results.  It is quite possible that you are hitting real limits of the format.  It's unclear at the moment why you are calculating \$\pi\$.  With more context, we might be able to give more help.