Computing sum and counting matches in an array The problem was pretty straightforward and used two simple algorithms from our examples; the one for computing the sum, and the one for counting matches. The array for the problem wasn't provided, but you can easily write a method around it without actually having it, since she gave us the name and type of the array. The code for my solution is this:

 public class Tests

 {

 private double[] grades;

 public int belowAverage() // Creates method for finding scores below the mean

 {

 double total = 0;

 for (double element1 : grades) // Computes total of array 'grades'

 {

  total=total + element1;

 }

  double mean = total / grades.length; //Uses total and the length of the array to computer the mean

  int matches = 0;

  for (double element2 : grades) //Uses mean to compute number of grades that are below the mean and

   {                                               // increases matches for each one that is.

    if (element2 < mean) {matches++;}

 }

 return matches; // Returns the number of matches.

  }

  }

Obviously the third line would change if you had the actual array. That was just kind of a placeholder. I tried to notate the code well enough to see where you get the total, then the mean, then compare each one to the mean to get matches that are less and increase your 'matches' variable for each match. I could have used the same 'element' variable for both loops since they're only used within the loops, but for clarity's sake in this example, I named them differently. I put all of these steps together in one method to solve this problem, but if I were going to write this sort of thing 'for real', I would assume it would probably need to calculate much more than just this small task (different averages, highest/lowest, etc.), so I would probably put each step in its own method so it could be used wherever you needed it.

Can someone comment on this and tell me if I could have done this in another way or an easier way? Maybe I could have used arrays.

From Steve McConnell, Code Complete 2nd Edition, 31.2 Layout Techniques, p737:

  Using blank lines is a way to indicate how a program is organized. You can use them
  to divide groups of related statements into paragraphs, to separate routines from one
  another, and to highlight comments.

It does not make sense if all groups contain only one line.
This comment is mostly noise:

return matches; // Returns the number of matches.

as well as this one:

// Uses total and the length of the
// array to computer the mean
double mean = total / grades.length;

They don't say anything more that the code already does. I'd remove them. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)
I would call both element1 and element2 as element. It's more common. Having element1 and element2 suggest a little bit that they are used together (but that's not the case).
Comments like this are not too easy to read on because of the horizontal scrolling and the unnecessary spaces:

