A pythonic way of de-interleaving a list (i.e. data from a generator), into multiple lists I've recently discovered the wonders of the Python world, and am quickly learning. Coming from Windows/C#/.NET, I find it refreshing working in Python on Linux. A day you've learned something new is not a day wasted.

I need to unpack data received from a device. Data is received as a string of "bytes", of arbitrary length. Each packet (string) consists of samples, for eight channels. The number of samples varies, but will always be a multiple of the number of channels. The channels are interleaved. To make things a bit more complex, samples can be either 8 or 16 bits in length. Check the code, and you'll see.

I've already got a working implementation. However, as I've just stumbled upon generators, iterators, maps and ... numpy, I suspect there might be a more efficient way of doing it. If not efficient, maybe more "pythonic". I'm curious, and if someone would spend some time giving me a pointer in the right (or any) direction, I would be very grateful. As of now, I am aware of the fact that my Python has a strong smell of C#. But I'm learning ...

This is my working implementation. It is efficient enough, but I suspect it can be improved. Especially the de-interleaving part. On my machine it prints:

time to create generator: 0:00:00.000040
time to de-interleave data: 0:00:00.004111
length of channel A is 750: True

As you can see, creating the generator takes no amount of time. De-interleaving the data is the real issue. Maybe the data generation and de-interleaving can be done simultaneously?

This is not my first implementation, but I never seem to be able to drop below approx 4 ms.

from datetime import datetime

def unpack_data(data):
    l = len(data)
    p = 0

    while p < l:
        # convert 'char' or byte to (signed) int8
        i1 = (((ord(data[p]) + 128) % 256) - 128)
        p += 1
        if i1 & 0x01:
            # read next 'char' as an (unsigned) uint8
            #
            # due to the nature of the protocol,
            # we will always have sufficient data
            # available to avoid reading past the end
            i2 = ord(data[p])
            p += 1
            yield (i1 >> 1 << 8) + i2
        else:
            yield i1 >> 1

# generate some test data ...
test_data = ''
for n in range(500 * 12 * 2 - 1):
    test_data += chr(n % 256)

t0 = datetime.utcnow()

# in this example we have 6000 samples, 8 channels, 750 samples/channel
# data received is interleaved: A1, B1, C1, ..., A2, B2, C2, ... F750, G750, H750
channels = ('A', 'B', 'C', 'D', 'E', 'F', 'G', 'H')

samples = { channel : [] for channel in channels}

# call unpack_data(), receive a generator
gen = unpack_data(test_data)

t1 = datetime.utcnow()

print 'time to create generator: %s' % (t1-t0)

try:
    while True:
        for channel in channels:
            samples[channel].append(gen.next())
except StopIteration:
    pass

print 'time to de-interleave data: %s' % (datetime.utcnow()-t1)

print 'length of channel A is 750: %s' % (len(samples['A']) == 750)

from datetime import datetime

def unpack_data(data):
    l = len(data)
    p = 0

I'd avoid such small variable names, it makes your code harder to follow

    while p < l:
        # convert 'char' or byte to (signed) int8
        i1 = (((ord(data[p]) + 128) % 256) - 128)
        p += 1
        if i1 & 0x01:
            # read next 'char' as an (unsigned) uint8
            #
            # due to the nature of the protocol,
            # we will always have sufficient data
            # available to avoid reading past the end
            i2 = ord(data[p])
            p += 1
            yield (i1 >> 1 << 8) + i2
        else:
            yield i1 >> 1

# generate some test data ...
test_data = ''
for n in range(500 * 12 * 2 - 1):
    test_data += chr(n % 256)

It usually better to put all the pieces of a string in a list and then join them. Python doesn't have good performance for added strings.

t0 = datetime.utcnow()

# in this example we have 6000 samples, 8 channels, 750 samples/channel
# data received is interleaved: A1, B1, C1, ..., A2, B2, C2, ... F750, G750, H750
channels = ('A', 'B', 'C', 'D', 'E', 'F', 'G', 'H')

samples = { channel : [] for channel in channels}

# call unpack_data(), receive a generator
gen = unpack_data(test_data)

t1 = datetime.utcnow()

print 'time to create generator: %s' % (t1-t0)

All you've done is created the generator, that won't do any actual work. So you aren't measuring much of anything here. You are still spending much of the time inside the function you've defined after this point.

try:
    while True:
        for channel in channels:
            samples[channel].append(gen.next())
except StopIteration:
    pass

It's best to avoid dealing with StopIteration directly if you can. In this case you can do:

for sample, channel in zip(gen, itertools.cycle(channels)):
     samples[channel].append(sample)

itertools.cycle() will give you a generator that goes repeatedly through all the channels in order.

print 'time to de-interleave data: %s' % (datetime.utcnow()-t1)

print 'length of channel A is 750: %s' % (len(samples['A']) == 750)

You can use numpy, I've done that for you. Basically, numpy lets you do operations over a whole array and that's faster then doing them in your loops. See below:

from datetime import datetime
import numpy

def unpack_data(data):
    # reads the string in as a sequence of uint8
    data = numpy.fromstring(data, numpy.uint8)
    # figure out if the most significant bit is set
    # for everything
    odds = numpy.logical_not(data & 0x01)

    # calculate the interpretation of each number
    # both possible ways
    singles = data.astype(numpy.int8) >> 1
    doubles = singles << 8 + numpy.roll(data, -1)

    # I couldn't vectorize this, it fills up the
    # result array with True for every actual starting value
    result = numpy.empty(data.shape, bool)
    current = True
    for index, byte in enumerate(odds):
        # the next bit is a starting bit if
        # if this isn't a starting bit, or the 1 bit wasn't set
        current = not current or byte
        result[index] = current

    # where chooses from the single and doubles
    # based on the lsb, and result filters those we actually want
    return numpy.where(odds, singles, doubles)[result]

# generate some test data ...
test_data = ''
for n in range(500 * 12 * 2 - 1):
    test_data += chr(n % 256)

t0 = datetime.utcnow()

# in this example we have 6000 samples, 8 channels, 750 samples/channel
# data received is interleaved: A1, B1, C1, ..., A2, B2, C2, ... F750, G750, H750
channels = ('A', 'B', 'C', 'D', 'E', 'F', 'G', 'H')

samples = { channel : [] for channel in channels}

# call unpack_data(), receive a generator
data = unpack_data(test_data)

t1 = datetime.utcnow()
print 'time to create generator: %s' % (t1-t0)

# reshape converts 1 dimensional array
# into two dimensional array
data = data.reshape(-1, len(channels))
for index, channel in enumerate(channels):
    samples[channel] = data[:,index]

print 'time to de-interleave data: %s' % (datetime.utcnow()-t1)

print 'length of channel A is 750: %s' % (len(samples['A']) == 750)


Removing contact from all mail groups I want to remove the mail contact from all Groups in the "Member Of" tab. I am able to remove all MemberOf for mail contact. How can I improve the speed of the script?

Get-MailContact <name> | ForEach-Object { Get-ADObject $_.distinguishedname -Properties memberof } | Remove-ADPrincipalGroupMembership -Identity $_ -MemberOf $_.memberof

This seems pretty optimal to me given how simple it is, only a question, why the foreach? Do you expect Get-MailContact to return multiple contacts ?
I don't have an AD I can test it against, but I'd be curious to know if "reversing" the operation by removing the member from the groups instead of removing the membership from the user could make it faster:
$ADContact = Get-MailContact <name> | Get-ADObject -Properties memberof
# Loop through all the groups and remove the user from them.
foreach ($group in $ADContact.MemberOf) {Remove-ADGroupMember -Identity $group -Member $ADContact}


Array of hosted games I have an array which handles all the games being hosting on the server. Users can host games and cancel the host as they please. What I have below seems to work, but I'm not sure if it adds the object to the lowest free index, but it adds them and removes just fine. The code is a bit of a mess so I've commented a lot to help you understand.

var hostingGames = [];

server.em.addListener('hostGame', function(settings)
{
    if (hostingGames.length != 0) //If the array that contains all the games being hosted, contains at least 1 game
        {
            for (i in hostingGames)//Scroll through every game being hosted
            {
                if(hostingGames[i] != null)//If the current index in the array does contain a game being hosted
                {
                    if(hostingGames[i].userId === settings.userId)//If the user thats trying to host a game, is already hosting a game
                    {
                        break; //Then break
                    }
                    else if (i == (hostingGames.length - 1))//If hes not hosting a game and all the games have finished being scrolled through
                    {
                        hostGame(settings, i + 1); //Then host one in the next avaliable space
                    }
                }
                else //If the game is null
                {
                    for(var i = 0; i < hostingGames.length; i++) //Scroll through the list of the games being hosted again
                    {
                        if(hostingGames[i] === null) //If theres a free slot
                        {
                            hostGame(settings, i); //Host a game
                        }
                    }
                }
            }
        }
        else
        {
            hostGame(settings, 0); //If there is no games currently being hosted, host one
        }

    function hostGame (settings, index)
    {
        var hostGame = new HostGame();

        hostGame.initialise(settings.userId, settings.userName, settings.boardSize, settings.gameMode, settings.gameNote);
        hostingGames.splice(index, 0, hostGame);
        server.updateGamesList(hostingGames);

        server.consoleLog('APP', settings.userName + ' is hosting a game. ID: ' + settings.userId);
    }
});

server.em.addListener('cancelHostGame', function(userId, userName)
{
    for (i in hostingGames)//For every game
    {
        if (hostingGames[i] != null) //If its not null
        {
            if (hostingGames[i].userId === userId) //If that user is hosting a game
            {
                hostingGames.splice(i, 1); //Remove their game
                server.updateGamesList(hostingGames);

                server.consoleLog('APP', userName + ' has stopped hosting a game. ID: ' + userId);
            }
        }
    }
});

Same dude, same code, same review (except I noticed hostingGames is not an object like I assumed in the first answer).

Never use for-in on arrays. You risk looping through properties in addition to the array items. Use a regular for loop with indexes, or better, array methods.

And when splicing an array multiple times, it's best you do it in reverse. You risk skipping items when you do it forwards.

Also, it's best not to leave any holes in the array. From a developer that knows nothing about the context of your app, why are they empty? Especially when cancelHostGame splices cancelled games?

server.em.addListener('cancelHostGame', function(userId, userName){
  for(let i = hostingGames.length; i--;){
    let game = hostingGames[i];
    if(!game || game.userId !== userId) continue;
    hostingGames.splice(i, 1);
  }
});

Now the first part of your code is somewhat different from the first review. But the advice from the first review still applies.

However, you've introduced the concept of "slots". But in reality, all your code does is limiting the number of hosted games. Slots is just an abstract way of putting it. A much easier approach is to just check if the hosted array length is at the limit. If not at the limit, just push to the array.

