Game of Life in Python I have this interpretation of John Conway's Game of Life in Python 3.3.1:

#JOHN CONWAY'S GAME OF LIFE
def countSurrounding(universe, a, b):
    count = 0
    surrounding = [[a - 1, b - 1],
                   [a - 1, b    ],
                   [a - 1, b + 1],
                   [a    , b - 1],
                   [a    , b + 1],
                   [a + 1, b - 1],
                   [a + 1, b    ],
                   [a + 1, b + 1]]
    for a in range(0, 8):
        try:
            if universe[surrounding[a][0]][surrounding[a][1]] == 1:
                count += 1
        except IndexError: a
    return count
def printUniverse(universe):
    for a in universe:
        print(a)
    print("\n")
nextUniverse = [[0, 0, 0, 0, 0, 0, 0, 0, 0],
                [0, 0, 0, 1, 0, 0, 0, 0, 0],
                [0, 0, 0, 0, 1, 0, 0, 0, 0],
                [0, 0, 1, 1, 1, 0, 0, 0, 0],
                [0, 0, 0, 0, 0, 0, 0, 0, 0],
                [0, 0, 0, 0, 0, 0, 0, 0, 0],
                [0, 0, 0, 0, 0, 0, 0, 0, 0],
                [0, 0, 0, 0, 0, 0, 0, 0, 0],
                [0, 0, 0, 0, 0, 0, 0, 0, 0]]
for gen in range(0, 10):
    universe = [a[:] for a in nextUniverse]
    printUniverse(universe)
    for a in range(0, 9):
        for b in range(0, 9):
            if universe[a][b] == 0 and countSurrounding(universe, a, b) == 3:
                nextUniverse[a][b] = 1
            elif universe[a][b] == 1 and countSurrounding(universe, a, b) not in (2, 3):
                nextUniverse[a][b] = 0

How can I improve/optimize my code?

EDIT: After the answers below, I have updated my code:

#JOHN CONWAY'S GAME OF LIFE
def countSurrounding(universe, a, b):
    count = 0
    surrounding = ((a - 1, b - 1),
                   (a - 1, b    ),
                   (a - 1, b + 1),
                   (a    , b - 1),
                   (a    , b + 1),
                   (a + 1, b - 1),
                   (a + 1, b    ),
                   (a + 1, b + 1))
    for a, b in surrounding:
        if not(a < 0 or b < 0 or a >= len(universe) or b >= len(universe[a])) and universe[a][b]:
            count += 1
    return count
def printUniverse(universe):
    for a in universe:
        print(a)
    print("\n")
nextUniverse = [[0, 0, 0, 0, 0, 0, 0, 0, 0],
                [0, 0, 0, 0, 0, 0, 0, 0, 0],
                [0, 0, 0, 1, 0, 0, 0, 0, 0],
                [0, 0, 0, 1, 0, 0, 0, 0, 0],
                [0, 0, 0, 1, 0, 0, 0, 0, 0],
                [0, 0, 0, 1, 0, 0, 0, 0, 0],
                [0, 0, 0, 1, 0, 0, 0, 0, 0],
                [0, 0, 0, 0, 0, 0, 0, 0, 0],
                [0, 0, 0, 0, 0, 0, 0, 0, 0]]
for gen in range(0, 10):
    universe = [a[:] for a in nextUniverse]
    printUniverse(universe)
    for a in range(0, len(universe)):
        for b in range(0, len(universe[a])):
            if universe[a][b] == 0 and countSurrounding(universe, a, b) == 3:
                nextUniverse[a][b] = 1
            elif universe[a][b] == 1 and countSurrounding(universe, a, b) not in (2, 3):
                nextUniverse[a][b] = 0

Any more improvements to be made?
So far I have:

Introduced a check in countSurrounding to replace the except IndexError, so that the board does not wrap.
Made surrounding a tuple instead of a list
Changed for a in range(0, 9) to for a in range(0, len(universe)), as I assume that is more pythonic

Overall, it seems really really good to me and I like the way you use IndexError to count surrounding cells in a concise way without bothering about edge cases.

Here are a few comments anyway :

Removing a useless check

This is not much but you can remove a check of universe[a][b] in

        if universe[a][b] == 0 and countSurrounding(universe, a, b) == 3:
            nextUniverse[a][b] = 1
        elif universe[a][b] == 1 and countSurrounding(universe, a, b) not in (2, 3):
            nextUniverse[a][b] = 0

and it becomes

        surr = countSurrounding(universe, a, b)
        if universe[a][b]:
            if surr not in (2, 3):
                nextUniverse[a][b] = 0
        else:
            if surr == 3:
                nextUniverse[a][b] = 1

(It looks slightly longer though) (I also took this chance to use an additional variable)

Use tuple instead of list if it's not something you plan to iterate over

Here, the surroundings are just pairs and that should be enough. Also, it makes the access of the different elements much more convenient through tuple unpacking.

Iterate in a pythonic way

You shouldn't use range to iterate over a structure when python provide such a convenient way to iterate over it.

For instance, you could do :

surrounding = [(a - 1, b - 1),
               (a - 1, b    ),
               (a - 1, b + 1),
               (a    , b - 1),
               (a    , b + 1),
               (a + 1, b - 1),
               (a + 1, b    ),
               (a + 1, b + 1),]
for a,b in surrounding:
    try:
        if universe[a][b]:
            count += 1
    except IndexError: a

Same kind of argument applies to for a in range(0, 9): for b in range(0, 9): even though you might want to introduce enumerate.