{                                               // increases matches for each one that is.

You could put them a line above of the method/loop/variable declaration (and eliminate them later).

  [...] if I were going to write this sort of thing 'for real',
  I would assume it would probably need to calculate much more
  than just this small task (different averages,
  highest/lowest, etc.), so I would probably put each step
  in its own method so it could be used wherever you needed it.

You should to that now. It would eliminate most of the comments and increase the abstraction level of the method. A maintainer would be grateful, it's easier to get an overview what the method does (without the details) and you can still check them if you are interested in. Furthermore, you need to read and understand less code (not the whole original method) if you modify just a small part of it.

Finally, cleaning the code usually costs less during writing than later. Now you probably know the details better than later, therefore it's faster.

public int scoresBelowMean() {
    double total = getGradesTotal();
    double mean = total / grades.length;
    return gradesCountBelowMean(mean);
}

private double getGradesTotal() {
    double total = 0;
    for (double element: grades) {
        total = total + element;
    }
    return total;
}

private int gradesCountBelowMean(double mean) {
    int matches = 0;
    for (double element: grades) {
        if (element < mean) {
            matches++;
        }
    }
    return matches;
}

(Clean Code by Robert C. Martin, One Level of Abstraction per Function, p36)
Instead of doubles (and floating point numbers) you might want to use BigDecimals. Floating point numbers are not precise. Here is an example:

Effective Java, 2nd Edition, Item 48: Avoid float and double if exact answers are required
Why not use Double or Float to represent currency?


Handling exceptions for providing log information while debugging I am catching exceptions to provide better information for the logs to make debugging easier in production. Does this code follow best practices, since I'm not really handling the error but just logging extra information? I was also considering creating a custom RazorException, and throwing that instead. What do you think?

public string ParseFile(string templatePath, object model = null)
{
    //some code removed for clarity

    try
    {
        ITemplate template = _templateService.Resolve(templatePath, model);
        ExecuteContext context = new ExecuteContext();
        content = template.Run(context);
    }
    catch (TemplateCompilationException tcex) {
        _logger.Fatal("Razor parse failed: check for syntax error", tcex);
        throw;
    }
    catch (InvalidOperationException ex) {
        _logger.Fatal("Razor parse failed: trying to compile layout on its own", ex);
        throw;
    }
    catch (Exception ex) {
        _logger.Fatal("Razor parse failed", ex);
        throw;
    }
    return content;
}

Exceptions have Message property, which you should use, if you want to tell someone what went wrong in human language. You can then use String.Format("Razor parse failed: {0}", ex.Message) for all your exceptions. This will allow you, for example, to throw multiple InvalidOperationExceptions in the future without worrying about how you are going to log those.

Not to mention, that the guy debugging your code will likely prefer to know what went wrong straight away, without the need to look for log messages in your code or in log files.


Alternative for checking the existence of a value in PHP I'm trying to create a wordpress plugin but checking for the existence of a value doesn't really feel right because there's a bunch of values that I have to check:

$flickr_key = '';
if(!empty($ecom['ecom_options_flickr_api'])){
  $flickr_key = $ecom['ecom_options_flickr_api'];
}

$flickr_secret = '';
if(!empty($ecom['ecom_options_flickr_secret'])){
  $flickr_secret = $ecom['ecom_options_flickr_secret'];
}

$freebase_key = '';
if(!empty($ecom['ecom_options_freebase_api'])){
  $freebase_key = $ecom['ecom_options_freebase_api'];
}

$ebay_key = '';
if(!empty($ecom['ecom_options_ebay_api'])){
  $ebay_key = $ecom['ecom_options_ebay_api'];
}

Is there a more elegant alternative for this?

As Hugo Dozois suggested, using isset with the ternary operator is a good way to do this.

If you can assume that the options have not been set to NULL or false and you can change the $ecom variable you can use array_merge or the union operator.

// Merge with your default values here.
$ecom += array('ecom_options_flickr_api'    => '',
               'ecom_options_flickr_secret' => '',
               'ecom_options_freebase_api'  => '',
               'ecom_options_ebay_api'      => '');

// Set the variables.
$flickr_key    = $ecom['ecom_options_flickr_api'];
$flickr_secret = $ecom['ecom_options_flickr_secret'];
$freebase_key  = $ecom['ecom_options_freebase_api'];
$ebay_key      = $ecom['ecom_options_ebay_api'];

If they could be NULL or false you could set them using a short ternary as Hugo Dozois mentioned.  The indexes will now be set thanks to the union so you won't get Notices for undefined indexes.

Other Suggestions

I think your structure would be better represented as a multidimensional array:

$ecom['options'] = array('flickr_api'    => '',
                         'flickr_secret' => 'etc.');
Often intermediary variables such as $flickr_secret etc. are a bad idea IMO.  I prefer to use $ecom['options']['flickr_secret'] in my code.  I find aliases normally just confuse the code.


Finding and pasting images into a specific cell The below macro finds and pastes images into column A. While the macro works, it starts to slow down when running 500+ images. I am not too familiar with the VBA language, does anyone have any suggestions to make this code quicker and/or more elegant?

My existing Macro is as follows:

    Sub Picture() 'This Sub Looks for Image names posted in column B
            'in the file folder and then resizes the images and pastes them
            'in Column A

     'Opens File Dialog Box to select File Folder
     With Application.FileDialog(msoFileDialogFolderPicker)
    .InitialFileName = Application.DefaultFilePath & "\"
    .Title = "Select the folder containing the Image/PDF files."
    .Show
    If .SelectedItems.Count = 0 Then
        Exit Sub
    Else
        FldrName = .SelectedItems(1)
    End If
    End With

    Dim PicName As String

    Dim pasteAt As Integer
    Dim lThisRow As Long

    Application.ScreenUpdating = False

    lThisRow = 2

    Do While (Cells(lThisRow, 2) <> "Please Check Data Sheet")

    pasteAt = lThisRow
    Cells(pasteAt, 1).Select 'This is where picture will be inserted

    PicName = Cells(lThisRow, 2) 'This is the picture name

    present = Dir(FldrName & "\" & PicName & ".jpg")

    If present <> "" Then

        ActiveSheet.Pictures.Insert(FldrName & "\" & PicName & ".jpg").Select 'Path to where pictures are stored
        '''''''''''''''''''''''''''''''''''''''''''''''''''''''
        ' This resizes the picture
        '''''''''''''''''''''''''''''''''''''''''''''''''''''''
        With Selection
            .ShapeRange.LockAspectRatio = msoFalse
            .Height = ActiveCell.Height
            .Width = ActiveCell.Width
            .Top = ActiveCell.Top
            .Left = ActiveCell.Left
            .Placement = xlMoveAndSize
        End With

    Else
        Cells(pasteAt, 1) = "No Picture Found"
    End If

       lThisRow = lThisRow + 1
Loop

Range("A10").Select
Application.ScreenUpdating = True

Exit Sub

ErrNoPhoto:
MsgBox "Unable to Find Photo" 'Shows message box if picture not found
Exit Sub
Range("B20").Select

End Sub

I'm not sure I can make it faster but hopefully can make it a bit elegant.
Let's start

First of all, use Option Explicit for all your VBA work. This will make your life easier once you use VBA more.

Error handling
Great you use it, unfortunately not correctly. Your label will never be hit

ErrNoPhoto:

You have to tell your code you want to handle errors

On Error Goto ErrNoPhoto

A cosmetic change I changed and mainly moved the code for selecting folder to a separate method, just to make it clear

Private Function GetFolder() As String

    Dim selectedFolder  As String
    With Application.FileDialog(msoFileDialogFolderPicker)
        .InitialFileName = Application.DefaultFilePath & "\"
        .Title = "Select the folder containing the Image/PDF files."
        .Show

        If .SelectedItems.Count > 0 Then
            selectedFolder = .SelectedItems(1)
            If Right$(selectedFolder, 1) <> Application.PathSeparator Then _
                selectedFolder = selectedFolder & Application.PathSeparator
        End If

    End With
    GetFolder = selectedFolder

End Function

One of the biggest change I made in your code is changing the way how you go through cells. This can be one of the most slowly operation in VBA.

I always try to convert it to an array which is "million" times faster than going directly through cells. You will see significant difference if you go through huge numbers of cells. I'm not sure you will see the difference in your code but this is one of the best practice.

Set wks = ActiveSheet
' this is not bulletproof but for now should work fine
lastRow = wks.Cells(1, "B").End(xlDown).Row
data = wks.Range(wks.Cells(1, "B"), wks.Cells(lastRow, "B")).Value2

this will load data from cells from column B, from cell B1 to Bn where n is the last row found by this

lastRow = wks.Cells(1, "B").End(xlDown).Row

this will give you an opportunity to use For Next loop instead While.
I didn't find anything important I could change in the insert picture logic except one thing that I removed all the Selection command from your code which should again make it a bit faster.

At the end this is what was in my VBE
Inserting 800 images took about 7 seconds

Option Explicit

'********************************************************************************
'Picture
'
' Purpose:  Looks for Image names posted in column B in the file folder and
'           then resizes the images and pastes them in Column A
'
' Inputs:   -none-
'
' Outputs:  -none-
'
' Created:  06/03/2015  proxy
'
' Modified: .
'
'********************************************************************************
Sub Picture()

    Const EXIT_TEXT         As String = "Please Check Data Sheet"
    Const NO_PICTURE_FOUND  As String = "No picture found"

    Dim picName             As String
    Dim picFullName         As String
    Dim rowIndex            As Long
    Dim lastRow             As Long
    Dim selectedFolder      As String
    Dim data()              As Variant
    Dim wks                 As Worksheet
    Dim cell                As Range
    Dim pic                 As Picture

    On Error GoTo ErrorHandler

    selectedFolder = GetFolder
    If Len(selectedFolder) = 0 Then GoTo ExitRoutine

    Application.ScreenUpdating = False

    Set wks = ActiveSheet
    ' this is not bulletproof but for now should work fine
    lastRow = wks.Cells(1, "B").End(xlDown).Row
    data = wks.Range(wks.Cells(1, "B"), wks.Cells(lastRow, "B")).Value2

    For rowIndex = 1 To UBound(data, 1)
        If StrComp(data(rowIndex, 1), EXIT_TEXT, vbTextCompare) = 0 Then GoTo ExitRoutine

        picName = data(rowIndex, 1)
        picFullName = selectedFolder & picName & ".jpg"

        If Len(Dir(picFullName)) > 0 Then
            Set cell = wks.Cells(rowIndex, "A")
            Set pic = wks.Pictures.Insert(picFullName)
            With pic
                .ShapeRange.LockAspectRatio = msoFalse
                .Height = cell.Height
                .Width = cell.Width
                .Top = cell.Top
                .Left = cell.Left
                .Placement = xlMoveAndSize
            End With
        Else
            wks.Cells(rowIndex, "A").Value = NO_PICTURE_FOUND
        End If

    Next rowIndex

    Range("A10").Select

ExitRoutine:
    Set wks = Nothing
    Set pic = Nothing
    Application.ScreenUpdating = True
    Exit Sub

ErrorHandler:
    Range("B20").Select
    MsgBox Prompt:="Unable to find photo", _
           Title:="An error occured", _
           Buttons:=vbExclamation
    Resume ExitRoutine

End Sub

Private Function GetFolder() As String

    Dim selectedFolder  As String
    With Application.FileDialog(msoFileDialogFolderPicker)
        .InitialFileName = Application.DefaultFilePath & "\"
        .Title = "Select the folder containing the Image/PDF files."
        .Show

        If .SelectedItems.Count > 0 Then
            selectedFolder = .SelectedItems(1)
            If Right$(selectedFolder, 1) <> Application.PathSeparator Then _
                selectedFolder = selectedFolder & Application.PathSeparator
        End If

    End With
    GetFolder = selectedFolder

End Function


Is this a good way to implement a tile map from a text file? I'm working on a small final fantasy type game. I have this for a tile map. I was just wondering if this is a good way to do this or if anyone has any suggestions, I would love to read them.

import com.stardust.main.gfx.Assets;

import java.awt.Graphics;
import java.awt.image.BufferedImage;
import java.io.BufferedReader;
import java.io.FileReader;
import java.io.IOException;

public class TileMap
{
   //placeholder for each tile
   BufferedImage[] tiles = new BufferedImage[37];

   //array of tile types
   int[][] map = new int[32][32];
   //position coordinates
   public static int posX, posY;
   //Edge of map Cooridinates
   public static int sx, sy;
   public String fileName;

   public TileMap(int posX, int posY, String fileName) throws IOException
   {
       this.fileName = fileName;
       int x = 0, y = 0;
       this.posX = posX;
       this.posY = posY;
       this.sx = map.length * 32;
       this.sy = map.length * 32;
       BufferedReader in = new BufferedReader(new FileReader(fileName));

       String line;

       while((line = in.readLine()) != null)
       {
           String[] values = line.split(",");

           for(String str : values)
           {
               int str_int = Integer.parseInt(str);
               map[x][y] = str_int;
               y += 1;
           }
           x += 1;
           y = 0;
       }

       in.close();
   }

   public void update()
   {
   }

   //Game.mapX and Game.mapY are the position variables in the main Game class
   public void render(Graphics g)
   {
       for(int x = 0; x < 32; x++)
       {
           for(int y = 0; y < 32; y++)
           {
               int textureType = map[x][y];
               setTile();
               BufferedImage texture = tiles[textureType];
               g.drawImage(texture, posX, posY, null);
               posY += 32;
           }
           posX += 32;
           posY = Game.mapY;
       }
       posX = Game.mapX;
       posY = Game.mapY;

   }

   public void setTile()
   {
       tiles[0] = Assets.grass;
       tiles[1] = Assets.dirt;
       tiles[2] = Assets.water;
       tiles[3] = Assets.tree;
       tiles[4] = Assets.multiTree;
       tiles[5] = Assets.NSpath;
       tiles[6] = Assets.uprtPath;
       tiles[7] = Assets.EWpath;
       tiles[8] = Assets.upltPath;
       tiles[9] = Assets.dnltPath;
       tiles[10] = Assets.dnrtPath;
       tiles[11] = Assets.watgrsdn;
       tiles[12] = Assets.watgrslt;
       tiles[13] = Assets.watgrsup;
       tiles[14] = Assets.watgrsrt;
       tiles[15] = Assets.watgrsSE;
       tiles[16] = Assets.watgrsSW;
       tiles[17] = Assets.watgrsNW;
       tiles[18] = Assets.watgrsNE;
       tiles[19] = Assets.wallWood;
       tiles[20] = Assets.pointer;
       tiles[21] = Assets.pathOpRt;
       tiles[22] = Assets.pathOpLt;
       tiles[23] = Assets.pathOpDn;
       tiles[24] = Assets.pathOpUp;
       tiles[25] = Assets.pathOpAll;
       tiles[26] = Assets.roughWood;
       tiles[27] = Assets.pathOpen;
       tiles[28] = Assets.counter;
       tiles[29] = Assets.storeDude;
       tiles[30] = Assets.pot;
       tiles[31] = Assets.singleHouse;
       tiles[32] = Assets.doubleHouse;
       tiles[33] = Assets.wepBanner;
       tiles[34] = Assets.armBanner;
       tiles[35] = Assets.innBanner;
       tiles[36] = Assets.blankTile;
   }
}

Single responsibility principle

This class is responsible for one too many things:

Model a tile map
Parse a text file

As such it violates the single responsibility principle.
I suggest to move the file parsing code somewhere else.
For example in a TileMapFactory.fromFile method, or TileMapFileParser.

In general, you know something's not right when the constructor is too big.
If it has to do IO, which is especially troublesome because of exception handling and resource management, that's really way too much burden for a constructor.

Prefer List instead of arrays

If you don't need specifically an array, then use a List instead:

List<BufferedImage> tiles = new ArrayList<>();

It will make your life easier. For example, when adding the tiles,
you won't need to worry about correctly incrementing the array indexes:

   tiles.add(Assets.grass);
   tiles.add(Assets.dirt);
   // ... and so on

Magic numbers

The number 32 keeps coming up at many places in the code.
Declare it as a static final variable with a meaningful name.

Get rid of unused stuff

For example this method is pointless, remove it:

   public void update()
   {
   }

Coding style

Instead of y += 1, use ++y.


Project Euler problem #1 in Python 3 According to Project Euler Problem #1

  If we list all the natural numbers below 10 that are multiples of 3 or
  5, we get 3, 5, 6 and 9. The sum of these multiples is 23.
  Find the sum of all the multiples of 3 or 5 below 1000.
  The answer being 233168

The following is my code, which gives me the right answer

import time
start = (time.time())

def getmul(x, *args):
    s=0
    for i in range(1,x):
        for arg in args:
            if (i%arg==0):
                s+=i
                break
    return(s)

res = getmul(1000,3,5)
end = time.time() - start
print(res, end)

I built the function in such a way that arbitrary arguments can be inputted and the result obtained.
Are there more efficient ways of solving this problem?

Yes there is a faster way! Your solution is \$O(n)\$, but a \$O(1)\$ (constant time) solution exists.

This is one of my favorite problems since you don't need a computer to solve it.

Triangular Numbers

The nth triangular number is a sum of integers from 1 to n. There are several proofs that show this sum is equivalent to (n + 1) * n / 2.

To get down to constant time, one needs to note that the sum of all multiples of 3 less than n is very similar to a triangular number. Here's a sum denoting what we want:

  3 + 6 + 9 + 12 + ... + 3*(n // 3)

Note the integer division //

If we factor a 3 out of this sum:

  3(1) + 3(2) + 3(3) + 3(4) + ... + 3(n//3)

We get three times a triangular number!

  3(1 + 2 + 3 + 4 + ... + n//3)

Which can be simplified down to:

  3 * ((n//3 + 1) * n//3 // 2)

The same can be done for 5 (or any natural number)

def sum_mul_less_than(m, n):
    return (m * (n//m + 1) * (n//m)) // 2

Now we add the multiples of three and five and subtract the extra set of multiples of 15 (I like how clever the break was in your code for this).

def getmul_3_5(n): # this solution is harder to generalize for any multiple
    n -= 1 # we don't want to include n
    s = 0
    s += sum_mul_less_than(3, n)
    s += sum_mul_less_than(5, n)
    s -= sum_mul_less_than(15, n)
    return s

Constant time is INCREDIBLY faster than linear time, \$O(n)\$. This solution can handle astronomically large inputs bigger than 10**30 even.

Edit: General case

Assuming the list of multiples could be of any size, you should get rid of the unpacking argument list operator since it's less convenient to users. I came up with a solution that is \$O(2^m)\$ where m is the number of multiples. Your solution is \$O(nm)\$ which would be asymptotically faster around \$n < 2^{m-1}\$, so you could fine tune this line and use the strategy pattern to pick the faster algorithm on a case by case basis.

First I removed all of the redundant multiples, (e.g. 4 if 2 is in m since any number divisible by 4 is already counted by 2). Then I found every possible multiple combination of the numbers in m.

Combinations with odd lengths are added; combinations with even lengths are subtracted (the inclusion exclusion principle as noted by user Gareth Rees).

from itertools import combinations
from functools import reduce

def geo(nums):
    '''Multiply a list of numbers together'''
    return reduce(lambda x, y: x*y, nums)

def getmul(n, args):
    n -= 1
    s = 0

    # removing the redundant multiples from args is left as an excercise
    # O(m^2)

    # there are 2^len(args) combinations of args
    for i in range(1, len(args) + 1):
        for c in combinations(args, i):
            if i % 2 == 1:
                s += sum_mul_less_than(geo(c), n)
            else:
                s -= sum_mul_less_than(geo(c), n)
    return s


Binary Search Tree implementation #include <stdio.h>
#include <stdlib.h>
#include <assert.h>

typedef struct Node NODE;

struct Node {
  int value;
  int is_leaf;
  struct Node *right;
  struct Node *left;
};

NODE *new_leaf(void) {
  NODE *leaf = malloc(sizeof(NODE));
  assert(leaf);
  leaf->is_leaf = 1;
  leaf->value = 0;
  leaf->right = NULL;
  leaf->left = NULL;
  return leaf;
}

int insert(NODE *node, int value) {
  assert(node);
  if (node->is_leaf) {
    node->is_leaf = 0;
    node->value = value;
    node->left = new_leaf();
    node->right = new_leaf();
  } else if (node->value < value) {
    return insert(node->left, value);
  } else if (node->value > value) {
    return insert(node->right, value);
  }
  return -1;
}

int contains(NODE *node, int value) {
  assert(node);
  if (node->is_leaf) {
    return 0;
  } else if (node->value < value) {
    return contains(node->left, value);
  } else if (node->value > value) {
    return contains(node->right, value);
  }
  return 1;
}

int max_height(NODE *node) {
  assert(node);
  if (node->is_leaf) {
    return 0;
  }
  int left_height = max_height(node->left);
  int right_height = max_height(node->right);
  if (left_height > right_height) {
    return 1 + left_height;
  } else {
    return 1 + right_height;
  }
}

void destroy(NODE *tree) {
  free(tree);
}

int main(void) {
  NODE *tree = new_leaf();
  insert(tree, 7);
  printf("contains 7? : %d\n", contains(tree, 7));
  insert(tree, 6);
  printf("contains 6? : %d\n", contains(tree, 6));
  insert(tree, 5);
  printf("contains 5? : %d\n", contains(tree, 5));
  printf("height should be 3, is : %d\n", max_height(tree));
  insert(tree, 8);
  printf("contains 8? : %d\n", contains(tree, 8));
  printf("height should be 3, is : %d\n", max_height(tree));
  printf("no contains 1? : %d\n", contains(tree, 9));
  destroy(tree);
}

Looking for general style advice, and if I missed any memory leaks.

Specific question: does destroying the tree at the end free up all of the memory allocated for every node in the tree? If so, how? If not, how can I ensure that occurs? Also, am I using asserts correctly? It's very different than the languages I have used previously.

Firstly, in C, ALL_CAPS is generally used only for macros. Hence, I wouldn't use the name NODE as a typedef for your struct node, but instead something like typedef struct Node node_type;. Further, generally C is written using lowercase_with_underscores, so I'd rename it to struct node.

Using an assert in your new_leaf function on what you get back from malloc is fine in my books, as this will terminate the process if the OS can't give you back any memory, in which case you're unlikely to be able to do anything anyway.

Your function int contains(NODE *node, int value) doesn't really need an assert. If the user gives you a NULL node, then you can immediately just return 0. Giving a NULL to this function shouldn't terminate the program. A similar argument can be made for max_height.

Of course, the biggest problem is that no, your destroy function definitely does NOT clean up all the memory allocated by the program. Tools like valgrind can help to inform you if you have any memory leaks lurking around. In this case, however, it's very easy to see that the only memory that will be reclaimed is the memory for the root node (whatever you call destroy with).

Fixing this is pretty simple: it's just using a recursive algorithm that you've basically already used in insert and contains:

void destroy(node_type root)
{
    // If we have a non-NULL pointer, we need to
    // recursively free its left and right children,
    // and then free it.
    if(root) {
        destroy(root->left);
        destroy(root->right);
        free(root);
    }
}

A few other comments:

I'm not a big fan of 2 space indentation. It's not enough to (easily) visually scope blocks. I'd much rather 4 spaces.
You might want to start looking into how to user header files to split up interface and implementation as a next step (if you haven't already).
I'm not a big fan of explicitly having keeping track of what is a leaf with is_leaf. It means that any node that is down the bottom will have 2 extra dynamic allocations performed to have leaf values (see insert). Instead, I'd just set the left and right pointers to be NULL, and leave it at that. You can easily write an is_leaf function that simply checks that both pointers are NULL.
The API is a bit deficient (no deletion of a value, for example), but I'm guessing this is done as a learning exercise, so that's ok.


User Specified Array Read and Modify How may this code be improved upon, the user inputs array of specified length and replaces each element that is smaller than the mean of the first and last element with the mean.

Is there a better way to pass the array to the function (ref/ptr, etc.), or more efficient way to manage memory (when handling array)?

I know the vector container from STL is ideal but just wanted to gain some insight on fundamentals.

#include <iostream>
using namespace std;

void replaverage(int arr[], size_t n){
    cout<<"Enter array length"<<endl;
    cin>> n;
    for(int i=0; i<n; i++){
        cout<<"Enter the numbers"<<endl;
        cin>>arr[i];
        cout<<endl;
    }
    int f=arr[0];
    int l=arr[n-1];
    double av=(f+l)/2;
    for(int j=0; j<n; j++){
        if(arr[j]<av){
            arr[j]=av;
        }
    }
    for (int i=0; i<n; i++){
        cout<<arr[i]<< " ";}
}

int main(int argc, char* argv[]){
    size_t x=10;
    int arr[x];
    replaverage(arr,x);
    return 0;
}

Something to keep in mind is that this statement is truncating data:
        arr[j]=av;

because it is assigning a double to an int.
As was mentioned in a comment there are c++ container types that would be better than a old C style array, two of these are std::array<type, arraySize> and std::vector<type>. std::array is a fixed size and std::vector is a variable size. Both would allow the use of iterators that might simplify the code.
Avoid using namespace std;
If you are coding professionally you probably should get out of the habit of using the using namespace std; statement. The code will more clearly define where cout and other identifiers are coming from (std::cin, std::cout). As you start using namespaces in your code it is better to identify where each function comes from because there may be function name collisions from different namespaces. The identifiercout you may override within your own classes, and you may override the operator << in your own classes as well. This stack overflow question discusses this in more detail.
std::endl
It is better for performance if "\n" is used rather than std::endl in output, std::endl flushes the output buffer as well as putting out a new line, and this can slow down loops.
Complexity
The function replaverage should really be 2 functions, one to input the data and a second to process the data. Perhaps a third to print the data.
Horizontal Spacing
It would be better if there were spaces around operators that separate operands.
#include <iostream>
#include <vector>

std::vector<int> getData()
{
    int n;
    std::vector<int> intputData;
    std::cout << "Enter array length\n";
    std::cin >> n;
    std::cout << "Enter the numbers\n";
    for(int i = 0; i < n; i++){
        int tmpIn;
        std::cin >> tmpIn;
        intputData.push_back(tmpIn);
    }

    return intputData;
}

void modifyData(std::vector<int> &data)
{
    int f=data[0];
    int l=data[data.size() - 1];
    double av = (f + l)/2;
    for (auto j: data)
    {
        if(j < av){
            j = av;
        }
    }
}

void printData(std::vector<int> &data)
{
    for (auto i: data)
    {
        std::cout << i << " ";
    }
}

int main(int argc, char* argv[]){

    std::vector<int> data = getData();
    modifyData(data);
    printData(data);

    return 0;
}


K&R2 solution to exercise 1-18 (discarding trailing whitespace and empty lines) Recently I have started learning C++ and I decided to learn C along the way to improve my understanding of how computers work. For C I am using "The C Programming Language Book" by Brian Kernighan and Dennis Ritchie.

The exercise in question is to print all input lines disregarding trailing whitespace and empty lines.

Here is my code:

#include <stdio.h>
#include <stdlib.h>
#define MAX_BLANKS 5

int main()
{
    int c; /*holds current char to be printed*/
    int* wsbuffer; /*buffer to hold possible trailing whitespace*/
    int buffer_size; /*size of wsbuffer*/
    int nws; /*number of whitespaces in buffer*/
    int nonempty; /*0 if current line is empty, 1 else*/
    int i; /*for loop*/
    nonempty  = nws = 0;
    buffer_size = MAX_BLANKS;
    wsbuffer = (int*) malloc(buffer_size*sizeof(int));

    while (EOF != (c=getchar()))
    {
        if (c==' ' || c=='\t')
        {
            ++nws;
            if (nws>buffer_size)
            {
                buffer_size *=2;
                wsbuffer = (int*) realloc(wsbuffer,buffer_size*sizeof(int));
            }
            wsbuffer[nws-1] = c;
        }
        else if (c=='\n')
        {
            if (nonempty==1) {
                putchar(c);
                nws = nonempty = 0;
            }
        }
        else
        {
            nonempty = 1;
            for (i=0;i<nws;++i)
                putchar(wsbuffer[i]);
            nws = 0;
            putchar(c);
        }
    }
    free(wsbuffer);
    return 0;
}

I compiled this using gcc -std=c90 -Wall -Wextra -pedantic to adhere to the ISO protocol used by the book. It compiled fine with no warnings.

I deliberately set MAX_BLANKS to 5 to test the code. I tested a few cases (empty lines, lines with only space, spaces at first etc) and the result seemed okay.

Since memory allocation is not yet covered in the book, I'd particularly like it if someone could take a look and let me know if I have any issues particularly with memory leaking or something. Of course, any other suggestions or improvements would be welcome!

Don't cast the return value from malloc and family - provided there's a prototype in scope (which there is here, due to our #include <stdlib.h>), the void* result will convert to any pointer type.

Always check whether the returned pointer is a null pointer before using it:

wsbuffer = malloc(buffer_size * sizeof *wsbuffer);
if (!buffer) {
    fputs("Memory allocation failure\n", stderr);
    return EXIT_FAILURE;
}

Robust code needs to take extra care when using realloc().  If we write something like p = realloc(p, new_size); then we have a problem when it fails - p is assigned null, and we have nothing pointing to the memory any more (i.e. a memory leak).  The usual pattern looks something like:

void *new_buf = realloc(wsbuffer, buffer_size * sizeof *wsbuffer);
if (!new_buf) {
    free(wsbuffer);
    fputs("Memory allocation failure\n", stderr);
    return EXIT_FAILURE;
}
wsbuffer = new_buf;

Minor things:

Instead of c==' ' || c=='\t', we might consider isspace(c), remembering that this test includes newlines and other whitespace.
We can store whitespace in an array of char rather than int, since we never need to store EOF into that buffer.
The logic might be simpler if use a larger buffer and read a whole line at a time.  Then we just overwrite the start of the last whitespace found in the line, and print it.  That's likely a bit more efficient than our character-at-a-time operation, too.


Restart network-manager service if connection down on wake shell script I use Ubuntu (Trusty and Xenial) and usually put my computer in sleep mode rather than power it down.  A common problem is that upon wake the network is down so one has to restart the network-manager service.  E.g.

sudo service network-manager restart

I am tired of doing this so I have hacked the following together:

#!/bin/sh

# should live in /lib/systemd/system-sleep/...
# sudo mv restart-network-on-wake.sh /lib/systemd/system-sleep/
# sudo chmod a+x /lib/systemd/system-sleep/restart-network-on-wake.sh

case $1/$2 in
    pre/*)
        echo "Going to $2..."
        exit 0
        ;;
    post/*)
        echo "Waking up from $2..."
        if ! ping -q -c 1 -W 1 8.8.8.8 > /dev/null; then
            echo "IPv4 is down, restarting network-manager"
            service network-manager restart
        fi
        ;;
esac

I am a novice at shell scripting I guess and don't have a strong understanding of Linux internals. Do you have any thoughts?

Welcome to Code Review.  Thanks for asking for a review.  Overall I think you did a great job, even for an experienced scripter.

good things

consistent indentation
helpful comments about installation
you didn't forget the #! line
using if to check for return values without creating extra variables
I'm a fan of bash and prefer it in most cases, but specifying an older shell will help this be portable to environment without bash like embedded systems.

suggestions

include a comment explaining what the purpose of the script is.  The file name is pretty descriptive, but someone might rename it accidentally.  So including the filename in the comment would not be too redundant.
run shell code through shellcheck.  Lucky for you it didn't find any issues with this code
it is a good practice to include a *) in your case to catch the unexpected.
I like for there to be a space before the ) in the case matches and a blank line after the ;;.  The space before the ) makes it clearer what is part of the pattern and what isn't.  case statements can get longish so having the extra space after the ;; may annoy you if you have a small screen, but I find it makes it easier to follow the flow when it is more separated.

main gripe

Why did you include $2 in your case?  Since you didn't cross a function call boundary there's nothing in the case statement that will prevent access from $2 inside.  I've integrated these suggestions into a rewrite I haven't tested.  But it should work and seems cleaner to me:

#!/bin/sh

# restart-network-on-wake.sh - systemd/Ubuntu helper

# INSTALLATION
#
# The script should live in /lib/systemd/system-sleep/...
#   sudo mv restart-network-on-wake.sh /lib/systemd/system-sleep/
#   sudo chmod a+x /lib/systemd/system-sleep/restart-network-on-wake.sh

case $1 in
    pre )
        echo "Going to $2..."
        exit 0
        ;;

    post )
        echo "Waking up from $2..."
        if ! ping -q -c 1 -W 1 8.8.8.8 > /dev/null; then
            echo "IPv4 is down, restarting network-manager"
            service network-manager restart
        fi
        ;;

    * )
        echo "ERROR: got $1 which is not expected"
        exit 1
        ;;
esac


REGEXless dispatcher I've recently helped fellow SOFer to answer his question here: Routing in my PHP MVC framework

Then I thought

  Hey, why not improve this code and put in my snippets library.

This is what I came up with:

/**
 * @author Gajus Kuizinas <g.kuizinas@anuary.com>
 * @copyright Anuary Ltd, http://anuary.com
 * @version 1.0.3 (2011 12 07)
 * @param array $routes e.g., array('page/:id' => array('controller' => 'page', 'parameters' => array('id' => 1)));
 */
function ay_dispatcher($url, $routes)
{
    $url            = array
    (
        'original'  => $url,
        'path'      => explode('/', parse_url($url, PHP_URL_PATH))
    );

    $url['length']  = count($url['path']);

    foreach($routes as $filter => $data)
    {
        // reset the parameters every time in case there is partial match
        $parameters     = array();

        $filter         = array
        (
            'original'  => $filter,
            'path'      => explode('/', $filter)
        );

        $filter['length']   = count($filter['path']);

        // this filter is irrelevent
        if($url['length'] <> $filter['length'])
        {
            continue;
        }

        foreach($filter['path'] as $i => $key)
        {
            if(strpos($key, ':') === 0)
            {
                $parameters[substr($key, 1)]    = $url['path'][$i];
            }
            // this filter is irrelevent
            else if($key != $url['path'][$i])
            {
                continue 2;
            }
        }

        if(!array_key_exists('parameters', $data))
        {
            $data['parameters'] = array();
        }

        $data['parameters'] = array_merge($data['parameters'], $parameters);

        return $data;
    }

    return FALSE;
}

The 1.0.2 version https://gist.github.com/68bcb1e087237bde76ea.

I'd like to hear comments on the code logic. It is most certainly better in terms of performance than any regex dispatcher. Did I forget any essential features, though?

The original index is never used and can currently be removed from both url and filter.

Your use of temporary variables is actually making the code less easy to read.

Replace the length indexes which are just a temporary values to check for validity.  I call this pathCount in the code below.
Replace parameters and the merging that you do with just continually adding to the array.  Adding to the array should overwrite any existing values equivalently to the array_merge operation.

I would also change the name of $key to something else as it confuses me with it being an array key. (I would call it $component or something similar).  I didn't make this change in the code below.

Edit:

In fact further to this $url and $filter are also temporary variables hurting your readability.  They could be replaced at the appropriate points with:

 $urlPath = explode('/', parse_url($url, PHP_URL_PATH);
 $filterPath = explode('/', $filter);

The code then becomes (not tested):

function ay_dispatcher($url, $routes)
{
   $urlPath = explode('/', parse_url($url, PHP_URL_PATH));

   foreach($routes as $filter => $data)
   {
      $filterPath = explode('/', $filter);

      if (count($filterPath) !== $count($urlPath))
      {
          continue;
      }

      if(!array_key_exists('parameters', $data))
      {
          $data['parameters'] = array();
      }

      foreach($filterPath as $i => $key)
      {
          if(strpos($key, ':') === 0)
          {
              $data['parameters'][substr($key, 1)] = $urlPath[$i];
          }
          // this filter is irrelevent
          else if($key !== $urlPath[$i])
          {
              continue 2;
          }
      }

      return $data;
   }

   return FALSE;
}

I am not sure that your logic for counting the filter path should just be a count?  Perhaps you need a function for that as parameters used in the filter would effect the count?


HackerRank - Array Manipulation - Follow-up This is a follow-up question for HackerRank - Array Manipulation

  Problem

  You are given a list (1-indexed) of size \$n\$, initialized with zeroes.
  You have to perform \$m\$ operations on the list and output the maximum of
  final values of all the  elements in the list. For every operation,
  you are given three integers \$a\$, \$b\$ and \$k\$ and you have to add value
  \$k\$ to all the elements ranging from index \$a\$ to \$b\$ (both inclusive).

  Example input

5 3
1 2 100
2 5 100
3 4 100

  Expected output

200

  Explanation

  After first update list will be:

100 100 0 0 0

  After second update list will be:

100 200 100 100 100

  After third update list will be:

100 200 200 200 100

  So the required answer will be: 200

Final solution

I changed the way of storing the values, which now get indexed into a map. I think now it's efficient in time and space, my main questions left are about style (taking into account the impositions of the hacker rank input output).

#include <algorithm>
#include <iostream>
#include <map>

int main() {
    int n; int m;
    std::cin >> n >> m;
    using val_type = long long;
    std::map<int,val_type> map;
    while(m--){
        val_type start, end, val;
        std::cin >> start >> end >> val;
        map[start] += val;
        map[end+1] -= val;
    }
    val_type max{0};
    val_type partial_sum{0};
    for (const auto& el : map) {
        partial_sum += el.second;
        if (partial_sum > max) {
            max = partial_sum;
        }
    }
    std::cout << max;
    return 0;
}

I don't like for example that I need to declare the variable partial_sum outside the for loop, being in the same scope as max. Any idea on how to write this partial sum to find the max cleaner?

Whitespace, let your code breathe

Your code has a few logical sections, you should use empty lines to break them appart visually. It makes your code easier to read.

Personal opinion: front-load your type aliases

General wisdom in C++ is to define things as close as possible to their first use, but that's generally meant for objects with lifetimes. I personally prefer to put any type alias at the start of its scope.

algorithm is unused, so it should not be included

Simple enough: do not #include stuff you don't need.

That's kind of it, really, good job!

Edit: to answer your question, you could finagle something with std::accumulate, or just create a scope for partial_sum, but it's not worth the trouble. What you have it fine.


Verify two numbers from a list will add up to a number in the same list using Python I am trying to solve this question:

Given a list of integers, verifies if any two numbers from this list add up to another number in the same list.

I came up with:

def func(l):
    sum_set = set() # using a set to store sums of two numbers.
    for index in range(len(l)-1):
        counter = 1
        while counter + index < len(l):
            sum = l[index] + l[index+counter]
            sum_set.add(sum)
            counter += 1
    for number in l:
        if number in sum_set:
            return True
    return False

print(func([1,2,3,4]))
print(func([1,2,1,4]))
print(func([1,1,1]))
print(func([1]))
print(func([1,2,5,9]))
print(func([2,1,1]))
print(func([1,-1,0]))

From the tests I ran above, my function is working. But there is at least one way I can think of my approach is lacking of:

I should filter the original list to get rid of duplicate numbers. If a number appears more than 2 times, its rest of appearance should be disregarded. E.g. l = [1,1,1,1,2] should be optimised to l = [1,1,2]

Is there any other aspect that I can improve my code snippet on to make it more Pythonic / more efficient?

The thing to be concerned with when writing functions like this is the size of the data it's going to be processing. If you're only ever going to be dealing with small sets of data (less than say 100 items) then there really isn't an issue with the way this is written. At the scale of maybe tens of thousands of items this starts to be a problem.

I'm not a data scientist or anything, I'm just a web developer and I very rarely delve into trying to optimise code, so any advice I'm giving is based on my personal experience + uni, and doesn't utilize math libraries in Python like numpy which you might want to look into depending on what this code is for. Another answerer could likely help more with this specific circumstance, my answer is going to be a bit more general; assuming you're going to be dealing with large amounts of data (or otherwise prematurely optimising is sometimes considered the root of all evil):

EDIT: I just noticed that one of your examples included negative numbers, so the example code below will need to be adapted with that in mind since it affects the movement of i and j; sorry about that, but the code provided can still be run as an example with positive numbers.

Ignoring data types used for the most part & how pythonic the code is, I see probably two areas for improvement. The first I'll touch on at the end in a code example & may depend a bit on your particular data-set as to how helpful it will be, but presently, the code has no chance of finishing early (assuming random inputs that'd give you an substantial speedup especially if your data is likely to find a match in the early items of the list).

However, the primary areas for improvement can be determined through analysing your code via Big-O / Big(O) notation. I'd suggest a read of the concept if you're unfamiliar, but according to Wikipedia:

  big O notation is used to classify algorithms according to how their running time or space requirements grow as the input size grows

Given the following section of your code:

for index in range(len(l)-1):  # (1)
    ...
    while counter + index < len(l): # (2)
        ...
        sum_set.add(sum) # (3)
        ...
for number in l: # (4)

If we look at, for instance, a 100,000 element array containing the numbers between 1 and 100,000, if we run your code as it's written, we will end up running the sum_set.add(sum) statement (3) about 4.99 billion times. At just 10,000 elements, on my machine, the code as written takes multiple seconds to finish.

Looking at (1), we see this statement runs through all elements in the list, therefore O(N); the time taken for the outer loop depends on a linear relationship to the input, e.g. O(N) means an input array of 200 elements, ignoring any constant time overhead, should take ~roughly~ 200x longer than an array with 1 element. Line (2) passes through N-1 elements on the first loop, then N-2, ... finally 1 item in last loop; averaging out at half as many loops as the outer index, but is still O(N) as it's linearly related to the amount of items included in the list. As you have an O(N) loop within an O(N), this gives it the overall O(N^2) performance.

(4) is tricky to estimate, as it depends on the passed in data. Iterating through the list l is O(N) again, and if we assume worst case, each element in sum_set could be unique, i.e. if the passed in array was something like [1, 11, 111, ...], which would mean there are ~N^2 terms in sum_set, actually causing this loop to degrade to O(N^3) performance. Best case, the sum_set is very small, but even assuming only 1 item, that would still be O(N) as we need to touch each element in l. Additionally, sum_set could potentially become very large, causing the loop not only to be expensive in time but also possibly memory (although as you are using a set there aren't going to be any duplicates, so it will totally depend on the input data. E.g. 100,000 elements, but the values range between 0 & 100, so sum_set ranges between 0 & 200).

I'd say your suggestion of pre-filtering to remove duplicates is a good idea, ideally something O(N) like the following (though there are likely more optimal approaches):

>>> input_list = [1,1,1,1,2,2,2,3,3,3]
>>> filtering_dict = collections.defaultdict(int)
>>> for item in input_list:
...     filtering_dict[item] += 1
...
>>> newList = []
>>> for key, value in filtering_dict.items():
...     newList.append(key)
...     if value > 1:
...         newList.append(key)
...
>>> newList
... [1, 1, 2, 2, 3, 3]

I'd then try and take advantage of sorting the array using an O(Nlog(N)) sort like Mergesort / Quicksort, or depending on your data an O(N) sort like Counting Sort. If you know your data is going to be ordered, you can skip this step. With sorted data, we don't have to use the sum_set set; we can instead pick an index in the array & determine whether it is the total of two other elements. We know that any index we suspect to be our sum will have to be made up of elements that are lower indexes than it in the list, i.e. [1, 2, 3, 4, 5] -> If we start looking at 3, we know we don't need to consider elements 4 & 5, as they will be larger than 3, so couldn't possibly sum to it. Finally, the halfway point for a number is also relevant, I.e. [1, 3, 5, 7, 9, 11, 99, 117] if we're looking at 99, we first look to add the next lowest index & the first index; however, since 11 < 99/2 we know we won't be able to find a match that adds to 99; on average this should be another speedup assuming the data isn't perfectly uniform.

Finally, since we aren't pushing results into sum_set & only checking once for each total, this will cause some repetition in our search. However, since we can return immediately upon finding a match, our best/average case just got a lot better.

def func2(l):
    # l = filter_inputs(l)
    # l.sort()
    for index in range(2, len(l)):
        i = 0
        j = index - 1
        half_val = l[index] / 2;
        while ( i < j and l[i] <= half_val and l[j] >= half_val ):
            if l[index] > l[i] + l[j]:
                i = i + 1
            elif l[index] < l[i] + l[j]:
                j = j - 1
            else:
                print(str(l[i]) + " + " + str(l[j]) + " = " + str(l[index]))
                return True
    return False

Using timeit, and comparing func & func2, using code like the following:

from timeit import timeit
timeit('func2(<TEST ARRAY>)', setup="from __main__ import func2; import random", number=20)
# Use the function that's been predefined in the python interpreter,
# pass the array, repeat the test 20 times, and output how many seconds taken

# All times listed are in seconds for 20 repeats

# list of all positive odd numbers up to 9,999 == [x for x in range(1,100000) if x % 2 is 1]
# (absolute worst-case condition for func2, behaves nearly identically to your original function)
# (this is because i, j, and half_val achieve 0% improvement & every value is checked)
# func2      # func
>>> 73.89    >>> 73.86

# all integers between 1 & 9,999 == [x for x in range(1,10000)]
# func2     # func
>>> 0.02    >>> 297.54

# 9,999 random integers between 1 & 10,000 == [random.randint(0,10000) for x in range (1,10000)]
# Re-ran this one about 5 times for func2 since it was so quick,
# with 20 loops its lowest was 0.25 & highest 0.32 seconds taken
# You'll also need to sort 'l' for this one to work with func2
# func2     # func
>>> ~0.3    >>> 312.83

Again, with a low number of entries in l, the cost of removing duplicates & sorting the array would probably cause my function to run slower than yours. None of these speedups change the fact that the overall operation is worst-case O(N^2); however, they should drastically improve the best/average-case scenarios. Additionally, getting a large average speedup with an O(N^2) operation is huge when it comes to a big dataset, as it will be the limiting factor:

I.e. 100,000 items:

Filtering     = ~2x O(N)    = ~200,000 ops
Mergesort     = O(NLog(N))  = ~1.6 million ops
Main Loop     = 1/2 O(N^2)  = ~5 billion ops

If you can come up with a better way to take advantage of the data such that you can get O(N^2) down to O(Nlog(N)) or similar, I think that'd be key here for optimizing the worst-case scenario.


TPL Thread Leak and Memory Leak I'm trying to track down what I suppose to be memory/thread leak in one of my programs.

My program uses a function to upload a file to a Windows Azure Storage Blob. In order to make this function resilient to various transient error conditions (such as intermittent network failures, etc.), I want to make use of the Transient Fault Handling Application Block for Windows Azure "topaz", part of Enterprise Library, which includes a configurable retry mechanism.

Also, in order to circumvent timeout conditions when dealing with large file and improve and reduce the scope for portions of the upload that should be repeated in case of failure, I try to upload the specified file stream in chunks, that each get uploaded independently.

Finally, in order to adhere to best practices using Windows Azure Storage, I make use of asynchronous operations as much as possible when dealing with Windows Azure, in an attempt to improve the scalability of my apps.

Here is the resulting snippet function :

    public static Task ChunkedUploadStreamAsync(CloudBlockBlob blob, Stream source, BlobRequestOptions options, int chunkSize, RetryPolicy policy)
    {
        var blockids = new List<string>();
        var blockid = 0;

        var count = 0;
        var bytes = new byte[chunkSize];

        // first create a list of TPL Tasks for uploading blocks asynchronously

        var tasks = new List<Task>();

        while ((count = source.Read(bytes, 0, bytes.Length)) != 0)
        {
            var id = Convert.ToBase64String(BitConverter.GetBytes(++blockid));

            Func<Task> uploadTaskFunc = () => new TaskFactory()
                .FromAsync(
                    (asyncCallback, state) => blob.BeginPutBlock(id, new MemoryStream(bytes, 0, count), null, null, null, null, asyncCallback, state)
                    , blob.EndPutBlock
                    , null
                )

                .ContinueWith(antecedent => blockids.Add(id), TaskContinuationOptions.NotOnFaulted);

            tasks.Add(policy.ExecuteAsync(uploadTaskFunc));
        }

        return new TaskFactory().ContinueWhenAll(
            tasks.ToArray(),
            array =>
                {
                    // propagate exceptions and make all faulted Tasks as observed
                    Task.WaitAll(array);

                    // create continuation task for committing uploaded blocks
                    Func<Task> commitTaskFunc = () => new TaskFactory()
                        .FromAsync(
                            (asyncCallback, state) => blob.BeginPutBlockList(blockids, asyncCallback, state)
                            , blob.EndPutBlockList
                            , null);

                    policy
                        .ExecuteAsync(commitTaskFunc)
                        .Wait();
                });
    }

With Performance Monitor, I can observe that after calling this function, the number of threads and amount of memory used by my program is increasing significantly. For instance, here is a snapshot of just one call of this function :

Please, can someone advise as to where I'm doing something wrong. Please, suggest a better design if necessary.

There are several issues in your code that make it error-prone:

you are using a single byte array to feed all memory streams, thus the same data may be sent in different chunks. Same stands for count.
the order of chunks in blockids can be different to the one you've sent so resulting BLOB will have incorrect order of blocks.
there is a potential thread contention on blockids as different threads may add a new element at the same time causing all sorts of problems like missing blocks in resulting list, incorrect block IDs etc.
use Task.Factory instead of new TaskFactory()
if you create a task and immediately wait for it then you don't need that task, so you can replace the last BeginPutBlockList with synchronous version

Other than that I don't see memory leak from the graphs you provided. Threads may still be alive in a thread pool, and memory may not be collected by GC yet.

Here is what I've got as a result (note that it may consume more memory as you need to store all the chunks that are currently being sent in memory):

    private static Task PutBlockAsync(CloudBlockBlob blob, string id, Stream stream, RetryPolicy policy)
    {
        Func<Task> uploadTaskFunc = () => Task.Factory
            .FromAsync(
                (asyncCallback, state) => blob.BeginPutBlock(id, stream, null, null, null, null, asyncCallback, state)
                , blob.EndPutBlock
                , null
            );
        return policy.ExecuteAsync(uploadTaskFunc);
    }

    public static Task ChunkedUploadStreamAsync(CloudBlockBlob blob, Stream source, BlobRequestOptions options, int chunkSize, RetryPolicy policy)
    {
        var blockids = new List<string>();
        var blockid = 0;

        int count;

        // first create a list of TPL Tasks for uploading blocks asynchronously
        var tasks = new List<Task>();

        var bytes = new byte[chunkSize];
        while ((count = source.Read(bytes, 0, bytes.Length)) != 0)
        {
            var id = Convert.ToBase64String(BitConverter.GetBytes(++blockid));
            blockids.Add(id);
            tasks.Add(PutBlockAsync(blob, id, new MemoryStream(bytes, 0, count), policy));
            bytes = new byte[chunkSize]; //need a new buffer to avoid overriding previous one
        }

        return Task.Factory.ContinueWhenAll(
            tasks.ToArray(),
            array =>
            {
                // propagate exceptions and make all faulted Tasks as observed
                Task.WaitAll(array);
                policy.ExecuteAction(() => blob.PutBlockList(blockids));
            });
    }


Sequence generation problem - Fibonacci Below is the representation and user interface fib_sequence for generating a Fibonacci sequence:

empty_rlist = None
#Representation - start
#Constructor
def rlist(first, rest):
    return (first, rest)

#Selector
def first(s):
    return s[0]

def rest(s):
    return s[1]

#Representation - end

def fib_sequence(k):
    prev, curr = 1, 0
    def generate_sequence(prev, curr, k):
        if k == 0:
           return empty_rlist
        elif k == 1:
           return (curr, empty_rlist)
        else:
           return rlist(curr, generate_sequence(curr, prev+curr, k - 1))
    return generate_sequence(prev, curr, k)

Program output:

>python -i fibonacci_sequence.py
>>> fib_sequence(0)
>>> fib_sequence(1)
(0, None)
>>> fib_sequence(2)
(0, (1, None))
>>> fib_sequence(3)
(0, (1, (1, None)))
>>> fib_sequence(4)
(0, (1, (1, (2, None))))
>>> fib_sequence(5)
(0, (1, (1, (2, (3, None)))))

The above program provides representation as (element, restOfTheList).

We have this sequence containers in Python:

>>> tuple =  (1, 2, 3, 4)
>>> r = range(1,5)
>>> str = '1234'

Is the representation of the aforementioned Python sequence containers same as the above representation of the Fibonacci sequence abstraction?

If no, can we make the representation of the Fibonacci sequence abstraction better, basically to forward this sequence for further processing by sequence processing functions like map() filter() sum() etc.?

You should call rlist() consistently, and write

elif k == 1:
    return rlist(curr, empty_rlist)

… for reasons that will become apparent.  Also be sure to use four spaces per level of indentation, as prescribed by PEP 8.

Are your tuple, range, and string the same as each other?  No, comparing them using == will obviously return False.  What they all have in common, though, is that they are all iterables.  Being iterable means that you can use them in for e in iterable expressions — either in a loop or a generator.

That means that if you want to be able to write things like print([e for e in fib_sequence(5)]), you'll need to make your rlist type iterable.  Here's one way you could accomplish that.

class rlist(tuple):
    def __new__(cls, first, rest):
        return super(rlist, cls).__new__(cls, (first, rest))

    def __iter__(self):
        seq = self
        while seq is not None:
            yield first(seq)
            seq = rest(seq)

This works as long as you have applied the rlist() fix to fib_sequence() as mentioned above.


Timer stop watch in AngularJS I wrote a script for a timer. Is there any way to improve the code, with fewer number of lines?

JSBIN

var app = angular.module('app', []);
app.controller('main', function($scope, $interval) {

    $scope.timer = 0;
    var clickme = true,
        startfun;
    $scope.status = "Start";
    var foo = function() {
        $scope.timer += 1;
    };
    $scope.start = function() {
        if (clickme === true) {
            $scope.status = "Stop";
            clickme = false;
            startfun = $interval(foo, 1000);

        } else if (clickme === false) {
            clickme = true;
            $interval.cancel(startfun);
        }

    };
  $scope.reset=function(){

    $interval.cancel(startfun);
    $scope.timer = 0;
    $scope.status = "Start";
    clickme = true;
  };
});

Aside from the issue of setIntervals resolution/accuracy there's a lot of room for improvement in your angular.

angular.module('app', []);

"app" isn't a good name for your module "fruitjs.timer" would be better as it is more descriptive and has a much lower chance of naming clashes.

app.controller('main', function($scope, $interval) {

Again "main" isn't a great name for your controller "timerController" would be more explicit.

You should also utilise angular's support for safe minification:

app.controller('main', ['$scope', '$interval', function($scope, $interval) {
    // code
}]);

Notice that the second parameter is now an array declaring the names of the services/modules you require and finally your controller function (this is the recommended way).

More bad naming clickme, foo, startfun - none of these names mean anything.

$scope.status isn't the status at all - it's the potential action.

You don't need the clickme variable (which should at least be camel cased to clickMe). You have 2 potential actions "Start" and "Stop" - you don't need the additional boolean to track what state you're currently in.

Use either 2 or 4 spaces for your indentation - don't mix and match.

You don't change the label after you stop the timer.

Code with above changes

var app = angular.module('fruitjs.timer', []);
app.controller('timerController', [ '$scope', '$interval', function($scope, $interval) {
    var interval, incrementTimer, actions;
    actions = { start: "Start", stop: "Stop" };

    $scope.timer = 0;
    $scope.action = actions.start;

    incrementTimer = function() {
        $scope.timer += 1;
    };

    $scope.toggle = function() {
        if ($scope.action === actions.start) {
            $scope.action = actions.stop;
            interval = $interval(incrementTimer, 1000);

        } else if ($scope.action === actions.stop) {
            $scope.action = actions.start;
            $interval.cancel(interval);
        }
    };

    $scope.reset = function () {
        $interval.cancel(interval);
        $scope.timer = 0;
        $scope.action = actions.start;
    };
}]);

All you need to do now is solve the problem of keeping time accurately :)


Thread-safe removewhere I'm trying to implement a HashSet that it's accessed from different thread for querying, adding or deleting elements. It doesn't make sense to use a Dictionary for I got several search keys so I won't get benefited much for the speed provided on querying.

My real problem is when trying to remove several items matching a given definition without locking the List for to long, I use this code right now

   public static void DeleteFromHashSet(string companyId, int deviceId)
   {
        lock (setLock)
        {
            set.RemoveWhere(x => x.CompanyId == companyId && x.y == deviceId);
        }
    }

but this has the problem of locking the access for querying the hashset for the duration of the RemoveWhere, I can change that with a foreach and just lock for each individual item but I wanted to know if there's a better way.

Update:

This is what I finally did, in case it's of some help

    class DeviceLocator
{
    #region  fields

    private HashSet<DeviceLocator> m_deviceList = new HashSet<DeviceLocator>();
    private object setLock = new object();
    private ReaderWriterLockSlim cacheLock = new ReaderWriterLockSlim();

    public  void AddLocator(DeviceLocator locator)
    {
        DeleteLocator(locator);

        cacheLock.EnterWriteLock();

        m_deviceList.Add(locator);

        cacheLock.ExitWriteLock();
    }

    public void AddLocators(IEnumerable<DeviceLocator> locators)
    {
        foreach (DeviceLocator locator in locators)
        {
            AddLocator(locator);
        }
    }

    #endregion Add items

    #region Query items

    public DeviceLocator GetLocator(string serialNumber)
    {
        try
        {
            return ReadSafely(x => x.SerialNumber == serialNumber).Single();
        }
        catch
        {
            throw new LocatorNotFoundException(ExceptionTexts.LocatorNotFoundException, null, ImportanceLevel.Low, m_ID);
        }
    }

    public IEnumerable<DeviceLocator> GetLocatorsFromCompany(string companyId)
    {
        return ReadSafely(x => x.CompanyID == companyId);
    }

    public IEnumerable<string> GetLocatorsStringFromCompany(string companyId)
    {
        return GetLocatorsFromCompany(companyId).Select(y => y.SerialNumber);
    }

    public IEnumerable<DeviceLocator> GetsLocatorBasedOnConnectionType(ConnectionType type)
    {
        return ReadSafely(x => x.ConnectionType == type); ;
    }

    private IEnumerable<DeviceLocator> ReadSafely(Func<DeviceLocator, bool> predicate)
    {
        try
        {
            cacheLock.EnterReadLock();

            var result = m_deviceList.Where(predicate);

            return result;
        }
        catch
        {
            throw;
        }
        finally
        {
            cacheLock.ExitReadLock();
        }
    }

    public  void DeleteLocator(DeviceLocator locator)
    {
        DeleteSafely(x => x.CompanyID == locator.CompanyID && x.SerialNumber == locator.SerialNumber);
    }

    /// <summary>
    /// Removes all locators from given company
    /// </summary>
    /// <param name="companyId"></param>
    public  void DeleteLocator(string serialNumber)
    {
        DeleteSafely(x => x.SerialNumber == serialNumber);
    }

    public  void DeleteLocatorsFromCompany(string companyId)
    {
        DeleteSafely(x => x.CompanyID == companyId);
    }

    private  void DeleteSafely(Predicate<DeviceLocator> predicate)
    {
        try
        {
            cacheLock.EnterWriteLock();

            m_deviceList.RemoveWhere(predicate);
        }
        catch
        {
            throw;
        }
        finally
        {
            cacheLock.ExitWriteLock();
        }
    }
}

Firstly, the biggest problem is that ReaderWriterLockSlim implements IDisposable so your DeviceLocator class should too as it holds a reference at the class level.

You could make better use of the locks if you moved things round a bit:

public void AddLocators(IEnumerable<DeviceLocator> locators)
{
    cacheLock.EnterWriteLock();
    foreach (DeviceLocator locator in locators)
    {
        AddLocatorUnsafe(locator);
    }
    cacheLock.ExitWriteLock();
}

private void AddLocatorUnsafe(DeviceLocator locator)
{
    m_deviceList.RemoveWhere(x => x.CompanyID == locator.CompanyID
            && x.SerialNumber == locator.SerialNumber);

    m_deviceList.Add(locator);
}

Now you only acquire the lock once and do everything while holding that lock instead of having to keep acquiring and releasing between items.

Don't do this:

try
{
    // ...
}
catch
{
   throw;
}
finally
{
    // ...
}

That catch block is entirely redundant - remove it.

Some people would probably tell you off for using regions. I'm not really that fussed about it but you should be aware that a lot of C# developers don't like using regions for splitting up classes.


Using Enum in java to store a fixed set of mutable objects I already posted this on Stack Overflow, but someone suggested to post it to Code Review instead.

I am creating an Android app that draws to the canvas. For this, I have defined rectangular screenareas that I want to draw to the canvas. As I am always drawing a fixed set of screenareas, I was thinking of using Enum (as Enum is designed for fixed sets).

Here is my enum:

public enum LayoutEnum {
    FULLSCREEN(
            new ScreenArea(
                new Rect(
                        0,
                        0,
                        MainActivity.getDevice().getWidth(),
                        MainActivity.getDevice().getHeight()),
                Attributes.BG_PAINT)),
    LOGO_AREA(
            new ScreenArea (
                    new Rect(
                            (int) (0.3 * FULLSCREEN.getScreenArea().getArea().width()),
                            (int) (0.3 * FULLSCREEN.getScreenArea().getArea().width()),
                            (int) (FULLSCREEN.getScreenArea().getArea().width() - 0.3 * FULLSCREEN.getScreenArea().getArea().width()),
                            (int) (0.7 * FULLSCREEN.getScreenArea().getArea().width())),
                    Attributes.BG_PAINT)
    );

    private ScreenArea screenArea;

    LayoutEnum(ScreenArea screenArea) {
        this.screenArea = screenArea;
    }

    public ScreenArea getScreenArea() {
        return screenArea;
    }
}

ScreenArea is a simple class that holds a Rect and a Paint and contains a draw method (and some getters and setters).

The question I have, is: is this a good approach? On one hand I am working with a fixed set of variables. On the other hand, these variables are mutable and I can change their attributes (e.g., using the getters and setters). For example, I can call FULLSCREEN.getScreenArea().getPaint().setColor(Color.BLUE)

When you look at Enum it says it is

  a special data type that enables for a variable to be a set of
  predefined constant

So I do have a fixed set, it is predefined, but not necessarily constant.

My original approach was to define a class called Layout which contained a HashMap of Screenarea's. In that case, I was using e.g., Layout.get("fullscreen").draw(canvas) to draw the screenarea to the canvas. In this new approach I am using e.g., FULLSCREEN.getScreenArea().draw(canvas).

One of the reasons I would like to switch is to introduce a typesafe solution. Of course, it would also be possible to switch from a HashMap to an EnumMap and store the names of my screenareas in an Enum.

Hope you can point me in the right direction: a direction that not only works (the above is already working) but is also acceptable and doesn't smell.

Enums are meant to be immutable. I'd  go with the EnumMap.


Applying Laplacian smoothing to vertices in a mesh I'm totally new in Python and I wrote some code. It is a simple algorithm to smooth objects. I need to find adjacent vertices in mesh and sum their coordinates and after that divide by a number of adjacent vertices. This algorithm is called Laplacian Smoothing.

Could anybody point out how to make it better?

n = mesh.VPos.shape[0]
final = []

for i in range(n):
    neighbors = mesh.vertices[i].getVertexNeighbors()
    indices = map(lambda x: x.ID, neighbors)
    z = len(indices)

    help = []
    for j in indices:
        help.append([mesh.VPos[j]])
    final += [sum(x)/z for x in zip(*help)]

final = np.array(final)
mesh.VPos = final

The code is not that bad at all.

Here are a few notes:

Use list comprehension – it's more readable than maps & lambdas

indices = [x.ID for x in neighbors]

indices isn't really necessary. Instead of

indices = map(lambda x: x.ID, neighbors)
z = len(indices)

help = []
for j in indices:
    help.append([mesh.VPos[j]])
final += [sum(x)/z for x in zip(*help)]

you can simply do this

help = ([mesh.VPos[j.ID]] for j in neighbors)
z = len(neighbors)
final.extend(sum(x)/z for x in zip(*help))

help is a generator expression here, since you don't really need a list. That will probably save some overhead of creating the list. You can also avoid creating a new list of sum(x)/z and directly append all values to final using the extend method.

Instead of reassigning a numpy array to final, I'd just do

mesh.VPos = np.array(final)

I'm not completely sure, but I think the inner lists [mesh.VPos[j]] combined with zip(*help) will amount to the same as just

for j in indices:
    help.append(mesh.VPos[j])
final += [sum(help) / z]

You can create a numpy.ndarray right at the beginning instead of using a Python list and then converting it.

So after the edits it'll look something like this

n = mesh.VPos.shape[0]
final = np.empty(n)  # change dtype if you need to

for i in range(n):
    neighbors = mesh.vertices[i].getVertexNeighbors()
    help = (mesh.VPos[j.ID] for j in neighbors)
    final[i] = sum(help) / len(neighbors)

mesh.VPos = final

Alternatively you can use numpy.mean to compute sum(help) / len(neighbors):

n = mesh.VPos.shape[0]
final = np.empty(n)  # change dtype if you need to

for i in range(n):
    neighbors = mesh.vertices[i].getVertexNeighbors()
    help = [mesh.VPos[j.ID] for j in neighbors]
    final[i] = np.mean(help)

mesh.VPos = final

Note that you cannot give a generator argument to numpy.mean, it has to be a list (or anything with __len__ method).


Text based fight game I learned code about a week ago and I wanna hear some recommendations and stuff I could add to my code. This is a fight system I made for my text based game. Please tell me what I could do better.

import random
import time
import sys
def fight_snake():
    fdmg1 = int(random.randint(1,7))
    edmg1 = int(random.randint(1,6))
    print("You hit a", fdmg1)
    print("The Snake hits a", edmg1)
    time.sleep(1)
    if edmg1 > fdmg1:
        print("You died")
    elif edmg1 < fdmg1:
        print("You killed the Snake")
    else:
        print("You didnt kill the Snake but you managed to escape")

def fight_enemy_a():
    fdmg1 = int(random.randint(1,7))
    edmg_a = int(random.randint(4,10))
    print("You hit a", fdmg1)
    print("The Enemy hits a", edmg_a)
    time.sleep(1)
    if edmg_a > fdmg1:
        print("You died")
    elif edmg_a < fdmg1:
        print("You killed the Enemy")
    else:
        print("You didnt kill the Enemy but you managed to escape")

def fight_enemy_b():
    fdmg1 = int(random.randint(1,7))
    edmg_b = int(random.randint(4,9))
    print("You hit a", fdmg1)
    print("The Enemy hits a", edmg_b)
    time.sleep(1)
    if edmg_b > fdmg1:
        print("You died")
    elif edmg_b < fdmg1:
        print("You killed the Enemy")
    else:
        print("You didnt kill the Enemy but you managed to escape")

def fight_enemy_c():
    fdmg1 = int(random.randint(1,7))
    edmg_c = int(random.randint(1,7))
    print("You hit a", fdmg1)
    print("The Enemy hits a", edmg_c)
    time.sleep(1)
    if edmg_c > fdmg1:
        print("You died")
    elif edmg_c < fdmg1:
        print("You killed the Enemy")
    else:
        print("You didnt kill the Enemy but you managed to escape")

Design

As it stands, your current design is very rigid, and not extensible in any way. The best approach for a situation like this could be to use Object Oriented Programming, but in accordance with the Python talk, Stop Writing Classes, all of your code can be shortened to one function.

def fight_enemy(enemy_name, min_enemy_damage, max_enemy_damage, min_player_damage, max_player_damage):
    enemy_damage_dealt = random.randint(min_enemy_damage, max_enemy_damage)
    player_damage_dealt = random.randint(min_player_damage, max_player_damage)

    if enemy_damage_dealt > player_damage_dealt:
        print("Uh-oh! You died!")
    elif enemy_damage_dealt < player_damage_dealt:
        print("You killed the {enemy_name}".format(enemy_name=enemy_name))
    else:
        print("You walk away unscathed, but the {enemy_name} still lives.".format(enemy_name=enemy_name))

Here's an example function call:

fight_enemy("Enemy", 1, 10, 1, 10)

Naming

Many of your variable names are so cryptic that it's understand what they're doing, even in the context of other code. Here's a small list of some of your bad variable names.

fdmg1
edmg_c

While identifying bad names is important, fixing them is even better. Here's a small list of tips to follow when naming variables.

Don't make the name too short. (Unless you're doing stuff like coordinates, with x, and y.)
Don't include abbreviations unless the name is long, and the abbreviations are still readable.
Use underscores _ to separate words.

Nitpicks

When getting an value from random.randint(), there's no need to convert to an integer using int. random.randint() already returns an integer value.

You should also be using string formatting with str.format, rather than passing multiple arguments to print(). For example, the following line:

print("The Enemy hits a", edmg_b)

Would become:

print("The Enemy hits a {damage}".format(damage=edmg_b))

For more on str.format, see the docs.

You also have to useless imports:

import time
import sys

If you don't use the contents of these modules, the imports can be removed.

In short, I've refactored your code down to this:

import random

def fight_enemy(enemy_name, min_enemy_damage, max_enemy_damage, min_player_damage, max_player_damage):
    enemy_damage_dealt = random.randint(min_enemy_damage, max_enemy_damage)
    player_damage_dealt = random.randint(min_player_damage, max_player_damage)

    if enemy_damage_dealt > player_damage_dealt:
        print("Uh-oh! You died!")
    elif enemy_damage_dealt < player_damage_dealt:
        print("You killed the {enemy_name}".format(enemy_name=enemy_name))
    else:
        print("You walk away unscathed, but the {enemy_name} still lives.".format(enemy_name=enemy_name))

Hope this helps! Enjoy!


Finding the indices of multiple words in a string In response to a recent SO question, I answered with the following code, and was wondering if anyone had any input on it. I'm always looking for an opprotunity to learn!

import java.util.HashMap;
import java.util.Map;
import java.util.Stack;

public class WordFinder {

    public static void main(String[] args) {
        String phrase = "0123hello9012hello8901hello7890";
        String[] words = { "hello", "90" };

        Map<String, Stack<Integer>> dest = findWordsInString(words, phrase);

        System.out.println(dest.toString());
    }

    public static Map<String, Stack<Integer>> findWordsInString(String[] words, String phrase) {
        Map<String, Stack<Integer>> dest = new HashMap<>();

        for (String word : words) {
            Stack<Integer> locations = new Stack<>();

            for (int loc = -1; (loc = phrase.indexOf(word, loc + 1)) != -1;) {
                locations.add(loc);
            }

            dest.put(word, locations);
        }

        return dest;
    }
}

Program Output:

{90=[9, 19, 29], hello=[4, 13, 22]}

Map<String, Stack<Integer>> dest = findWordsInString(words, phrase);

I don't see the need at all for a Stack here. And not a Map either.

Let's say that you would want to find the third occourance of the word, and then randomly the first, and the fifth, and the second... Then you would want to store the indices in a List, not a Stack.

And why deal with multiple words at the same time? The way that your method is written, it makes more sense to search for all occurances of one word at a time.

As for the very important for loop, while it is totally alright to write it like that, I would personally prefer a while loop with something like this: (there is a bunch of variations for how you could do this)

int loc = phrase.indexOf(word);
while (loc != -1) {
   locations.add(loc);
   loc = phrase.indexOf(word, loc + 1);
}

It makes it easier to read the code, in my opinion.


Reusable, generic save method I am developing a Python script that will process large amounts of data from an experiment. Files are read in, calculations are made, images are plotted and saved as .pngs, and calculated parameters are outputted to a .txt file.

When I save a file, the user is prompted if they want to overwrite any existing files. If yes, a file overwrite is attempted. If the file is locked/open in another program, a number is appended to the filename and then saved. If they do not want to overwrite, a number is appended to the filename.

In order to not have redundant code, I wrote a generic "save" method:

def _handle_file_save(path, save, overwrite, *args):
    """
    Handles file collisions when saving files. If "overwrite" is true,
    overwrites file, if "overwrite" is false or the file to be overwritten
    is locked from editing, appends a number on end of filename to avoid
    file collision.

    Args:
        path (str): the path to the file to be saved
        save(): The save function used to save file
        overwrite (bool): true if user wants to overwrite file
        args: generic tuple of arguments used for different save functions

    """
    if os.path.isfile(path):
        if overwrite:
            try:
                print "Saving \"" + path + "\""
                save(path, args)
            # Signifies problem writing file (lack of permissions, open
            # in another program, etc.)
            except EnvironmentError:
                # save as incremented file number
                print "Whoops! Looks like %s is locked!" % path
                path = gen_alt_filename(path)
                save(path, args)
        else:
            # save as incremented file number
            path = gen_alt_filename(path)
            print "Saving as \"" + path + "\" instead"
            save(path, args)
    else:
        print "Saving \"" + path + "\""
        save(path, args)
    print "\n"

I pass in a generic save function and any arguments that save function needs if any. Some file outputs don't require any arguments.

Flaws:

The client could pass in any arbitrary function as "save" and execute it. Is this a security risk?
The variable args gets passed to the save function regardless of whether that method actually takes any args (i.e. args could be empty, but gets passed anyway.
Doesn't check if appended filepath is also a locked/open file (could fix this with a while loop I suppose).

Possible improvements:

Use Python's functools.partial to reduce the function call's signature to one without "args" if args is empty.

Alternative:

Instead of passing a generic save function, I could replace "save(path, args)" with "new_file = open(path, 'w')" (i.e. test file write, if successful, return the path to that file and write there).
Don't merge code, let every save method handle its own collisions separately.

Nothing about any of this seems elegant. I'd greatly appreciate some advice on this, and how I can improve my coding strategies.

Security risk? The client could pass in any arbitrary function as "save" and execute it.

A client can change your code, and it's there fault for not using the function as intended.

  variable "args" gets passed to the save function regardless of whether that method actually takes any args

save(path, *args)

This way it will see there is no arguments and not error.

  Instead of passing a generic save function, I could replace "save(path, args)" with "new_file = open(path, 'w')"

This is a better idea. This is as things like json.dump use a file object.
You could make it so that it works in a with statement, and return the file object.

print "Saving \"" + path + "\""

You should use str.format.

print "Saving \"{}\"".format(path)

You can add the ability of making it work with with statements with the below. If you want to know about these special functions look here.

class _handle_file_save:
    def __init__(self, path):
        self.path = path
        self.file_handler = open(self.path)

    def __enter__(self):
        return self.file_handler

    def __exit__(self, exc_type, exc_value, traceback):
        self.file_handler.close()

Now, we can enter your function without the save parameter. This will allow us to do what you want.

class _handle_file_save:
    def __init__(self, path, overwrite, *args):
        self.path = path
        self.overwrite = overwrite
        self.args = args

    def _open(self):
        if os.path.isfile:

        # ... Just copy the rest.

However we now have the problem of

  Doesn't check if appended filepath is also a locked/open file

And so we can make it a recursive function!

class _handle_file_save:
    def _open_recursive(self, path):
        try:
            self.file_handler = open(path, *self.args)
            print "Saving \"{}\"".format(path)
        except EnvironmentError:
            self._open_recursive(gen_alt_filename(path))

Lets now get the _open function to work now.

class _handle_file_save:
    def _open(self):
        if os.path.isfile(self.path):
            if overwrite:
                self._open_recursive(self.path)
            else:
                self._open_recursive(
                    gen_alt_filename(self.path))
        else:
            self._open_recursive(self.path)

You may want to change it so there is only one if statement. However you may not want the first else to be recursive.

If we wrap it all up we get:

class _handle_file_save:
    def __init__(self, path, overwrite, *args):
        self.path = path
        self.overwrite = overwrite
        self.args = args
        self.file_handler = None

    def _open(self):
        if os.path.isfile(self.path):
            if overwrite:
                self._open_recursive(self.path)
            else:
                self._open_recursive(
                    gen_alt_filename(self.path))
        else:
            self._open_recursive(self.path)

    def _open_recursive(self, path):
        try:
            self.file_handler = open(path, *self.args)
            print "Saving \"{}\"".format(path)
        except EnvironmentError:
            self._open_recursive(gen_alt_filename(path))

    def __enter__(self):
        self._open()
        if self.file_handler is None:
            raise TypeError('self.file_handler never set.')
        return self.file_handler

    def __exit__(self, exc_type, exc_value, traceback):
        self.file_handler.close()

You may want to change the error in __enter__.

This should work, however I don't know how to generate a locked file. But it seems like you know how to check.

Also it has the added support of allowing you to put all your file checking functions, I'm looking at you gen_alt_filename, into it to if you wish.

It's more complex, however it would work better then passing a save function as usage is just like open.

with _handle_file_save('file', False, 'rb') as file_:
    # Do something with `file_`


Codeigniter 3: how can I avoid repeating this chunk of code in my controllers? I am working on a basic blog application in Codeigniter 3.1.8 and Bootstrap 4.

Several entities are present in all controllers (except Login.php and Register.php): static data, categories and pages.

$data = $this->Static_model->get_static_data();
$data['pages'] = $this->Pages_model->get_pages();
$data['categories'] = $this->Categories_model->get_categories();

Further more, in most controller, the code above appears more then one time.

I am afraid this is mot the only case of repetitive code in the application. (See the entire application, at its current state, on my Github account).

I am looking for specific and/or general advice from experienced PHP developers that would help me reduce code redundancy and make it more efficient.

What is the best way to avoid the repeating of the code above in my controllers?

I had a very short look at lightblog/application/controllers/Pages.php and I noticed that these two values were not used in the page() method:

$data['pages']
$data['posts']

Then I realized that you hand over data to other objects and they might be using these two values. Who knows? This makes your code very hard to understand and to debug.

Basically you're treating the $data array as a sort of object you hand over to other classes.

But wait, did I say 'object'? But what if it was? Clearly you have a need to keep data accessible, but why use an array that you need to recreate every time? Why not use a class?

Now I don't have any idea how your code is structure, and which class would be the best to use for this. But it would make sense to extend the basic CodeIgniter controller class to hold this data, like so:

class Data_Controller extends CI_Controller
{
    private $data;

    public function __construct()
    {
        parent::__construct();
        $this->data = $this->Static_model->get_static_data();
        $this->data['pages'] = $this->Pages_model->get_pages();
        $this->data['categories'] = $this->Categories_model->get_categories();
        $this->data['posts'] = $this->Posts_model->sidebar_posts($limit=5, $offset=0);
        $this->data['page'] = $this->Pages_model->get_page($page_id);
    }

    public function getData()
    {
        return $this->data;
    }

}

or something to the same effect and then extend this for your pages class:

class Pages extends Data_Controller {

    public function __construct()
    {
        // currently this whole method doesn't do anything, it can be removed
        parent::__construct();
    }

    public function page($page_id) {
        $data = $this->getData();
        ..........................

    }

}

And, of course, those controller classes that don't need all that data can just extend CI_Controller.

I'm sure there are other ways of doing this. I don't like the fact that now you still collect all that information without actually knowning if it is needed. I would make the retrieval of the data dependent on the fact that it is actually used. In other words use getter methods, not a data array. I'll give an example for 'pages' and 'posts', but you have to create the other ones yourself:

class Data_Controller extends CI_Controller
{

    public function getPages()
    {
        return $this->Pages_model->get_pages();
    }

    public function getCatagories()
    {
        return $this->Categories_model->get_categories();

    }

}

If you need to buffer the data you can do:

class Data_Controller extends CI_Controller
{

    public function getCatagories()
    {
        static $buffer = NULL;
        if (is_null($buffer)) $buffer = $this->Categories_model->get_categories();
        return $buffer;
    }

}

Keep in mind that this buffer will work for all instances of Data_Controller. That can be an advantage, or a disadvantage.

OK, that was a bit long. I hope you got some new ideas from my ramblings.


Client/Server, Asynchronous ping-pong exchange I'm mainly wondering about whether or not I have used the (TAP) async/await pattern correctly. I get a bunch of these warnings, "Because this call is not awaited, execution of the current method continues before the call is completed.", however that sounds like exactly what I want. Which is for the server to be able to handle clients independently and asynchronously.

As a side note, I know it's bad for performance to re-initialize the streams in for example HandleConnection, I just kept it that way for brevity's sake.

Client.cs:

using System.IO;
using System.Net;
using System.Net.Sockets;
using System.Threading.Tasks;

class Client {
    TcpClient tcpClient;
    public void Connect() {
        ConnectAsync();
    }

    public async void Ping() {
        await SendMessage("Ping!");
    }

    private async Task ConnectAsync() {
        tcpClient = new TcpClient();
        await tcpClient.ConnectAsync(IPAddress.Parse("127.0.0.1"), 3344);
        MainWindow.ClientLog("Connected to server!");

        HandleConnection(tcpClient);
    }

    private async Task HandleConnection(TcpClient tcpClient) {
        NetworkStream ns = tcpClient.GetStream();
        StreamReader sr = new StreamReader(ns);
        string message = await sr.ReadLineAsync();
        MainWindow.ClientLog("Received message from server: " + message);

        HandleConnection(tcpClient);
    }

    private async Task SendMessage(string message) {
        NetworkStream ns = tcpClient.GetStream();
        StreamWriter sw = new StreamWriter(ns);
        await sw.WriteLineAsync(message);
        await sw.FlushAsync();
        MainWindow.ClientLog("Sent Ping! to server.");
    }
}

Server.cs:

using System.Collections.Generic;
using System.IO;
using System.Net;
using System.Net.Sockets;
using System.Threading.Tasks;

class Server {
    List<TcpClient> connectedClients;
    public void Host() {
        connectedClients = new List<TcpClient>();
        HostAsync();
    }

    private async Task HostAsync() {
        IPEndPoint local = new IPEndPoint(IPAddress.Parse("127.0.0.1"), 3344);
        TcpListener mainListener = new TcpListener(local);

        mainListener.Start();
        MainWindow.ServerLog("Server started.");
        while(true) {
            TcpClient client = await mainListener.AcceptTcpClientAsync();
            MainWindow.ServerLog("Client connected!");

            connectedClients.Add(client);
            HandleClient(client);
        }
    }

    private async Task HandleClient(TcpClient client) {
        NetworkStream ns = client.GetStream();
        StreamReader sr = new StreamReader(ns);
        string message = await sr.ReadLineAsync();
        MainWindow.ServerLog("Message from client nr. " + connectedClients.IndexOf(client) + ": " + message);

        if (message == "Ping!") {
            await SendMessage(client, "Pong!");
        }

        HandleClient(client);
    }

    private async Task SendMessage(TcpClient client, string message) {
        NetworkStream ns = client.GetStream();
        StreamWriter sw = new StreamWriter(ns);
        await sw.WriteLineAsync(message);
        await sw.FlushAsync();
        MainWindow.ServerLog("Sent message to client: " + message);
    }

    private async Task SendMessages(IEnumerable<TcpClient> clients, string message) {
        foreach (TcpClient client in clients) {
            SendMessage(client, message);
        }
    }
}

As far as I know, it seems to be only a "warning". Async/Await are supposed to be used as a pair in most cases. Let's have a simple example based on your code :

 HostAsync();

Here you have written a basic method call, and the method is :

private async Task HostAsync()

So the warning appears because compiler is expecting something like :

await ...

inside the HostAsync method.

By not putting the await keyword inside this async method, we get basically a synchronous call, a standard method. Not a big issue in most cases but have a look at :

private async Task SendMessages(IEnumerable<TcpClient> clients, string message)

This one has no await too.

Hope this helps !