server.em.addListener('hostGame', function(settings){

  const slotsMaxed = hostingGames.length === MAX_SLOTS;
  if(slotsMaxed) return;

  const userHasGame = hostingGames.map(g => g.userId).includes(settings.userId);
  if(userHasGame) return;

  const hostGame = new HostGame();
  hostGame.initialise(settings.userId, settings.userName, settings.boardSize, settings.gameMode, settings.gameNote);
  hostingGames.push(hostGame);
  server.updateGamesList(hostingGames);

  server.consoleLog('APP', `${settings.userName} is hosting a game. ID: ${settings.userId}`);
});


Popping from a list in state while a condition is true I'm dealing with data that stores its state as a String, treating the string like a stack. I also have to combine that with error handling.

To that end, I'm using the type StateT String Maybe a. I have a function to pop and to push a Char from and to the string:

pop :: StateT String Maybe Char
pop = do
    x:xs <- get
    put xs
    return x

push :: Char -> StateT String Maybe ()
push x = do
    xs <- get
    put (x:xs)
    return ()

I wrote a function to repeatedly pop from the string while the characters being popped fulfilled a condition. It behaves as follows:

> runStateT (popWhile (<'a')) "HELLO world"
Just ("HELLO ","world")

> runStateT (popWhile (>'a')) "HELLO world"
Just ("","HELLO world")

My implementation is the following:

popWhile :: (Char -> Bool) -> StateT String Maybe [Char]
popWhile f = do
    s <- get
    if null s
        then return []
        else popAgain

    where
        popAgain = do
            x <- pop
            if f x
                then liftM (x:) (popWhile f)
                else push x >> return []

But that seems pretty bulky, and has two if then else's in it. Is there a better way to write this function?

You can simplify the code by using span:

  span :: (a -> Bool) -> [a] -> ([a], [a])

  span, applied to a predicate p and a list xs, returns a tuple where
  first element is longest prefix (possibly empty) of xs of elements
  that satisfy p and second element is the remainder of the list

popWhile :: (Char -> Bool) -> StateT String Maybe String
popWhile p = do
    s <- get
    let (xs, ys) = span p s
    put ys
    return xs

Thanks to @bisserlis for the suggestion to use state

popWhile = state . span


Executing certain code from a spreadsheet Here is some code which loops through an area in a spreadsheet and executes the code based on the condition that the source cells do not contain the value "(blank)".

The code works, but it's very inefficient to run nested if statements in this manner.  I've tried to have a go at making it more efficient in the long run but I'm out of ideas.

Any suggestions?

Sub NestedIfStatement()
Dim lastrow1 As Long
Dim I As Integer, J As Integer, N As Integer, MaxPriority as Integer

N = 3
Set Maxnumber = WS1.Range("A" & I & ":A" & lastrow1)
    MaxPriority = Application.Max(Maxnumber)

For J = 1 To lastrow1
    If WS1.Cells(J, 1) <= MaxPriority Then
       If WS1.Cells(J, 6).Value <> "(blank)" Then
          WS3.Cells(N, 7).Value = WS1.Cells(J, 6).Value
       End If
       If WS1.Cells(J, 5).Value <> "(blank)" Then
          WS3.Cells(N, 6).Value = WS1.Cells(J, 5).Value
       End If
       If WS1.Cells(J, 4).Value <> "(blank)" Then
          WS3.Cells(N, 4).Value = WS1.Cells(J, 4).Value
       End If
       If WS1.Cells(J, 3).Value <> "(blank)" Then
          WS3.Cells(N, 3).Value = WS1.Cells(J, 3).Value
       End If
       If WS1.Cells(J, 2).Value <> "(blank)" Then
          WS3.Cells(N, 2).Value = WS1.Cells(J, 2).Value
       End If
       N = N + 1
    End If
Next J

End Sub

Suggestion #1: Use Option Explicit and/or use fewer global variables

Either MaxNumber, MaxPriority are never explicitly defined, which is a very bad idea and is why you should use Option Explicit, or they're Global Variables, which is redundant, because they're self-contained within this subroutine.

Suggestion #2: Don't work directly with worksheet ranges

Accessing a worksheet is slow. You don't notice because doing it once is almost-instant, but do it a thousand times and it starts adding up really fast.

Whenever I'm working with worksheet data, the first thing I do is read all of it into an Array:

Dim wsData As Variant
    wsData = Array()

Dim dataTable As Range
Set dataTable = "" '/ Define your worksheet range

    ReDim arrWsData(1 To Rows, 1 To columns)  '/ Same size as the range

    wsData = dataTable

Now you have a 2-dimensional array, with all your worksheet data laid out in exactly the same way as it was on the sheet. Excel can iterate over an Array orders of magnitude faster than over a range because it doesn't have to access the worksheet object every time.

You can print an array to a sheet by doing the same in reverse: Define your range to be the same size as your array. Range = Array.

Suggestion #3: Recursion

Assuming you put the range A1:F50 from WS1 in an array, you can now iterate over it like so:

Dim i As Long, j As Long, n As Long
Dim testValue As Variant

Dim printData As Variant
    printData = Array()

ReDim printData(1 To lastRow, 1 To 5)

For j = 1 To lastRow
    If wsData(j, 1) <= MaxPriority Then
        n = n + 1
        For i = 1 To 5
            testValue = wsData(j, i + 1)
            If testValue <> "(blank)" Then printData(j, i) = testValue
        Next i
    End If
Next j

you can now change the size of your data, or the progression of the sequence, or a whole host of other things by changing 1 or 2 numbers. And 20 lines of code just became 4.

You can now pick any range on WS3 and print your output data there.

I don't know why your output columns on WS3 Jump a column from 6 to 4, but you can always write specific parts of your array to specific parts of the output sheet if needed.

Suggestion #4: Meaningful Variable Names

What is WS1? What is WS3? I know they're worksheets, but which worksheets? They appear next to each other in multiple places in this subroutine. If you accidentally put a 1 instead of a 3 somewhere, you can't tell at a glance where you've gone wrong. Give your variables names

Even a simple wsInput, wsOutput would be so much better.

MaxNumber Sounds like it should be, well, a number. Except it's a range. variables should say what they are. Even if that means writing VariableThatHoldsThisThing, it'll save you so much time down the road in debugging and maintaining and less technical debt.

