Simplification of consecutive `map`s I've written the following code:

{- ...
   ...
   ... -}

data Ellipsoid
data Halfplane
data Intersection

data PointSet s a where
    Halfplane :: RealFrac a =>
                 a -> a -> a -> (a -> a -> Bool) -> a ->
                 PointSet Halfplane a
    Ellipsoid :: RealFrac a =>
                 a -> a -> a -> (a -> a -> Bool) -> a ->
                 PointSet Ellipsoid a
    Intersection :: [PointSet s a] -> PointSet Intersection a

type TestFunc a = RealFrac a => (a -> a -> a ->  Bool)

test :: (Show  a) => PointSet t a -> TestFunc a
test (Ellipsoid a b c f r) = f'
                       where f' z y x = ((x/a)^2 + (y/b)^2 + (z/c)^2) `f` r
test (Halfplane a b c f t) = f'
                       where f' z y x = (a*x + b*y + c*z) `f` t
test (Intersection ps) =     f'
        where f' z y x = and . map ($ x) $ map ($ y) $ map ($ z) ts
              ts = map test ps

In particular I'd like to have input on f' z y x = and . map ($ x) $ map ($ y) $ map ($ z) ts, whether there is a way to express this more succinctly.

Of course, I'm happy about any other comment as well.

The expression

and . map ($ x) . map ($ y) . map ($ z) . map test $ ps

contains several map calls connected with .. But since map f . map g === map (f . g), we can simplify it as

and . map (($ x) . ($ y) . ($ z) . test) $ ps

or more concisely

and . map (\g -> test g x y z) $ ps

Also and . map p can be simplified by all, so finally we get

all (\g -> test g x y z) ps

Just one more note, please post code that compiles, especially for CR, it's much easier to work with; see http://sscce.org/


"Histogram of Oriented Gradients" (HOG) feature detector for computer vision Here is a function that I've only slightly modified from its original context, found here.

Before mentioning anything else, it should be noted that I'm desperately trying to optimize this code for speed.  It presently takes about 5.25 seconds to execute and it appears as though the bottleneck is happening in the for-loop.

In a nutshell, this function expects the user to have SimpleCV installed and expects, at a minimum, to be passed a SimpleCV.Image instance.

Does anybody have some clever approach for speeding things up?  Ideally I'd be able to run this on a real-time webcam feed at 30 frames-per-second, but I'm not getting my hopes up.

from itertools import product
from math import floor, pi
import numpy as np
import cv2  # opencv 2

def findHOGFeatures(img, n_divs=6, n_bins=6):
    """
    **SUMMARY**
    Get HOG(Histogram of Oriented Gradients) features from the image.

    **PARAMETERS**
    * *img*    - SimpleCV.Image instance
    * *n_divs* - the number of divisions(cells).
    * *n_divs* - the number of orientation bins.

    **RETURNS**
    Returns the HOG vector in a numpy array

    """
    # Size of HOG vector
    n_HOG = n_divs * n_divs * n_bins

    # Initialize output HOG vector
    # HOG = [0.0]*n_HOG
    HOG = np.zeros((n_HOG, 1))
    # Apply sobel on image to find x and y orientations of the image
    Icv = img.getNumpyCv2()
    Ix = cv2.Sobel(Icv, ddepth=cv.CV_32F, dx=1, dy=0, ksize=3)
    Iy = cv2.Sobel(Icv, ddepth=cv.CV_32F, dx=0, dy=1, ksize=3)

    Ix = Ix.transpose(1, 0, 2)
    Iy = Iy.transpose(1, 0, 2)
    cellx = img.width / n_divs  # width of each cell(division)
    celly = img.height / n_divs  # height of each cell(division)

    #Area of image
    img_area = img.height * img.width

    #Range of each bin
    BIN_RANGE = (2 * pi) / n_bins

    # m = 0
    angles = np.arctan2(Iy, Ix)
    magnit = ((Ix ** 2) + (Iy ** 2)) ** 0.5
    it = product(xrange(n_divs), xrange(n_divs), xrange(cellx), xrange(celly))
    for m, n, i, j in it:
        # grad value
        grad = magnit[m * cellx + i, n * celly + j][0]
        # normalized grad value
        norm_grad = grad / img_area
        # Orientation Angle
        angle = angles[m*cellx + i, n*celly+j][0]
        # (-pi,pi) to (0, 2*pi)
        if angle < 0:
            angle += 2 * pi
        nth_bin = floor(float(angle/BIN_RANGE))
        HOG[((m * n_divs + n) * n_bins + int(nth_bin))] += norm_grad

    return HOG.transpose()

As you indicated in the question, you need to vectorize the for loop:

it = product(xrange(n_divs), xrange(n_divs), xrange(cellx), xrange(celly))
for m, n, i, j in it:
    # grad value
    grad = magnit[m * cellx + i, n * celly + j][0]
    # normalized grad value
    norm_grad = grad / img_area
    # Orientation Angle
    angle = angles[m*cellx + i, n*celly+j][0]
    # (-pi,pi) to (0, 2*pi)
    if angle < 0:
        angle += 2 * pi
    nth_bin = floor(float(angle/BIN_RANGE))
    HOG[((m * n_divs + n) * n_bins + int(nth_bin))] += norm_grad

If you look at what this is doing, you're effectively labelling every pixel in the magnit array with a number below n_HOG, and then summing the normalized values for the pixels with each label.

Operations on labelled regions of images are jobs for the scipy.ndimage.measurements module. Here we can use scipy.ndimage.measurements.sum:

bins = (angles[...,0] % (2 * pi) / BIN_RANGE).astype(int)
x, y = np.mgrid[:width, :height]
x = x * n_divs // width
y = y * n_divs // height
labels = (x * n_divs + y) * n_bins + bins
index = np.arange(n_HOG)
HOG = scipy.ndimage.measurements.sum(magnit[...,0], labels, index)
return HOG / img_area

Notes:

I've used % (2 * pi) to get the angles in the range [0, 2π). An alternative that's more like your code would be angles[angles < 0] += 2 * pi but using the modulus is shorter and, I think, clearer.
I postponed the division by img_area until after the summation, because it looks to me as though in the common case n_HOG is much less than img_area and so it's cheaper to do the division later when there are fewer items. (This means that the results differ very slightly from your code, so bear that in mind when checking.)
I measure the vectorized version as being about 60 times faster than your for loop, but it's still not going to be fast enough to run at 30 fps!
I've written angles[...,0] and magnit[...,0] here in order to drop the third axis. But I think it would make more sense if you dropped this axis earlier, before computing angles and magnit, by writing Ix = Ix[...,0] or just Ix = Ix.reshape((height, width)) if you know that the last axis has length 1.

Update

Based on comments, it looks as if you are using Python 2.7, where the division operator / takes the floor of the result if both arguments are integers. So I've changed the code above to use:

x = x * n_divs // width
y = y * n_divs // height

which is portable between Python 2 and Python 3, and simpler than my first attempt:

x = (x / width * n_divs).astype(int)
y = (y / height * n_divs).astype(int)


Putting data from 3 text files into 3 variables I have the following code, which gets the data from 3 text files and puts them into 3 variables. How can I refactor this to make it smaller?

$handle = fopen($template_filename, "r");
if (filesize($template_filename) > 0)
{
    $email_template = fread($handle, filesize($template_filename));
}
fclose($handle);

// get the subject from a text file
$handle = fopen($subject_filename, "r");
if (filesize($subject_filename) > 0)
{
    $subject_template = fread($handle, filesize($subject_filename));
}
fclose($handle);

// get the footer from a text file
$handle = fopen($footer_filename, "r");
if (filesize($footer_filename) > 0)
{
    $footer_template = fread($handle, filesize($footer_filename));
}
fclose($handle);

For one thing, you can use file_get_contents:

$email_template = file_get_contents($template_filename);
$subject_template = file_get_contents($subject_filename);
$footer_template = file_get_contents($footer_filename);


Data transfer objects vs Entities in JAVA REST server application I am quite new in web application back-end development and I have an architectural question.
I have defined Hibernate entities in my application and I want to send them to client. Currently for each entity I have defined data transfer object, that is almost the same as entity. Is it good practice to create DTO for each entity? Or should I send Hibernate entity for it? I find it quite redundant to make another class that is the same, but on the other hand - it is for different purpose.
Here is the example of what I mean:

User user = userPreferenceService.getUser(userLogin.getEmail(), userLogin.getPassword());
        UserDto userDto = new UserDto();
        userDto.setId(user.getId());
        userDto.setName(user.getName());
        userDto.setSurname(user.getSurname());
        userDto.setEmail(user.getEmail());
        userDto.setPhone(user.getPhone());
        userDto.setCountryId(user.getCountry().getId());

Here the User object is entity in database and userDto is the one that I send to the client. They contain the same info. Should I do it that way? Or should I just send the User?

Short answer:

Don't create a DTO per entity. Use the exposed domain model pattern instead.

Very long answer:

When you say "send to the client," as it is a webapp, this really means render to the browser (or mobile app) in the servlet tier. In which case, it's typically your own teams code and there is little or no value to copying entities into DTOs. There is overhead to copying into DTOs which is a reason not to do it. The exposed domain model pattern considers copying to DTOs as an anti-pattern. The POJOs In Action book by Chris Richardson is old, but still excellent and covers the exposed domain model pattern. The library versions in the sample code are now very out of date as it is many years later but the design and implementation approaches described in detail are IMHO timeless.

There is a use for some DTOs when writing code with entities. The canonical use is a search results screen which is, say, a grid with many columns which come from many entities. An example would be a summary of customer orders. That would have each row be taken from entities such as customers, addresses, products, orders etc. In such a case it is wasteful and slow to load all the entities to memory to render them in the flat search results screen. In that case you can create a DTO which represents a row on the screen and use JPA select new syntax as described in this answer to join across entities which is translated into a database join query that is efficiently loaded into the DTO object.

We should note that the DTO described above models the search result screen not the business domain. That's an important distinction. The DTO may change completely if the screen changes or be deleted if the screen is deleted. What you really want to do is model the business problem as entities and mature that model carefully and keep DTOs to an absolute minimum. Screens which work closely with a few entities in detail should work with root entities that control other entities directly.

I would strongly recommend putting as much business logic as you can into entities as public methods, and try to have all the code which creates and connects entities default/package protected and as hidden as possible. When you do that you get better OO design. An example would be the Order entity has a method getOrderItems to list the OrderItem entities but a protected setOrderItems method. The client would invoke a public method on the Order called addItem which took all the fields needed to construct the OrderItem entry internally using a protected constructor and add it into its list of order items. The Order becomes a "root/aggregate entity" which manages the relations and business logic of subordinate entities. This is a more naturally OO design than having "behaviourless" DTOs with the business logic entirely in service classes; or worse spread out across UI code. Have the service classes that query and load root entities then call method on the root entities that contain the business logic. That way different service class methods that work with the same root entities won't duplicate logic as the logic will be within the entities that are shared.