Switching font and background color on button click I am currently building a web app where I need to randomly switch the font and color of a title (inside the #content div) and the background of the page every time a button is clicked.

I wrote the following code that works perfectly, but I'm wondering if it could be optimized (or at least written in a more elegant way).

// Switch font randomly
var font = ["lobster", "shadows", "oswald", "josefin", "gloria", "pacifico"];
var color = ["blue", "purple", "yellow", "red", "orange", "green"];
var background = ["bg-blue", "bg-purple", "bg-yellow", "bg-red", "bg-orange", "bg-green"];

$("#try-me").click(function() {
    $("#content")
        .hide()
        .removeClass()
        .addClass(font[Math.floor(Math.random()*font.length)]) // Font
        .addClass(color[Math.floor(Math.random()*color.length)]) // Color
        .fadeIn(600);

    $("html")
        .removeClass()
        .addClass(background[Math.floor(Math.random()*background.length)]); // Background
});

The repetition of getting a random font, color, background class cries to be extracted to a utility function:

function pickRandom(arr) {
    return arr[Math.floor(Math.random() * arr.length)];
}

$("#try-me").click(function() {
    $("#content")
        .hide()
        .removeClass()
        .addClass(pickRandom(font))
        .addClass(pickRandom(color))
        .fadeIn(600);

    $("html")
        .removeClass()
        .addClass(pickRandom(background));
});

I also agree with @MainMa about defining background in terms of color.

Other than that, I think it's nice code!

Ensuring change

If you want to make sure that each font/color/background are different every time the user clicks on the button, then perhaps you can create another helper:

function pickRandomExcept(arr, previous) {
    while (true) {
        var pick = arr[Math.floor(Math.random() * arr.length)];
        if (pick != previous) {
            return pick;
        }
    }
}

However, in the caller you will need to keep track of the previous picks of each. For example:

var picked_color;

$("#try-me").click(function() {
    picked_color = pickRandomExcept(color, picked_color);
    $("#content")
        .hide()
        .removeClass()
        .addClass(pickRandom(font))
        .addClass(picked_color)
        .fadeIn(600);


Is dynamically generating routes from Model enum safe? I am generating routes and responding to it dynamically by looking at the enum defined in the model. Is this safe and is there more proper method?

# Place
enum post_type: [:restaurant, :bar, :chill]

# routes
scope '/places' do
    get '/:category', to: 'places#index', constraints: { category: Regexp.new( Place.post_types.keys.join("|") ) }
  end

# places_controller
def index
    if Place.respond_to? parmas[:category]
      @places = Place.send(params[:category]).order(:created_at).limit(10)
    end
end

Place.send(params[:category])

Using raw user data with send is very dangerous, as it allows the user to call any method (params[:category] could be "delete_all" for example).
Using raw user data with responsd_to? can also be problematic, as this method exposes inner details of the class.

Instead, compare the user data against Place.post_types. It will look like this: (Untested)

 if  Place.post_types.has_key? params[:category]
   @places = Place.where(post_types: params[:category]).order(:created_at).limit(10)
 end

Also, dynamically creating a string for a regex is unreliable:

Regexp.new( Place.post_types.keys.join("|") )

It will break if a key contains any special characters, for example.

Instead of using a regex, test the data against Place.post_types.

EDIT: Actually, it seems this contraint is unneeded as it's tested in the controller.


Scala Binary Search Tree In an attempt to get deeper down into Scala, I decided to make a BST using as many interesting concepts as possible in order to explore everything Scala has to offer.

Taking inspiration from this question/accepted answer, I wrote:

package bst

object BST {
  def apply[T <% Ordered[T]]() : BST[T] = new EmptyBST[T]()
}

abstract sealed class BST[T <% Ordered[T]] {
  def insert(e: T) : BST[T]
  def remove(value: T) : BST[T]
  def search(e: T) : Boolean
  def min : T
}

case class EmptyBST[T <% Ordered[T]]() extends BST[T] {
  def insert(e: T) : BST[T] = NonEmptyBST[T](EmptyBST[T](), EmptyBST[T](), e)
  def remove(value: T) : BST[T] = this
  def search(e: T) : Boolean = false
  def min : Nothing = ??? ///Uncertain of how to handle this
}

case class NonEmptyBST[T <% Ordered[T]](left: BST[T], right: BST[T], value: T) extends BST[T] {

  def min : T = this.left match {
    case EmptyBST() => this.value
    case NonEmptyBST(l, _, _) => l.min
  }

  def insert(e: T) : BST[T] = e match {
    case this.value => this
    case `e` if e > value => NonEmptyBST[T](left, right.insert(e), value)
    case _ => NonEmptyBST[T](left.insert(e), right, value)
  }

  def remove(target: T) : BST[T] = target match {
    case this.value => this match {
    case NonEmptyBST(EmptyBST(), EmptyBST(), _) => EmptyBST()
    case NonEmptyBST(EmptyBST(), r, _) => r
    case NonEmptyBST(l, EmptyBST(), _) => l
    case NonEmptyBST(l, r, _) => val min = r.min
                 NonEmptyBST(l, r.remove(min), min)
    }
    case _ if target > this.value => NonEmptyBST(left, right.remove(target), value)
    case _ => NonEmptyBST(left.remove(target), right, value)
  }

  def search(target: T) : Boolean = target match {
    case `value` => true
    case _ if value > target => left.search(target)
    case _ => right.search(target)
  }
}

I would love to hear about how I could improve this. My intention with this is to have practice using some of Scala's more powerful tools, so feel free to suggest improvements that would otherwise be unecessarily complex for something like this.

Something that I'm specifically looking for guidance on is commented as such. It is the min method from EmptyBST.

EmptyBST.min

Since min can only return type T, EmptyBST.min has to throw an exception.  There is no logical alternative.

You could consider also implementing minOption: Option[T], which gives the caller more flexibility. Then EmptyBST.minOption would return None.

Tail recursion

None of your methods are tail recursive - not even the simple ones min and search.  You can test this by placing @annotation.tailrec before any of them. The compiler will give an error.

min and search

These cannot be tail recursive, even though the calls to l.min or left.search/right.search seem to be in tail positions. Class methods (as opposed to object methods - functions, in other words) have to be final or private and have to call themselves, not - as you are doing - a superclass method.

For these two methods, this can be solved simply.  One way is to use a while loop and a var to descend the branch (this is how many methods in the collections library do it).  Another is to use an internal helper function like this (for NonEmptyBST.min):

def min : T = {
  @annotation.tailrec
  def go(node: NonEmptyBST[T]): T = node.left match {
    case EmptyBST() => value
    case l: NonEmptyBST[T] => go(l)
  }
  go(this)
}

search can be fixed in the same fashion.

Updating the tree safely

Removals and insertions are much harder to do safely (that is, without risking stack overflow) in a functional, immutable tree. It is much easier in a mutable tree, where an insertion will only modify one node.  In an immutable tree, the final node and each node back down the path has to be modified.

The best way I know to do this is with a zipper.  Zippers allow you to navigate the tree with a minimum of copying to the point where the modification should be made.  Once that is done, the tree can be reconstructed from the zipper (again with a minimum of copying).  I could write a zipper implementation here but as it happens this set of slides begins with a description of how to write one for a binary tree like yours. You can stop once you get to "A Zipper Complete" - that shows you enough for this problem.

If you are still struggling after reading the slides, let me know and I will update this answer.

Easier building

For building a BST from a given set of values, it would be useful for you to modify your apply method to take a sequence of values of type T as input.  That would allow you to create a BST from a list in one concise line of code.


Helper function to return a string based on passed object and some defined rules In my application, a notification should be displayed when a chat message arrives.  There are some rules which decide what notification message is to be displayed.

I have created a helper method which takes a ChatMessage object and returns the message to be displayed in the notification.

How can I refactor or improve this piece of code? Any feedback will be appreciated.

private String getNotificationDisplayString(ChatMessage chat) {
    String message;
    boolean isUpdateMessage = (chat.getTag().equals(TAG_UPDATE_MSG));
    if (isUpdateMessage) {
        // for update message
        UserRelationshipInfo relationshipInfo = UserRelationshipInfoManger.getInstance().getInfo(chat.getUserId());
        boolean isUserMyFriend = (relationshipInfo != null && relationshipInfo.getSource() == SOURCE_FRIEND);
        message = Resources.string(isUserMyFriend ? R.string.text_user_is_friend : R.string.text_user_is_stranger);
    } else {
        boolean isDirectMessage = (chat.getType() == DIRECT_MESSAGE);
        boolean isTextMessage = (chat.getTag().equals(TAG_TEXT_MSG));

        if (isDirectMessage) {
            // for direct message (special case)
            String messageName = Resources.string(getMessageType(chat.getTag()));
            // message = "<name> sent you a direct <image/video/etc>"
            message = String.format(Resources.string(R.string.text_new_direct_message), chat.getUserInfo().getDisplayName(), messageName);
        } else if (isTextMessage) {
            // for text (non-direct)
            message = new TextInfo(chat).getText();
        } else {
            // for non-text (non-direct)
            // message = "<name> sent you a message"
            message = String.format(Resources.string(R.string.text_new_message), chat.getUserInfo().getDisplayName());
        }
    }
    return message;
}

boolean isUpdateMessage = (chat.getTag().equals(TAG_UPDATE_MSG));

Consider using a chat.isUpdateMessage() method for that logic.

UserRelationshipInfo relationshipInfo = UserRelationshipInfoManger.getInstance().getInfo(chat.getUserId());

Singleton pattern detected! I would not recommend using the singleton pattern, I am quite certain that there's a better way to do this, as I don't know the entire context of your code I am not sure which method would be the best, but to get you started take a look at a previous answer of mine.

boolean isUserMyFriend = (relationshipInfo != null && relationshipInfo.getSource() == SOURCE_FRIEND);

Again, consider using as method such as relationshipInfo.isMyFriend()

message = Resources.string(isUserMyFriend ? R.string.text_user_is_friend : R.string.text_user_is_stranger);

Again, this seems like you have a Context stored in a static variable somewhere. I wouldn't recommend that. Tell, don't ask. If something needs your context, tell them about it. Don't have them ask some other object/place about it.

boolean isDirectMessage = (chat.getType() == DIRECT_MESSAGE);
boolean isTextMessage = (chat.getTag().equals(TAG_TEXT_MSG));

Two methods: chat.isDirectMessage() and chat.isTextMessage().

String messageName = Resources.string(getMessageType(chat.getTag()));

getMessageType should be a method in the ChatMessage class. chat.getMessageType(). And again, I'm not a fan of the Resources.string method as it tells me you got a static Context object somewhere.

message = String.format(Resources.string(R.string.text_new_message), chat.getUserInfo().getDisplayName());

When using Android resources, there's a special method for combining the getString with String.format. See this method


Module that warns (a chatbot) about high rate of messages This is a module I use in chatbot type programs which issue responses to certain private messages from other users. I run incoming messages through this in order to limit the number of responses a user is able to provoke within a short amount of time.

guardian.py

import time

class Guardian:
    ''' Anti-flooding mechanism. Tracks how fast users are sending messages. '''

    def __init__(self, flood_limit):
        # Abstract number that represents how quickly you can flood
        self.FLOOD_LIMIT = flood_limit
        # "username": (int) goes down with time, goes up with every message
        self.dict = {}
        self.timer = time.time()

    def update_timer(self):
        ''' Count down the flood timer values for every logged user. '''
        now = time.time()

        time_elapsed = now - self.timer

        self.timer = now

        cooled_down_users = []

        # Check every logged user against the timer and prune cooled-down ones.
        for user in self.dict:
            self.dict[user] -= time_elapsed

            if self.dict[user] < 0:
                cooled_down_users.append(user)

        for user in cooled_down_users:
            del self.dict[user]

    def is_flooding(self, user, limit=None):
        ''' Tallies a user and returns True if they are above the limit. '''
        if limit == None:
            limit = self.FLOOD_LIMIT

        self.update_timer()

        if user in self.dict:
            self.dict[user] += 2

            if self.dict[user] > limit:
                return True
            else:
                return False

        else:
            self.dict[user] = 2
            return False

Usage looks something like this:

import guardian

anti_flood = guardian.Guardian()

... # Receive some message from the network, etc..

flood_warning = anti_flood.is_flooding(username, OPTIONAL_FLOOD_LIMIT_INTEGER)

if flood_warning:
    reply = "You are sending too many requests too quickly."
else:
    reply = "Here is the response you requested."

... # Send a message back, change something based on the request, and so on.

I have tried to make the module more or less general purpose, so that it could be used for other, similar tasks, such as kicking / banning users who flood a chat room. However, the way it is actively running through its internal dict of users on every message might cause issues in high traffic applications.

I can also imagine a potential issue if many different users conspire to flood the script with messages, therefore bypassing the individual limit.

I'm primarily looking for critique and guidance on readability, usability, generally anything pertaining to having my code read and used by other developers, but design, functionality, optimization, anything goes, of course.

My aim is to take a step toward becoming a useful programmer in the future, rather than just making stuff that never leaves the confines of my hard drive.

is_flooding is apparently triggered by the particular user's action. However, it calls update_timer, which affects much wider community. This seems wrong. I recommend keeping each user's activity along with the user's instance, and test only her, along the lines of

def on_message_received(user):
    user.update_tally()
    if user.flooding():
        user.warn_and_ignore_message()
    else:
        user.proceed_normally()

The (anti)-idiom

    if self.dict[user] > limit:
        return True
    else:
        return False

is a long way to say

    return self.dict[user] > limit

The significance of 2 in dictionary updates is very unclear.


pattern for validating an object against some condition I have an object that's generated from an incoming json string. The string is deserialized in a custom converter that returns an object. This object has a method called IsValidForDB that checks if the properties are valid. This method looks like this:

public class MyObject
{
   public bool IsValidForDB()
   {
      if (this.SomeProperty != SomeCondition)
      {
          return false;
      }

      if (this.SomeOtherProperty != SomeOtherCondition)
      {
          return false;
      }
      ....... several other conditions

      return true;
   }
}

My question is this: is this pattern a good way of validating an object's properties?

You have two things to consider here:

the entity shouldn't know anything about any database or repository. I would suggest you rename your method to IsValid().
the entity will be manipulated or operated upon by other classes, i.e. view models or controllers, etc. It can be an anti-pattern (or just smelly) to include business rules in the entity when those business rules rightfully belong elsewhere

So, I would suggest that the validation within your object simply evaluates the property values in an immediate way, avoiding any business rules.


VCF parser for eventual genomic data visualization I've just started out writing an app that will visualize genomic data for anybody to understand.

When you get your genome sequenced the raw data usually comes in the form of a VCF file. I started out by drafting parser below that essentially creates a JavaScript object out of a VCF text file. Next I think want to write a schema-like JSON file (since there doesn't seem to be any database with an API that correlate genotypes to phenotypes based on GWAS) that I can query for phenotypes with the genotypes generated by the parser. For example, if I want to know if a specific genome has the genotype for lactose-intolerance, red hair or alcoholism pre-disposition, I can consult this manually written schema that maps genotypes to phenotypes.

If you have genomics background all the better, but I'm mostly looking for advice concerning my usage of OOP and any other insights. How would you go about the more downstream front-end parts of the app (the actual visualization, was thinking something like a timeline and a 'humonculus'. React? D3?)?

(function iife() {

'use strict'

const fs = require('fs');

// Output class for parsing genotype file
class Genome {
  constructor(name) {
      this.name = name
      // Find genotype by rs-ID without having to know chromosome
      this.findByRsId = (rs_id) => {
        for(var chr in this) {
          if (chr.substring(0, 1) === 'c') {
            for(var id in this[chr]) {
              if (id == rs_id) {
                return this[chr][rs_id]
              }
            }
          }
        }
      }
      // Get actual sample's genotype
      this.getSampGenotype = (rs_id) => {
        let rs = this.findByRsId(rs_id)
        switch(rs.fit) {
          case '0/0':
            return rs['genotype'][0]+rs['genotype'][0]
            break
          case '0/1':
            return rs.genotype
            break
          case '1/1':
            return rs['genotype'][1]+rs['genotype'][1]
            break
          case '0':
            return rs.genotype
            break
          case '1':
            return rs.genotype
            break
          default:
            return rs.genotype
            break
        }
      }
      // Get ref genotype
      this.getRefGenotype = (rs_id) => {
        var rs = this.findByRsId(rs_id)
        return rs.genotype
      }
  }

}

// Helpers
var handleRow = (row) => {
  // Parse row into data items
  if (row.length === 10 && row[0] != '#CHROM' && row[2].substring(0,2) === 'rs') {
    // If no chr_[num] exists, make one
    if(!myGenotypes["chr_"+row[0]]) {
      myGenotypes["chr_"+row[0]] = {}
    } else {
      // If no rs_[num] exists, make one and set it to genotype
      if (!myGenotypes["chr_"+row[0]][row[2]]) {
        myGenotypes["chr_"+row[0]][row[2]] = {
          'genotype': row[3]+row[4],
          'quality': row[6],
          'fit': row[9]
        }
      }
    }
  }
}

var handleGFGChunk = (data) => {
  // Cut up chuck into array of rows, then pass to row handler
  var lines = data.split('\n')
  var cols = lines.map((l) => l.split('\t'))
  cols.forEach( (r) => handleRow(r) )
}

var check = (obj) => {
  console.log(obj.findByRsId('rs1556032'))
  console.log(obj.getRefGenotype('rs1556032') === 'CT')
  console.log(obj.getSampGenotype('rs1556032') === 'CC')
}

/*=====================
  Parsing of VCF file
=====================*/

// Have e.g. genotypes class on the ready
var myGenotypes = new Genome('edmund')

// Read VCF
var readFromFile = __dirname + 'vcf/GFG_filtered_unphased_genotypes.vcf'
var GFGfile = fs.createReadStream(readFromFile)
GFGfile.setEncoding('utf8')

// Write Genome upon code
// var writeToFile = 'GFG.js'
// var GFGclass = fs.createWriteStream(writeToFile)

// Start the parsing!
GFGfile.on('data', handleGFGChunk)
GFGfile.on('close', d => console.log('Checked genotype class') )
GFGfile.on('end', d => check(myGenotypes) )

})()

Your code is good, but it could be improved:

Nesting

Try and avoid massive nesting like this, but reversing the conditions and continueing:

class Genome {
  constructor(name) {
      this.name = name
      // Find genotype by rs-ID without having to know chromosome
      this.findByRsId = (rs_id) => {
        for(var chr in this) {
          if (chr.substring(0, 1) === 'c') {
            for(var id in this[chr]) {
              if (id == rs_id) {
                return this[chr][rs_id]
              }
            }
          }
        }
      }

Into something a little less nested:

class Genome {
  constructor(name) {
      this.name = name
      // Find genotype by rs-ID without having to know chromosome
      this.findByRsId = (rs_id) => {
        for(var chr in this) {
          if (chr.substring(0, 1) !== 'c') continue;
          for(var id in this[chr]) {
            if (id != rs_id) continue;
            return this[chr][rs_id]
          }
        }
      }

Consistency

This is the only semi-colon you use in your entire script:

const fs = require('fs');

You should keep your consistency in check, and either do or don't use semicolons (I would suggest the former), but keep consistent.

Naming

You're shouldn't be sacrificing readability in your code just to save space.

getSampGenotype: Literally two characters missing to massively improve readability
handleGFGChunk: This could be improved, however I would assume those working your code would know what it means, I, on the other hand, don't.

What is rs??!??

You use the abbreviation/term rs everywhere in your code, but to anyone without a doctorate, this might not be clear.

switch it to better code:

Excuse the horrible pun there, but...

Your switch statement can be simplified:

      case '0':
        return rs.genotype
        break
      case '1':
        return rs.genotype
        break
      default:
        return rs.genotype
        break

Here, you're defining multiple cases that have the same result.

There's two ways to approach this problem, while the first is better for readability, it's redundant in this case not to use the second

switch statements use fall throughs, which means, if you don't explictly declare break, it will keep falling down and down.

Take this snippet for example:

document.getElementById("testbutton").addEventListener('click', function(){
    var sample = "ACTA",
        result = "";
    switch(sample){
        case "ACTA":
            result += "A";
        case "AATC":
            result += "B";
        case "UUCA":
            result += "C";
        case "FAIL":
            result += "D";
        default:
            result += "E";
    }
    document.getElementById("content").innerText = result;
});
<button width="200" height="200 id="testbutton">Test</button>

What do you think would print?

   You would expect that to print A, because it's the only case that matches, but because it falls through, it catches all of them and alerts ABCDE.

Change sample to UUCA and watch what happens:

   CDE is alerted.

Number 1: So, seeing as content inside a case is optional, as shown above, you can "merge" cases, like the following:

      case '0': case '1': default:
        return rs.genotype
        break

However, Number 2: default is a catch-all of sorts, anything that isn't declared as a case above is caught by default, so not declaring case '0' and case '1' is  the same as declaring them in this case.

Depending on how you feel about readability, you could make it look like:

      case '0': case '1': default:
        return rs.genotype
        break

Which, while redundant, it readably shows that cases 0, 1 have been considered and their result determined.

Indentation:

Your block at the moment, looks like this:

(function iife() {

'use strict'
class Genome {
  constructor(name) {
  }
}
})()

I see two things wrong with this:

After your IIFE declaration, there isn't an added indentation
You're using two-space indentation, when you should be using four-space indentation.

Duplication:

You have a bit of duplication in this code block:

if(!myGenotypes["chr_"+row[0]]) {
  myGenotypes["chr_"+row[0]] = {}
} else {
  // If no rs_[num] exists, make one and set it to genotype
  if (!myGenotypes["chr_"+row[0]][row[2]]) {
    myGenotypes["chr_"+row[0]][row[2]] = {
      'genotype': row[3]+row[4],
      'quality': row[6],
      'fit': row[9]
    }
  }
}

See how many times you needed to call "chr_" +row[0]? That's how you know you deserve a variable.

var propertyName = "chr_" + row[0]
if(!myGenotypes[propertyName]) {
  myGenotypes[propertyName] = {}
} else {
  // If no rs_[num] exists, make one and set it to genotype
  var subPropertyName = row[2]
  if (!myGenotypes[propertyName][subPropertyName]) {
    myGenotypes[propertyName][subPropertyName] = {
      'genotype': row[3] + row[4],
      'quality': row[6],
      'fit': row[9]
    }
  }
}

Magic numbers:

Building on the last point, you call array indexes at 0, 2, 3, 4, 6, 9:

What are these points? What is at row[0] that may or may not be in myGenotypes?

You ought to move these array indexes to their own variables to declare the importance that each of these indexes holds.

Object and array properties

An object's properties can be accessed in two different ways: Object.property or Object[property].

In JavaScript, an array is basically an object where the index position is the key, however you cannot use the first syntax option with arrays.

Normally, you would use the Object.property version to visually show that this variable is an Object and not an array,
whereas you would use Object[property] when you have to load the property in from a variable, and not specifically by name:

var property = "cats";
Object.cats == Object[property];

IIFE:

In your IIFE, you give it the name iife, however in an IIFE that function name has no meaning, the function itself is discarded in favour of its result, meaning the name is redundant.

Miscellanous:

  this.getRefGenotype = (rs_id) => {
    var rs = this.findByRsId(rs_id)
    return rs.genotype
  }

This can be simplified down to:

  this.getRefGenotype = (rs_id) => {
    return this.findByRsId(rs_id).genotype
  }


Pathmunge function in zsh pathmunge is an often-seen function in shell initialization files, used to add entries to the PATH environment variable without duplication. Examples:

Fedora /etc/profile's pathmunge.
How can I cleanly add to $PATH? - Unix & Linux
Unix shell function for adding directories to PATH

I wrote this pathmunge variation in zsh:

pathmunge() {
    local path_array before
    # The default is to prepend the argument to PATH
    before=1
    case $1 in
        after)
            # Append to PATH
            before=
            ;&
        before)
            shift
            ;;
    esac
    # Split PATH on : into an array
    path_array=(${(As.:.)PATH})
    # Then remove the arguments from it, since they are to be added in the
    # order specified
    path_array=(${path_array:|argv})
    if [[ -n $before ]]
    then
        # Reverse the order of the arguments, so that:
        #    for f in ...; pathmunge f
        # and
        #    pathmunge ...
        # have the same effect.
        path_array=(${(Oa)argv} $path_array)
    else
        path_array=($path_array $argv)
    fi
    # Concatenate the array back to PATH
    PATH="${(j.:.)path_array}"
}

Note on implementation: Usually pathmunge implementations don't add to PATH if the entry already exists. I took a slightly different path - I'll remove the old instance of entry, and then add the entry as specified. The purpose is to ensure that entries are available in the expected order, since the order matters.

For example, if an entry /foo has commands shadowing another entry /bar, and I do pathmunge before /foo or pathmunge after /foo with PATH already containing /foo and /bar in some order, I can reasonably expect that now PATH will have the expected relative order between /foo and /bar.

Compatibility with other shells is not a concern. I am fairly competent when it comes to shell scripts. However, I have never made much use of zsh's extensive parameter expansion features. So, I'm looking for pitfalls and performance improvements.

1. Use parameter expansion for great effect
You can replace most of the case construct with a two ${name:#pattern} expansions. If pattern matches the value of name, the empty string is substituted, otherwise the value of name. If name is an array, matching elements will be removed from the array. So instead of this:
local before

before=1
# [...]
case $1 in
    after)
        # Append to PATH
        before=
        ;&
    before)
        shift
        ;;
esac
#[...]
if [[ -n $before ]]
then
#[...]
fi

it would look like this:
local before=${1:#after}
argv=(${argv:#[^/]*})
if [[ -n $before ]]
then
#[...]
fi

With before=${1:#after} before will be empty if the first parameter is "after", non-zero otherwise. argv=(${argv:#[^/]*}) will remove any element from argv that does not start with an /, including "before" and "after"
One difference would be, that before contains the value of the first parameter instead of 1 unless said parameter was "after". But this does not matter as long as you check only if before is non-zero.
The second difference would be, that in addition to removing "before" and "after" from argv, any relative paths (or anything that does not start with an /) will be removed, too.
2. Make use of path instead of PATH
There is no need to split PATH into an array on your own because zsh already makes the value of PATH available as an array with the array parameter path. Any change to PATH is automatically mirrored in path and vice versa. So you can manipulate path directly without the need for the local path_array parameter.
Instead of
path_array=(${(As.:.)PATH})
path_array=(${path_array:|argv})
if [[ -n $before ]]
then
    path_array=(${(Oa)argv} $path_array)
else
    path_array=($path_array $argv)
fi
PATH="${(j.:.)path_array}"

You need only
path=(${path:|argv})
if [[ -n $before ]]
then
    path=(${(Oa)argv} $path)
else
    path=($path $argv)
fi


Business with dbcontext and static class Many speak of using dependency injection. I do not use repository. The entity framework is uow/repository. What is wrong with using static class like the one below? My context is a new instance per request.

public class TenantManager
  {
    public static IEnumerable<Tenant> GetAll(AppDbContext context)
    {
      return context.Tenants;
    }
    public static Tenant GetById(int id, AppDbContext context)
    {
      return context.Tenants.Find(id);
    }
    public static Tenant Create(Tenant entity, AppDbContext context)
    {
      var tenant = context.Tenants.Add(entity);

      context.SaveChanges();
      return tenant;
    }
    public static void Update(Tenant entity, AppDbContext context)
    {
      context.Entry(entity).State = EntityState.Modified;
      context.SaveChanges();
    }
    public static void Delete(int id, AppDbContext context)
    {
      var tenant = context.Tenants.Find(id);
      context.Tenants.Remove(tenant);
      context.SaveChanges();
    }
  }

What is wrong with using static class like the one below?

For one thing, your class isn't static, which means your client code can create an instance of it... which would make no sense at all.

Before the advent of static classes, you would prevent instantiation by sealing the class and providing a non-public default/parameterless constructor, like this:

public sealed class TenantManager
{
    private TenantManager() { }

Making the class static pretty much does that for you, and prevents anyone from inheriting the class and/or calling its constructor.

In Object-Oriented Programming, a class defines the "blueprint" for an object - its type. By making a class static, you're denying it the ability to create objects; you're writing procedural code, like vba macro-recorder code in a module - you're giving anyone anywhere the ability to run this code, because the code lives in a global state, a static context, a type, as opposed to an object. This simply isn't OOP code.

It wouldn't be that bad, if all there ever was to your application was a Tenant entity. I don't know about the rest of your app, but say you have new requirements and now you have to add a Landlord entity, with the corresponding LandlordManager class.

So you go and implement another static class:

public static class LandlordManager
{
    public static IEnumerable<Landlord> GetAll(AppDbContext context)
    {
        return context.Landlords;
    }

    public static Landlord GetById(int id, AppDbContext context)
    {
        return context.Landlords.Find(id);
    }

    public static Landlord Create(Landlord entity, AppDbContext context)
    {
        var landlord = context.Landlords.Add(entity);

        context.SaveChanges();
        return landlord;
    }

    public static void Update(Landlord entity, AppDbContext context)
    {
        context.Entry(entity).State = EntityState.Modified;
        context.SaveChanges();
    }

    public static void Delete(int id, AppDbContext context)
    {
        var landlord = context.Landlords.Find(id);
        context.Landlords.Remove(landlord);
        context.SaveChanges();
    }
}

Notice anything? Yes, I fixed the vertical whitespace and the shy indentation. Anything else? Isn't it striking how similar the two classes are? The two types desperately want to share an interface, but the classes being static, they can't.

public interface IEntityManager<TEntity>
{
    IEnumerable<TEntity> GetAll(AppDbContext context);
    TEntity GetById(int id, AppDbContext context);
    TEntity Create(TEntity entity, AppDbContext context);
    void Update(TEntity entity, AppDbContext context);
    void Delete(TEntity entity, AppDbContext context);
}

The fact that every single member of the interface takes an AppDbContext parameter, is a very strong indicator that the parameter belongs at instance level. This simplifies the interface to this:

public interface IEntityManager<TEntity>
{
    IEnumerable<TEntity> GetAll();
    TEntity GetById(int id);
    TEntity Create(TEntity entity);
    void Update(TEntity entity);
    void Delete(TEntity entity);
}

This is really starting to look like something familiar - in fact only the name is different:

public interface IRepository<TEntity>
{
    IEnumerable<TEntity> GetAll();
    TEntity GetById(int id);
    TEntity Create(TEntity entity);
    void Update(TEntity entity);
    void Delete(TEntity entity);
}

  I do not use repository. The entity framework is uow/repository.

The thing is, you are using a repository. You just gave it a different name. I can call an apple an orange all I want, it's still an apple. Yes, the EF DbContext is a powerful unit-of-work. Yes, an EF DbSet<T> is a nice repository. Yet you still feel the need to tuck it into its own type - and that's a good thing!

The difference between your TenantManager and the above IRepository<TEntity> interface, is that TenantManager is tightly coupled with AppDbContext, while it's an implementation detail as far as IRepository<TEntity> is concerned - the consuming/client code doesn't need to even know about AppDbContext. In your code, AppDbContext is a dependency: not only your code needs to be tightly coupled with a TenantManager, it's also coupled with AppDbContext. "Manager" is a smell all by itself - you don't have a "Manager", you have a repository... whether you like it or not!

Good design doesn't spread its dependencies to the client/consuming code; good design is cohesive, and has low coupling. That's why SOLID OOP guidelines tell us to depend on abstractions, not concrete types. With client code that only needs to know about an IRepository<Tenant>, the only concrete type it's working with is Tenant. With your code, it needs to know about Tenant, TenantManager, and AppDbContext. That's not exactly what low coupling is about!

  Many speak of using dependency injection.

There's a reason for this. Loosely coupled code is much easier to write tests for. And the DI principle (all of SOLID actually) completely steer your code/design in that direction.

Say you have a controller - with your code it could look like this:

public class SomeController : Controller
{
    public ActionResult Tenant(int id)
    {
        using (var context = new AppDbContext(connectionString))
        {
            var tenant = TenantManager.GetById(id, context);
            return View(tenant);
        }
    }
}

With proper DI it could look like this:

public class SomeController : Controller
{
    private readonly IRepository<Tenant> _repository;

    public SomeController(IRepository<Tenant> repository)
    {
        _repository = repository;
    }

    public ActionResult Tenant(int id)
    {
        var tenant = _repository.GetById(id);
        return View(tenant);
    }
}

Both do exactly the same thing. Yet, only one of these two snippets can be unit tested.


Count the number of ways to obtain a sum, with consecutivity restriction A cricketer can score 1, 2, 4 or 6 in a ball. Find in how many ways the player can score a total of "n" runs without hitting 3 consecutive boundaries. Note: Scoring 4 or 6 is considered as a boundary.

My solution is:

int *arr; //global array to save calculated ways
int waysUtil(int n, int boundaries){

     if(n==0)
        return 1;
     if(n<1)
        return 0;

    if(arr[n]>0)
        return arr[n];//if already calculated return the value
    int hit_one = waysUtil(n-1, 0);
    if(n-1 >=0) arr[n-1]= hit_one;
    int hit_two = waysUtil(n-2, 0);
    if(n-2 >=0) arr[n-2] = hit_two;
    int hit_four=0, hit_six=0;
    // for 4 and 6 check number of consecutive boundaries hit.
    //if less than two then calculate
    if(boundaries<2 ){

        hit_four = waysUtil(n-4, boundaries + 1);
        if(n-4 >=0) arr[n-4] = hit_four;
        hit_six = waysUtil(n-6, boundaries + 1);
        if(n-6 >=0) arr[n-6] = hit_six;

    }
    return hit_one + hit_two + hit_four + hit_six;
}

int ways(int score){
    arr =(int*)malloc(sizeof(int)*score);
    memset(arr, -1, sizeof(arr));
    int noOfWays = waysUtil(score, 0);
    free(arr);
    return noOfWays;
}

Is better solution possible?

This looks wrong:

memset(arr, -1, sizeof(arr));

This is setting sizeof(int*) bytes of arr to -1.  It doesn't set the entire contents of arr.

You should probably be doing this:

int arr_size = sizeof(int)*score;
arr = (int*)malloc(arr_size);
memset(arr, -1, arr_size);

Although as pointed out by @Andrea Biondo, this only works because most architectures and compilers are based around twos complement numbers.  There are some exceptions to this rule, which you're likely to know about if they effect you.


Print 10 × 10 square of X and O characters, split diagonally I'm going through a beginners exercise for writing a small program in Java. My task is to print out (i.e. find an algorithm) the following output:

XOOOOOOOOO
XXOOOOOOOO
XXXOOOOOOO
XXXXOOOOOO
XXXXXOOOOO
XXXXXXOOOO
XXXXXXXOOO
XXXXXXXXOO
XXXXXXXXXO
XXXXXXXXXX

I have figured out how to do it but the code I wrote seems repetitive and tedious. How can I shorten my code?

package helloWorld;

public class HelloWorld {

public static void main(String[] args)
{
    // Output

    // Algorithm 1

    boolean ft = true;

    String s = new String();
    for(int i = 0; i < 10; i++)
    {
        s  += "X";
        for(int j = 0; j < 9; j++)
        {
            if(i == 0)
            {

                s += "O";
            }

            if(i == 1)
            {

                if(ft == true)
                {
                    s+="X";
                    ft = false;
                }
                if(j == 8)
                {
                    continue;
                }
                s += "O";
            }

            if(i == 2)
            {

                if(ft == true)
                {
                    s+= "XX";
                    ft = false;

                }

                if(j == 7)
                {

                    break;
                }
                s += "O";

            }

            if(i == 3)
            {

                if(ft == true)
                {
                    s+= "XXX";
                    ft = false;

                }

                if(j == 6)
                {

                    break;
                }
                s += "O";
            }

            if( i == 4)
            {
                if(ft == true)
                {
                    s+= "XXXX";
                    ft = false;

                }

                if(j == 5)
                {

                    break;
                }
                s += "O";
            }

            if( i == 5)
            {
                if(ft == true)
                {
                    s+= "XXXXX";
                    ft = false;

                }

                if(j == 4)
                {

                    break;
                }
                s += "O";
            }

            if( i == 6)
            {
                if(ft == true)
                {
                    s+= "XXXXXX";
                    ft = false;

                }

                if(j == 3)
                {

                    break;
                }
                s += "O";
            }

            if( i == 7)
            {
                if(ft == true)
                {
                    s+= "XXXXXXX";
                    ft = false;

                }

                if(j == 2)
                {

                    break;
                }
                s += "O";
            }

            if( i == 8)
            {
                if(ft == true)
                {
                    s+= "XXXXXXXX";
                    ft = false;

                }

                if(j == 1)
                {

                    break;
                }
                s += "O";
            }

            if( i == 9)
            {
                if(ft == true)
                {
                    s+= "XXXXXXXXX";
                    ft = false;

                }

                if(j == 0)
                {

                    break;
                }
                s += "O";
            }

        }
        System.out.println(s);
        s = "";
        ft = true;

    }
}

}

Lots of answers already, but most of them seem to be about alternative solutions. I'll review your code on the details and show you how to refactor your code step by step. (This is a long answer, link to bottom of answer)

String s = new String();

Creating a new String could be simplified to "". The only time you'll actually use the String constructor is for creating strings from arrays like char[] or byte[].

The first line of output

for(int i = 0; i < 10; i++)
{
    s  += "X";
    for(int j = 0; j < 9; j++)
    {
        if(i == 0)
        {

            s += "O";
        }

Seems simple enough. Add an X, then add 9 O's.

The second line of output

for(int i = 0; i < 10; i++)
{
    s  += "X";
    for(int j = 0; j < 9; j++)
    {

        if(i == 1)
        {

            if(ft == true)
            {
                s+="X";
                ft = false;
            }
            if(j == 8)
            {
                continue;
            }
            s += "O";
        }

ft starts off true. Like that, you'll print 1 X, then enter the j loop, add one more X, then an O, and you loop from there to add the remaining characters. This leads to the problem that you have one character extra... so you have to add the j == 8 check to stop the algorithm from printing a line with 10 characters.

You can check boolean conditions without an equality operator, like so:

if(ft)
{
    s+="X";
    ft = false;
}

This will work just as well as the ft == true check.

The third line of output

for(int i = 0; i < 10; i++)
{
    s  += "X";
    for(int j = 0; j < 9; j++)
    {

        if(i == 2)
        {

            if(ft == true)
            {
                s+= "XX";
                ft = false;

            }

            if(j == 7)
            {

                break;
            }
            s += "O";

        }

Same as the second line, except here you add two X on the first iteration, and to compensate, you break one iteration earlier. This is kind of problematic, since you now have to duplicate your code for each iteration. You're missing out on the power of the for-loop.

Did you know Strings come with a length() method? You can retrieve the current length with it.

We can use that method to change the for loop so that each iteration adds just one character.

        if(i == 2)
        {

            if(ft == true)
            {
                s+= "XX";
                ft = false;

            }

            if(j == 7)
            {

                break;
            }
            s += "O";

        }

If we use the length() method here to determine ft...

        if(i == 2)
        {
            if(ft == true)
            {
                s+= "XX";
                ft = s.length() < 3;
            }

            if(j == 7)
            {

                break;
            }
            s += "O";

        }

Then we can change the code so that it only adds one character at a time, removing the condition that was added to prevent going over 10 characters.

        if(i == 2)
        {
            if(ft == true)
            {
                s+= "X";
                ft = s.length() < 3;
            }
            else
            {
                s += "O";
            }
        }

We could copy this change for all cases...

        if(i == 3)
        {
            if(ft == true)
            {
                s+= "X";
                ft = s.length() < 4;
            }
            else
            {
                s += "O";
            }
        }

but that seems like we're still duplicating code.

Fortunately, it seems we can make use of the fact that... if i == n, then the check needs to be s.length() < n + 1.

So let's apply that change.

        if(i == 3)
        {
            if(ft == true)
            {
                s+= "X";
                ft = s.length() < (i + 1);
            }
            else
            {
                s += "O";
            }
        }

Now all the cases look like this:

        if(i == 2)
        {
            if(ft == true)
            {
                s+= "X";
                ft = s.length() < (i + 1);
            }
            else
            {
                s += "O";
            }
        }

        if(i == 3)
        {
            if(ft == true)
            {
                s+= "X";
                ft = s.length() < (i + 1);
            }
            else
            {
                s += "O";
            }
        }

Seems like we can merge the cases i == 1 through i == 9 because they're all the same. And since i only goes to 9 because of the i < 10 condition, all we're interested in is checking that it's not the first iteration:

        if(i != 0)
        {
            if(ft == true)
            {
                s+= "X";
                ft = s.length() < (i + 1);
            }
            else
            {
                s += "O";
            }
        }

Here's where we're at now.

boolean ft = true;

String s = "";
for(int i = 0; i < 10; i++)
{
    s  += "X";
    for(int j = 0; j < 9; j++)
    {
        if(i == 0)
        {

            s += "O";
        }

        if(i != 0)
        {
            if(ft)
            {
                s+= "X";
                ft = s.length() < (i + 1);
            }
            else
            {
                s += "O";
            }
        }

    }
    System.out.println(s);
    s = "";
    ft = true;

}

Now... if we modified the setting of ft to do the s.length() < (i + 1) check before we tried to add a new character to the line, we could even merge the i == 0 case.

for(int i = 0; i < 10; i++)
{
    s  += "X";
    for(int j = 0; j < 9; j++)
    {
        ft = s.length() < i;
        if(i == 0)
        {

            s += "O";
        }

        if(i != 0)
        {
            if(ft)
            {
                s += "X";
            }
            else
            {
                s += "O";
            }
        }

    }
    System.out.println(s);
    s = "";
    ft = true;

}

Now, in the case i == 0, ... you enter the j-for loop with one "X" in your string already. So ft would be set to false, always. And in the i != 0 case, if ft is not true... we do the same thing as the i == 0 case. So we can remove the checks whether i is or is not 0, as it will work just fine:

for(int i = 0; i < 10; i++)
{
    s  += "X";
    for(int j = 0; j < 9; j++)
    {
        ft = s.length() < i;
        if(ft)
        {
            s += "X";
        }
        else
        {
            s += "O";
        }
    }
    System.out.println(s);
    s = "";
    ft = true;

}

Now that we've made that change, the code is short. But it's not that clear yet. ft is a pretty meaningless variable. If we renamed it to notEnoughXYet, the code becomes a bit more clearer:

boolean notEnoughXYet = true;

String s = "";
for(int i = 0; i < 10; i++)
{
    s  += "X";
    for(int j = 0; j < 9; j++)
    {
        notEnoughXYet = s.length() < i;
        if(notEnoughXYet)
        {
            s += "X";
        }
        else
        {
            s += "O";
        }
    }
    System.out.println(s);
    s = "";
    notEnoughXYet = true;

}

... We can still go further, though.

There are two places we set notEnoughXYet to true and s to "". Maybe we can merge them?

What about putting it at the start of each iteration of the i for loop?

for(int i = 0; i < 10; i++)
{
    boolean notEnoughXYet = true;

    String s = "";
    s  += "X";
    for(int j = 0; j < 9; j++)
    {
        notEnoughXYet = s.length() < i;
        if(notEnoughXYet)
        {
            s += "X";
        }
        else
        {
            s += "O";
        }
    }
    System.out.println(s);
    s = "";
    notEnoughXYet = true;

}

Like this, we see that we can eliminate some more duplication: the setting of s and notEnoughXYet at the bottom can disappear. And s could be initialized to X.

for(int i = 0; i < 10; i++)
{
    boolean notEnoughXYet = true;
    String s = "X";
    for(int j = 0; j < 9; j++)
    {
        notEnoughXYet = s.length() < i;
        if(notEnoughXYet)
        {
            s += "X";
        }
        else
        {
            s += "O";
        }
    }
    System.out.println(s);
}

You could even move notEnoughXYet to the inner for loop...

for(int i = 0; i < 10; i++)
{
    String s = "X";
    for(int j = 0; j < 9; j++)
    {
        boolean notEnoughXYet = s.length() < i;
        if(notEnoughXYet)
        {
            s += "X";
        }
        else
        {
            s += "O";
        }
    }
    System.out.println(s);
}

Or get rid of it entirely.

for(int i = 0; i < 10; i++)
{
    String s = "X";
    for(int j = 0; j < 9; j++)
    {
        if(s.length() < i)
        {
            s += "X";
        }
        else
        {
            s += "O";
        }
    }
    System.out.println(s);
}

There's still a double setting of X to s, though. If you wanted to change the characters you'd have to change it in two places. Maybe you should just make the inner loop also do 10 iterations?

for(int i = 0; i < 10; i++)
{
    String s = "";
    for(int j = 0; j < 10; j++)
    {
        if(s.length() <= i)
        {
            s += "X";
        }
        else
        {
            s += "O";
        }
    }
    System.out.println(s);
}

Note that I have changed the s.length() < i to s.length() <= i because now the first iteration also needs to put an X.

The result

What we've just done, is step for step, identifying duplication or similarities, and then looking for the differences in that duplication to merge the duplication. The end result is code which, whilst containing more complexity because it doesn't spell out each case individually, is considerably shorter and should be more understandable.


Creating a basic secure php mailer I am busy creating a basic php mailer script to post to _self and email to a address.

Is the script secure?

How can I avoid someone clicking on submit the whole time, to spam the mailbox, with minimal extra code

<?php
//Mail header removal
function remove_headers($string) {
   $headers = array(
   "/to\:/i",
   "/from\:/i",
   "/bcc\:/i",
   "/cc\:/i",
   "/Content\-Transfer\-Encoding\:/i",
   "/Content\-Type\:/i",
   "/Mime\-Version\:/i"
 );
$string = preg_replace($headers, '', $string);
return strip_tags($string);
}

$to      = "email@tosendto.com";
$subject = "Sent from site";

$uname    = remove_headers($_POST['fname']);
$uemail   = remove_headers($_POST['femail']);
$umessage = remove_headers($_POST['fmessage']);
$umessage = "Name : " . $uname . " Email : " . $uemail . " Message : " . $umessage;

if(isset($_POST['submit']))
{
   mail($to, $subject, $umessage, "From: page@website.com");
}

?>

Message

  <form name="test" action="<?php echo htmlentities($_SERVER['PHP_SELF']); ?>" method="post">

Your Name:

<input type="text" size="20" name="fname">

Your Email:

<input type="text" size="20" name="femail">

Your Message:

<textarea name="fmessage" rows="4" cols="20"></textarea>

<input type="submit" name="submit" value="Send Message">
  </form>
  <?php if(isset($_POST['submit']))
  {
   echo "
Sent. We will be in contact shortly.
";
  } ?>

Your using the email sanitize filter on each field, you need to use FILTER_SANITIZE_STRING for the name and FILTER_SANITIZE_FULL_SPECIAL_CHARS for the message field.

Sanitizing is not the same as validating...

VALIDATE Filters

SANITIZE filters


Vertically print a list of strings to STDOUT into 3 columns with column lengths as balanced as possible This was a interview question I struggled with, I am posting my solution hoping to look for more elegant/efficient solutions.

Problem: Given a list of strings, print its element out to STDOUT one by one vertically into 3 columns, while making sure the 3 columns' length are as balanced as possible.

For example, given list = ['a','b','c','d','e','f','g','h','i','j']

The output could be either:

a e h
b f i
c g j
d

OR

a d g
b e h
c f i
    j

But not:

a e i
b f j
c g
d h

My approach in Python 3:

Basically I put each word into a "N by 3" 2D list horizontally and try to print them out vertically. If the length is not divisible by 3, I manually increase the length of the first 2 rows depending on the remainder. This creates quite a few edge cases for me to handle separately, such as when the length of the list is between 3 and 6 or when the length is less than or equal to 3.

def assign_words(grid, words):
    i = 0
    for row in range(len(grid)):
        for col in range(len(grid[row])):
            grid[row][col] = words[i]
            i += 1
            if i == len(words):
                return

def print_words(words):
    word_count = len(words)
    rows = word_count // 3
    extra_columns = word_count % 3

    grid = [[''] * 3 for _ in range(rows)]

    # special case
    if word_count <= 3:
        grid.append(['']*3)
        assign_words(grid, words)
        for row in grid:
            print (' '.join(row))
        return

    if extra_columns == 1:
        if rows > 2:
            grid[0].append('')
        elif rows == 2:
            grid[1].pop()
            grid.append(['']*3)
    elif extra_columns == 2:
        if rows > 2:
            grid[0].append('')
            grid[1].append('')
        else:
            grid.append(['']*3)

    assign_words(grid, words)

    # special case
    if 3 < word_count < 6:
        print (grid[0][0]+ ' '+grid[0][2]+' '+grid[1][1])
        print (grid[0][1]+ ' '+grid[1][0]+' '+grid[1][2])
        return

    # print grid
    for col in range(len(grid[0])):
        for row in range(len(grid)):
            if col == len(grid[row]):
                break
            print (grid[row][col], end=' ')
        print ()
 print_words(['a','b','c','d','e','f','g'])

You should take a look into Python advanced iteration capabilities, i.e. the itertools module. You’ll find a number of recipe; amongst which is take:

import itertools

def take(n, iterable):
    "Return first n items of the iterable as a list"
    return list(itertools.islice(iterable, n))

Which will help you extract columns out of your list of words. You can divide the length of the list by 3 to know how much data to take in each column, and adjust using the modulo of the length by 3 (the modulo first columns get to pick one more word); which can be eased thanks to divmod. Lastly, you need to organize your columns into rows, which is easily done using itertools.zip_longest.

That being done, you will need to print the data. The simplest solution is to only print them, as you do:

def print_words(words):
    columns, dangling = divmod(len(words), 3)
    iterator = iter(words)
    columns = [take(columns + (dangling > i), iterator) for i in range(3)]
    for row in itertools.zip_longest(*columns, fillvalue=''):
        print('{}  {}  {}'.format(*row))

Which behaves like so:

>>> print_words(['a', 'b', 'ccccccc', 'dd', 'e', 'ffffffff', 'gg', 'h', 'i', 'j'])
a  e  h
b  ffffffff  i
cccccc  gg  j
dd

Which is not pretty. So you might want to save the intermediate result to check, for each column, the length of the longest word, and apply some padding:

def print_words(words):
    columns, dangling = divmod(len(words), 3)
    iterator = iter(words)
    columns = [take(columns + (dangling > i), iterator) for i in range(3)]
    paddings = [max(map(len, column)) for column in columns]
    for row in itertools.zip_longest(*columns, fillvalue=''):
        print('  '.join(word.rjust(pad) for word, pad in zip(row, paddings)))

Which behave more nicely:

>>> print_words(['a', 'b', 'ccccccc', 'dd', 'e', 'ffffffff', 'gg', 'h', 'i', 'j'])
      a         e  h
      b  ffffffff  i
ccccccc        gg  j
     dd

Lastly, we can generalize to support an arbitrary number of columns:

def print_words(words, column_count=3):
    columns, dangling = divmod(len(words), column_count)
    iterator = iter(words)
    columns = [take(columns + (dangling > i), iterator) for i in range(column_count)]
    paddings = [max(map(len, column)) for column in columns]
    for row in itertools.zip_longest(*columns, fillvalue=''):
        print('  '.join(word.rjust(pad) for word, pad in zip(row, paddings)))

Which allows

>>> print_words(['a', 'b', 'ccccccc', 'dd', 'e', 'ffffffff', 'gg', 'h', 'i', 'j'])
      a         e  h
      b  ffffffff  i
ccccccc        gg  j
     dd
>>> print_words(['a', 'b', 'ccccccc', 'dd', 'e', 'ffffffff', 'gg', 'h', 'i', 'j'], 4)
      a        dd  gg  i
      b         e   h  j
ccccccc  ffffffff
>>> print_words(['a', 'b', 'ccccccc', 'dd', 'e', 'ffffffff', 'gg', 'h', 'i', 'j'], 5)
a  ccccccc         e  gg  i
b       dd  ffffffff   h  j


Here is a function I wrote that, with the help of jQuery, will randomize a bunch of properties Here's some code... please, be brutal
function twisty (elem, container) {
    elem = $('.some-elem');
    container = $('.container').hide();
    var spots = elem.text().replace(' ', '').split('');
    for(var i = 0; i < 300; i++) {
    $('', {
        'class': 'spot '+ i,
        text: spots[Math.ceil(Math.random() * spots.length-1)] ,
        css: {
            fontSize: Math.ceil(Math.random() * 50),
            left: Math.ceil(Math.random() * 1000),
            top: Math.ceil(Math.random() * 30),
            bottom: Math.ceil(Math.random() * 50),
            opacity: Math.random() / 3,
            color: 'rgb('+ Math.ceil(Math.random() * 70) + ',' +
                           Math.ceil(Math.random() * 70) + ',' +
                           Math.ceil(Math.random() * 70) + ')',
            transform: 'rotate(' + Math.ceil(Math.random() * 360)  + 'deg)'
        }
        }).appendTo(container.fadeIn(800));
    }
}

jQuery(document).ready(function($) {
twisty();
$('.some-elem').on('click', function(){
    $('.container').hide();
    $('span.spot').remove();
    twisty();
});

});
Here's the html

hello world

Here's a link to the fiddle output
Probably not the most useful piece of code, but some things I'm trying to learn are:

How to extend native javascript objects (so how could i minimize the
excessive use of calling Math.random)
How to make my code as clean and as portable as possible
How to animate stuff
How to do all of the above without using jQuery
How to extend jQuery

But mostly, I'm looking for feedback :]
edit: fixed so it actually fades in
(updated link to include some css, cause the transform prop wasn't working)

Notes

I had a go at converting it to native JavaScript, was fun :P Pretty nifty bit of code. Here are my notes:

I pulled Math.ceil(Math.random() * x)) out into a function randomInt(x), I recommend against extending native JavaScript objects. They tend to be difficult problems to debug, just create a new function instead.
I would probably prefer to have .container outside of .some-elem or programmatically generated. I didn't make this change though. The main issue with having it inside .some-elem next to the text is that when you attempt to fill spots in JS you need to get rid of the .container markup from .innerHTML as it comes out like this:

hello world

Which leads to a bit of ugly code to extract everything before the '<':

var spots = [].splice.call(elem.innerHTML, 0, elem.innerHTML.indexOf('<'));

You can add the speak: none; to .container so screen readers don't read "hello world lakmsdlkaslkadsadfas...".
You can extend jQuery like so:

$.fn.customFunction = function () {
    console.log(this);
}

$('.some-elem').customFunction();

I've used CSS3 transitions in place of $.fadeIn(), browser support isn't quite as good (looking at you IE), but the implementation is worth it in my opinion. Especially when most people wouldn't even notice like this super subtle fade in.
I'm not so sure about adding a number as a class name, I don't think it's legal to have class names start with a number.
Your indentation when you were applying everything to the  was a little ugly :P
Overall it was a pretty decent and clean jQuery implementation.

Code

jsFiddle

HTML

hello world

CSS

body { background: black; font-family: consolas;}

div.content {
    background: white;
    opacity: 0.9;
    min-height: 1000px;
    width: 960px;
    margin: auto;
    color: black;
    padding: 10px;
    list-style: none;
    position: relative;
}

div.content > .some-elem {
    text-shadow: 1px 1px 6px rgba(0, 0, 0, 1), 0px -1px 2px rgba(0, 0, 0, 1);
    border-bottom: 7px solid #41919B;
    border-top: 7px solid #41919B;
    border-right: 1px solid #BDBDBD;
    border-left: 1px solid #BDBDBD;
    padding-bottom: 7px;
    box-shadow: 0px 11px 17px 1px rgba(0, 0, 0, 0.5), inset 0px 0px 0px 1px rgba(0, 0, 0, 0.6);
    border-radius: 5px 5px 5px 5px;
    text-align: center;
    color: black;
    width: 960px;
    overflow: hidden;
    position: relative;
    letter-spacing: 11px;
    line-height: 60px;
    cursor: pointer;
}
div.content > .some-elem:hover {
    color: #fff;
    text-shadow: 1px 1px 6px rgba(0, 0, 0, 1), 0px -1px 2px rgba(0, 0, 0, 1);
}

span.spot {
    position: absolute;
    top: 0px;
    z-index: -1;
    text-shadow: none;
    box-shadow: none;
}

.container {
    opacity:0;
    -webkit-transition:opacity .8s ease;
    -moz-transition:opacity .8s ease;
    transition:opacity .8s ease;
    speak: none;
}

.container.show {
    opacity:1;
}

JS

function twisty(elemClass, containerClass) {
    var elem = document.getElementsByClassName('some-elem')[0];
    var container = elem.getElementsByClassName('container')[0];
    var spots = [].splice.call(elem.innerHTML, 0, elem.innerHTML.indexOf('<'));

    for (var i = 0; i < 300; i++) {
        var span = document.createElement('span');
        var color = 'rgb('+ randomInt(70) + ',' +
            randomInt(70) + ',' +
            randomInt(70) + ')';
        var transform = 'rotate(' + randomInt(360) + 'deg)';

        span.innerHTML = spots[randomInt(spots.length - 1)];
        span.className = 'spot '+ i;
        span.style.fontSize = randomInt(50) + 'px';
        span.style.left = randomInt(1000) + 'px';
        span.style.top = randomInt(30) + 'px';
        span.style.bottom = randomInt(50) + 'px';
        span.style.opacity = Math.random() / 3;
        span.style.color = color;
        span.style.webkitTransform = transform;
        span.style.mozTransform = transform;
        span.style.transform = transform;
        container.appendChild(span);
    }

    container.classList.add('show');
}
twisty();

document.getElementsByClassName('some-elem')[0].onclick = function () {
    var container = document.getElementsByClassName('container')[0];
    container.classList.remove('show');
    while (container.hasChildNodes()) {
        container.removeChild(container.lastChild);
    }
    twisty();
};

function randomInt(max) {
    return Math.ceil(Math.random() * max);
}


Fancy padding of wide strings I have a function that adds to a std::wstring some filler characters. The user gives a single filler character. They can also optionally given an initial filler sequence (std::wstring) and an optional final filler sequence (std::wstring) that start and end the padding. These will only be added however if they fit inside the padded length specified by the user.

static std::wstring fillWString(
    const std::wstring &stringToFill,
    size_t fillLength,
    const wchar_t fillChar = L' ',
    const std::wstring &initialFill = L"",
    const std::wstring &endFill = L"") {

  const size_t originalStringLength = stringToFill.size();
  if (fillLength <= originalStringLength) {
    return stringToFill;
  }

  std::wstring result(stringToFill);
  result.resize(fillLength, fillChar);

  if (originalStringLength + initialFill.size() <= fillLength) {
    result.replace(originalStringLength, initialFill.size(), initialFill, 0u);
    if (originalStringLength + initialFill.size() + endFill.size() <= fillLength) {
      result.replace(fillLength - endFill.size(), endFill.size(), endFill, 0u);
    }
  }
  return result;
}

Here is some output:

Analyzed folder: .... \Path\To\Some\Folder\
Time step: .......... 0.007045000
Out Flow: ........... 1.2460679423999952
In Flow: ............ -1.2461052960000008

where each line was generated using

fillWString(L"string", 22, '.', L" ", L" ") << value;

Specifically, I was wondering what sort of tips you guys might give to improve this. Also, I was just wondering if copy-elision is guaranteed to occur here if the string is indeed padded?

The point where the parameters stop and the function-body begins isn't easy to see.
Either indent more, or better consider changing to what c2-wiki calls form 6.

You are allocating way too much when passing arguments.
Using std::wstring_view instead of std::wstring const& lets you avoid that overhead and reduces indirection, which is also a win.

Also, you potentially re-allocate after copying the source when widening to the target-length.

static std::wstring fillWString(
    std::wstring_view source,
    size_t target,
    wchar_t fillChar = L' ',
    std::wstring_view startFill = {},
    std::wstring_view endFill = {}
) {
    if (target <= source.size())
        return source;

    std::wstring result;
    result.reserve(target);
    result += source;
    target -= source.size();

    if (startFill.size() > target)
        return result;
    result += startFill;
    target -= source.size();

    if (endFill.size() > target) {
        result.append(target, fillChar);
        return result;
    }
    result.append(target - endFill.size(), fillChar);
    result += endFill;
    return result;
}


Search for directories with a certain prefix that contain no files of a given type These two functions search for directories with a certain prefix that contain no files of a given type.

Does the code adhere to Ruby standards and conventions?
Am I doing something the hard way or the "non-Ruby" way?
Should I use more/fewer/different comments?

def find_empties(directory, dirprefix, fsuffix)
  empty_dirs = Array.new
  Dir.chdir directory do
    Dir["#{dirprefix}**/"].each do |subdir|
      Dir.chdir subdir do
        empty_dirs.push subdir if Dir["**.#{fsuffix}"].empty?
      end
    end
  end
  return empty_dirs
end

def find_empties_sorted(directory, dirprefix, fsuffix)
  empties = find_empties(directory, dirprefix, fsuffix)
  nums = Hash.new
  non_numeric = Array.new
  empties.each do |x|
    # if starts with digits, trim leading zeroes and extract; else, nothing
    num = x.sub(/^#{Regexp.escape dirprefix}0*(\d+).*/, '\1')
    is_num = num !~ /\D/
    if is_num
      n = Integer num
      nums[n] = Array.new unless nums[n]
      nums[n].push x
    else
      non_numeric.push x
    end
  end
  result = Array.new
  nums.sort.each do |k, v|
    v.each do |x|
      result.push x
    end
  end
  result.concat non_numeric.sort
  result
end

The code works as intended, but here's a test case:

$ ls
emptyfinder.rb
emptyrunner.rb
$ cat emptyrunner.rb
require_relative 'emptyfinder.rb'

puts find_empties_sorted(Dir.pwd, 'e', 'txt')
$ mkdir testcase
$ cd testcase
$ mkdir e1 e2 e003 e004 e4 e5 e9 e10 e11 empty emptier emptynot
$ touch emptynot/file.txt
$ touch empty/not_a_txt.csv
$ ruby ../emptyrunner.rb
e1/
e2/
e003/
e004/
e4/
e5/
e9/
e10/
e11/
emptier/
empty/
$

Looks alright.

Your style is more spread out than is usually the case in ruby (though that's not necessarily a bad thing.) For example

is_num = num !~ /\D/
if is_num
  ...

could as well be one line sans assignments, without loosing clarity. if num !~ /\D/  # is a number

Instead of declaring nums to be a Hash, then having an if statement to check if a pair needs initialised with an array, you can just declare the hash with a block that it then uses to default all values to new arrays like so nums = Hash.new { |h,k| h[k] = [] }.

Instead of iterating through the array v of sorted nums and pushing each item to result, you can just do result.concat v. Sometimes for simple logic, denser one liners are arguably easier to read in ruby: e.g. nums.sort.each { |k, v| result.concat v }

Be aware that you can initialise arrays and hashes as literals, e.g. a = [] and this is sometimes prefered.

Your level of commenting seems appropriate to me, i.e. explaining what the code is meant to do (or why but not how) when it's not obvious from the code itself.


Moving average calculation I am hoping to make this method run a little faster. I typically need to run this on a list with more than 100000 entries. Note that at the start and end of the list, I wish to weight the average so that it is closer to the start or end value, hence the smaller framesize.

public PointPairList GetMovingAverage(int frameSize, PointPairList data)
{
    PointPairList movAvgPoints = new PointPairList();
    //Smooth each point in the list
    for (int i = 0; i < data.Count; i++)
    {
        //get the window range
        int actualFrameSize = frameSize;
        int start = i - (frameSize/2);
        int end = i + (frameSize/2);
        //ensure the window is the same size at the source list boundaries
        if (start < 0)
        {
            actualFrameSize = frameSize + start;
            start = 0;
        }
        if (end >= data.Count)
        {
            actualFrameSize = frameSize + (data.Count - 1 - end);
        }
        //Now get the sum of the window
        double sum = data.Skip(start).Take(actualFrameSize).Sum(p => p.Y);
        //add the average point to the result
        movAvgPoints.Add(new PointPair(data[i].X, sum/actualFrameSize));
    }
    return movAvgPoints;
}

Skip() is not optimized for IList<T>, it always enumerates the skipped elements. This means that your algorithm is O(n2) for no good reason. What you should do instead is to manually iterate just the required part of the input in each iteration using for. Something like:

double sum = 0;
for (int i = 0; i < actualFrameSize; i++)
{
    sum += data[start + i].Y;
}

(I don't like using GetRange() for this, as in tinstaafl's answer, because it unnecessarily copies the frame into a new list.)

If frameSize is large, another optimization would be not to recompute the sum of the frame for each index. Instead, just add the item from the front and subtract the item from the back:

public PointPairList GetMovingAverage(int frameSize, PointPairList data)
{
    var movAvgPoints = new PointPairList();

    //Before zero
    int currentFrameSize = frameSize/2;
    double sum = data.Take(currentFrameSize).Sum(p => p.Y);

    //Smooth each point in the list
    for (int i = 0; i < data.Count; i++)
    {
        int removed = i - (frameSize/2) - 1;
        int added = i + (frameSize/2);

        if (removed >= 0)
        {
            sum -= data[removed].Y;
            currentFrameSize--;
        }
        if (added < data.Count)
        {
            sum += data[added].Y;
            currentFrameSize++;
        }

        movAvgPoints.Add(new PointPair(data[i].X, sum/actualFrameSize));
    }

    return movAvgPoints;
}

If the length of data is n and frameSize is f, then the previous algorithm would be O(fn), but the improved one just O(n).

Another thing: there is usually no need to create types that derive from List, just use List<PointPair> directly.


Trying to find a better way of finding something rather then looping through list from top to bottom. I am having a slight issue here, i am trying to think of a faster way of finding two elements from 2 different lists that match. The problem is that if i have lists which both have 1000 elements ( rules ) and the very last one [index.1000] matches then in order to find it i have to loop through booth lists from top to bottom. What i have is fine and it works however its not very efficient. Could any one perhaps suggest a better way ? (if there is any ? )

The loop in this meethod is where the iteration through the lists happen. For furthere reference below this method you will find what ContextRule and Context is.

public ContextRule match(Context messageContext, ContextRuleList contextRules) {
    ContextRule matchedContextRule = null;

    for(ContextRule contextRule : contextRules) {
        if(this.match(messageContext, contextRule)) {
            matchedContextRule = contextRule;
            break;
        }
    }
    if(matchedContextRule == null) {
        matchedContextRule = this.getDefaultContextRule();
    }
    return matchedContextRule;
}

match() method which does comparason.

private boolean match(Context messageContext, ContextRule contextRule) {
        return match(contextRule.getMessageContext().getUser(), messageContext.getUser())
                && match(contextRule.getMessageContext().getApplication(), messageContext.getApplication())
                && match(contextRule.getMessageContext().getService(), messageContext.getService())
                && match(contextRule.getMessageContext().getOperation(), messageContext.getOperation());
    }

    private boolean match(String value, String contextValue) {
        return value.equals(ContextRuleEvaluator.WILDCARD) || value.equals(contextValue);
    }

Context ( which is an interface )

public interface Context {
  public String getUser();
  public void setUser(String user);
  public String getApplication();
  public void setApplication(String application);
  public String getService();
  public void setService(String service);
  public String getOperation();
  public void setOperation(String operation);
}

And finally ContextRule

public interface ContextRule {
  public Context getMessageContext();
  public int getAllowedConcurrentRequests();
}

any help or suggestions appreciated.

Can your context rules be precomputed into any sort of hierarchical match tree once at application initialization?

HashMap<String, HashMap<String, List<ContextRule>>> contextRuleMap

public boolean match (Context context, Map<String, Map<String, List<ContextRule>> contextRuleMap)
{
    Map<String, List<ContextRule>> rulesByApplication = contextRuleMap[context.getUser()];

    for (ContextRule contextRule : rulesByApplication[context.getApplication()]) {
         // Existing logic for remaining 'match' of service and operation
    }

    return false;
}

You can pre-compute your 'match tree' to as many 'levels deep' as you need to by User -> Application -> Service -> Operation to achieve the necessary performance.


Receive data from SQL and CSV and send it to MySQL database using Python Details:

I have a code, which does the following:

Get data from CSV and insert it into MySQL table
Get data from SQL Server and insert it into MySQL table
Run this code every 240 seconds - to refresh the data, in order to have up to date infromation.

I'd like to know whether the code below, I am about to provide, can be improved? In terms of performance and whether the code is readable or anything can be changed.

import pyodbc
import csv
import mysql.connector
import time

#MySQL connection
MySQLdb = mysql.connector.connect(
  host="SVR",
  user="root",
  passwd="",
  database="production"
)

#SQL Sevrer
sqlServerData = pyodbc.connect(
    "Driver={SQL Server Native Client 11.0};"
    "Server=svr;"
    "Database=SQL_Server;"
    "Trusted_Connection=yes;")

input("Press Enter to continue...")

starttime=time.time()
while True:
    try:
        print("---START---")

        #MySQL cursor
        MySQLtruncateCSV = MySQLdb.cursor()
        MySQLtruncateSQL_Server = MySQLdb.cursor()
        MySQLcursor = MySQLdb.cursor()
        MySQLcursor2 = MySQLdb.cursor()

        #SQL Server Cursor
        SqlData = sqlServerData.cursor()

        #Truncate MySQL Tables
        CSVTruncate = 'Truncate CSV;'
        SQL_ServerTruncate = 'Truncate SQL_Server;'
        print("Truncate Table CSV...")
        #CSV table
        MySQLtruncateCSV.execute(CSVTruncate)
        MySQLtruncateCSV.close()
        MySQLdb.commit()
        print("Truncate Table SQL_Server...")
        #SQL_Server table
        MySQLtruncateSQL_Server.execute(SQL_ServerTruncate)
        MySQLtruncateSQL_Server.close()
        MySQLdb.commit()

        print("Receiving data from CSV...")
        print("Sending to MySQL..")
        #Insert CSV file into MySQL database
        with open('G:/Technical/Labels/Production/Data/CSVExport.csv', 'r') as f:
            data = csv.reader(f)
            next(data, None) #Skip header
            for row in data:
                MySQLcursor.execute('insert into CSV (customer_code,customer_logo, product_code,product_description,allergen_info, barcode_inner,barcode_outer) VALUES (%s,%s,%s,%s,%s,%s,%s);', row)
        MySQLdb.commit()
        MySQLcursor.close()

        print("Receiving SQL_Server data...")
        #Get data from SQL_Server
        SqlData.execute("select p.id, p.code,p.description, p.searchRef1, so.number, c.code, c.name \
             from salesorderline sol join \
             salesorder so \
             on sol.salesorderid = so.id join \
             product p \
             on sol.productid = p.id join \
             customer c \
             on so.customerid = c.id \
             where so.orderdate > dateadd(dd,-10,cast(getdate() as date));")

        print("Sending to MySQL..")
        #Send SQL_Server data into MySQL
        for x in SqlData.fetchall():
            a,b,c,d,e,f,g = x
            MySQLcursor2.execute("insert into SQL_Server (product_id, product_code, product_description, product_weight, \
                    salesorder_number, customer_code, customer_name) values (%s,%s,%s,%s,%s,%s,%s);", (a,b,c,d,e,f,g))

        SqlData.close()
        MySQLdb.commit()
        MySQLcursor2.close()
        print("---END---")
        time.sleep(240 - ((time.time() - starttime) % 240))

    except:
        print("An error has occured.. Please contact Technical")
        break

Externalize your connection strings

This information:

  host="SVR",
  user="root",
  passwd="",
  database="production"

    "Driver={SQL Server Native Client 11.0};"
    "Server=svr;"
    "Database=SQL_Server;"
    "Trusted_Connection=yes;")

'G:/Technical/Labels/Production/Data/CSVExport.csv'

should not be baked into your program, for multiple reasons:

configurability and maintainability
security

Put these into a configuration file, a command-line argument, or an environmental variable.

Typo

#SQL Sevrer -> #SQL Server

Context management

Doing a search through the MySQL Connector Python source code as well as their bug tracker, it seems that the library has a deficiency where cursors cannot be used as context managers. You can do the next-best thing: try/finally whenever you make a connection or cursor that needs to be closed, or (probably better) make your own small context manager utilities for such cases.

The situation for pyodbc seems to be better. Connections and cursors should be used in with statements.

In all cases you should prefer this to explicit close() calls.

Multi-line strings

Since this is SQL:

    SqlData.execute("select p.id, p.code,p.description, p.searchRef1, so.number, c.code, c.name \
         from salesorderline sol join \
         salesorder so \
         on sol.salesorderid = so.id join \
         product p \
         on sol.productid = p.id join \
         customer c \
         on so.customerid = c.id \
         where so.orderdate > dateadd(dd,-10,cast(getdate() as date));")

Indentation does not matter. Your current string literal includes indentation, and it might as well stay that way but losing the continuation escapes and using a multi-line string:

    SqlData.execute("""
        select p.id, p.code,p.description, p.searchRef1, so.number, c.code, c.name
        from salesorderline sol
        join salesorder so on sol.salesorderid = so.id
        join product p on sol.productid = p.id
        join customer c on so.customerid = c.id
        where so.orderdate > dateadd(dd,-10,cast(getdate() as date));
    """)

I also think it is clearer and more logical to line-break before the join keyword rather than after, and include the on with its corresponding join.

Unpacking

This:

a,b,c,d,e,f,g = x

is not helpful. Either give these meaningful names, or don't unpack at all:

MySQLcursor2.execute("""
    insert into SQL_Server (
        product_id, product_code, product_description, product_weight,
        salesorder_number, customer_code, customer_name
    ) values (%s,%s,%s,%s,%s,%s,%s);
    """,
    x
)

Magic numbers and home-rolled time math

Do not do this:

    time.sleep(240 - ((time.time() - starttime) % 240))

It's difficult to understand. I guess 240 seconds is 4 minutes. You're

finding the elapsed time since the start,
modulating that by 4 minutes? Why?
subtracting that from 4 minutes.

At a wild guess, what you are trying to do is "wait until 4 minutes have passed since the start of the program", which would actually require

from datetime import datetime
from time import sleep

start_time = datetime.now()
# ...

elapsed = datetime.now() - start_time
until_4m = (timedelta(minutes=4) - elapsed).total_seconds()
if until_4m > 0:
    sleep(until_4m)


Wrapping an IntPtr in a Struct for safer Interop Consider the following C#/C interop scenario:

public static extern IntPtr lua_newstate();
public static extern void lua_close(IntPtr state);

IntPtr luaState = lua_newstate();
// use lua ...
lua_close(luaState)

Obviously this works well enough, however there's a fringe possibility that an IntPtr not pointing to a lua state could be passed to lua_close, since it accepts any IntPtr.

I realized that it's possible to use the behavior of struct marshaling to make this interop a bit more type-safe

[StructLayout(LayoutKind.Sequential)]
public struct LuaState
{
    public readonly IntPtr Pointer;
}

public static extern LuaState lua_newstate();
public static extern void lua_close(LuaState state);

LuaState luaState = lua_newstate();
// use lua ...
lua_close(luaState)

Since LuaState contains only an IntPtr, the marshaler happily converts the native pointer returned by lua_newstate to an instance of LuaState, and will also quite happily convert a LuaState struct to a native pointer when passed back to a native function.

This essentially creates a typed pointer. There is a cost associated with passing LuaState though the Marshaling system instead of passing IntPtr directly to the native side, but in testing the cost appears to be negligible.

Questions: Does using LuaState to create a typed wrapper for IntPtr in this way raise any red flags? Has anyone done something similar and run into issues?

I think, I would experiment with a safe or smart pointer approach in a way like this:

public sealed class LuaHandle : IDisposable
{
  private static extern IntPtr lua_newstate();
  private static extern void lua_close(IntPtr state);

  private IntPtr m_handle = IntPtr.Zero;

  public LuaHandle()
  {
    m_handle = lua_newstate();
  }

  public IntPtr Handle => m_handle;

  public void Dispose()
  {
    if (m_handle != IntPtr.Zero)
    {
      lua_close(m_handle);
    }

    m_handle = IntPtr.Zero;
  }
}

Usage:

  using (LuaHandle luaHandle = new LuaHandle())
  {
    // do something with luaHandle.Handle
  }

In this way you - as a client of LuaHandle - don't have to deal with the extern api's at all and no invalid IntPtr can be past as argument to lua_close()


Count number of nodes in a binary tree the OO way I am trying to write a simplify the algorithm of finding the number of nodes in a binary tree by using good object oriented design. I have been into good OOP style recently and found it really compelling in way that it can simplify the mental load by proper polymorphism.

Having said that, I don't know if it's a right strategy to follow in the interview since it may take some time to get to a good design.

import java.util.*;
import java.lang.*;
import java.io.*;

interface Node {
    public int count();
}

class Empty implements Node {
    public int count() {
        return 0;
    }
}

class NonEmpty<Item> implements Node {
    private final Item item;
    private final Node left;
    private final Node right;

    NonEmpty(Item item) {
        this(item, new Empty(), new Empty());
    }

    NonEmpty(Item item, Node left, Node right) {
        this.item = item;
        this.left = left;
        this.right = right;
    }

    public int count() {
        return left.count() + right.count() + 1;
    }
}

/* Name of the class has to be "Main" only if the class is public. */
class Main
{
    public static void main (String[] args) throws java.lang.Exception
    {
        Node n = new Empty();
        System.out.println(n.count()); // 0

        // Single node tree
        n = new NonEmpty(5);
        System.out.println(n.count()); // 1

        // Complete Full tree
        n = new NonEmpty(5, new NonEmpty(2), new NonEmpty(6));
        System.out.println(n.count()); // 3

        // Right skewed
        n = new NonEmpty(5, new NonEmpty(2, new NonEmpty(6), new Empty()), new Empty());
        System.out.println(n.count()); // 3
    }
}

I am thinking to write some common Tree based algorithm this way and see how far I can go.

Questions:

Is this a good strategy for interviews?
Have I divided the responsibility correctly?
Does this scale well to other tree-based algorithms?

Major Doubt:

I have seen in books such as this the author focuses on different classes like for above it could be:

class CountNodes {
  public CountNodes(Node n) {
    //...
  }
}

Which is a better strategy?

I agree with the other posters that a visitor pattern would be a viable solution. One important reason being, that a node usually holds some payload, not just other nodes, and you want to have a mechanism that allows you to be very versatile on what you want to do with the data in the tree.

You may want to count the number of nodes, you might also want to count the number of nodes, where the payload matches some criteria, or you want to count the number of nodes that do not have a left and a right sub node or whatnot. You don't want your node to contain a method for every possible question you want to ask your tree.

You might want to read my blog post on the LOD and structure agnostic access for a rather freaked out, but general solution. But be warned it's possibly not the kind of code you might be used to and you might not find it particularly "pretty", as it trades type safety for "simplicity".

But a visitor is not the only good solution out there. You may prefer to implement a method that returns an iterator, or even a Stream of nodes. This way you could use the Java stream API and be very versatile.

As for the interview: I often feel that programming tasks in interviews are misplaced. The résumé should give you enough of an idea if you've got an experienced programmer or not. Once the person is hired, code conventions, code reviews and tests should shape the quality and style of the person's code to your needs anyway - and there's a probation period to see if the person is not up to your standards or toxic. The only thing that a little piece of code is good for, in my opinion, is to sort out freshmen and students who don't know how to code, but claim to do so. In job interviews I might like to talk about a piece of code and discuss its style, not for the sake of labeling the answers "right" or "wrong", but to see how they think about code: Are they the pragmatic kind of programmer, are they code-poets with a love of style and readability, will you have to brake their tendency for over-engineering or will you have to nurse them a bit more? Do they make good points, are they the timid or the outspoken kind of person? You might feel you lack the one or the other kind in your team and that might make a difference in the end. But I feel that, if I look at a résumé with plenty of projects on it, that I should be able to assume they know how to code and I don't want to insult them with giving them programming tasks that are not related to their daily work at this company, but rather to their first year in algorithms and data structures class.

Discussing your piece of code above, I would, for example find it perfectly reasonable, if the candidate said: "This is a pragmatic solution for counting nodes and maybe something similar, so if that's all there is to it, I'd be fine with it. Still, if it is foreseeable that we need more than just two or three queries, we should have another object do the query." That'd be bang on.


Get column of a matrix python with index operator How can I refactor this code to generalize the argument parsing? I feel like this code can be better but I can't find a way. The usage syntax of the index operator is similar to the NumPy indexing syntax.

    def __getitem__(self, arg):
    """
    [] operator to index elements in the matrix
    Usage:
    >> mat[0] returns first row
    >> mat[3, :] return the 4th row
    >> mat[:, 7] return the 8th column
    >> mat[0, 0] returns first element
    """
    if isinstance(arg, int):
        return self.matrix[arg]
    elif isinstance(arg, tuple):
        if isinstance(arg[0], int) and isinstance(arg[1], int):
            y, x = arg
            return self.matrix[x][y]
        elif isinstance(arg[0], slice) and isinstance(arg[1], int):
            return self.get_column(arg[1])
        elif isinstance(arg[0], int) and isinstance(arg[1], slice):
            return self.get_column(arg[1])
    else :
        raise TypeError('Invalid indexing arguments type', arg)

Also, how can I make a setitem function that uses the [] operator to set columns without rewriting the getitem function inside it? Here is my current implementation without the possibility of setting a column.

    def __setitem__(self, arg, value):
    if isinstance(value, Fraction):
        self[arg] = value
    elif isinstance(value, list) and all(isinstance(elem, Fraction) for elem in value):
        self[arg] = value
    else:
        raise TypeError('Invalid value type', value)

Before improving the code it would be worth fixing the following problems.

The docstring says:

>> mat[3, :] return the 4th row

but the implementation tries to return a column:

elif isinstance(arg[0], int) and isinstance(arg[1], slice):
    return self.get_column(arg[1])

but since arg[1] is a slice object, this will fail in some way. (I can't say exactly how since you didn't show us the code for get_column.)
The indexing is inconsistent. As it says in the docstring:

>> mat[3, :] return the 4th row

but to get the first element of the fourth row, you have to write:

mat[0, 3]

because the indexes get swapped:

y, x = arg
return self.matrix[x][y]

This is contrary to the way indexing works in NumPy and so seems very likely to lead to confusion.
If you pass a tuple of length 1 then this causes an IndexError:

>>> mat[:,]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "cr189408.py", line 21, in __getitem__
    elif isinstance(arg[0], slice) and isinstance(arg[1], int):
IndexError: tuple index out of range

If you pass a pair of slices then this is ignored and __getitem__ returns None.

>>> mat[:,:] is None
True

In NumPy this would return the whole matrix.
The code only checks whether the argument is a slice or not, and if it is a slice, it assumes that the whole row or column is required. This is right if a bare : was passed, but misleading if some other slice was passed. In NumPy mat[0:2, 0] gets the first two element of column 0, but the code in the post returns the whole of column 0.