Here, I recommend something like dataTable<description> (I don't know the context of your data, so you can think of a much more succinct name yourself).

On the other side, MaxPriority is a good variable name. It holds a number which represents the maximum priority from some table of priority numbers.

Also NestedIfStatement is a terrible procedure name. It tells me nothing about what the procedure does. Procedure names should be of the form <DoThing> or Perfom<Action>On<Thing> or any larger combination thereof. If you can't describe your procedure in a (reasonably) short name, then it's a good sign that it's trying to do too much, and should be split into several smaller procedures.


Worksheet that lists a person's name I have a worksheet that lists a person's name (column A; many duplicate names are in this column) with associated data (columns B through G). I have code below that takes this list of a ~ 1000 rows and then:

Copies and pastes each row three times (to create four identical rows for each entry).
Loops through the now ~4000 rows and creates a new worksheet for each person.  As there are many duplicate names in column A this only creates a handful of new worksheets.

The thing is, it runs but runs quite slowly (and I receive the Excel not responding warning at times). Is there anything to clean this up to make it more efficient? And after this I run another macro to save the new worksheets to a new workbook. Would it be faster to do that with code here?

Sub Split_Data()

    Const NameCol = "A"
    Const HeaderRow = 1
    Const FirstRow = 2
    Dim SrcSheet As Worksheet
    Dim TrgSheet As Worksheet
    Dim SrcRow As Long
    Dim LastRow As Long
    Dim TrgRow As Long
    Dim person As String
    Dim lRow As Long
    Dim RepeatFactor As Variant

'Optimize Macro Speed
    Application.ScreenUpdating = False
    Application.EnableEvents = False
    Application.Calculation = xlCalculationManual

'Copy each row and paste three times
    lRow = 2
    Do While (Cells(lRow, "B") <> "")
        RepeatFactor = 4

        Range(Cells(lRow, "A"), Cells(lRow, "G")).Copy
        Range(Cells(lRow + 1, "A"), Cells(lRow + RepeatFactor - 1, "G")).Select

        Selection.Insert Shift:=xlDown
        lRow = lRow + RepeatFactor - 1
        lRow = lRow + 1
    Loop

    Set SrcSheet = ActiveSheet
    LastRow = SrcSheet.Cells(SrcSheet.Rows.Count, NameCol).End(xlUp).Row
    For SrcRow = FirstRow To LastRow
        person = SrcSheet.Cells(SrcRow, NameCol).Value
        Set TrgSheet = Nothing
        On Error Resume Next
        Set TrgSheet = Worksheets(person)
        On Error GoTo 0
        If TrgSheet Is Nothing Then
            Set TrgSheet = Worksheets.Add(After:=Worksheets(Worksheets.Count))
            TrgSheet.Name = person
            SrcSheet.Rows(HeaderRow).Copy Destination:=TrgSheet.Rows(HeaderRow)
        End If
        TrgRow = TrgSheet.Cells(TrgSheet.Rows.Count, NameCol).End(xlUp).Row + 1
        SrcSheet.Rows(SrcRow).Copy Destination:=TrgSheet.Rows(TrgRow)
    Next SrcRow

ResetSettings:
  'Reset Macro Optimization Settings
    Application.EnableEvents = True
    Application.Calculation = xlCalculationAutomatic
    Application.ScreenUpdating = True

End Sub

Here are my thoughts ( I'm using a tablet and so cannot test my suggestions). You are already turning off screen updating and calculations which is good.

Avoid using Select where possible - there are plenty of examples of this.
The Insert in your code is probably a very slow process because Excel does a lot of work. Why not simply copy the entire data in the columns you want and then paste four times onto another sheet? No need for the Insert or the 1,000 copy & pastes.
Testing each row for the person's name to see if a worksheet exists and then copying just that row can also be changed. Why not sort the data by name, then run down the data keeping track of start row of current name and looking to see when the name changes? Then you copy the data from the start row up to the row before the current one. That way you could do just one copy & paste per name into the named worksheet.


Making basic physics object class I'm creating a 2D game in JS, and I've made the generic class that all objects in the game will be (I've decided they are all circular for now).

An object has motion and angMotion array properties which contain the 0th, 1st and 2nd derivatives of the object's (angular) motion.

I've also introduced aliases for each of these derivatives (like acc for acceleration, etc).

Upon initialization, the default for all derivatives should be 0.

I'm satisfied with how the class works as it is now as long as it remains 2 dimensional and I don't need higher derivatives of (angular) motion.

However, I feel that my code is repetitive in some places which would require changes in multiple places supposing I want to change how it works slightly.

Eg. I've considered the possibility of wanting to include higher derivatives of motion (without introducing more aliases like jerk) or changing the dimension I'm working in (ignore angular motion in this case).

Note: I wanted the derivatives of (angular) motion to be stored in arrays so that I can loop over them to simulate Physics using Taylor's Expansion as shown below. The simulatePhysics function works regardless of the number of derivatives or dimension.

I'm also wondering from a design point of view - would it be a better idea to put simulatePhysics as a method of the game instead of the object?

class Game2DObject {
  constructor(options) {

    if (options) {

      this.motion = options.motion || []
      this.angMotion = options.angMotion || []

      this.dis = options.dis || [0, 0]
      this.vel = options.vel || [0, 0]
      this.acc = options.acc || [0, 0]

      this.angDis = options.angDis || 0
      this.angVel = options.angVel || 0
      this.angAcc = options.angAcc || 0

      this.radius = options.radius || 0
      this.mass = options.mass || 0

    } else {

      this.motion = [[0, 0], [0, 0], [0, 0]]
      this.angMotion = [0, 0, 0]
      this.radius = 0
      this.mass = 0
    }
  }
  //motion
  get dis() {
    return this.motion[0]
  }

  set dis(x) {
    this.motion[0] = x
  }

  get vel() {
    return this.motion[1]
  }

  set vel(x) {
    this.motion[1] = x
  }

  get acc() {
    return this.motion[2]
  }

  set acc(x) {
    this.motion[2] = x
  }
  //angMotion
  get angDis() {
    return this.angMotion[0]
  }

  set angDis(x) {
    this.angMotion[0] = x
  }

  get angVel() {
    return this.angMotion[1]
  }

  set angVel(x) {
    this.angMotion[1] = x
  }

  get angAcc() {
    return this.angMotion[2]
  }

  set angAcc(x) {
    this.angMotion[2] = x
  }

  simulatePhysics(dt) {
    function taylor(arr) {
      return arr.reduceRight((acc, cur, i) => acc * dt / (i + 1) + cur, 0)
    }

    for (var dim = 0; dim < this.motion[0].length; dim++) {
      for (var deriv = 0; deriv < this.motion.length; deriv++) {
        this.motion[deriv][dim] = taylor(this.motion.slice(deriv).map((cur) => cur[dim]))
      }
    }

    for (var deriv = 0; deriv < this.angMotion.length; deriv++) {
      this.angMotion[deriv] = taylor(this.angMotion.slice(deriv))
    }
  }
}

//*DEBUG

var a = new Game2DObject({acc: [-6, 0], angVel: 9})
console.log(a)
console.log(a.dis[0] = 1)
console.log(a.dis[1] = 5)
console.log(a.acc[1] = -2)
console.log(a)

a.simulatePhysics(1)
console.log(a)
a.simulatePhysics(1)
console.log(a)
a.simulatePhysics(1)
console.log(a)

Output:

Poor performance

For games performance dominates all levels of design but becomes critical as you approach the low level building blocks of the game. Every increase in performance opens up your market, there are far more low end devices than high end with a 20% performance increase doubling the number of devices that can run the game at acceptable levels.

If you are not interested in marketing and its just for fun, then you can solve performance problems with hardware and ignore this answer.

Some points on performance

Avoid getters and setters as they are slower than direct property access.
Avoid array iteration for small arrays. Inline the operations.
Be memory efficient, pre-allocate, reuse rather than deference (delete)
Use small generic objects to cover a wide range of uses, rather than many complex targeted objects

Array.slice and Array.map require allocation and GC overhead.
Array iterators require closure, counters, and logic overheads. For large arrays the overheads become insignificant, but for small arrays 2,3 items long the overhead becomes a significant part of the processing.

One of the biggest overheads of using array iteration is that the coder hides a large number of redundant operations because the iterators obscure the expanded calculations.

Needless CPU wastage

Lets analyze and simplify the function simulatePhysics as it is very inefficient with a lot of repeated calculations, or completely unneeded calculations. Every time it is called it create a lot of garbage for GC to clean up (15 new Array objects)

  simulatePhysics(dt) {
    function taylor(arr) {
      return arr.reduceRight((acc, cur, i) => acc * dt / (i + 1) + cur, 0)
    }

    for (var dim = 0; dim < this.motion[0].length; dim++) {
      for (var deriv = 0; deriv < this.motion.length; deriv++) {
        this.motion[deriv][dim] = taylor(this.motion.slice(deriv).map((cur) => cur[dim]))
      }
    }

    for (var deriv = 0; deriv < this.angMotion.length; deriv++) {
      this.angMotion[deriv] = taylor(this.angMotion.slice(deriv))
    }
  }

This function requires the creation of 15 new arrays, 27 call stack push and pops, 30 temp variable, and 108 math operations.

It can be significantly reduced to no new arrays, no call stack overhead, 6 temp variable, and 19 math operations.

Reducing simulatePhysics

The steps I used to simplify the function and remove unneeded overhead.

1 First to remove all the coding noise we can create some aliases for the names.

 const m = this.motion;
 const aM = this.angMotion;

2 Inline the 2 loops removing all the iterators slice and map overhead.

 m[0][0] = taylor([m[0][0], m[1][0], m[2][0]])
 m[1][0] = taylor([m[1][0], m[2][0]])
 m[2][0] = taylor([m[2][0]])
 m[0][1] = taylor([m[0][1], m[1][1], m[2][1]])
 m[1][1] = taylor([m[1][1], m[2][1]])
 m[2][1] = taylor([m[2][1]])

 aM[0] = taylor([aM[0], aM[1], aM[2]])
 aM[1] = taylor([aM[1], aM[2]])
 aM[2] = taylor([aM[2]])

3 Replace the taylor function with inlined calculations

  m[0][0] = ((0 * dt / 3 + m[2][0]) * dt / 2 + m[1][0]) * dt / 1 + m[0][0]
  m[1][0] = (0 * dt / 2 + m[2][0]) * dt / 1 + m[1][0]
  m[2][0] = 0 * dt / 1 + m[2][0]
  m[0][1] = ((0 * dt / 3 + m[2][1]) * dt / 2 + m[1][1]) * dt / 1 + m[0][1]
  m[1][1] = (0 * dt / 2 + m[2][1]) * dt / 1 + m[1][1]
  m[2][1] = 0 * dt / 1 + m[2][1]

  aM[0] = ((0 * dt / 3 + aM[2]) * dt / 2 + aM[1]) * dt / 1 + aM[0]
  aM[1] = (0 * dt / 2 + aM[2]) * dt / 1 + aM[1]
  aM[2] = 0 * dt / 1 + aM[2]

4 Remove the multiply by zero and divide by ones

  m[0][0] = (m[2][0] * dt / 2 + m[1][0]) * dt + m[0][0]
  m[1][0] = m[2][0] * dt + m[1][0]
  m[2][0] = m[2][0]
  m[0][1] = (m[2][1] * dt / 2 + m[1][1]) * dt + m[0][1]
  m[1][1] = m[2][1] * dt + m[1][1]
  m[2][1] = m[2][1]

  aM[0] = (aM[2] * dt / 2 + aM[1]) * dt + aM[0]
  aM[1] = aM[2] * dt + aM[1]
  aM[2] = aM[2]

5 Cache and substitute with constants, remove unneeded assignments.

 const dt2 = dt / 2;

 m[0][0] = (m[2][0] * dt2 + m[1][0]) * dt + m[0][0]
 m[1][0] = m[2][0] * dt + m[1][0]
 m[0][1] = (m[2][1] * dt2 + m[1][1]) * dt + m[0][1]
 m[1][1] = m[2][1] * dt + m[1][1]

 aM[0] = (aM[2] * dt2 + aM[1]) * dt + aM[0]
 aM[1] = aM[2] * dt + aM[1]

6 More aliases to reduce indexing overhead and rebuild the function

simulatePhysics(dt) {
    const m = this.motion, m0 = m[0], m1 = m[1], m2 = m[2];
    const aM = this.angMotion;
    const dt2 = dt / 2;

    m0[0] = (m2[0] * dt2 + m1[0]) * dt + m0[0];
    m1[0] =  m2[0] * dt  + m1[0];
    m0[1] = (m2[1] * dt2 + m1[1]) * dt + m0[1];
    m1[1] =  m2[1] * dt  + m1[1];
    aM[0] = (aM[2] * dt2 + aM[1]) * dt + aM[0];
    aM[1] =  aM[2] * dt  + aM[1];

}

Whats the gain

Arguably there is some loss in readability, personally it makes a lot more sense than a collection of arrays iterators and copying. But the gain is (tested on chrome) huge.

// tested on random set of 1000 objects
// µs is 1/1,000,000th second. OPS is operations per second.
// An operation is a single call to the function being tested.
Optimized..: MeanTime 0.175µs OPS 5,710,020 Test Total   322ms 1,836,000 operations
Original...: MeanTime 4.198µs OPS   238,185 Test Total 6,566ms 1,564,000 operations

The optimized version is 25 times faster, able to do 5.7million operations in the same time as the original could do 0.3million

Is readability more important than performance? for games you must seriously consider what you lose via traditional coding styles.


Idiomatic ruby to calculate distance to points on a graph Can this code be made more like idiomatic ruby using methods such as map or inject?

@random_tour is a variable length array of points on a graph:
[[0, 0], [2, 1], [4, 3]]

I want to calculate the distance to reach each point using the manhattan distance (number of moves horizontally + number of moves vertically).

For example, starting at [0, 0] - the next point [2, 1] is 2 moves horizontally, 1 move vertically which is 3 moves. The next point is [4, 3] which is 4 moves away from the last. Total number of moves is 7.

def cost
  i = 0
  total_cost = 0
  while i < @random_tour.length - 1 do
    total_cost += (@random_tour[i + 1][0] - @random_tour[i][0]).abs + (@random_tour[i + 1][1] - @random_tour[i][1]).abs
    i += 1
  end
  total_cost
end

Enumerable.each_cons(n) will pass the array items in groups of n, using a sliding window.  So [pt1, pt2], [pt2, pt3], [pt3, pt4], etc.

After that you can use inject to calculate the distance and sum the points.

def manhattan(pt1, pt2)
  (pt1[0]-pt2[0]).abs + (pt1[1]-pt2[1]).abs
end

def cost
  @random_tour
    .each_cons(2)
    .inject(0) { |sum, pts| sum + manhattan(pts[0], pts[1]) }
end


Algorithm to group connected graphs I am working on an algorithm, which should group connected Arcs. The situation is described via code examples below.

I have Arcs:

public class Arc
{
    public Point StartPoint { get; set; }
    public Point EndPoint { get; set; }
    public bool Visited { get; set; }
    public Arc Successor { get; set; }
    public Arc Predecessor { get; set; }
}

that consist of Points:

public class Point
{
    public int X { get; set; }
    public int Y { get; set; }

    public override bool Equals(Object obj)
    {
        if (ReferenceEquals(null, obj)) return false;
        if (ReferenceEquals(this, obj)) return true;
        return obj.GetType() == GetType() && Equals((Point)obj);
    }

    protected bool Equals(Point other)
    {
        return X.Equals(other.X) && Y.Equals(other.Y);
    }

    public override int GetHashCode()
    {
        unchecked
        {
        return (X.GetHashCode() * 397) ^ Y.GetHashCode();
        }
    }
}

Given these Points and resulting Arcs:

var p1 = new Point {X = 0, Y = 0};
var p2 = new Point { X = 0, Y = 1 };
var p3 = new Point { X = 0, Y = 2 };
var p4 = new Point { X = 1, Y = 0 };
var p5 = new Point { X = 1, Y = 1 };

var arc1 = new Arc { StartPoint = p1, EndPoint = p2};
var arc2 = new Arc { StartPoint = p4, EndPoint = p5};
var arc3 = new Arc { StartPoint = p2, EndPoint = p3};

I would like to end up with groups of connected Arcs as a list of list:

var arcListOfList = new List<List<Arc>>();

This is the crude algorithm I came up with so far:

foreach (var arc in arcList)
{
    var successor = arcList.FirstOrDefault(x => x.StartPoint.Equals(arc.EndPoint));
    var predecessor = arcList.FirstOrDefault(x => x.EndPoint.Equals(arc.StartPoint));
    arc.Successor = successor;
    arc.Predecessor = predecessor;
}

Arc next;
do
{
    next = arcList.FirstOrDefault(x => x.Visited == false);
    var temp = new List<Arc>();

    if (next == null) continue;
    do
    {
        next.Visited = true;
        temp.Add(next);
        if (next.Successor != null)
        {
        next = next.Successor;
        }
        else
        {
        break;
        }
    } while (true);

    arcListOfList.Add(temp);
} while (next != null);

I would appreciate any help/suggestions to improve it.

I'm going to leave off suggesting anything about the algorithm for now as I don't fully understand what you want from it. Instead, I'm going to talk about your Point class in laborious depth...

public class Point
{
    public int X { get; set; }
    public int Y { get; set; }

I would argue that a point should be immutable - i.e. it doesn't change after you've created it. I would implement with full readonly properties:

private readonly int x;
public int X { get { return x; } }

private readonly int y;
public int Y { get { return y; } }

public Point(int x, int y)
{
   this.x = x;
   this.y = y
}

However, most people would let you off with private set auto properties:

public int X { get; private set; }

Let's look at your Equals implementations

public override bool Equals(Object obj)
{
    if (ReferenceEquals(null, obj)) return false;
    if (ReferenceEquals(this, obj)) return true;
    return obj.GetType() == GetType() && Equals((Point)obj);
}

protected bool Equals(Point other)
{
    return X.Equals(other.X) && Y.Equals(other.Y);
}

Firstly, bool Equals(Point other) -> that's IEquatable<Point> if made public so your class should implement that:

public class Point : IEquatable<Point>

Now, simplify the object.Equals override:

public override bool Equals(Object obj)
{
    return Equals(obj as Point);
}

And sharpen up the IEquatable<Point>.Equals:

public bool Equals(Point other)
{
    return other != null
            && other.X == X
            && other.Y == Y;
}

We should also override the == and != operators so that we're sure we are consistent with out equality:

public static bool operator ==(Point left, Point right)
{
    if (object.ReferenceEquals(left, null))
    {
        return object.ReferenceEquals(right, null);
    }

    return left.Equals(right);
}

public static bool operator !=(Point left, Point right)
{
    if (object.ReferenceEquals(left, null))
    {
        return !object.ReferenceEquals(right, null);
    }

    return !left.Equals(right);
}

Then you have a nice immutable class for your Point with consistent equality behaviour... Phew!

Edit

As EBrown pointed out in the comments, Point is a perfect fit for being a struct. It's less than 16 bytes, immutable and (0, 0) is a perfectly valid value (i.e. unitialized).


Secure PHP login with SQL injection prevention I am working on a project where users have to log in then if it is correct it creates a cookie to process requests. I would like to find a way to auto-renew sessions every day without affecting the user or requiring the user to log in again.

I would like to also know if I overlooked any security flaws such as SQL injection or session-hijacking and all those, are there any other one that I could prevent or improve?

The login page is

<?php
//Login.php
if(isset($_POST['username']) && isset($_POST['password'])){
    $username = filter_input(INPUT_POST, "username", FILTER_SANITIZE_ENCODED, FILTER_FLAG_STRIP_HIGH); //Changes to unicode characters to prevent injection
    $password = $_POST['password']; //Get password

    $result = LoginManager::Login($username, $password); //Run login method

    if($result == "invalid_username"){
        echo 'Invalid Username!';
    }

    if($result == "invalid_login"){
        echo 'Invalid Login!';
    }

    $expires = gmdate('Y-m-d', strtotime($Date. ' + 1 days')); //sessions expire in 1 day
    DataBase::query("INSERT INTO sessions VALUES(:session, :userid, :expires)", array(':session' => $result, ':userid' => $userid, ':expires' => $expires)); //insert into database

    setcookie("session", $result); //set as cookie
    return;
}

echo '<form action="Login.php" method="post">

Username: <input type="text" name="username" placeholder="Username"/>

Password: <input type="password" name="password" placeholder="Password"/>

<input type="submit" name="login" value="Login"/>

    </form>';
?>

The LoginManager class

<?php
//LoginManager.php
public static function Login($username, $password){
    if(strlen($username) > 16 || strlen($password) > 50 || strlen($password) < 5){ //usernames less than 16, password from 5-50 char
        return 'invalid_bounds';
    }

    $sql = "SELECT * from accounts WHERE username = :username";
    $users = Database::query($sql, array(':username' => $username)); //get account

    if(sizeof($users) == 0){ //no accounts found
        return 'invalid_username';
    }

    $userdata = $users[0];

    $salt = '-45dfeHK/__yu349@-/klF21-1_\/4JkUP/4'; //salt for password
    if(password_verify($password . $salt, $userdata['password'])){ //check if password is correct
        $session = com_create_guid(); //create unique session
        return $session;
    }else{
        return 'invalid_login';
    }
}

?>

And the main page / feed

<?php
//Main.php
$session = $_COOKIE['session']; //get cookie session

$sessions = DataBase::query('SELECT * FROM sessions WHERE id=:id', array(':id' => $session));

if(sizeof($sessions) == 0){
    //Redirect to login page
    return;
}

//get first session
$session = $sessions[0];

$date = new DateTime($session['expires']);
$now = new DateTime();

if($date < $now) {
    //Expired session : Redirect to login
    return;
}

//Display feed

?>

Magic variables

You are using invalid_username, invalid_login and some more magic names that are directly related to the login procedure. Factor those out and put them as static properties on your LoginManager class, so the actual contents are irrelevant and you can simply check against LoginManager::INVALID_LOGIN to see if that is the state of the session.

Factor out your session management

Creation and renewing of sessions should happen in their own class. Other code doesn't need to know how to make sessions, just that a session exists and who is authenticated by that session.

Factor out the salt

Move the salt to a config file somewhere. You don't want to have to dig through your code when you have to change the salt. If you do not want to have a config file, at least put it as a static or private variable in your LoginManager.

Odd sessions

You are making a session even if the login was invalid. Later you only check if a session exists, not if that session was valid. This allows someone to log in by entering a wrong username, or a right username with a wrong password.

Your LoginManager should also not disqualify usernames or passwords based on length, as this is a thing that should be enforced when setting a password or username. The login will always fail in those cases, so don't worry about it. You want to do the same amount of work for every login attempt to prevent an attacker gathering information about valid usernames/passwords based on the time it takes for a request to return.

Timezones

You are creating a date with gmdate(..) which is a UTC date. Later you are parsing it with new DateTime(..) as a local date and comparing it with a local date. This causes sessions to last longer or shorter based on where the server is.

Undefined variables

You are making sessions where the $userid is undefined. This means you cannot do anything useful with the session, as you lose information about who is logged in.

You are also setting the expiration date with $Date, a misspelled variable that is not defined either.

Queries can return false

You should always keep in mind that queries may fail. If your username query fails, it will return false, and sizeof(false) === 1. Your code would then error out on $userdata['password'], which is not that great.

Expired sessions

You are currently not doing anything with your expired sessions. That means they are not removed from the database after they expired.

Various

You specifically asked about sql injection and session hijacking. I believe your code to be reasonably safe on both accounts. If you are using truely prepared statements (not a setting that uses sprintf behind the scenes), you should be safe from sql injection. Your queries may still fail, but you should not be able to execute unintended sql queries.

Your sessions are based around com_create_guid, which I believe is cryptographically secure. The generated string is also long enough that guessing a session guid is practically impossible. You are making sessions less secure by essentially wanting a session to last forever, increasing the time they could be used by some malicious third party. I do not believe this to be a great risk in this case though.


Compute leaderboard of competitive riders throughout a season I have a table in my DB that contains Riders as rows and their points throughout the season as columns. Here are the columns:

['name', 'wwq_id', 'wwq_code', 'year', 'round1_qualy', 'round1_final',
'round1_timed', 'round1_total_qualy', 'round1_total_qualy_position',
'round1_total_final', 'round1_total_final_position', 'round1_total_timed',
'round1_total_timed_position', 'round1_total', 'round1_total_position'...etc

The 'round1_qualy', 'round1_final', 'round1_timed' all come from different tables and are added using another function. Once they are in the DB all of total columns are calculated using generated columns in MySQL.

I need to calculate the position of the rider at different points in the season. I.E. After round one qualy, after round two final, etc.

Here is the code I have right now:

def set_positions():
    """Add positions to the DB for each column in our overall table"""
    engine = sql.create_engine(MYSQL_CON_STRING)
    connection = engine.connect()
    trans = connection.begin()

    # Men, Women, Junior
    for category in CATEGORIES:
        table = category + '_overall'
        df = pd.read_sql('SELECT * FROM {0}'.format(table), engine)
        points_columns_list = [col for col in df.columns
                               if 'total' in col and 'position' not in col]

        # Calculate the rank based on current total points column and
        # insert into current column index + 1
        for points_column in points_columns_list:
            position_index = df.columns.get_loc(points_column) + 1
            position_column = df.columns[position_index]
            current_points = df.groupby('year')
            position = \
                current_points[points_column].rank(ascending=0,
                                                   method='min').astype(int)
            df.loc[:, position_column] = position.values

            # No points awarded in this and/or previous rounds. Everyone is
            # at 0 so everyone is rank 1
            df.loc[(df[points_column] == 0)
                   & (df[position_column] == 1), position_column] = 'NULL'

            # Add rows to the DB
            for row in df.itertuples():
                connection.execute('UPDATE {0} '
                                   'SET {1} = {2} '
                                   'WHERE year = {3} and name = "{4}"'
                                   ''.format(table, position_column,
                                             getattr(row, position_column),
                                             getattr(row, 'year'),
                                             getattr(row, 'name')))
        trans.commit()
    connection.close()

The code works perfectly, it just takes a long time. This function doesn't get executed very often, but considering there are about 8000 rows per table, it takes a bit of time.

Is there any way to get the complexity down?

You should strive to reduce the number of queries to your DB. Instead of updating the whole table, row by row, each time you compute a new value, you could:

load the data (using pd.read_sql_table);
perform the computation of the positions (this may require you to use None or pd.np.NaN instead of 'NULL');
write the whole new table at once at the end (using df.to_sql).

The rewrite could look like:

def set_positions():
    """Add positions to the DB for each column in our overall table"""
    engine = sql.create_engine(MYSQL_CON_STRING)
    connection = engine.connect()
    trans = connection.begin()

    # Men, Women, Junior
    for category in CATEGORIES:
        table = category + '_overall'
        df = pd.read_sql_table(table, engine)
        compute_positions_for_table(df)
        df.to_sql(table, engine, if_exists='replace')
        trans.commit()
    connection.close()

def compute_positions_for_table(df):
    points_columns_list = [col for col in df.columns
                           if 'total' in col and 'position' not in col]

    # Calculate the rank based on current total points column and
    # insert into current column index + 1
    for points_column in points_columns_list:
        position_index = df.columns.get_loc(points_column) + 1
        position_column = df.columns[position_index]
        current_points = df.groupby('year')
        position = current_points[points_column].rank(ascending=0, method='min').astype(int)
        df.loc[:, position_column] = position.values

        # No points awarded in this and/or previous rounds. Everyone is
        # at 0 so everyone is rank 1
        df.loc[(df[points_column] == 0)
               & (df[position_column] == 1), position_column] = None

And even if you can't "replace" the table, you should still update the table only once at the end of the computation rather than at each new column computation.


Sums for list of ranges (CodeFights Basic Skills) I can solve this problem with a brute force naive solution, but need to optimize it for time. I'm not in school, but trying to learn fundamentals on my own.

I know I need to store the sum of the indices already counted so that I am not recounting ranges I've already covered. Like in the example below if I already summed [0,2] and [2, 5] I could just add those sums to get [0, 5] sum without iterating over array. But I don't know how to implement this.

Here is the description:

  You have an array of integers nums and an array queries, where
  queries[i] is a pair of indices (0-based). Find the sum of the
  elements in nums from the indices at queries[i][0] to queries[i][1]
  (inclusive) for each query, then add all of the sums for all the
  queries together. Return that number modulo 10^9 + 7.

Example:

  For nums = [3, 0, -2, 6, -3, 2] and queries = [[0, 2], [2, 5], [0,
  5]], the output should be sumInRange(nums, queries) = 10.

  The array of results for queries is [1, 3, 6], so the answer is 1 + 3
  + 6 = 10.

My solution:

func sumInRange(nums: [Int], queries: [[Int]]) -> Int {

    var sumArray = [Int]()

    for q in queries {

        var tempSum = 0

        for i in q[0]...q[1] {

            tempSum += nums[i]
        }

        sumArray += [tempSum]
    }

    let sum = sumArray.reduce(0, +)
    let bigNumber = 1000000000 + 7

    return sum > 0 ? sum % bigNumber : bigNumber + sum
}

First let's simplify your existing code.

You already use reduce() to add the elements of sumArray, the same
can be done to replace

    var tempSum = 0
    for i in q[0]...q[1] {
        tempSum += nums[i]
    }

by

let tempSum = nums[q[0]...q[1]].reduce(0, +)

Here an "array slice" is created and then reduced. Note that this
does not duplicate the element storage.

Instead of appending a single-element array

   sumArray += [tempSum]

you can append a single element:

sumArray.append(tempSum)

Each element sumArray is the result of applying one query
to the given numbers, this can be simpler done as a map
operation:

let sumArray = queries.map { q in
    nums[q[0]...q[1]].reduce(0, +)
}

I would define the modulus \$ 10^9+7\$ as a constant (interspersed
with _ for better readability):

let modulus = 1_000_000_000 + 7

There is one problem at your

return sum > 0 ? sum % bigNumber : bigNumber + sum

which becomes apparent only with large input: A negative sum must
also be reduced modulo \$ 10^9+7\$, before adding the modulus
to make it non-negative.

Putting it together, your code would look like this:

func sumInRange(nums: [Int], queries: [[Int]]) -> Int {

    let sumArray = queries.map { q in
        nums[q[0]...q[1]].reduce(0, +)
    }

    let sum = sumArray.reduce(0, +)

    let result = sum % modulus
    return result >= 0 ? result : result + modulus
}

which is probably not faster, but simpler and cleaner ("Swiftier") code.

In order to pass the coding challenge in the given time, you need
a different algorithm. The idea is (and to be honest, I did not invent
this myself but found it here):

First create hashes (dictionaries) which associate with every
index the number of queries starting (resp. ending) at this index.
Then traverse the nums array once, keeping track of a
"multiplier" which indicates how often the number at the current index
occurs in the queries, and accumulate the sum.

An implementation in Swift could look like this:

func sumInRange(nums: [Int], queries: [[Int]]) -> Int {
    var startIndices: [Int: Int] = [:]
    var endIndices: [Int: Int] = [:]

    for q in queries {
        startIndices[q[0], default: 0] += 1
        endIndices[q[1], default: 0] += 1
    }

    var multiplier = 0
    var sum = 0
    for (idx, num) in nums.enumerated() {
        multiplier += startIndices[idx] ?? 0
        sum += num * multiplier
        multiplier -= endIndices[idx] ?? 0
    }

    let result = sum % modulus
    return result >= 0 ? result : result + modulus
}

Further remarks:

You probably need to reduce modulo \$ 10^9+7\$ not only the
final sum but also the intermediate results, in order to avoid an
integer overflow.
A Swiftier way to represent pairs is to use tuples instead
of two-element arrays:

func sumInRange(nums: [Int], queries: [(from: Int, to: Int)]) -> Int


Learning OOP PHP, simple MySQL connection class. I have posted an earlier version of this and here is the improved version from the feedback I recieved. Some of the feedback I received was;

Don't chain method (tried my best to limit this)
Do not use print() or die() and an error response (still a little lost, but I did attempt to use a redirect to a custom error page)

I tried my best to rewrite the code, I am still very new to this so go easy. I did read up on OOP methodology and read about Interfacing and Implementation, so I tried to incorporate that in my class.

I would love to see some different ideas on how to make this the most efficient as possible. I am sure it needs a ton of changing, but that is why I am here to help me learn and grow. I am a visual learner so if possible actual code would be awesome, but any response would be greatly appreciated.

<?php
class Mysql
{
        private $user;
        private $pass;
        private $data;
        private $host;

        public function __construct($user,$pass,$data,$host)
        {
                $this->user = $user;
                $this->pass = $pass;
                $this->data = $data;
                $this->host = $host;
                $this->process();
        }

        /* INTERFACE */
        private function process()
        {
                if($this->verifyNullFields()==true)
                {
                        if($this->verifyDatabaseConnection()==true)
                        {
                                if($this->verifyDatabaseExist()==true)
                                {
                                        print('ALL PASSED'); //for debugging
                                }
                                else
                                {
                                        print('redirect to custom error page will go here');
                                }
                        }

                        else
                        {
                                  print('redirect to custom error page will go here');
                        }

                }
                else
                {
                        print('redirect to custom error page will go here');
                }
        }

        /* IMPLEMENTATIONS */
        private function verifyNullFields()
        {
                if($this->user != NULL)
                {
                        if($this->data != NULL)
                        {
                                if($this->host != NULL)
                                {
                                        return true;
                                }
                                else
                                {
                                        return false;
                                }

                        }
                        else
                        {
                                return false;
                        }
                }
                else
                {
                        return false;
                }
        }
        private function verifyDatabaseConnection()
        {
                $link = @mysql_connect($this->host,$this->user,$this->pass);
                if(!$link)
                {
                        return false ;
                }
                else
                {
                        return true;
                }
        }
        private function verifyDatabaseExist()
        {
                $db = @mysql_select_db($this->data);
                if(!$db)
                {
                        return false;
                }
                else
                {
                        return true;
                }
        }
}
?>
<?php
$m = new Mysql("root","","magic","localhost");
?>

Let me show my version of this code:

DatabaseException.php:

class DatabaseException extends Exception {
}

Database.php:

abstract class Database {
    protected $login;
    protected $password;
    protected $database;
    protected $hostname;

    public function __construct($login, $password, $database, $hostname) {
        // NB: password not checked and may be empty
        $this->throwExceptionIfNotSet('login', $login);
        $this->throwExceptionIfNotSet('database', $database);
        $this->throwExceptionIfNotSet('hostname', $hostname);

        $this->login    = $login;
        $this->password = $password;
        $this->database = $database;
        $this->hostname = $hostname;
    }

    private function throwExceptionIfNotSet($argName, $argValue) {
        if (empty($argValue)) {
            throw new DatabaseException("'${argName}' not set");
        }
    }

}

Mysql.php:

class Mysql extends Database {

    private $link = null;

    public function __construct($login, $password, $database, $hostname) {
        parent::__construct($login, $password, $database, $hostname);
        $this->connect();
        $this->selectDatabase();
    }

    public function connect() {
        if (! is_null($this->link)) {
            return;
        }

        $link = @mysql_connect($this->hostname, $this->login, $this->password);
        if (! $link) {
            throw new DatabaseException(
                sprintf(
                    'Cannot connect to database. mysql_connect() to %s with login %s fails',
                    $this->hostname,
                    $this->login
                )
            );
        }
    }

    public function selectDatabase() {
        $ret = @mysql_select_db($this->database, $this->link);
        if (! $ret) {
            throw new DatabaseException("Cannot select database {$this->database}");
        }
    }

}

application.php:

try {
    $db = new Mysql('root', '', 'magic', 'localhost');
    print('ALL PASSED'); //for debugging

} catch (DatabaseException $ex) {
    print('redirect to custom error page will go here');
}


Display related records, if they exist, in Rails view Is this much logic in view is justified to check for the presence of record?

# device model
class Device < ApplicationRecord
  validates :abbr, uniqueness: { case_sensitive: false }, presence: true
  validates :name, presence: true

  def abbr=(value)
    self[:abbr] = value.to_s.strip
  end
end

Here is the code for DeviceVendor that also saves abbr column of Device to uniquely identify the device:

# model code
class DeviceVendor < ApplicationRecord
  validates :vendor, uniqueness: {scope: :device}
end

# controller code
def index
  @device_vendors = DeviceVendor.order(:device, :vendor)
end

# view code
<% @device_vendors.each do |device_vendor| %>
  <% device = Device.where(abbr: device_vendor.device).first %>
  <% vendor = Vendor.where(abbr: device_vendor.vendor).first %>

    <%= device.name if device %>
    <%= vendor.name if vendor %>
    <%= link_to 'Show', device_vendor %>
    <%= link_to 'Edit', edit_device_vendor_path(device_vendor) %>

<% end %>

What is the rails way to do this?

There's a lot of ways to check the presence of an object, i prefer to use try :

device.try(&:name)


Dominion card game - how can I take advantage of interfaces? I'm writing a card game (Dominion) as a pet project. I'm new to C++ but not programming.
A player has a deck, containing the hand and cards in play (tableau). Outside the player, there are piles of cards to buy from (supply piles). I want to display these objects (the hand, tableau and supply piles) on the screen. I had chosen to represent the hand and tableau using std::vector<Card> and the supply piles as std::vector<SupplyPile>. Card and SupplyPile implement an interface for displaying contents to the screen
class IInfo {
public:
  virtual std::string Info() const = 0;
  virtual std::string ToString() const = 0;
};

I have a class I'm calling View that will take on displaying things to the screen.
class View {
public:
  View(const std::vector<IInfo*>& items, int window_starty, int window_startx);
  virtual ~View() { }

  const IInfo& CurrentItem() const;
  const int CurrentIndex() const;
  bool IsEmpty() const;
  void ItemDown();
  void ItemUp();
  void SetActive();
  void SetInactive();
  virtual void Update();
protected:
  virtual WINDOW* InitializeWindow(int lines,
    int cols,
    int starty,
    int startx);

  virtual ITEM** MakeMenuItems();
private:
  // Some constants

  const std::vector<IInfo*>& items_;

  MENU *menu_;
  WINDOW *window_;
};

I now know that C++ doesn't support covariance in templates, so I can't create instances of View with the supply pile, hand, and tableau. How can I redesign the system to maximize DRY? The View class operates on its container member in a very simple way, ripe for abstraction.
Examples of things I want to do
std::vector<Card> hand = player.hand();
std::vector<Card> tableau = player.tableau();
std::vector<SupplyPile> supply_piles = game.supply_piles();

View hand_view = new View(hand, starty, startx);
View tableau_view = new View(tableau, starty, startx);
View supply_piles_view = new View(supply_piles, starty, startx);

One answer suggested making View into a template, and it almost worked until I remembered that I want to track which View is active at a given time:
View active_ = hand_view;
// Later
active_ = supply_piles_view;

I can't do this with View<Card> and View<SupplyPile>.
Other code
class Card : public IInfo {
public:
  Card(std::string name,
    int cost,
    int initial_supply,
    std::string text,
    std::string type);
  ~Card();

  void Play();
  std::string Info() const;
  std::string ToString() const;

  int cost() const;
  int initial_supply() const;
  std::string name() const;
  std::string text() const;
private:
  int cost_;
  int initial_supply_;

  std::string name_;
  std::string text_;
  std::string type_;

  std::string set_;
};

class SupplyPile : public IInfo {

public:
    SupplyPile(const Card& card, int initial_count);
    SupplyPile(const SupplyPile& other);
    virtual ~SupplyPile();
    virtual bool operator==(const SupplyPile& other) const;

    bool BuyOrGain();
    std::string Info() const;
    std::string ToString() const;

    const Card& card() const;
    int count() const;
    std::string name() const;
private:
    const Card& card_;

    int count_;
};

Is there any reason why View itself can't be a template?

template <typename T> class View {
public:
  View(const std::vector<T>& items, int window_starty, int window_startx);
  virtual ~View() { }

  const T& CurrentItem() const;
  // ...
  const std::vector<T>& items_;
  // ...
};

View<Card> hand_view = new View<Card>(hand, starty, startx);


How can use array to imitate repeated individual instances? I'm working an example to list directory contents. When I process each directory the code works. Now I'd like to put both directories in an array and iterate the array. I get an error when I declare 'dir_c' because I've not given it a directory name which will change.

  def outfile = new File('Y:/TEMP/dirlist.txt')
  def dir_a = new File('z:/pathA/pathB')
  def dir_b = new File('z:/pathA/pathQ')
  // def dir_c = new File() //causes error ==>Could not find which method <init>(to invoke from this list
outfile.write('')

 dir_a.eachFile {
  if (it.isFile()) {
  outfile.append(it.name + '\n')
  }

  dir_b.eachFile {
   if (it.isFile()) {
    outfile.append(it.name + '\n')
   }
  // Up to this point code works

   def searchDir = ["z:/pathA/pathB","z:/pathA/pathQ"]
   searchDir.each{
  //was thinking using  ==>dir_c = it
  // then carry on like singleton cases
  // but I get error message when I declare dir_c
  }

Do you mean like this:

def searchDir = ["z:/pathA/pathB","z:/pathA/pathQ"]
searchDir.each { d ->
    new File( d ).eachFile {
        if (it.isFile()) {
            outfile.append( "$it.name\n" )
        }
    }
}


Compute a students grade based on weight of assignments Hoping to get feedback on how this program could be written in the most pythonic way. Also welcome any criticisms of styling as I know this is not in accordance with pep-8

from collections import namedtuple
a = namedtuple('assignment', 'weight scores')
def average(scores:list):
    '''average a list of scores'''
    return sum(scores)/len(scores)
def get_grade(grades:list):
    '''compute final grade based on scores and their weights'''
    result = 0
    for a in grades:
        result += average(a.scores) * a.weight
    return result
def main():
   grades = [a(.30, [18/45, 11/55]),
             a(.20, [3/10, 7.5/10, 10/10]),
             a(.10, [9/10, 9/10, 10/10, 10/10, 8/10,
                6/10, 10/10, 10/10]),
             a(.40, [29/65]),
             a(.01, [1/1])]
   print(get_grade(grades))
main()

from collections import namedtuple

You should put blank lines after imports, and between function/class definitions.

a = namedtuple('assignment', 'weight scores')

Using a namedtuple for a simple data class like this is a good idea, but it should still be named as a class; Assignment.

def average(scores:list):
    '''average a list of scores'''
    return sum(scores)/2

This function has an obvious bug; what about lists of scores with lengths other than two? Also you type the parameter but not the return value, and should again have some more whitespace; I'd write:

def average(scores: list) -> float:
    """Average a list of scores."""
    return sum(scores) / len(scores)

See PEP-257 for docstring conventions.

def get_grade(grades:list):
    '''compute final grade based on scores and their weights'''
    result = 0
    for a in grades:
        result += average(a.scores) * a.weight
    return result

I think you could simplify this with a bit more OOP. If we subclass the named tuple we can add a read only calculated attribute to it:

class Assignment(namedtuple('Assignment', 'weight scores')):

    @property
    def result(self) -> float:
        return average(self.scores) * self.weight

Then your function becomes:

def get_grade(assignments: list) -> float:
    """Compute final grade based on scores and their weights."""
    return sum(a.result for a in assignments)

Note the revised parameter name, which better describes the input.

def main():
   grades = [a(.30, [18/45, 11/55]),
             a(.20, [3/10, 7.5/10, 10/10]),
             a(.10, [9/10, 9/10, 10/10, 10/10, 8/10,
                6/10, 10/10, 10/10]),
             a(.40, [29/65]),
             a(.01, [1/1])]
   print(get_grade(grades))
main()

To keep things looking consistent without introducing a lot of indentation, I usually split lists with long elements across multiple lines as follows:

assignments = [
    Assignment(.30, [18/45, 11/55]),
    ...
    Assignment(
        .10,
        [9/10, 9/10, 10/10, 10/10, 8/10, 6/10, 10/10, 10/10]
    ),
    ...
]

Note splitting between, rather than within, the two parameters.

Also you should guard the invocation of the entry point as follows:

if __name__ == '__main__':
     main()

This means you can import this functionality more easily for testing and reuse.


Reducing steps for long addition program This is a program that I wrote to add two numbers together that are too large for the C language to store.  It stores the numbers as strings instead and processes one digit at a time. But I suspect that the program could be done in fewer steps.  In what ways can I reduce the size of my program?  Where am I taking more steps than necessary?

#include <stdio.h>
#include <string.h>

int main()
{
    char *a = "1490438498359508";
    char *b = "1575390843059845";
    char *sum = "";
    char str[15] = "";
    char src[512];
    int carry = 0;
    int alength = strlen(a);
    int blength = strlen(b);
    int n;
    int i;
    int c, d;

    for (n=1; n<blength+1; n++) {
        int c=0, d=0;
        i = 0;
        c = a[alength-n]-48;
        d = b[blength-n]-48;
        i = c+d;
        if (carry == 1) {
            i=i+1;
        }
        if (i > 9) {
            i = i-10;
            carry=1;
        } else {
            carry=0;
        }
        int aInt = i;
        char str[15] = "";
        sprintf(str, "%d", aInt);
        strcat(src, str);

    }
    int nlength = strlen(src);
    int x;
    for (x=0; x<nlength+1; x++) {
        printf("%c", src[nlength-x]);
    }
    printf("\n");
    return 0;
}

1) It seems that you assumed length of a and b are the same. But if a has a bigger length than b you don't process all digits! Your loop should go over maximum of lengths of a and b and whenever one operand has no digit any more, use 0.

2) You can write these lines

i = c+d;
if (carry == 1) {
    i=i+1;
}

like this:

i = c + d + carry

use arithmetic operations when branches are not necessary.

3) The result of addition has always at most one digit more than the bigger operand. So you can allocate a string once and avoid concatenation in each iteration (just put the result digit in its right place).

4) You can reduce number of iterations by using the fact that you can add in higher bases! This way instead of a string, use an integer array and treat the
operands, say in base 10000. Now each digit in this base is a 4 digit number in base 10. Everything will be the same, except for setting the value of carry.
With base 10^4, you reduced number of iteration to one-forth. You can go further till where integer range supports safely (10^8).


Enterprise FizzBuzz in C++11 I've decided to write a C++ FizzBuzz with the focus on some new C++11 features, ridiculous optimizations and unit-testability.

Are there improvement possibilities in those regards?

template <typename T>
static void fizzbuzz(std::ostream& out, const T& number, const std::vector<std::pair<std::string, T>>& values)
{
    bool found = false;

    std::for_each(values.begin(), values.end(),
        [&](decltype(*values.begin()) i)
        {
            if (0 == number % i.second)
            {
                out << i.first.c_str();
                found = true;
            }
        }
    );

    if (!found)
    {
        out << number;
    }
}

template <typename T>
static void filter_fizzbuzz(const T& end, std::vector<std::pair<std::string, T>>& values)
{
    // Needs to be sorted as the filtering only looks for divisible numbers up to the current one
    std::sort(values.begin(), values.end());

    for (auto i = values.begin(); i != values.end();)
    {
        // i > end will never become 0
        if (i->second > end)
        {
            i = values.erase(i);
            continue;
        }

        // Is the current number already handled by a divisor?
        bool alreadyHandled = false;
        for (auto j = values.begin(); j != i; j++)
        {
            if (0 == i->second % j->second)
            {
                alreadyHandled = true;
            }
        }

        if (!alreadyHandled)
        {
            i++;
            continue;
        }

        i = values.erase(i);
    }
}

template <typename T>
void fizzbuzz(std::ostream& out, const T& start, const T& end, std::vector<std::pair<std::string, T>>& values)
{
    if (start > end)
        throw std::out_of_range("start > end");
    if (!values.size())
        return;

    filter_fizzbuzz(end, values);
    for (T i = start; i < end; i++)
    {
        fizzbuzz(out, i, values);
        out << std::endl;
    }
}

Know when to use which feature

C++11 offers range based for loops so you should use them instead of for_each with lambdas:

for(auto i: values) {
    if (0 == number % i.second) {
        out << i.first.c_str();
        found = true;
    }
}

As you can see this facilitates auto instead of the ugly decltype hack.

Don't overuse static

Why are all the functions declared static? Are they part of a class?
If so, why?

Design: Unnecessary complexity

I have troubles tracing the algorithm in your code.
If you would really want an enterprise solution I would try to integrate with standards like iterators and function(/functor) interfaces that can be passed into algorithms like std::transform.


Is this the right way to retrieve all the fields from any Object? I have a method that returns a List<Object> from any given Object containing its values. It doesn't get the values of any static or final field in the class.

public List<Object> objectData(Object object) {
    Field[] fields = object.getClass().getDeclaredFields();
    Set<String> realFields = new LinkedHashSet<String>();
    for(Field field : fields) {
        if (!(java.lang.reflect.Modifier.isStatic(field.getModifiers()) ||
              java.lang.reflect.Modifier.isFinal(field.getModifiers()))) {
            realFields.add(field.getName());
        }
    }
    List<Object> row = new ArrayList<Object>();
    for(String realField : realFields) {
        Field currentField = object.getClass().getDeclaredField(realField);
        currentField.setAccessible(true);
        row.add(currentField.get(object));
    }
    return row;
}

Is this the right approach? Isn't something else I can be missing?

EDIT: The method above was adapted from this one. I'm assuming Collection<T> data is a valid non-empty collection.

public List<List<Object>> getData(Collection<T> data) {
    try {
        Field[] fields = data.iterator().next().getClass().getDeclaredFields();
        Set<String> realFields = new LinkedHashSet<String>();
        for(Field field : fields) {
            if (!(java.lang.reflect.Modifier.isStatic(field.getModifiers()) ||
                  java.lang.reflect.Modifier.isFinal(field.getModifiers()))) {
                realFields.add(field.getName());
            }
        }
        List<List<Object>> realData = new ArrayList<List<Object>>();
        for(T element : data) {
            List<Object> row = new ArrayList<Object>();
            for(String realField : realFields) {
                Field currentField = element.getClass().getDeclaredField(realField);
                currentField.setAccessible(true);
                row.add(currentField.get(element));
            }
            realData.add(row);
        }
    } catch (Exception e) {
        //basic error handling, this will be improved;
        throw new RuntimeException(e);
    }
}

In principle, your code is correct (provided you really do not want to deal with fields in superclass). But it can be optimized: inside the first loop, you already have fields so there is no need to save their names in a collection and retrieve later - just use them in place.


In AngularJS, create lots of directives or use ng-controller? This is a question about whether my coding style follows AngularJS best practices.

One of the selling points to Angular is directives. Directives allow you to create custom DOM with elements or attributes you create. I've found myself creating lots of directives, thinking of each one as little controls to be placed on the page. Looking back, I'm wondering if I'm too directive focused and if Angular directives should only be used when you need a custom link method?

What I find myself doing a lot is wrapping up some HTML in a directive with a simple controller that does one or two basic things. I have a lot of code that looks like this code of mine from a blog post:

'use strict';

angular.module('solrAngularDemoApp')
  .directive('searchResults', function () {
    return {
      scope: {
        solrUrl: '=',
        displayField: '=',
        query: '&',
        results: '&'
      },
      restrict: 'E',
      controller: function($scope, $http) {
        console.log('Searching for ' + $scope.query + ' at ' + $scope.solrUrl);
        $scope.$watch('query', function() {
          $http(
            {method: 'JSONP',
             url: $scope.solrUrl,
             params:{'json.wrf': 'JSON_CALLBACK',
                    'q': $scope.query,
                    'fl': $scope.displayField}
            })
            .success(function(data) {
              var docs = data.response.docs;
              console.log('search success!');
              $scope.results.docs = docs;

            }).error(function() {
              console.log('Search failed!');
            });
        });
      },
      template: '<input ng-model="query" name="Search"></input>' +
                '
Search Results for {{query}}
' +
                '' +
                '
{{doc[displayField]}}
'  +
                ''
    };
  });

So now I can think of my search results as a little widget to embed on the page:

 <search-results solrUrl="..." etc></search-results>

OK Let's say I take the next step in my application and want to display more information about the individual documents from the search results. My inclination up to this point has been to create a custom directive, like a little "widget" that handles all the functionality here. If I need to say wrap-up how a document from the search results gets displayed, I'll add a directive like lets say "docDisplay". Something like:

angular.module('solrAngularDemoApp')
   .directive('docDisplay', function() {
         // same pattern as above
    });

Then in the template for search Results, I'll change out my template:

      template: '<input ng-model="query" name="Search"></input>' +
                '
Search Results for {{query}}
' +
                '' +
                '  <doc-display></doc-display>'  + // THIS LINE CHANGED
                ''

I may or may not use prototypical inheritance of $scope or use an isolate $scope, depending on how strongly I feel this directive is tied to the parent.

After writing a lot of code this way and learning more about Angular, I realized that I could just have

Thrown all this HTML into my main HTML
Used ng-controller on specific pieces of HTML

As nothing here takes advantage of the link functionality of the directive (binding DOM events to create some custome functionality) and my directives are just a bunch of HTML and controller code I'm left wondering the following two questions:

Is it ok to use directives like this to break up my UI? or am I thinking too much in the directives==controls mindset
Should I avoid directives when there's no link function? and just stick with plain-jane HTML with ng-controller?

AngularJS encourages slim controllers. They'd rather you have fat directives and services than controllers.

I think you are absolutely correct to have many directives.

In my angular apps, I have some directives without link to make my templates clean and readable. You can compress trees of HTML into their high-level meaning. Plus, if things get more complex later on, you can add a link function and add functionality with relative ease, and you don't have to change code in multiple places. As a Rails developer, I believe in DRY (don't repeat yourself). The fewer places the same code is, the easier it is to maintain.

So, directive or ng-controller?

If you are going to reuse a "widget" in multiple places, directives are ideal!

If it's a one-off or the main feature of the page, you're probably better off making it a ng-controller.


Count "Word Order" with further optimization Question:
You are given n words. Some words may repeat. For each word, print its number of occurrences. The output order should correspond with the input order of appearance of the word.

Sample Input:

4
bcdef
abcdefg
bcde
bcdef

Sample Output

3
2 1 1

Here's what I came up with:

n = int(input())
array = []
elements = {}
for index in range(n):
    value = input()
    if value not in array:
        array.append(value)
        elements[value] = 1
    else:
        elements[value] += 1

print(len(elements))
print(*(i for i in elements.values()), end=' ')

I stress tested it on Try it Online with the random string generator and found the runtime to be around 1.98s. But I'm getting TLE on the coding platform. How do I improve the speed (bit offtopic - is there any other approach)?

TLE

You're probably getting a TLE because,

if value not in array:

Is a O(N) operation meaning it has to traverse the entire array (worst case) to check if the value exists in the array

I can understand why you felt the need to have an extra array, because of dictionaries are not ordered.

But you can make use of the collections.OrderedDict module, to have an ordered dictionary!

Other

join!

print(*(i for i in elements.values()), end=' ')

This can be done cleaner with joining the values instead of unpacking

print(" ".join(map(str, e.values())))

Counter

Python is often described as batteries included,

Your element dictionary is the same the collections.Counter

Code

We can make an OrderedCounter class to combine these two and make get the most out of the included modules.

from collections import Counter, OrderedDict

class OrderedCounter(Counter, OrderedDict):
    pass

c = OrderedCounter(input() for _ in range(int(input())))
print(len(c))
print(" ".join(map(str, c.values())))


reverse a list of generic type Q: Given a list, reverse the order of its elements (don't use
  collections).

Here is my solution:

public static <T> void reverse(List<T> list) {
    if (list.size() > 0) {
        T t;
        t = list.get(0);
        list.remove(0);
        reverse(list);
        list.add(t);
    }
}

I want to know if this is an efficient way of reversing a list
Is this working for every list as input?

No and no. This is a very inefficient way to reverse, for example, a dynamic array based list (it's a different story with a linked list). With a dynamic array based (e.g. ArrayList) The remove(0) operation will require moving every other element in the list (linear in the length), so the whole method is quadratic in the total length of the list. It also requires a linear number of stack frames, which are actually pretty expensive and if you ask for too many the program will crash, which means this method will not work for long lists.

An efficient solution would work in linear time with a constant space requirement.

When thinking about inputs, it's important to also consider null as a possible input. The deficient spec does not indicate what should be done with a null input, which means you have had to make a decision, and that decision should be documented. The 'easy' (and often best) option is to explicitly test for a null input and throw an IllegalArgumentException exception, so that anyone using your method receives a useful error message. You could argue that your code should do nothing with a null, but that's a bit iffy, because the spec doesn't qualify the behaviour, and that would 'silently' fail if someone passed it a null incorrectly: throwing an exception forces any unexpected usage to be address directly, which may or may not prompt a decision change, which makes it a good 'default' choice.

There is no need here to pre-declare t: just declare and assign it in one go:

T t = list.get(0);

This makes it easier to find out what it means. You could also make it final in this case, so that it is entirely clear that it is only assigned once, which makes it that little bit easier to reason about the code.

When writing recursive code like this, it can pay select the 'most ideal' base case. In this instance, any list of length no more than 1 is already reversed, so you could change the if condition to list.size() > 1. This doesn't change the complexity, but is something of which to be aware.


Searching multiple Excel sheets for the cheapest item I am creating an estimation tool, where users can input part numbers and quantities, and the tool will output the lowest cost for that part number based on searches through multiple databases.

I accomplish this by loading data into multiple sheets and then evaluate all possibilities on one sheet.

I believe my code is struggling, because I have multiple instances of this loop below which I'm hoping someone can help me improve.  The code below runs 3 separate times, for 3 different sheets.  There can be up-to 1000 part numbers run at a time.  Once the tool has been run, to start over I delete all the sheets, so each sheet is created every time the macro runs.

'Add Content to Summary

       'MPN and Qty add to Summary page
            Sheet22.Visible = True
            Sheet1.Select
            Range("A2").Select

            Sheet22.Select
            Range("A2").Select

            For i = 1 To 3
            Sheet1.Select
            If Len(ActiveCell.Value) > 0 Then

            xmpn = ActiveCell.Value
            xqty = ActiveCell.Offset(0, 1).Value

            Sheet22.Select
            ActiveCell.Value = xmpn
            ActiveCell.Offset(0, 1).Value = xqty
            ActiveCell.Offset(1, 0).Select
            Sheet1.Select
            ActiveCell.Offset(1, 0).Select

            Else
                i = 10
            End If
                i = i - 1
            Next

If there are 1000 part number entries, this code can take about 45 seconds to run, and causes excel to show not responding.  Any help or suggestions to improve would be greatly appreciated.

Based on the array video provided by TinMan, I now declared the data as an array, and then populate each tab referencing the array.  The code is now instantaneous for 1000s records.

Thanks TinMan!!

'Set MpnQty array

Dim MpnQty() As Variant
Dim Dimension1 As Long, Dimension2 As Long

Sheet1.Activate

Dimension1 = Range("A2", Range("A2").End(xlDown)).Cells.Count - 1
Dimension2 = 1

ReDim MpnQty(0 To Dimension1, 0 To Dimension2)

For Dimension1 = LBound(MpnQty, 1) To UBound(MpnQty, 1)
    For Dimension2 = LBound(MpnQty, 2) To UBound(MpnQty, 2)
        MpnQty(Dimension1, Dimension2) = Range("A2").Offset(Dimension1,   Dimension2).Value
    Next Dimension2
Next Dimension1

'Add MPN and Qty to Summary page

Sheet22.Visible = True
Sheet22.Activate

For Dimension1 = LBound(MpnQty, 1) To UBound(MpnQty, 1)
    For Dimension2 = LBound(MpnQty, 2) To UBound(MpnQty, 2)
        Range("A2").Offset(Dimension1, Dimension2).Value = MpnQty(Dimension1, Dimension2)
    Next Dimension2
Next Dimension


Method to read the number of lines in a web page Needing a way to read the number of lines on a web page for the purpose of a progress bar the following method is created:

private static int countPageLines(String url) throws IOException {
        URL page = null;
        BufferedReader in = null;
        int count = 0;

        try {
            page = new URL(url);
            in = new BufferedReader(new InputStreamReader(page.openStream()));

            while ((in.readLine()) != null) {
                count++;
            }
        } catch(IOException ioe) {
            ioe.printStackTrace();
        } finally {
            if (in != null) {
                in.close();
            }
        }

        return count;
    }

Now, of course, it works but is it optimal? Can anything be done to improve the speed?
Java 8 is available, if pertinent.

There are a bunch of things which you should consider revising, including the basic concept itself.

Here your concept is to read each line from the URL, count it, and throw it away. Your code does nothing with the data? What's the point?

Regardless, even if the sole purpose of the code is to count a URL's lines, the code should be revised to take advantage of Java Best practices.

First up, it's a cheap shot to treat the new URL(...) constructor the way you do. Yes, it throws a MalformedURLException, and yes, that is a subclass of an IOException, but they really should not be lumped together.

Your method is listed as throwing an IOException anyway, so why handle the exception inside the method and throw nothing? The calling code will need to handle the IOException anyway. Just throw the exception and be done.... but also throw the MalFormedURLException.

Next try-with-resources is your friend. Resources treated that way will not need the finally block at all.

Finally, Java 8 does have some fantastic InputStream-to-Stream methods, use them. Specifically, the BufferedReader.lines() method:

Your code could be something like:

private static int countPageLines(String url) throws IOException {
    URL page = new URL(url);
    try (BufferedReader reader = new BufferedReader(new InputStreamReader(page.openStream()))) {
        long count = reader.lines().count();
        return count > Integer.MAX_VALUE ? Integer.MAX_VALUE : (int)count;
    }
}

As a side note, you have tagged your question with performance. The limiting factor here in terms of performance is your network... I cannot suggest a way to make your code above improve the performance of your network without knowing more about your setup ;-)


Assembling edges of a graph I'm making a neural network that comprises five populations of feature-selective neurons and one population of non-selective neurons.  Each neuron in this network receives connections from

c * f * N_E randomly selected neurons from each of the selective populations
c * (1 - f * p) * N_E randomly selected non-selective neurons.

Below is the code I've written to define all of the connections in this network.  Specifically, the code produces two arrays that together represent the source (ee_i) and target (ee_j) nodes of every connection.

Can anyone think of a way to reorganize this code to speed it up?

import numpy as np

N_E = 8000
p = 5
f = 0.1
c = 0.2
encoders = np.random.choice(np.arange(N_E), (p, int(f * N_E)), False)
non_selec = np.array([i for i in range(N_E) if i not in encoders])
ee_i = []
ee_j = np.repeat(np.arange(N_E), int(c * N_E))
for j in range(N_E):
    ee_i = np.concatenate([ee_i,
                           np.concatenate([np.random.choice(encoders[0, :],
                                                            int(c * f *
                                                                N_E),
                                                            False),
                                           np.random.choice(encoders[1, :],
                                                            int(c * f *
                                                                N_E),
                                                            False),
                                           np.random.choice(encoders[2, :],
                                                            int(c * f *
                                                                N_E),
                                                            False),
                                           np.random.choice(encoders[3, :],
                                                            int(c * f *
                                                                N_E),
                                                            False),
                                           np.random.choice(encoders[4, :],
                                                            int(c * f *
                                                                N_E),
                                                            False),
                                           np.random.choice(non_selec,
                                                            int(c * (1 - f *
                                                                     p) *
                                                                N_E),
                                                            False)])])

as far as I could see by googling np.concatenate is the fastest way for concatenate arrays, so I'd keep that.

but talking about code organization, I suggest you to create a function (something like it. it may have syntax errors, so that's just to give you an idea)

def randomChoice(value, select):
    if select:
        return np.random.choice(encoders[value, :], int(c * f * N_E), False);
    else:
        return np.random.choice(non_selec, int(c * (1 - f * p) * N_E), False);

and have a simple and single call instead of a huge block of code into one line with two concatenate calls

ee_i = np.concatenate([ee_i, randomChoice(0, true), randomChoice(1, true), randomChoice(2, true), randomChoice(3, true), randomChoice(4, true), randomChoice(0, false)])

I hope this helps


C++ class for a flag register in a Chip8 emulator In order to learn C++ I decided to code a Chip8 emulator following a tutorial. When I came across the idea of a flag register and decided it would be fun to implement in order to get familiar with bitwise operations. I'd like this review to center on what can be improved from the point of view of C++, but of course any other suggestions and improvements are welcome.

class FlagRegister {
private:
    unsigned char state = 0x00;
    enum Flag: unsigned char {
        CF = 0x01,
        PF = 0x02,
        AF = 0x04,
        ZF = 0x08,
        TF = 0x10,
        IF = 0x20,
        DF = 0x40,
        OF = 0x80
    };

    bool checkFlag(Flag flagToCheck) {
        return (bool) ((this->state >> (flagToCheck - 1)) % 2);
    }

    void setFlag(Flag flagToSet) {
        this->state = this->state | flagToSet;
    }

    void unsetFlag(Flag flagToUnset) {
        this->state = (this->state & (~flagToUnset));
    }

public:
    bool checkCF() {
        return this->checkFlag(CF);
    }

    void setCF() {
        this->setFlag(CF);
    }

    void unsetCF() {
        this->unsetFlag(CF);
    }
// Idem for all other flags.
};

I see some things that may help you improve your program.

Avoid writing this

The overuse of this clutters the code and makes things harder to understand.  For example, the current code contains this:

void setFlag(Flag flagToSet) {
    this->state = this->state | flagToSet;
}

It's much easier to read and understand if written instead like this:

void setFlag(Flag flag) {
    state |= flag;
}

Think about the user

Consider actually using this class.  For example, to appropriately set the ZF flag we'd have to do something like this:

if (result == 0) {
    flags.setZF();
} else {
    flags.unsetZF();
}
return flags.checkZF();

That's way too much code for just one flag!  Here's what I'd rather write:

return flags.Z(result == 0);

Use const where practical

I would expect that checking the value of a flag shouldn't alter any flag.  To assure the reader of the code that this is the case, the functions should be marked const.  This also tells the compiler to make sure that 1) the code implementing the function does not alter the associated object and 2) the code calling the function can be a const object.  See this for a more detailed and rigorous explanation.

Simplify your code

Rather than having to go through another level of indirection, for such simple functions, I would remove the private methods.  To use the syntax I advocated earlier, here's one way to write a pair of functions:

bool C() const {
    return state & CF;
}
bool C(bool val) {
    if (C() != val) {
        state ^= CF;
    }
    return val;
}

You could even get crazy with preprocessor macros and write this:

#define FLAGIZE(x) bool x() const { return state & x ## F; } \
    bool x(bool val) { if (x() != val) state ^= x ## F; return val; }

    FLAGIZE(C)
    FLAGIZE(P)
    // etc.

This macro is just a shortcut to writing all of those functions out by hand.

Consider multithreading

It may be convenient at some point to have multiple threads setting flags simultaneously.  If that's the case, you want to make sure they don't overwrite, so either using something like std::atomic_uchar would provide a simple way to support such use.

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people.  Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code.  This affects not only code reviews, but also maintenance of the code in the future, by you or by others.  One good way to address that is by the use of comments.  Another good technique is to include test code showing how your code is intended to be used.


Unit tests for directory-entry counter I have a function that simply counts the number of files in a directory. To test this function I am using temporary files which are great in the respect that they are deleted upon creating. However, when I want to test my function to count multiple file it can become quite tedious. The below code works but you can probably guess there will be many lines in my test case if i wanted to count say 100 files.

#myfunction

class FileHandler:
    def count_files(output_file_dir):
        return len([f for f in listdir(output_file_dir) if isfile(join(output_file_dir, f))])

#test_myfunction
class FileHandlerTests(unittest.TestCase):
    def test_if_files_exist_return_count_of_those_files(self):
        f= FileHandler
        #test with 1 file
        with tempfile.TemporaryDirectory() as tmpdirname:
            print('created temporary directory\n', tmpdirname)
            with tempfile.NamedTemporaryFile(dir=tmpdirname) as test_file:
                print('created temporary file\n', test_file.name)
                self.assertEqual(1,f.count_files(tmpdirname))

        #test with 2 file
        with tempfile.TemporaryDirectory() as tmpdirname:
            print('created temporary directory\n', tmpdirname)
            with tempfile.NamedTemporaryFile(dir=tmpdirname) as test_file1:
                print('created temporary file\n', test_file1.name)
                with tempfile.NamedTemporaryFile(dir=tmpdirname) as test_file2:
                    print('created temporary file\n', test_file2.name)
                    self.assertEqual(2,f.count_files(tmpdirname))

   #test with 34 file
   with tempfile.TemporaryDirectory() as tmpdirname:
        print('created temporary directory\n', tmpdirname)
        #loop to create 34 temp files?

Why do you create named temporary files within your named temporary directory? The whole directory (including its contents) will be deleted as soon as the context ends.

So, just create a fixed (or random if you want to) number of zero byte files in that directory:

import os
import tempfile

...

with tempfile.TemporaryDirectory() as tmpdirname:
    print('created temporary directory\n', tmpdirname)
    n = 34 # create 34 files
    for i in range(n):
        open(os.path.join(tmpdirname, str(i)), "w").close() # create the empty file
    print('created', n, 'files')
    self.assertEqual(n, f.count_files(tmpdirname))