If your entities are dumb with no logic then they are just DTOs which is the Anemic Domain Model anti-pattern. If your entities are strong with business logic then copying the entities to DTOs would make no way for a client to use that business logic.

Given all this advice, it may seem strange that so much j2ee code still copies all entities into DTOs. One reason for this may be that the original "Core J2EE Design Patterns" book published by Sun had DTOs as a core pattern. That was required reading for a decade at the turn of the century. It's still around on the Oracle site, but that version makes it clear that it's to abstract over JDBC or LDAP and clearly JPA is the modern and standards compliant way of abstracting over a database; therefore, that page is obsolete.

Edit I have created a sample project on GitHub that demonstrates some of the ideas in this answer.


Instead of repeated statements like this:

echo "==============================================="

I suggest adding a helper function, for example:

print_hr() {  # hr - as in horizontal rule
    echo "==============================================="
}

The advantage of this is if you later decide to change the length of the = in the line, you can change it in one place.

Instead of entering into sub-directories with cd dirname and later stepping back with cd .., I recommend to use sub-shell environments with (...). For example, instead of this:

cd include
rm *.o
scons
# Move all Object Files into Build Directory
cp *.o ../build
cd ..

Do like this:

(
    cd include
    rm *.o
    scons
    # Move all Object Files into Build Directory
    cp *.o ../build
)

Notice that there is no more cd ...
Working directory changes in a (...) sub-shell environment are only in effect within.

This maybe a matter of taste, but I find that the many comment lines like this are just noise, and make the script hard to read, I suggest to remove them:

# ----------------------------------------------------------------------------------------------


ES5 class to handle analytics fetching and caching This class reads a log file stored on my server, parses it, and returns it. It also caches the data so that subsequent calls to get the analytics data will not have to read from the file. The analytics files will be relatively small, so memory usage is not a concern.

The callback passed to getAnalytics() is expected to accept an error as its first parameter and data as its second parameter.

Suggestions on improvements and cleanups are welcome.

Link to file on GitHub

const fs = require('fs');
const geoip = require('geoip-native');

/**
 * Constructor for an Analytics object.
 * @constructor
 * @param {string} analyticsFile The path to the server analytics log
 */
function Analytics(analyticsFile) {
  this.analyticsFile = analyticsFile;
  this.cache = {};
}

/**
 * Milliseconds in an hour.
 * @const
 * @type {number}
 */
Analytics.CACHE_KEEP_TIME = 3600000;

/**
 * Factory method for an Analytics object.
 * @param {string} analyticsFile The path to the server analytics log
 * @return {Analytics}
 */
Analytics.create = function(analyticsFile) {
  return new Analytics(analyticsFile);
};

/**
 * Fetches analytics on recent site traffic and passes it to a callback.
 * Caches the results for a day.
 * @param {function()} callback The callback function to which the analytics
 *   are passed.
 * @return {?}
 */
Analytics.prototype.getAnalytics = function(callback) {
  /**
   * First check if we have analytics cached. If not, then we should fetch it
   * again.
   */
  var currentTime = (new Date()).getTime();
  if (this.cache.analytics && currentTime < this.cache.expires) {
    return callback(null, this.cache.analytics);
  }
  var context = this;
  fs.readFile(this.analyticsFile, 'utf-8', (error, data) => {
    if (error) {
      return callback(error);
    }
    try {
      data = data.trim().split('\n').map(function(entry) {
        entry = JSON.parse(entry);
        entry.country = geoip.lookup(entry.ip).name;
        return entry;
      });
      context.cache.analytics = data;
      context.cache.expires = currentTime + Analytics.CACHE_KEEP_TIME;
      return callback(null, data);
    } catch (error) {
      return callback(error);
    }
  });
};

/**
 * This line is needed on the server side since this is loaded as a module
 * into the node server.
 */
module.exports = Analytics;

First of all, why restrict to ES5? Unless you're running/targetting Node < 4 (which is a security hazard, a story for another time), just write using the syntax available to the Node version you're running. Restricting your syntax is unnecessary overhead.

Now you don't need to go OOP on this one. I find that once people resort to OOP, the code usually ends up very boilerplate-y, contains lots of indirection, you worry about what this is. You can simply have a global cache keyed by file name and holds the data and expiry. Keep it simple.

data = data.trim().split('\n').map(function(entry) {
  entry = JSON.parse(entry);
  entry.country = geoip.lookup(entry.ip).name;
  return entry;
});

Why are you storing multiple JSON data inside one file separated by newline? The split by \n can be brittle. If it happens that your writer newlines the data in the middle, this will blow up. If you want to keep this data together, consider writing them in a stringified array instead. Stringified arrays are perfectly valid JSON.

Use promises. Callbacks are so 2 years ago, and are very clunky. There's a module called fs-extra that promisifies fs operations.

And Date.now() is available since ES5 to get the current millisecond timestamp.

const fs = require('fs-extra')
const geoip = require('geoip-native')
const cacheKeepTime = 3600000
const cache = {}

exports.getAnalytics = function(path){
  const currentTime = Date.now()
  const cacheHit = cache[path] && cache[path].data && cache[path].expiry < currentTime

  return cacheHit ? Promise.resolve(cache[path].data) : fs.readFile(path, 'utf8').then(contents => {
    const data = JSON.parse(contents) // Error in a callback rejects the promise
    const expiry = currentTime  + cacheKeepTime

    // Update cache
    cache[path] = { data, expiry }

    return data
  })
}

// Usage
const analytics = require('./analytics')
const someAnalyticsFile = 'path/to/analytics.json'

analytics.getAnalytics(someAnalyticsFile).then(data => {
  // We got data
}, error => {
  // Something blew up
})


Improving JavaScript - using objects vs. just functions I'm no stranger to JS, but I think I still have a lot to learn. I originally wrote this code to manipulate a "help" bar, which would be positioned as a thin ribbon on the side of the browser window, which when hovering over or clicking would open up and display it's contents. Nothing major, but an okay example to use here.

$(document).ready(function () {
var open = false;
var peek = false;
var helpBar = $(".help-bar");
var helpIcon = $(".help-icon");
var help = helpBar.add(helpIcon); // Put both together to do the same things

helpBar.click(function () {
    toggleHelp();
});

helpBar.hover(function () {

    togglePeek();
});

function toggleHelp() {
    if (open) {
        changeRightValue(help, "0", "slow");
        toggleArrow("Open");
        open = false;
    }
    else {
        changeRightValue(help, "280", "fast");
        toggleArrow("Close");
        open = true;
    }
}

function togglePeek() {
    if (open == false) {
        help.stop();
        if (peek) {
            changeRightValue(help, "0", "fast");
            peek = false;
        }
        else {
            changeRightValue(help, "5", "fast");
            peek = true;
        }
    }
}

function changeRightValue(element, amount, speed) {
    element.animate({ right: amount }, speed);
}

function toggleArrow(direction) {
    helpIcon.css("background", "url('/images/buttons/zapp/arrow" + direction + ".png') no-repeat 0px 50%");
}
});

It works fine, but then I wondered whether grouping related items into objects might help a little:

    $(document).ready(function(){
    var open = false;
    var peek = false;

    var helpBar = $(".help-bar");
    var helpIcon = $(".help-icon");

    var helpSection = helpBar.add(helpIcon); // Put both together to do the same things

    var help = {};
    var utils = {};

    help.toggle = function(){
        if (open) {
            utils.changeRightValue(helpSection, "0", "slow");
            utils.toggleArrow("Open");
            open = false;
        }
        else {
            utils.changeRightValue(helpSection, "280", "fast");
            utils.toggleArrow("Close");
            open = true;
        }
    };

    help.peek = function(){
        if (open == false) {
            helpSection.stop();
            if (peek) {
                utils.changeRightValue(helpSection, "0", "fast");
                peek = false;
            }
            else {
                utils.changeRightValue(helpSection, "5", "fast");
                peek = true;
            }
        }
    };

    utils.changeRightValue = function(element, amount, speed){
        element.animate({right:amount}, speed);
    };

    utils.toggleArrow = function(direction){
        helpIcon.css("background", "url('/images/buttons/zapp/arrow" + direction + ".png') no-repeat 0px 50%");
    };

    // jQuery DOM
    helpBar.click(function () {
        help.toggle();
    };

    helpBar.hover(function () {

        help.peek();
    };
});

Is this that much of an improvement over the first? Is there another way that I should be writing this kind of code?

If this is already your entire code, then you won't benefit much from namespacing your functions. Namespacing is basically assigning stuff to an object property. Accessing object properties is a minor overhead, but an overhead nevertheless.

Also, it doesn't look like anything else is accessing the code made for click and hover, so I'd rather place them directly into the handler instead.

You should be fine with simple functions in this case.

I'd rather concentrate more on reducing redundant code. Here's what I have done:

//$(document).ready(fn) can be shortened to $(fn)
$(function () {

  //declare variables in a function block up top
  //this is how the compiler sees them, so to avoid unexpected behavior
  //just follow suit
  var open = false
    , peek = false
    , helpBar = $(".help-bar")
    , helpIcon = $(".help-icon")
    , help = helpBar.add(helpIcon)
    ;

  //as explained, namespacing isn't an advantage at this level of code
  //and so, I have removed the functions and placed the code into the handlers

  //you can modify "this" in a function by using .call() or .apply()
  //in this case, element argument is removed and will be passed as "this"
  //this method will be explained later
  function changeRightValue(amount, speed) {
    this.animate({
      right: amount
    }, speed);
  }

  function toggleArrow(direction) {
    helpIcon.css("background", "url('/images/buttons/zapp/arrow" + direction + ".png') no-repeat 0px 50%");
  }

  //here we have our merged functions

  helpBar.on('click',function () {

    //if blocks that call the same functions but differ in parameters are messy
    //what I did here is made ternary ifs to pick arguments, not pick invocations
    //the calls will be made later
    var changeRightParams = open ? [0, 'slow'] : [280, 'fast']
      , direction = open ? 'Open' : 'Close'
      //this is a very short way to toggle booleans in the same variable
      , open = !open
      ;

    //here we call the functions with the "picked" arguments

    //here we use .apply to set the invoked function's this as help
    //and the arguments as the picked arguments
    //it's the same as calling "help.changeRightValue(amount,speed)
    changeRightValue.apply(help, changeRightParams);
    toggleArrow(direction);

  }).on('hover',function () {

    //like above, declare variables up top to avoid unexpected behavior
    var changeRightParams;

    //here we use the "early return" pattern, to avoid nesting ifs
    //since nothing executes when open is true, we return early
    //we saved 1 indent level in the code
    if (open) return;

    //code after the early return assumes "open" is false
    //it would be similar to if(!open){/*stuff here*/}

    help.stop();

    //same as above, we pick arguments, toggle the boolean, call with the selected
    changeRightParams = peek ? [0, 'fast'] : [5, 'slow'];
    peek = !peek
    changeRightValue.apply(help, changeRightParams);

  });
});


Python 3 Program to download Homestuck I've been working for a program over the last few days which should download a range of pages from the webcomic Homestuck. I've created a working version in python 3, but it is horribly inefficient. Can anyone see ways to improve and shorten this code?

import urllib.request
range1 = int(input("Enter the 1st page you want: "))
range2 = int(input("Enter the last page you want: ")) + 1
current = range1 + 1900
final = range2 + 1900
page = ''
nextPage = ''
while current != final:
    page = str(current)
    nextPage = str(current+1)
    while len(page) != 6:
        page = '0'+ page
    while len(nextPage) != 6:
        nextPage = '0'+ nextPage
    html = 'http://www.mspaintadventures.com/?s=6&p='+page
    site = urllib.request.urlopen(html)
    s = site.read()
    s = s.decode("utf8")
    s = s.replace("<!-- end comic content -->", "<!-- begin comic content -->")
    s = s.replace("http://cdn.mspaintadventures.com/storyfiles/hs2/", "")
    s = s.replace("?s=6&p=" + str(nextPage), str(int(nextPage))+".html")
    s = s.replace(page+"/"+page, page)
    a,b,c = s.split('<!-- begin comic content -->')
    b = "<title> Page " + page + "</title>" + b
    t = open(str(current)+".html", 'w+')
    t.write(b)
    t.close()
    page = str((int(page)-1900))
    while len(page) != 5:
        page = '0'+ page

    t = open(str(current)+".html", 'a')
    swfname=page+".swf"
    t.write("<object width='1000' height='1000'> <param name='movie' value='"+swfname+"'>")
    t.write("<embed src="+swfname+" width=650 height=450>")
    t.write("</embed>")
    t.write("</object>")
    t.close()
    try:
        img = "http://cdn.mspaintadventures.com/storyfiles/hs2/"+page+".gif"
        urllib.request.urlretrieve(img, page+".gif")
    except:
        try:
            img = "http://cdn.mspaintadventures.com/storyfiles/hs2/"+page+"_1.gif"
            urllib.request.urlretrieve(img, page+"_1.gif")
            img = "http://cdn.mspaintadventures.com/storyfiles/hs2/"+page+"_2.gif"
            urllib.request.urlretrieve(img, page+"_2.gif")
        except:
            try:
                img = "http://cdn.mspaintadventures.com/storyfiles/hs2/"+page+"/"+page+".swf"
                urllib.request.urlretrieve(img, page+".swf")
            except:
                print("Image "+img+" failed to download")
    print ("Page " + str(page) + " of " + str(final-1901) + " downloaded")
    current+=1
    print("DONE")

The main performance issue is the blocking nature of your script. You don't process the next url until you are done with the current. Think of using asynchronous tools like Scrapy web-scraping framework which is based on twisted; or something like grequests.

Other notes:

if you would stick to synchronous approach, switch to requests, initialize a session (requests.Session) once and reuse - this should be faster than using urllib.request
when you generate HTML files, pre-define a template with placeholders, render the template on the fly filling up the placeholders. You may use a template engine like mako or Jinja2, or use the built-in str.format()
it also looks like you are reopening each of the generated files twice - once for the initial write in the w+ mode and then to append in a mode


Performing several hundred raster multiplications I have several hundred "A" rasters, numbered from 001_A.tif to 100_A.tif.

I also have several hundred "B" rasters, numbered from 001_B.tif to 100_B.tif.

I want to multiply them like this:

001_A * 001_B = 001_result
002_A * 002_B = 002_result
003_A * 003_B = 003_result
.
.
.

I have a loop in R which works fine, but it is relatively slow:

library(raster)

A_Files <- list.files(pattern="A.tif") # files A
All_A <- unique(A_Files)

B_Files <- list.files(pattern="B") # files B
All_B <- unique(B_Files)

for (i in 1:100) {
  A.i <- All_A[i]
  B.i <- All_B[i]

  Raster_A.i <- raster(A.i) # create rasters from files A
  Raster_B.i <- raster(B.i) # create rasters from files B

  Result.i <- (Raster_A.i * Raster_B.i) # multiplying A by B

  writeRaster(Result.i, paste(A.i, "_result.tif", sep = ""), datatype='FLT4S',
 overwrite=TRUE) # write results in .tif format with the name "001_A_result.tif"
    }

Is there a way to speed it up?

Rasters

Rasters are highly memory intensive in R's already fragile memory space. However, one does not necessarily need to read a raster into memory each time. In fact, it is highly frowned upon to read in a raster in that way as it just eats up so much memory and is computationally intensive.

stack()'s beauty

It's much better to make a stack of rasters as it does not load them into memory, but opens their file location.

library(raster)

A_Files <- list.files(pattern="A.tif") # files A
All_A <- stack(unique(A_Files))

B_Files <- list.files(pattern="B") # files B
All_B <- stack(unique(B_Files))

Notice, all of the elements are now in a "vectorized" form.

overlay() to calculate

From here, we can use overlay() to perform a calculation. Note, since the number of layers is equivalent between (ALL_A) and (ALL_B) we do not need to worry about an element being repeated.

x <- overlay(All_A, All_B, fun=function(x,y) x*y )

Export

The only downside to this approach is you will have to go through a loop to export the answers:

out <- unstack(x)
outputnames <- paste0(seq_along(out), "_result.tif")
for(i in seq_along(out)){ writeRaster(out[[i]], file=outputnames[i]) }

References

For multiplying stacks see:

overlay
calc

For more information on stacks (see bricks as well)

stack
brick


Iterator for traversing a tree What I want to achieve is the following:

#include "tree.h"

#include <iostream>

int main() {

  Tree<std::string>* tA = new Tree<std::string>("A");
  Tree<std::string>* tB = new Tree<std::string>("B", tA);
  Tree<std::string>* tC = new Tree<std::string>("C", tA);
  Tree<std::string>* tD = new Tree<std::string>("D", tB);
  Tree<std::string>* tE = new Tree<std::string>("E", tB);
  Tree<std::string>* tF = new Tree<std::string>("F", tB);

  for (auto cur : *tA) {
    std::cout << "traversed from " << (cur->parent ? cur->parent->val : "NULL") << " to " << cur->val <<  std::endl;
  }

  // std::next(std::next(tF->begin()));

  return 0;
}

And here is my implementation.

#ifndef TREE_H
#define TREE_H

#include <iterator>
#include <stack>
#include <vector>

#include <iostream>

template <typename T>
class Tree {
 public:
  class iterator {
   public:
    using iterator_category = std::forward_iterator_tag;
    using value_type = T;
    using difference_type = std::ptrdiff_t;
    using pointer = T*;
    using reference = T&;

   public:
    iterator() = default;

    iterator(Tree<T>* const root);

    Tree<T>* operator*() const;

    iterator& operator++();

    bool operator!=(iterator const& other) const;

    Tree<T>* cur;

   private:
    std::stack<Tree<T>*> s_;
  };

 public:
  Tree() = default;

  Tree(T const& val, Tree<T>* parent = NULL);

  void* operator new(size_t size);

  iterator begin();

  iterator end();

  T val;

  Tree<T>* const parent = NULL;

 private:
  std::vector<Tree<T>*> children_;
};

template <typename T>
Tree<T>::iterator::iterator(Tree<T>* const root)
    : cur(root) {}

template <typename T>
Tree<T>* Tree<T>::iterator::operator*() const {
  return cur;
}

template <typename T>
typename Tree<T>::iterator& Tree<T>::iterator::operator++() {
  if (cur == NULL) {
    throw std::out_of_range("No more nodes for traversal");
  }

  for (auto& child : cur->children_) {
    s_.push(child);
  }

  if (s_.empty()) {
    cur = NULL;
  } else {
    cur = s_.top(); s_.pop();
  }

  return *this;
}

template <typename T>
bool Tree<T>::iterator::operator!=(Tree<T>::iterator const& other) const {
  return cur != other.cur;
}

template <typename T>
Tree<T>::Tree(T const& val, Tree<T>* const parent)
    : val(val), parent(parent) {
  if (parent) {
    parent->children_.push_back(this);
  }
}

template <typename T>
void* Tree<T>::operator new(size_t size) {
  void* p = ::new Tree<T>();
  return p;
}

template <typename T>
typename Tree<T>::iterator Tree<T>::begin() {
  return iterator(this);
}

template <typename T>
typename Tree<T>::iterator Tree<T>::end() {
  return iterator();
}

#endif

User should be able to directly modify val (hence it is public). However, they shouldn't be able to modify any parent/children relationship. Therefore I try to make parent a const pointer, but I cannot make children a list of const pointers because anything inside a std::vector must be assignable. I am considering exposing a GetChildren() method which returns a const std::vector, but that would be inconsistent with how I dealt with parent. Can anyone show a good way to resolve this?

General reviews are welcome. More specifically, I want to know

Is this a good and correct use of pointers?
Is the interface well designed?
Are there any const that I missed?

Clean up allocated memory (everything newed should be deleted), or use smart pointers to do it for you. (Perhaps each tree node should own its children, and store them in unique_ptrs, and the parent should be a raw pointer).
There doesn't seem to be any benefit to overloading operator new for the Tree class.
Currently, the iterator is more of a sub-tree iterator, as it never returns to the parent. e.g. for a tree with edges: A->B->C and A->D if you start iterating at B, you will only iterate from B to C, and never reach D.
Iterators are normally defined outside of the corresponding container class. We can use an iterator typedef in the container to refer to the iterator class.
operator* should return a reference, not a pointer (operator-> should return a pointer).
We have !=, but no ==.
We have a pre-increment operator++, but not post-increment operator++.
It would probably be best to write unit tests for things that are expected to work, e.g.: *i (returns a modifiable reference), *i++ (returns a modifiable reference and then increments i to point at the next value), etc. This can be tedious, but is really the only way to ensure correctness.
The full requirements for a forward iterator are listed here (and in the linked pages), and many of them could be translated into tests cases. This iterator should arguably be a bidirectional iterator.
Standard containers also supply a const_iterator version (and reverse iterators, though that's usually simple to add).

To be honest, I'd suggest starting with something a bit simpler like an array class, and corresponding iterators.

Note that we can traverse a tree in many different ways (depth-first in-order, depth-first pre-order, depth-first post-order, breadth-first). For a full-featured implementation of this sort of thing, see the ASL Forest class.

Since we're iterating nodes, not values, we don't have to worry about "order", but we should still call the class something like tree_depth_first_iterator, to distinguish it from the alternative(s).


Bash Script for file auditing, push information to server and be able to view in a web page Edit: The goal of this Question was only seeking code review and guidance in learning or tips for improvement. I apologize if that was not clear or aligned properly with the goals of this forum.
Script Goal: audit file existence and differences between two red hat Enterprise Linux Servers

Place information in sql server tables to use for analysis
Maintain ability to view the diff in a web page

Process Steps
Set array with Directorys to check

Set array of file types to look for
Build a list of existing files for each Environment

Script will solve the following
If the files exist in both servers are they the identical? (md5)*

If no what is different (Diff)?
Does the file contain hard coded values (grep)

View Diff in web page (currently reading text file using into site php)

Code

DIR=(/Dir/Durp/DurpaDurp/Scriptdir1 /Dir/Durp/DurpaDurp/Scriptdir2 /Dir/Durp/DurpaDurp/Scriptdir3 )

f_Type=("*.sh" "*.txt" "*.log")
f_Type2=("*.img" "*.rpt" )

for((i=0; i<${#DIR[@]}; i++))
do
  echo "CHECKING: ${DIR[$i]}"
  cd "${DIR[$i]}"
      for((x=0; x<${#f_Type[@]}; x++))
       do
         echo "FOR FILE TYPE: ${f_Type[$x]}"
        find $PWD -type f -name "${f_Type[$x]}" | sed 's/^/ENV1|/'| column -t >> "$filelistENV1"
      done
if [[ ${DIR[$i]} == "/Dir/Durp/DurpaDurp/Script3" ]];
  then
     for((y=0; y<${#f_Type[@]}; y++))
    do
       echo "FOR FILE TYPE: ${f_Type2[$y]}"
      find $PWD -type f -name "${f_Type2[$y]}" | sed 's/^/ENV1|/'| column -t >> "$filelistENV1"
    done
fi
done

After these files are output to the text files the script will delete the existing data from a staging table in SQL Server 2008 R2 and insert the new data.

for((i=0; i<${#ENV[@]}; i++))
do
  sqlcmd -S $_DB_CONN -d $_DB  -Q "DELETE FROM ['$_DB']..['$_TABLE'] WHERE ENV = '${ENV[$i]}'";
done

bcp $_DB_CONN.."$_TABLE" in "$filelistENV1" -f "$_SCRIPTDIR/STG.fmt" -e $_ERRDIR/ERROR_STG$(date -d "today" +"%Y%m%d%H%M").txt -S $_DB_CONN -d "$_DB"

the format file creates 2 columns

AbsoluteFilePath | ENV
gets a list of files from the database to compare

`

sqlcmd -S $_DB_CONN -d $_DB -s "|" -h-1 -m -1 -W -i $_SCRIPTDIR/SQL/EXPORT_COMPARE.sql -o $_INPUT/comp_list.txt  set NOCOUNT ON;

compare the md5sum of the files.

for i in $(cat "$_INPUT/comp_list.txt")
do
export filename=$(basename "$i")
export path=$(dirname "$i")
env1_md5sum=$(md5sum "$i")
env1="${env1_md5sum%% *}"
export tmpdir=("$_TMPDIR$path")

if ssh "$_CONN" stat $path'$filename' \> /dev/null 2\>\&1
then
    env2_md5sum=$(ssh $_CONN "cd $path; find -name '$filename' -exec md5sum {} \;")
    env2_md5="${env2_md5sum%% *}"

    if [[ $env1_md5 == $env2_md5 ]]; then
        echo $filename $path >> "$matchingMD5"
    else
        echo "md5 does not match, getting copy of file"
        echo "$i" >> "$no_matchMD5"
        mkdir -p $tmpdir
        scp $_CONN:$i $tmpdir
    fi
fi done

run a diff on files that do not match

for x in $(cat "$no_matchMD5")
do
comp_filename=$(basename "$x")
env2file=(/"$ScriptsDir"/tmp"$x")
DIFF=$(diff --ignore-all-space --ignore-blank-lines --brief  "$x" "$env2file" &>/dev/null

Performance pitfalls

Running ssh in a loop tends to be slow. Running it twice for every file in a list is probably extremely slow. There's no easy fix for this. You need to rethink how to solve the problem of matching paths between two systems.

Off the top of my head:

Get the list of paths from both systems, and then try to match those locally. This can be done with one ssh call per system: a major improvement.
For the list of matched paths, get the md5 sums. Again this can be done with one ssh call per system.
Compare the hashes, and build a new list of mismatched files.
For the final comparison of files, you could fetch them one by one to conserve disk space. If the number of remaining files is expected to be small, then one scp call per file might be acceptable. Or if disk space is not an issue, then you could transfer all the files with one call.

A much smaller performance issue is running sed ... | column ... for each file type for each base directory. You could instead make the loop body output only the output of the multiple find calls, and run the sed ... pipeline on the entire loop (writing as done | sed ...).

Looping over arrays

Instead of this:

for((i=0; i<${#DIR[@]}; i++))
do
  echo "CHECKING: ${DIR[$i]}"

When you don't need the array indexes, just the elements, you can iterate like this:

for dir in "${DIR[@]}"
do
  echo "CHECKING: $dir"

Most of the loops in the posted script can be replaced with this simpler, more intuitive style.

Simple mistakes

Use . instead of $PWD
Double-quote variables used in command arguments: instead of find $var, write find "$var"
Don't export if you don't need to
Don't create arrays if you need a simple variable: instead of tmpdir=("$_TMPDIR$path") write tmpdir="$_TMPDIR$path"
Strive for simple writing style: instead of env2file=(/"$ScriptsDir"/tmp"$x"), write env2file="$ScriptsDir/tmp$x"


Model binding with a ViewModel I have two ViewModels with the one referencing the other:

public class GiveViewModel
{
    [Required]
    public int GivenLifelines { get; set; }
    public string ToUser { get; set; }
    public string FromUser { get; set; }
}

 public class UserDetailsViewModel
{
    public GameDetail gameDetail { get; set; }
    public UserDetail userDetail { get; set; }
    public IEnumerable<GameItem> gameItems { get; set; }
    public IEnumerable<UserDetail> allUsers { get; set; }
    //REF
    public GiveViewModel givenLifeLines { get; set; }
}

The main viewmodel is used in the controller to get all the info on the foreign keys for the page. I have IEnumerables to list all users and game items, and references to the main models to get the current users details. This enables users to give points to other users while on their details page (the DetailsController).

Technically I do not use the givenLifeLines on the first load for the view. It is only used during the POST.

I can get the values back from the POST and use it on the [HttpPost] controller
but it seems a bit awkward.

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Give(UserDetailsViewModel model)
{
   int givenLifeLines = model.givenLifeLines.GivenLifelines; //got value
   string givenTo = model.givenLifeLines.ToUser; // got value
   ....
}

You see, the current logged in user selects another user based on their image by clicking the selected image:

<img src="~/@item.AppImage.ImagePath" class="img-responsive img-thumbnail user-image" data-username="@item.FirstName" style="width:100px;height:100px;" />

which gets added to a model textbox:

 @Html.TextBoxFor(model => model.givenLifeLines.ToUser, new { @class = "give-to form-control" })

That process is controlled by jQuery:

  $(".user-image").click(function () {
      $(".give-to").val($($(this)).attr('data-username'));
   })

This all works fine. I am getting the values I need in the model for the POST, but I get the feeling that this is not a good idea. Ideally I would want the textbox to be disabled, however, if I do that, the value is then not passed to POST controller model. A dirty work around would be to just display:none on the textbox and still have it enabled but that would feel wrong. I don't feel like I should be required to depend on jQuery like this.

Is there a better way to approach this that is more inline with the practices of model binding?  Should I be separating my ViewModels like I have?

First I would fix the variable naming to have a consistent naming structure. In the first class you have variables starting with capital letters (I agree) but in the second they start with lowercase.

I would suggest only passing in what you need for the view and leaving the rest out.

Can you show us your view? I see you have two IEnumerables that look like they are passing domain data to the view. How are you displaying them?

Is GivenLifeLines a Foreign Key or a count? Hard to tell.


Shuffle Symfony entities I've got an object $contest which is a Symfony entity that has some other entities assigned. I can get them by $content->getEntries();. That will return an object with the lazyloaded db objects. I need to display them in a random order.

$tempEntries = $contest->getEntries();
$entries = array();
if (count($tempEntries)) {
    foreach ($tempEntries as $temp) {
        $entries[] = $temp;
    }
    shuffle($entries);
}

This is the code I use currently. I loop through all elements (if there are any) and then shuffle them. I feel like the looping through them is a wast of resources.

Any advice on how to improve this?

You don't need a temporary variable $tempEntries
You don't need to check if the array is empty, if the array is empty shuffle will just do nothing

The code you need may look like:

$entries = $contest->getEntries();
shuffle($entries);


Function to get NGrams fast in Haskell Is this a good way to get all sublists of a sequence/list that have a given length?

An inefficient way to do it would be something like

f n = (filter (\x -> (length x) > n)) . (take n) . tails

This just takes the n first elements of each tail of the original list. I think it should be slow because of the length check on every tail-element.

A smarter way would should be to "slide" a sequence of length n over an input sequence and save the result of each slide by one to the right.

-- | Get all the subsequences of a given sequence sq of length n
ngrams::Int -> Seq a -> Seq (Seq a)
ngrams n sq | length sq < n = empty
ngrams n sq | otherwise = ngrams' restSequence initialWindow empty
            where
              initialWindow = take n sq
              restSequence  = drop n sq
              ngrams' (viewl -> EmptyL) window acc = acc |> window
              ngrams' (viewl -> x :< s) window@(viewl -> a :< r) acc =
                                            ngrams' s (r |> x) (acc |> window)

Somehow I have the feeling I am missing an obvious way to do this better...

A little math goes a long way.

ngrams :: Int -> [a] -> [[a]]
ngrams n l = take (length l - (n - 1)) . map (take n) . tails $ l


Ordered Doubly Linked List Insertion I'm working on a school project right now. It's an ordered(ascending) doubly linked list. I just wrote the Insert operation on it but I feel like the way I wrote it isn't that great.

void OrdListClass::Insert(ItemType item)
{
    if(Find(item))
    {
        //throw DuplicateKeyException;
    }//End Duplicate Key Check
    else
    {
        //Due to Find, currPos points to the node that we're inserting AFTER
        node* newNode = new node;
        newNode->data = item;

        if(currPos == nullptr)
        {
            //We're inserting at the head.
            newNode->prev = nullptr;
            newNode->next = head;
            head->prev = newNode;
            head = newNode;
        }
        else if(currPos->next == nullptr)
        {
            //We're inserting at the tail
            newNode->next = nullptr;
            newNode->prev = tail;
            tail->next = newNode;
            tail = newNode;
        }
        else
        {
            //We're inserting in the middle of the list.
            newNode->prev = currPos;
            newNode->next = currPos->next;
            currPos->next->prev = newNode;
            currPos->next = newNode;
        }
        currPos = newNode;
    }//End Not Duplicate Else
}//End Insert

EDIT: Of note.. ItemType is just a typedef for an int.

bool OrdListClass::Find(/*IN*/ItemType searchKey)
{
    //Skip operations if empty.
    if(IsEmpty()) return false;

    currPos = head;
    while(!EndOfList())
    {
        if(currPos->data == searchKey)
        {
            return true;
        }
        else if (currPos->data > searchKey)
        {
            currPos = currPos->prev;
            return false;
        }
        //else continue;
    }
}

I think your Insert function is about as tight as you can make it without getting into strictly-organizational functions to tuck the list details (head, tail) and node details (next, prev) out of sight. Not a big deal for this one function and probably not for the class overall.

If I could make one suggestion, though, it would be that you reconsider having the Find function serve as both a match-finding function (returning true or false accordingly) and a state-modifying function that updates member currPos.

If I saw the definition of Find in the header, I would wonder why it was not a const function since the name and return value give me no reason to think it's modifying the object itself -- surely it just returns true if a match is found and nothing else. Also, since the list is ordered, it would seem that currPos must always be updated before any meaningful modification is performed: it determines where the current insert/delete occurs (not an arbitrary insert/delete like against head or tail) so currPos has no value to insertion/deletion before or after the call and thus no value as a member. So currPos as a member seems like unnecessary temporary storage at best and error-prone storage at worst.

Assuming my assessment of Find and currPos are fair, consider removing member currPos and adding a function to manage the list search without storing the node or the match flag in the object, like so:

node* OrdListClass::FindNode(/*IN*/ItemType searchKey, bool& matchFound) const
{
    matchFound = false;

    //Skip operations if empty.
    if(IsEmpty()) return nullptr;

    node* currPos = head;

    while(currPos != nullptr)
    {
        if(currPos->data == searchKey)
        {
            matchFound = true;
            return currPos;
        }
        else if (currPos->data > searchKey)
        {
            currPos = currPos->prev;
            return currPos;
        }
    }
}

Find then becomes:

bool OrdListClass::Find(/*IN*/ItemType searchKey) const
{
    bool matchFound = false;
    //FindNode is only called to determine if there is an exact match.
    FindNode(searchKey, matchFound);
    return matchFound;
}

And Insert becomes:

void OrdListClass::Insert(ItemType item)
{
    bool matchFound = false;
    node* currPos = FindNode(item, matchFound);

    if (matchFound)
    {
      //Nothing to insert.
      return;
    }

    //currPos points to the node that we're inserting AFTER
    node* newNode = new node;
    newNode->data = item;

    if(currPos == nullptr)
    {
        //We're inserting at the head.
        newNode->prev = nullptr;
        newNode->next = head;
        head->prev = newNode;
        head = newNode;
    }
    else if(currPos->next == nullptr)
    {
        //We're inserting at the tail
        newNode->next = nullptr;
        newNode->prev = tail;
        tail->next = newNode;
        tail = newNode;
    }
    else
    {
        //We're inserting in the middle of the list.
        newNode->prev = currPos;
        newNode->next = currPos->next;
        currPos->next->prev = newNode;
        currPos->next = newNode;
    }
}//End Insert

There are alternative ways to implement FindNode rather than passing a bool reference for the matching flag (e.g., returning something like a NodeSearchResult object that contains the flag and the node together), but the gist is that FindNode is a function that manages the insertion/deletion point without resorting to using a member for temporary storage.

Whether or not you choose to go with something like this, I hope it gives you something to think about. Otherwise, like I said, Insert itself seems fine.


Reading file and writing to output file with line numbers I started learning java relatively recently as my first computer language independently and would like to see if my approach is efficient. An exercise in the book I am studying from tells me to read a file and write it into a seperate file with each line numbered.

Here is my solution

public class FileStuff
{
    public static void main (String [] args)
    {
        System.out.println("Which file do you want to read");
        Scanner in = new Scanner(System.in);
        String fileInput = in.next();
        System.out.println("Which file do you want to write to?");
        String fileOutput = in.next();
        Scanner goThrough = null;
        PrintWriter print = null;
        String content ="";
        try
        {
            print = new PrintWriter(fileOutput);
            goThrough = new Scanner(new File(fileInput));
            while(goThrough.hasNextLine())
            {

                content = goThrough.nextLine();
                writeToLine(content, print);

            }
        } catch(FileNotFoundException e)
        {
            e.getMessage();
        }finally
        {
            goThrough.close();
            print.close();
        }

    }
static int lineNumber = 1;
    static void writeToLine(String line, PrintWriter out) throws FileNotFoundException
    {
        try
        {
        out.println(String.format("/* %d */ %s ",lineNumber,line));
        lineNumber+=1;
        }
        catch(Exception e)
        {
            e.getMessage();
        }
    }

}

Concerns

I have several inquiries about my code as I cannot understand my book clearly, and coding practicing questions.

Firstly, is it ok to initialize goThrough and print outside the try block as null? I did this because I wanted to close the Scanner and PrintWriter inside the finally block, but did not want to define something inside the try block since it is a different scope from the finally block

Secondly: Is it fine to throw FileNotFoundException at my method writeToLine? Or should I be throwing it after public static void main(String [] args), I have seen it done both ways in my book.

Here are my comments:

1) File handling

Your file handling logic is in the right direction:

the File handlers (goThrough and print) are defined outside of the try block,
are opened at the beginning inside the block
are closed in the finally clause.

However, you forget to protect the code from NPE (NullPointerException):
at the beginning inside the try block, both handlers contain null.  You first open print the output handler, and then goThrough.  However, in the finally clause, the order is reversed. So, if an exception is thrown while opening print, goThrough is never initialized and you might encounter NPE in the finally clause. it is absolutely essential to ask separately about each file handler before closing it:

if (goThrough != null) goThrough.close();
if (print != null) print.close();

Note: the best practice rule dictates that you first open the input handler, then the output one, and closes them in the reverse order.

As you can see, proper file handling is a delicate process that requires careful attention to details at different points in the code.  Since many errors were made by many a good developers in many production programs, Java 7 added a feature that automatically handles this logic for you.  it is called try-with-resources and it means that you declare and open the handlers at the point of the try block and the compiler adds the correct finally clause. I leave to you as study exercise to go though the tutorial and implement the feature into your code.

2) Exception handling

Exception handling in writeToLine() is incorrect:

The method is declared with throws FileNotFoundException clause.  However, the code is surrounded with try-catch that catches  all exceptions, including the supposedly thrown one.
The catch block contains what looks like a statement fragment that does nothing (but passes compilation).  perhaps you meant to throw an Exception?

3) Design

This last comment falls into "best practice" category: the main() method does too many unrelated tasks: receive input from user, file handling and main loop. if, for example, you have a separate method that is responsible for communicating with the user, receiving user input and validating it, then perhaps you would be able to check for existence of input file as part of validation... then again, one method for receiving user input and validating it? hmm maybe we can design further break down ...


Console program, let users to enter prices calculate subtotal, tax, and total with C Sharp I am posting this here because it was suggested that it is the best place by a seasoned user.

This is a project I already submitted, but I never received feedback. All my upcoming assignments will be based on this, so I wanted to make sure it is correct. I got this program to work as far as calculating the information, but I was hoping someone could please let me know if it meets the parameters/requirements?

I did attempt it. It works and adds the items exactly how the example showed in the video. However, I wanted to make sure my code is solid, and not just a mishmash or working because I got lucky, and if there is a better more solid way to do this I want to make sure I can. I do want feedback so I can learn and get better and I am trying my hardest. I always right out all the code I see here and try it and learn what it does piece by piece, because it helps me learn and improve. I also want to make sure it is going to work once I start the code for the next half of the requirements.

**Write a Console program, let users to enter prices calculate subtotal, tax, and total. - Delcare variable data type as decimal (6 variables needed) decimal apple;

Conver string to decimal from input apple = Convert.ToDecimal(Console.ReadLine());
Do addition for subtotal
Multiplication for tax (0.065M) --- M or m stands for money value, read page 94.
Addition for total
You need to show only two digits after decimal point
The class example:

`Console.WriteLine(" Subtotal: ${0}", String.Format("{0:0.00}", subTotal)); blank line: Console.WriteLine("") Console.Write(" Apple: $"); apple = Convert.ToDecimal(Console.ReadLine());`

My code:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace CalculatorRun
{
    class Calculator
    {
        static void Main(string[] args)
        {
            decimal FirstOperand, SecondOperand, Result, result;
            Console.Write("Addition Calculation");
            Console.Write("      \n\n");
            Console.Write("   Enter first operand:       ");
            FirstOperand = Convert.ToDecimal(Console.ReadLine());
            Console.Write("   Enter second operand:      ");
            SecondOperand = Convert.ToDecimal(Console.ReadLine());
            Console.Write("-------------------------\n\n");
            Result = FirstOperand + SecondOperand;
            result = Convert.ToDecimal(Result);
            Console.WriteLine("Addition Result: {0}", string.Format("{0}", result));
            Console.WriteLine("Press any key to continue.....");
            Console.ReadLine();
        }
    }
}

The next part of the assignment is this, which is what I need to make sure my code is correct so I can build towards:

Delcare variable data type as decimal (5 variables needed) count, price, subtotal, tax, total

Write a do while loop, if the price is not -1, loop continues show count number enter price get subtotal increase count

Calculate tax, total

Display total count, subtotal, tax, total

Feedback I received I am working on:
What happens if the user enters an invalid number for either operand? (Thanks yo PaulF. I am not quit sure what this means, I think it means if someone enters a number such as -1. However, I will have to research a fix as this is a bit outside my realm).

This line is uncessary: result = Convert.ToDecimal(Result); because Result is already a decimal. You don't need two of them... (Thanks to Rufus L. I have to look into this and understand decimals more.)

This is the book I am using, and what I have to match so if some code seems oddly specific or old, that is why.
https://cscnt.savannahstate.edu/StudentFiles/Data_Structure/Visual%20C%23%202012%20How%20to%20Program.pdf

This isn't bad for a beginner, but there is much room for improvement.

Organization

The app is small enough that everything can fit inside Main, but a more seasoned developer will have Main be more barebones, with calls to other methods.

You would also want to separate out the UI, that is the prompting for input values, from the other Logic, that is the addition and display of values.

Variable Declaration and Naming

The good news is that your variables have some meaning in their names.  The bad news is you begin local variables with a capital letter (known as PascalCasing), when it should be camelCasing.  Thus, FirstOperand should be firstOperand, etc.

Also, CodeReview prefer to see each variable declared on its own line.  What I would expect you to end up with is something like:

decimal firstOperand;
decimal secondOperand;
decimal result;

Checking for Valid Input

You have no checks for valid input.  What happens if someone enters "dog" instead of 123.45?  You may want to read up on the Decimal.TryParse method to help out with that.

Things not needed

As mentioned, there is no need for Result and result since result is already a Decimal.

This line is too confusing:

Console.WriteLine("Addition Result: {0}", string.Format("{0}", result));

There is an overload for Console.WriteLine that accepts a string format.  This would simplify the line to:

Console.WriteLine("Addition Result: {0}", result);

The link you provide is for C# 2012.  Newer versions of C# support Interpolated Strings, prefaced with a $".  So you could also do the above with:

Console.WriteLine($"Addition Result: {result}");

If you wanted to control formatting of the numbers, it could be done one of 2 ways:

Console.WriteLine($"Addition Result: {result.ToString("0.00")}");

Or

Console.WriteLine($"Addition Result: {result:0.00}");

More Reading

To learn more, I found a decent post here on CR 'Welcome to Buzzway Subs'.  I recommend reading the original code, AND then follow up by reading the good advice in the answers.


Game of Life improvement I have finished my code for Conway's Game of Life. I am looking for any advice on how to improve it. I am a C++ beginner, so I used only basic libraries. I am accepting all suggestions.

/*
Author: Stevan Milic
Date: April 2018
Class: COSC 1560
Description: Game Of Life
*/
#include <iostream>
#include <cstdlib>  // Library needed for CLS command
#include <iomanip>
using namespace std;

const int boardsize = 10;

void Display(bool board[boardsize + 1][boardsize + 1])
{
    for (int x = 1; x < boardsize; x++)
    {
        for (int y = 1; y < boardsize; y++)
        {
            if (board[x][y] == true)
            {
                cout << " * ";
            }
            else
            {
                cout << " - ";
            }
            if (y == boardsize - 1)
            {
                cout << endl;
            }
        }
    }
}
// Copying the board in order to compare the existing cells
void CopyBoard(bool board[boardsize + 1][boardsize + 1], bool board2[boardsize + 1][boardsize + 1])
{
    for (int x = 0; x < boardsize; x++)
    {
        for (int y = 0; y < boardsize; y++)
        {
            board2[x][y] = board[x][y];
        }
    }
}
//Calculates function to determine if the cell is alive or dead
void Calc(bool board[boardsize + 1][boardsize + 1])
{
    bool board2[boardsize + 1][boardsize + 1] = {};
    CopyBoard(board, board2);
    for (int x = 1; x < boardsize; x++)
    {
        for (int y = 1; y < boardsize; y++)
        {
            int alive = 0;
            for (int z = -1; z < 2; z++)
            {
                for (int t = -1; t < 2; t++)
                {
                    if (!(z == 0 && t == 0))
                    {
                        if (board2[x + z][y + t])
                        {
                            alive++;
                        }
                    }
                }
            }
            if (alive < 2)
            {
                board[x][y] = false;
            }
            else if (alive == 3)
            {
                board[x][y] = true;
            }
            else if (alive > 3)
            {
                board[x][y] = false;
            }
        }
    }
}

int main()
{

    //const int boardsize = 10;
    bool board[boardsize + 1][boardsize + 1] = {};

    // Starting cells
    board[5][4] = true;
    board[5][5] = true;
    board[5][6] = true;
    board[4][4] = true;
    board[4][5] = true;
    board[4][6] = true;
    board[3][4] = true;
    board[3][5] = true;
    board[3][6] = true;

    while (true)
    {
        Display(board);     // Displaying function
        Calc(board);        // Calculating function
        system("CLS");      // Clearing the Screen
    }
}

I'm unsure what you mean by saying "improve the user interface". AFAIK Game of life doesn't have much of a user interface as it's a simulation that only displays the grid and whatever happens on it.
However there are some other things that can definitely be improved.

Don't use using namespace std
Generally functions in C++ are lowercase
Think about better variable names.
x and y might be acceptable but t and z are probably not the best names.
Prefer using \n over std::endl
Prefer prefix over postfix operator
Don't compare to booleans. Instead do if (condition) and
if (!condition).
You might want to use vectors for your board instead of arrays.
Don't use global variables (which you don't need when you're using vectors)
There is another way to write an endless loop which is a for loop without conditions for (;;) (read as forever).
Which one is better is highly opinion based however.
Your cluster of for loops is quite hard to grasp. It would improve readability and understanding if you break it up into smaller parts. For example you could write a function to iterate over the cells and for every cell call another function that checks the adjacent ones for live cells.


Sorting dictionary according to letter patterns I've written a JavaScript dictionary sorting algorithm, which takes a .txt file (the dictionary), and loads it via node's file system. The purpose of this algorithm is to sort every word into its corresponding array based on its letter pattern. For example, the word little would have the letter pattern of ABCCDE, and hello would have the letter pattern of ABCCD. The purpose of this sorting algorithm is to use it to decode substitution ciphers; however, this is the only working code I've written so far for it. This code works, and I have a sorted dictionary file here.

However, I want to know how I might be able to improve this algorithm to make it as fast and efficient as possible. Even though its only run once, I want to get into the habit of writing efficient code.

To break down exactly what's going on in the algorithm I've added a few comments, but to elaborate it loads each word from the dictionary and stores it in an array, in which I then iterate through the array and each letter of the word. The clpl variable starts at 'A', and it checks if it exists in a temp object. If it does, it adds that letter to the letter pattern, if it does not, it creates a new property on the object and assigns it the current letter as a value. It adds the assigned letter to lp, which is initialized as an empty string.

The line clpl = String.fromCharCode(clpl.charCodeAt(0) + 1);gets the next letter. After it has iterated through each letter in the word, it checks if the current letter pattern exists in the sortedDictionary object, if it does it pushes it to an array containing all words with its letter pattern. If it does not, it creates a new array for that letter pattern and pushes the current word to that array. After it has gone through every word, it writes the sortedDictionary object to an external JSON file.

//load file system
var fs = require("fs");
//load jsonfile
var jsonfile = require("jsonfile");

//create var for dictionary file
var dictFile = "american-english", sortedDictFile = "sortedDictionary.json";

//empty object to hold sorted dictionary according to letter patterns
//empty dictionary array to hold words
var sortedDictionary = {}, dictionary = [];

//declare variables for use
var temp, clpl, lp, word;

console.time("Dictionary Sort");

fs.readFile(dictFile, "utf8", function(error, data){
    if(error) throw error;
    //push all words into an array
    dictionary = data.toString().split("\n");

    for(var i = 0; i < dictionary.length; i++){
        //set word to current word in dictionary
        word = dictionary[i];
        //set temp to empty object, clpl to A, and lp to an empty string
        //this is used to get the current letter pattern
        temp = {}, clpl = 'A', lp = '';

        for(var j = 0; j < word.length; j++){
            if(word[j] in temp){
              lp += temp[word[j]];
            } else {
              temp[word[j]] = clpl;
              lp += clpl;
              clpl = String.fromCharCode(clpl.charCodeAt(0) + 1);
            }
        }

        //if letter pattern of word exists in sorted dictionary
        if(lp in sortedDictionary){
            //add word to the array of words with same letter pattern
            sortedDictionary[lp].push(word);
        } else {
            //if letter pattern is new, create new array to store words
            sortedDictionary[lp] = [];
            //add word to the array of words with same letter pattern
            sortedDictionary[lp].push(word);
        }
    }

    //write the sortedDictionary object to the sortedDictionary.json file
    jsonfile.writeFile(sortedDictFile, sortedDictionary, {spaces: 2}, function(error){
        if(error) throw error;
    });

    //time to sort and write to json file - Dictionary Sort: 687ms (137602 lines)
    console.timeEnd("Dictionary Sort");
});

This can be improved and shortened by naming the concepts you are using:

A function called encodeWord that takes a single word and returns its encoded value
A function called groupedByCodes which takes an array of words, and returns the object you are seeking: codes as keys, and arrays of words that get encoded to those keys.

This will have the following benefits:

What your code is doing will be crystal clear.
You'll have less code
Most of your temporary variables will vanish

I've also purposefully left out the writing and reading of the files, as that's really a separate concern from your main program.

Rewriting it this way, you'll get something that looks like this:

var sampleFile = "CAT\nDOG\nTOM\nBOB\nTOT",
    words = sampleFile.split("\n");

console.log(groupedByCodes(words));
// { ABC: [ 'CAT', 'DOG', 'TOM' ], ABA: [ 'BOB', 'TOT' ] }

function groupedByCodes(words) {
  return words.reduce(function(dict, word) {
    var code = encodeWord(word);
    if (!dict[code]) dict[code] = [];
    dict[code].push(word);
    return dict;
  }, {});
}

function encodeWord(word) {
  var encodingDict = {}, nextCode = 65; //'A'
  return word.split('').map(encodeLetter).join('');

  function encodeLetter(l) {
    return encodingDict[l] || (encodingDict[l] = String.fromCharCode(nextCode++));
  }
}


Interface for a CRM system I have a CRM system that has Notes, Payments, DairyEvents and anything else that needs to be bolt on over time.

I created an ITimeline interface that has a datetime and message method and implemented them on those models.

This is the simplest form of code I have started with:

public static List<Interfaces.ITimeline> GetAll(long personID)
{
    List<Interfaces.ITimeline> timelineEvents = new List<Interfaces.ITimeline>();

    timelineEvents.AddRange(Controllers.FollowUp.GetAll(personID));
    //add more to the list as the system grows

    return timelineEvents;
}

That is fine, as long as one of the controllers doesn't throw and error, then the whole Interface collapses. I had something similar happen on a another project, production environment, where a single provider went haywire due a bug on their system, amongst several other providers that were being interfaced with and worked fine. It wouldn't be so bad if it didn't crash all 600+ sites, just because of that mistake somebody else made.

I don't want to do try catches around every single addrange - I think that is just cluttering. Also, with integration like that, you build things to work, but exceptions happens here and there.

What is a proper way of taking data from various sources, controllers or integration, adding them to a collection of items for the interface? That way, if something fails in the middle, it won't bring down the entire page, or even worse, the whole framework.

The error should be caught where it occurs, namely in the controller. If possible try to implement something like this

TimelineEventCollection personTimelineEvents; // Change to the appropriate collection type.
if (Controllers.FollowUp.TryGetAll(personID, out personTimelineEvents)) {
    timelineEvents.AddRange(personTimelineEvents);
}

The advantage of this approach is that the error does not get swallowed up silently by some try-catch-statement. You get a Boolean return value from TryGetAll telling you if all went right. At the same time the basic error handling happens in a low level method and not in your list filling routine.

If you cannot alter the Controllers.FollowUp class, you can still write an extension method giving you the same functionality.


I want a nice design pattern for "All", "many" or "one" In my particular usage here, I'm using two functions to deal with one thing.

Lets say the user request comes in to select "ALL" the records, in which case I would pass on some code such as:

function getRecords() {
    return some_sql("Select * from whatever WHERE id > 0;");
}

But then I want to run just one record, I'd do

function getRecords(SomeID) {
    return some_sql("Select * from whatever WHERE id = SomeID;");
}

And if I wanted to get a specific list of records, I'd do something like

function getRecordsByID(someArray(1,2,3,10,11,99)) {
    var someIDs = "";
    for (i in someArray) {
        someIDs = "(" + someArray.join(",")  + ")";
    }
    return some_sql("Select * from whatever WHERE id IN SomeIDs;");
}

It dawns on me that I could of course use getRecordByID everywhere I've used getRecord, but I can't do this for getRecords.

Is there a common or overlooked design pattern I'm missing here? My first guess would be something like checking the argument's contents for a valid array and running based on that, but I'd like to see a better option.

Why would you want a single method to do three logically different things? Each method you make should really only perform one task. What's the problem in just having the consumer choose the method of selection based upon their need, instead of having a one method fits all sort of thing? If having three methods really isn't what you want, perhaps you could pass a Command pattern object to a database object, to execute the desired functionality?


Pokemon Turn Based battle (Python) This is based off the popular Pokemon Turn Based project (here).

  GOAL

  Write a simple game that allows the user and the computer to take
  turns selecting moves to use against each other. Both the computer and
  the player should start out at the same amount of health (such as
  100), and should be able to choose between the three moves:

  The first move should do moderate damage and has a small range (such as 18-25).
  The second move should have a large range of damage and can deal high or low damage (such as 10-35).
  The third move should heal whoever casts it a moderate amount, similar to the first move.

  After each move, a message should be printed out that tells the user
  what just happened, and how much health the user and computer have.
  Once the user or the computer's health reaches 0, the game should end.

  SUBGOALS

  When someone is defeated, make sure the game prints out that their health has reached 0, and not a negative number.
  When the computer's health reaches a set amount (such as 35%), increase it's chance to cast heal.
  Give each move a name.

I am new to Python and I am trying to use OOP. Did I use my classes properly and is there a way to reduce the number of if statements in my code? Since this is my first code review, how is the formatting, number of comments, etc?

import random

class Pokemon:

    """Blueprint for turn based Pokemon battle"""

    def __init__(self, attack_choice):

        self.__attack_choice = attack_choice

    def attack(self):

        if self.__attack_choice == 1:
            attack_points = random.randint(18,25)
            return attack_points

        elif self.__attack_choice == 2:
            attack_points = random.randint(10,35)
            return attack_points

        else:
            print("That is not a selection. You lost your turn!")

    def heal(self):

        heal_points = random.randint(18,25)
        return heal_points

###########################################################################

user_health = 100
mew_health = 100
battle_continue = True

while battle_continue == True:
    print("\nATTACK CHOICES\n1. Close range attack\n2. Far range attack\n3. Heal")
    attack_choice = eval(input("\nSelect an attack: "))

    # Mew selects an attack, but focuses on attacking if health is full.
    if mew_health == 100:
        mew_choice = random.randint(1,2)

    else:
        mew_choice = random.randint(1,3)

    mew = Pokemon(mew_choice)
    user_pokemon = Pokemon(attack_choice)

    # Attacks by user and Mew are done simultaneously.
    if attack_choice == 1 or attack_choice == 2:
        damage_to_mew = user_pokemon.attack()
        heal_self = 0
        print("You dealt",damage_to_mew,"damage.")

    if mew_choice == 1 or mew_choice ==2:
        damage_to_user = mew.attack()
        heal_mew = 0
        print("Mew dealt", damage_to_user, "damage.")

    if attack_choice == 3:
        heal_self = user_pokemon.heal()
        damage_to_mew = 0
        print("You healed",heal_self,"health points.")

    if mew_choice == 3:
        heal_mew = mew.heal()
        damage_to_user = 0
        print("Mew healed", heal_mew, "health points.")

    user_health = user_health - damage_to_user + heal_self
    mew_health = mew_health - damage_to_mew + heal_mew

    # Pokemon health points are limited by a min of 0 and a max of 100.
    if user_health > 100:
        user_health = 100

    elif user_health <= 0:
        user_health = 0
        battle_continue = False

    if mew_health > 100:
        mew_health = 100

    elif mew_health <= 0:
        mew_health = 0
        battle_continue = False

    print("Your current health is", user_health)
    print("Mew's current health is", mew_health)

print("Your final health is", user_health)
print("Mew's final health is", mew_health)

if user_health < mew_health:

    print("\nYou lost! Better luck next time!")

else:

    print("\nYou won against Mew!")

The heal method and the handling of health points strike me as odd. In the current setup,

heal does not heal; it simply returns a random number. It also currently has no direct relation to any part of the Pokemon class, so it doesn't really make sense as a method of the class.
The health points of each Pokemon are "loose" in the script, along with instances of the Pokemon class. You have a Pokemon class representing a Pokemon, and it makes sense that health would be an attribute of a Pokemon itself.

The design can be improved by doing something like (stripped down):

class Pokemon:
    def __init__(self, start_health):
        self.hp = start_health

    def heal(self, heal_amount):
        self.hp += heal_amount

    def hurt(self, damage):
        self.hp -= damage

Now, you can do something like:

mew = Pokemon(100)
mew.hurt(50) # Ow
mew.heal(49) # Back to almost full
print(mew.hp) # Prints 99

And you don't need to have loose health values floating around in the code for each Pokemon in use. Using the method like this also lets you check the health after healing to ensure that you didn't exceed the maximum health allowed. As long as you're going to make use of a class, you can benefit by having it encapsulate all the data (like health) that's directly relevant to it.

I also decided to not have heal generate the random amount to heal for the following reasons:

Are you sure that every time you want to heal you want it to be a random value, and are you sure you will always want it to be a random value in the range (10, 25]? As you mentioned, what about potions? Are you sure you want potion healing to be random, and always in that range? What about when Pokemon level up and have more health? Are you sure that you'll only want to heal them in that narrow range? The Pokemon class simply does not have enough information to always decide how much Pokemon should be healed by, and that shouldn't be its responsibility in the first place anyways.
Using random data complicates testing. Say you want to add tests to ensure the correctness of the hurt and heal methods. You would expect this to always pass:

start_health = 100
poke = Pokemon(start_health)
poke.hurt(50)
poke.heal(50)

# A simplified test
print("Test Passed?", poke.hp == start_health)

If the methods are correct, you would expect this test to always pass. With random data though, your tests can't be as definitive. You may know that it was hurt by some value, then healed by some value, but that isn't enough information to ensure correct functionality (unless you're only testing that it produces results in a certain range).

For small toy projects like this, testing isn't really necessary (although it's always good to practice). When you start dealing with larger projects split across multiple files, written by multiple people, and changing code that you haven't looked at in potentially months, you need tests to ensure that code stays correct from one change to the next. Making testing easier helps ensure the validity of the tests, and prevents you from wasting your time trying to Jerry-rig some test in place after the fact.

If some code wants to hurt the Pokemon by a random amount, they can generate the random data themselves to pass in to hurt.

while battle_continue == True: has a redundant condition. while (and if) just check if their condition is truthy. You can just write:

while battle_continue:


JavaScript function that composes other functions to create a master function I wrote a little Compose function, which takes a variable number of arguments of type function and then calls each of those in order, passing in the value of the previously called function.

My function works just like I want it to, however since the function provides a way to program functionally, I figured it's application should be functional as well--but it's not.

The Compose function:

const Compose = (...functions) => {
    return val => {
        let i = 0,
            len = functions.length,
            fnResult = functions[i](val);

        while (++i < len) {
            fnResult = functions[i](fnResult);
        }

        return fnResult;
    };
};

Application:

const add7 = num => num + 7;
const squared = num => num * num;
const subtract13 = num => num - 13;

const add7ThenSquareThenSubtract13 = Compose(add7, squared, subtract13);

add7ThenSquareThenSubtract13(5);   // 131
add7ThenSquareThenSubtract13(0);   // 2
add7ThenSquareThenSubtract13(100); // 11436

What it's doing under the hood:

// Let's say 'val' is 5.

// 1. Call add7(val), result is 12.
// 2. Call squared(12), result is 144
// 3. Call subtract13(144), result is 131

// subtract13( squared( add7(val)  ) ) === 131

Anyone have any idea how I can make the logic inside the Compose function more functional? I am having a hard time since I am just now learning to think in a functional way. Is it even possible? I don't know of any ways to loop an array without using for or while.

UPDATE:

Never mind, I figured it out. Didn't think I could use reduce for this but I suppose I can:

const Compose = (...functions) => {
    return val => {
        return functions.reduce((prevVal, currFunc) => {
            return currFunc(prevVal);
        }, functions[0](val));
    };
};

It's not pretty, but it works--and it's functional. ;)

One little suggestion on your reduce version is to make the initial value not the first function's result. This way, if ever the arguments is empty functions[0](val) won't call a missing function and throw up. Also makes reduce call the callback for each item, which makes it less of a cognitive burden.

Also, arrow functions that have expressions as its body implicitly returns the expression's value. You can have something as tiny as the following:

const Compose = (...args) => v => args.reduce((p, c) => c(p), v)


Is my Insertion Sort optimal? I'm learning sorting algorithms and wrote my own implementation of Insertion Sort. Is it optimal? Is there anything that can be done better?
#include <iostream>
#include <vector>
#include <algorithm>
#include <vector>

void display(std::vector<int> arr)
{
  for (int i = 0; i < arr.size(); ++i)
    {
      std::cout << arr[i] << ' ';
    }
  std::cout << std::endl;
}

std::vector<int>& insertionSort(std::vector<int>& arr)
{
  int i = 1;
  while (i < arr.size())
    {
      int j = i;
      while ((j > 0) && (arr[j-1] > arr[j]))
    {
      std::swap(arr[j-1],arr[j]);
      --j;
    }
      ++i;
    }
  return arr;
}

int main()
{
  std::vector<int> arr{1,4,3,5,6,2};
  display(insertionSort(arr));
  return 0;
}

Your insertionSort() is OK, but I would write:
template<std::forward_iterator Iter>
void insertion_sort(Iter first, Iter last) {
    for (Iter it = first; it != last; ++it)
        std::rotate(std::upper_bound(first, it, *it), it, std::next(it));
}

insertion_sort(v.begin(), v.end());

display() copys your vector, so make the parameter as const (lvalue) reference.
Use ranged-for loop for printing:
for (auto n : vec) {
    std::cout << n << ' ';
}
std::cout << '\n'; // don't use std::endl

Or you can use one-liner
std::copy(v.begin(), v.end(), std::ostream_iterator<int>(std::cout, ' '));


Print sum of numbers 1 to 200 except mults of 4 or 7 in Python I had a question about using the modulus operator in Python and whether I have used it in a understandable way.

This is how I've written the script:

#sum numbers 1 to 200 except mults of 4 or 7
def main():
    sum = 0
    for x in range(200+1):
        if (x % 4 and x % 7): #is this bad???
            sum = sum + x
    print sum

if __name__ == '__main__':
    main()

So my questions are:

Should I spell out the modulo more clearly? ie if x % 4 != 0 and x % 7 != 0
Should I be approaching this in a very different way?

I think your use of % is fine, but that could be simplified to:

def main():
    print sum([i for i in range(201) if i % 4 and i % 7])

if __name__ == '__main__':
    main()

Edit: Since I had a bug in there, that's a pretty clear indication that the % is a tripwire. Instead, I'd probably do:

def divisible(numerator, denominator):
    return numerator % denominator == 0

def main():
    print sum(i for i in range(201) if not(divisible(i, 4) or divisible(i, 7)))


Print out number x followed by everything in the range of 1 to (x - 1) The original idea is to print the following numbers:

7, 1, 2, 3, 4, 5, 6

So some number x followed by everything in the range 1  to (x - 1). The key is to skip 0.

I have two versions of this. The first is a pretty straightforward for loop with modular arithmetic, skipping 0:

for (int i = 7; i <= 14; i++)
{
    int result = i % 8;

    if (result == 0)
        continue;
    else
        Console.WriteLine(result);
}

The second (and, as a disclaimer, I will almost certainly not be using this in actual production code, I wrote it more for my own learning), is based on closures:

static void Main(string[] args)
    {
        Func<int> counterFunction = RollAround(1, 8);

        for (int i = 0; i < 7; i++)
            Console.WriteLine(counterFunction());

        Console.ReadLine();
    }

    private static Func<int> RollAround(int min, int max)
    {
        int counter = max - 1;

        return () =>
        {
            int result = counter % max;

            while (result < min)
            {
                counter++;
                result = counter % max;
            }

            counter++;

            return result;
        };
    }

I'm curious if people had feedback on how these can be improved. The first one is probably "more important" in that it's intended for production code, but I'm curious about the second one as well just for my own learning (especially since I'm admittedly a bit rusty on closures).

The first is a pretty straightforward for loop with modular arithmetic, skipping 0:

The problem in this code is that you are mixing output with the algorithm logic.

  The second (and, as a disclaimer, I will almost certainly not be using this in actual production code, I wrote it more for my own learning), is based on closures:

I vouch for your decision. This code while might be interesting it has a behaviour that is hard to follow. You basically implemented something that looks like an IEnumerable. However in this code you return a function and what that function does is not documented. You don't know how many times you have to call it.

The solution is then resolving the first problem while using the IEnumerable mechanism that is understood by everyone and has a more comprehensible use:

public static IEnumerable<int> RollAround(int n)
{
    if(n < 1)
    {
        throw new ArgumentException("n must be greater or equal to 1", nameof(n));
    }

    var elems = new[] { n };
    return elems.Concat(Enumerable.Range(1, n - 1));
}

static void Main(string[] args)
{
    foreach(var elem in RollAround(3)){
        Console.WriteLine(elem);
    }
}


Constructor for a packagetarget struct As many of you know goto is usually signs of code smell. However I thought this could be an appropriate case, and would like confirmation or criticism.

Unnecessary section such as the called functions were removed. Every non-standard function returns an int as status, 0 is "OK"; except linkedlist_open() which returns a pointer which could be NULL if the system runs out of memory.

packagetarget_close() does implement the necessary NULL checks.

packagetarget *packagetarget_open()
{
    packagetarget *target = (packagetarget*) malloc(sizeof(packagetarget));

    if (!target) return NULL;

    if (packagetarget_setname(target, ""))
        goto disposer;
    if (packagetarget_setsys(target, PACKAGETARGET_SYS))
        goto disposer;
    if (packagetarget_setarch(target, PACKAGETARGET_ARCH))
        goto disposer;
    if (packagetarget_setmin(target, PACKAGETARGET_MIN))
        goto disposer;
    if (packagetarget_setver(target, PACKAGETARGET_VER))
        goto disposer;
    if (packagetarget_setmax(target, PACKAGETARGET_MAX))
        goto disposer;

    linkedlist *comp = linkedlist_open();

    if (!comp) goto disposer;

    target->comp = comp;

    return target;

    disposer:
        packagetarget_close(target);
        return NULL;
}

Each setter function follows a simple pattern. Since each setter is identical, I will only put the code for packagetarget_setname.

int packagetarget_setname(packagetarget *target, char *name)
{
    if (!target) return 1;

    if (!name) return 2;

    target->name = realloc(target->name, strlen(name) * sizeof(char));

    if (!target->name) return 3;

    strcpy(target->name, name);

    return 0;
}

Here is the packagetarget_close function:

void packagetarget_close(packagetarget *target)
{
    if (!target) return;

    if (target->name) free(target->name);
    if (target->sys) free(target->sys);
    if (target->arch) free(target->arch);
    if (target->min) free(target->min);
    if (target->ver) free(target->ver);
    if (target->min) free(target->min);
    if (target->comp) linkedlist_close(target->comp);

    free(target);

    return;
}

linkedlist_open is a similar function to the packagetarget_open.

linkedlist *linkedlist_open()
{
    linkedlist *list = (linkedlist*) malloc(sizeof(linkedlist));

    if (!list) return NULL;

    list->head = NULL;
    list->length = 0;
    list->remhook = NULL;

    return list;
}

The one thing most of these functions is they may fail when the system runs out of memory. So I have implemented checks for each step.

packagetarget_close()

First of all, you have a copy-and-paste error in packagetarget_close(), where you attempt to free target->min twice, but not target->max.

Next, note that most of the if statements are superfluous.  As per the standard behaviour for free(),

  If ptr is a null pointer, the function does nothing.

packagetarget_open()

You have undefined behaviour due to the way your error-handling works.  The chunk of memory returned by malloc() contains arbitrary junk.  If packagetarget *target = (packagetarget*) malloc(sizeof(packagetarget)) succeeds, but one of the setters fails, then you would call packagetarget_close(), which would then interpret that arbitrary junk as pointers to memory to be freed.  A good way to fix that is to zero the memory before calling any of the setters.  You can either use calloc() instead of malloc(), or memset(), or a struct initializer.

Avoiding malloc()

In C, I prefer the init-cleanup idiom over new-destroy (which you call open-close).  In the init-cleanup idiom, the caller is responsible for providing the chunk of memory to be initialized, which gives the caller the option of providing either stack-based or heap-based memory.

Avoiding goto

While indiscriminate use of goto leads to spaghetti code, there are some circumstances where goto is justifiable, if used in a readily recognizable pattern.

I think that your use of goto isn't horrible, but personally I would prefer to write the if statements as a chain of && expressions.

Suggested solution

#include <stdlib.h>
#include <string.h>

typedef struct packagetarget {
    char *name;
    char *sys;
    char *arch;
    char *min;
    char *ver;
    char *max;
    linkedlist comp;
} packagetarget;

int packagetarget_setname(packagetarget *target, const char *name) {
    if (!target) return 1;
    if (!name) return 2;
    target->name = realloc(target->name, strlen(name) * sizeof(char));
    if (!target->name) return 3;
    strcpy(target->name, name);
    return 0;
}

int packagetarget_setsys(packagetarget *target, const char *sys) {
    …
}

int packagetarget_setarch(packagetarget *target, const char *arch) {
    …
}

int packagetarget_setmin(packagetarget *target, const char *min) {
    …
}

int packagetarget_setver(packagetarget *target, const char *ver) {
    …
}

int packagetarget_setmax(packagetarget *target, const char *max) {
    …
}

packagetarget *packagetarget_cleanup(packagetarget *target) {
    if (target) {
        free(target->name);
        free(target->sys);
        free(target->arch);
        free(target->min);
        free(target->ver);
        free(target->max);
        linkedlist_cleanup(&target->comp);
    }
    return target;
}

packagetarget *packagetarget_init(packagetarget *target) {
    static const packagetarget empty = {};
    if (target) {
        *target = empty;
        if (!(0 == packagetarget_setname(target, "") &&
              0 == packagetarget_setsys(target, PACKAGETARGET_SYS) &&
              0 == packagetarget_setarch(target, PACKAGETARGET_ARCH) &&
              0 == packagetarget_setmin(target, PACKAGETARGET_MIN) &&
              0 == packagetarget_setver(target, PACKAGETARGET_VER) &&
              0 == packagetarget_setmax(target, PACKAGETARGET_MAX) &&
              linkedlist_init(&target->comp))) {
            packagetarget_cleanup(target);
            return NULL;
        }
    }
    return target;
}

int main() {
    packagetarget t;
    if (packagetarget_init(&t)) {
        …
    }
    packagetarget_cleanup(&t);
}


Increase performance for this keyup function? I have a keyup function that expands an input field as text is added. There are a lot of fields where this code will be applied, what is the best way to write this for best performance?

Here is a working demo: http://jsfiddle.net/J9Ner/

$('input.name','#bakery').live('keyup', function() {
    var el = $(this);
    var len = el.val().length;

    if (len < 20) {
        el.width('200px');
        return;
    } else if (len > 56) {
        return;
    }

    el.width((len * 10) + 'px');
});

You can rearrange the if/else to be a little more efficient.

You can retrieve the length a little more efficiently.

And, use .delegate() (pre jQuery 1.7) or .on() (jQuery 1.7+) with a selector of a parent object close to the elements.  That will perform much better than .live() for large numbers of elements.

It could look something like this:

$('#bakery').on('keyup', 'input.name', function() {
    var el = $(this);
    var len = this.value.length;

    if (len < 20) {
        el.width('200px');
    } else if (len <= 56) {
        el.width((len * 10) + 'px');
    }
});

If you don't actually need .live() or .on() type behavior, it's even more performant to avoid the need for event propagation and bind the event handlers directly to the input elements themselves so no selector matching must be done on each key event:

$('#bakery input.name').keyup(function() {
    var el = $(this);
    var len = this.value.length;

    if (len < 20) {
        el.width('200px');
    } else if (len <= 56) {
        el.width((len * 10) + 'px');
    }
});

And, then removing the jQuery .width() call would speed it up some more:

$('#bakery').on('keyup', 'input.name', function() {
    var len = this.value.length;

    if (len < 20) {
        this.style.width = '200px';
    } else if (len <= 56) {
        this.style.width = (len * 10) + 'px';
    }
});

You can see this last one working here: http://jsfiddle.net/jfriend00/7Pb8b/