Extracting nodes from a road network My code takes 143.023 seconds for extracting nodes from a road network  of city like Göteborg in Sweden. Please check it out if I can optimize it.

# -*- coding: utf-8 -*-
import arcpy
from Class import loading_layer
import time

def node_extraction(network_input):
    time_start=time.time()
    nodes_list = []
    list_of_existing_nodes = []
    for feature in network_input:
        coordinates_list = list()

        feature_startX = feature["Shape"].firstPoint.X
        feature_startY = feature["Shape"].firstPoint.Y
        start_list =[]
        start_list.append(feature_startX)
        start_list.append(feature_startY)

        feature_endX = feature["Shape"].lastPoint.X
        feature_endY = feature["Shape"].lastPoint.Y
        end_list =[]
        end_list.append(feature_endX)
        end_list.append(feature_endY)

        if nodes_list.__len__() is not 0:
            if start_list in list_of_existing_nodes:
                point_index=list_of_existing_nodes.index(start_list)
                nodes_list[point_index][1].append(feature["FID"])
            else:
                nodes_list.append((feature["Shape"].firstPoint, [feature["FID"]]))
                list_of_existing_nodes.append(start_list)
            if end_list in list_of_existing_nodes:
                point_index=list_of_existing_nodes.index(end_list)
                nodes_list[point_index][1].append(feature["FID"])
            else:
                nodes_list.append((feature["Shape"].lastPoint, [feature["FID"]]))
                list_of_existing_nodes.append(end_list)
        else:
            nodes_list.append((feature["Shape"].firstPoint, [feature["FID"]]))
            nodes_list.append((feature["Shape"].lastPoint, [feature["FID"]]))
            list_of_existing_nodes.append(start_list)
            list_of_existing_nodes.append(end_list)
    time_end=time.time()
    print time_end-time_start

layer1 = loading_layer("D:/path/to/your/road/data.shp")
layer1.create_new_dict()
node_extraction(layer1.new_features_dict)

Load_layer class is as follows:

import arcpy
from arcpy.sa import *
from arcpy import env
import re
from _collections import defaultdict

class loading_layer(object):

    def __init__(self, input_data):
        self.features_dict = defaultdict(list)
        self.new_features_dict = []
        self.Address = input_data
        self.file_name = re.search(r'\w+.shp',input_data).group()
        self.coord_sys = arcpy.Describe(self.Address).spatialReference
        self.coord_sys_name = arcpy.Describe(self.Address).spatialReference.name

        self.fieldList = [f.name for f in arcpy.ListFields(self.Address)]
        if "matched_to".upper() not in self.fieldList:
            arcpy.AddField_management(input_data, u"Matched_to", "TEXT")
            self.fieldList.append(u"Matched_to")
            print "field is added"
        print str(self.fieldList)

    def iterate_features(self):
        feature_set = arcpy.SearchCursor(self.Address)
        for feature in feature_set:
            yield feature

    def create_new_dict(self):
        iterator = self.iterate_features()
        for item in iterator:
            feature = dict()
            for f in self.fieldList:
                feature[f] = item.getValue(f)
            self.new_features_dict.append(feature)

Performance:

if start_list in list_of_existing_nodes:
    point_index=list_of_existing_nodes.index(start_list)
    nodes_list[point_index][1].append(feature["FID"])

This is not an efficient way to update an existing element. The fact that you want to lookup a node based on it's coordinates means that you are using the wrong data structure. A dictionary is explicitly designed to make key-based lookups fast, making this type of manipulation easy. This way you can have the point object as the key and the value would be the data object that might have a FID value.

What you have now will cause each look to iterate over the entire list to see if the node exists. Then when it does, iterate again to find the index. As the number of nodes increase, the lookup will take longer and longer. Using a dictionary will take a constant amount of time independent of how many nodes are in the collection.

def iterate_features(self):
    feature_set = arcpy.SearchCursor(self.Address)
    for feature in feature_set:
        yield feature

def create_new_dict(self):
    iterator = self.iterate_features()
    for item in iterator:
        # ...

This is not providing any benefit. If SearchCursor creates a list, then the list has already been allocated containing all of the values. This means your generator method is just adding additional iteration overhead when switching back and forth between contexts. If SearchCursor is already a generator (which I suspect is the case based on the name cursor), you are just wrapping it without adding any additional values.

Other Points:

coordinates_list is never used, so remove it. Additionally, this is the only case where you use list() instead of [], you should be consistent and use [].

Don't suffix your variables with _list, making the name plural should be sufficient to convey that the value is a collection.

A list is not a great data structure for when each index has specific meaning. If you just need a simple data store object, namedtuple is great for that.

The data you are pulling values from has X and Y attributes. Is there a reason you don't just use that object directly?

Creation of the start and stop point can be cleaned up a lot, even if you stick with lists.

shape = feature["Shape"]
start = [shape.firstPoint.X, shape.firstPoint.Y]
end = [shape.lastPoint.X, shape.lastPoint.Y]

if nodes_list.__len__() is not 0:

should be

if len(nodes_list) == 0:

The built-in function is much cleaner and doesn't depend in the internal implementation.

is is an identity comparison and == is an equivalence comparison. The fact that an identity comparison works is an implementation detail and is not guaranteed to always be true.

Edit:
Since we are dealing with a list, this check is also equivalent to

if not nodes_list:

This is the recommended style by the style guide.

You are performing the same operation for the start and end points. you can create a single function that does the operation instead of repeating the code. When you change to using a dictionary, write the generic function first, then you can call it once for each point.

The naming convention for instance variables is lower_case. Address will make other people think it is a class.


Random line remover from file This asks the user for the file name and the number of lines they want to remove. It then stores each line into a vector. A dynamic array is used to hold the random line numbers (generated by random generator) and the corresponding element in the vector is deleted. This vector is then copied to a new file named "temp.txt" where each element is its own line.

Questions:

What can I improve to make this code for efficient? i.e. Better methods to perform this task?
What should I not use and should change (for example any bad habits)?
What did I use that is good and should keep it as a habit?
Are there any exceptions I can use to ensure code?

Warnings I received:

  Warning    C26495  Variable 'LineEditor::array' is uninitialized. Always initialize a member variable (type.6). for LineEditor().
  Avoid unnamed objects with custom construction and destruction (es.84).    for getFileName() in void removeLines().

#include <iostream>
#include <string>
#include <fstream>
#include <random>
#include <ctime>
#include <cstring>
#include <vector>
#include <algorithm>
#include <iterator>

template<typename T>
void writeTo(const std::string& filename, std::vector<T>& v)
{
    std::ofstream fstream(filename);
    if (!fstream)
        exit(1);
    std::copy(v.begin(), v.end(), std::ostream_iterator<T>(fstream, "\n"));
    fstream.close();
}

class LineEditor
{
    std::vector<std::string> buf;
    std::string fileName;
    int noOfLines{ 0 };
    int* array;
public:
    LineEditor()
    {
    }

    ~LineEditor()
    {
        std::cout << "\"" << fileName << "\"" << " " << noOfLines << " lines removed.\n";
        delete[] array;
    }

    void removeLines()
    {
        getFileName();
        TotalLinesToRemove();

        std::ifstream fileIn;
        std::string line;
        int lineCounter{ 0 };
        int* array = new int[noOfLines];

        fileIn.open(fileName);
        if (!fileIn)
        {
            std::cerr << fileName << " can not be opened." << std::endl;
            exit(1);
        }
        while (fileIn.is_open())
        {
            while (std::getline(fileIn, line))
            {
                lineCounter++;
                buf.push_back(line);
            }
            for (int i{ 0 }; i < noOfLines; ++i)
            {
                array[i] = getRandomNumber(0, lineCounter - 1);
                buf.erase(buf.begin() + (array[i]));
                --lineCounter;
            }
            writeTo("temp.txt", buf);
            fileIn.close();
        }
    }
private:
    std::string getFileName()
    {
        std::cout << "Enter name of text file: ";
        std::cin >> fileName;

        return fileName;
    }
    int TotalLinesToRemove()
    {
        std::cout << "Enter the number of lines to remove: ";
        std::cin >> noOfLines;

        return noOfLines;
    }
    int getRandomNumber(int min, int max)
    {
        std::mt19937 seed{ static_cast<std::mt19937::result_type>(std::time(nullptr)) };
        std::uniform_int_distribution<> rand(min, max);
        return rand(seed);
    }
};

int main()
{
    LineEditor file;
    file.removeLines();
    return 0;
}

I am fairly beginner to intermediate at C++ and want to improve.

Use the force library Luke!

Your code does use std::vector, but for reasons I don't understand, it also uses new to allocate a manually managed dynamic array as well. You don't seem to gain anything from this, so I'd advise using std::vector throughout.

I'd also look at the standard library's algorithms. Some of them (e.g., std::remove_copy_if) could be put to excellent use for this task.

Avoid std::endl

std::endl not only writes a new-line to a stream (which is what you usually want) but also flushes the stream (which you usually don't want). If you do that very often, it can slow a program significantly.

In your case, you using it when writing to std::cerr, which automatically flushes in any case, so the flushing done by std::endl is unnecessary in any case.

Random generator seeding

When you're using <random> (a good idea) the generally accepted way of seeding a generator is by using std::random_device. This is usually a truly random device (e.g., reading from /dev/random) so it normally does a substantially better job of seeding than using the system time.

As @1201ProgramAlarm already pointed out, you normally want to seed a generator exactly once, then just generate numbers from it.

Unfortunately, doing a good job of seeding a random number generator is a somewhat nontrivial task. I posted some code that demonstrates what I believe is one reasonable possibility in a previous answer. The short summary is that regardless of whether you use time(nullptr) or std::random_device, if you only use a 32-bit seed, you're limiting yourself to only \$2^{32}-1\$ possible sequences that you can generate, which is a huge limitation compared to the full capabilities of MT19377.

Single Responsibility Principle

It seems to me that your LineEditor does more than I'd like a single class to do. For most practical purpose, the entire program is embodied in that one class. That doesn't strike me as entirely ideal. I'd rather see individual pieces responsible for the individual parts of doing the job.

This also fits better with having clearly defined layers of abstraction. For example, you might have one layer that deals with the file name, and a separate one that deals with the actual content of the file.

Convenient Interface

For most uses, I'd rather run the program something like removeLines foo.txt 10, rather than doing removeLines, then having to separately walk through a questionairre (so to speak) to tell it the name of the file and number of lines to remove. Of course, depending on the situation, that might be better as something like removeLines -f foo.txt -n 10 (especially if this were part of a larger program that might have more/other command line arguments as well.

If you are going to ask questions interactively, I'd ask for the filename first, then read in the file, then ask for the number of lines to remove. This will allow you to check how many lines you've actually read, which (in turn) you can use to advise the user as to the maximum number of lines they can have removed (e.g., "Please enter the number of lines to remove (1-723): ").

Constructor Definition

You've currently defined your constructor with no member initializer list and nothing in the body. It's not clear what you're hoping to accomplish by defining it explicitly rather than just accepting what the compiler would define if you didn't define one at all. Until/unless your ctor will actually accomplish something, it's probably better not to define it explicitly.

Possible Code

#include <iostream>
#include <vector>
#include <string>
#include <set>
#include <fstream>
#include <iterator>
#include "rand.h"

struct InputSpec {
    std::string filename;
    std::size_t count;

    InputSpec(int argc, char **argv) {
        if (argc != 3)
            throw std::runtime_error("Usage: removeLines <filename> <count>");

        filename = argv[1];
        count = std::atoi(argv[2]);

        if (count == 0)
            throw std::runtime_error("numbers of lines to remove can't equal 0");
    }
};

int main(int argc, char **argv) {
    InputSpec input{argc, argv};

    std::ifstream infile(input.filename);
    std::vector<std::string> lines;
    std::string line;
    while (std::getline(infile, line))
        lines.push_back(line);

    int maxLines = std::min(input.count, lines.size());

    generator g(maxLines);
    std::set<int> removals;

    while (removals.size() < maxLines)
        removals.insert(g());

    int current_line = 0;
    std::remove_copy_if(lines.begin(), lines.end(),
        std::ostream_iterator<std::string>(std::cout, "\n"),
        [&](std::string const &) { return removals.find(++current_line) != removals.end(); });
}

Note that for the moment, this presumes that the number of lines to be removed from the file is small compared to the number of lines in the file. If there's a significant chance that won't be true, you may want to see the selection algorithm in yet another old answer.


Daily Coding Challenge solution - day 21 The Challenge
Given an array of time intervals (start, end) for classroom lectures (possibly overlapping), find the minimum number of rooms required.

For example, given [(30, 75), (0, 50), (60, 150)], you should return 2.

I believe this code has no errors but if you find any, please do let me know. Otherwise, feel free to use this solution if you get stuck

My Solution

def item_2(element):
    return (element[1]) #Returns second element of tuple for sorting

classes = [(15,50),(51,53),(54,58),(60,70),(61,63),(65,69),(71,83)]
#classes = [(30, 75), (0, 50), (60, 150), (0, 500), (40, 70)]
classes.sort(key=item_2)

print("The following classes will be needed")
while classes:
    item = classes[0]
    index = 0

    end_time = item[1]
    remaining = classes[1:]
    room_bookings = []
    room_bookings.append(item)
    for next_item in remaining:
        if next_item[0] > end_time:
            room_bookings.append(next_item)
            classes.remove(next_item)
            end_time = next_item[1]
    print(room_bookings)
    classes.remove(item)

To clearly show which part of your code is the interesting part, you should define a function:

def rooms_required(classes):
    classes = classes[:] # do not modify the argument, make a copy of it
    rooms_bookings = []

    # TODO: insert your code here.

    return len(rooms_bookings)

To describe the function in more details, you should add some documentation about the expected data types:

from typing import List

def rooms_required(classes: List[(int, int)]) -> int:
    ...

This way, you can easily define several test cases:

def test_rooms_required():

    # The example from the challenge.
    assert rooms_required([(30, 75), (0, 50), (65, 150)]) == 2

    assert rooms_required([]) == 0

    # This class needs no time at all, therefore it doesn't need a room.
    assert rooms_required([(0, 0)]) == 0

    # Classes that go backwards in time should either be ignored or raise an exception.
    assert rooms_required([(1200, 1100)]) == 0

    # Ensure that rooms_required does not modify the given list,
    # as that would be surprising.
    classes = [(0, 1), (0, 2), (0, 3), (2, 3)]
    assert rooms_required(classes) == 3
    assert rooms_required(classes) == 3

Read more about pytest to see how to run these automatic tests.

Some more detailed remarks, from top to bottom:

def item_2(element):
    return (element[1]) #Returns second element of tuple for sorting

Since this challenge is about time spans, it's more helpful to the reader of your code if you call this function end, since it returns the end of a time span.

def end(span):
    return span[1]

And since the meaning of [0] in the code is not really obvious, you should also define:

def start(span):
    return span[0]

Then, instead of span[0] and span[1], you can write start(span) and end(span), which clearly tells a story.

classes = [(15,50),(51,53),(54,58),(60,70),(61,63),(65,69),(71,83)]
#classes = [(30, 75), (0, 50), (60, 150), (0, 500), (40, 70)]

As I mentioned above, instead of testing only one scenario at a time, you should define a function that does the interesting work, which then allows you to have automatic tests, and several of them at the same time.

print("The following classes will be needed")

That's wrong. It should be "the following rooms" instead.

while classes:
    item = classes[0]

The word item is a very general term. A better name for this variable is span.

    index = 0

This variable is not used in the rest of the code. You can remove this line.

    room_bookings = []
    room_bookings.append(item)

You can simplify this into room_bookings = [span].

    for next_item in remaining:
        if next_item[0] > end_time:

Instead of the end_time variable (end time of what exactly?), you can write start(next_span) > end(span), which makes the expression look symmetrical and easy to read aloud: if the next span starts later than the current span ends …

When you use this pattern, you can remove the end_time variable, which will make the code a little shorter.

    print(room_bookings)

The function that computes the room bookings should not print anything. Printing things is the job of the main program. That's a general rule.


Python script to convert hierarchical JSON to LaTeX itemize I have the following JSON:

{
  "items": [
    {
      "item": "abc",
      "status": "ready"
    },
    {
      "item": "def",
      "status": "ready"
    },
    {
      "item": "ghi",
      "status": "done",
      "items": [
        {
          "item": "ABC",
          "status": "ready"
        },
        {
          "item": "DEF",
          "status": "ready"
        },
        {
          "item": "GHI",
          "status": "done"
        }
      ]
    }
  ]
}

which I would like to convert to:

\begin{itemize}
    \item abc\hfill{}ready
    \item def\hfill{}ready
    \item ghi\hfill{}done
      \begin{itemize}
      \item ABC\hfill{}ready
      \item DEF\hfill{}ready
      \item GHI\hfill{}done
      \end{itemize}
\end{itemize}

The items can be nested arbitrarily deep. I have the following Python code:

import json
import os

TAB = '  '
BEGIN = '\\begin{itemize}'
ITEM = '\\item {0}\\hfill{{}}{1}'
END = '\\end{itemize}'

def main(path):
    with open(os.path.join(path, 'test.json')) as data_file:
        data = json.load(data_file)
    print('\n'.join(convert(data)))

def convert(data):
    stack = [data['items'].__iter__()]
    yield (TAB * (len(stack) - 1)) + BEGIN
    while len(stack) > 0:
        iterator = stack[len(stack) - 1]
        try:
            current = iterator.next()
            yield (TAB * (len(stack) - 1)) + ITEM.format(current['item'], current['status'])
            try:
                stack.append(current['items'].__iter__())
                yield (TAB * (len(stack) - 1)) + BEGIN
            except KeyError:
                pass
        except StopIteration:
            yield (TAB * (len(stack) - 1)) + END
            stack.pop()

I am new to Python and am looking for suggestions for the code to be more idiomatic Python.

Unless you specifically want a tab of 2 spaces in the resulting LaTeX, you can use '\t' to get the actual tab character.

TAB = '\t'

When working with strings with one or more backslashes, you can declare them as 'raw' strings by prefixing them with an r, which allows you to write backslash characters without having to escape them.

BEGIN = r'\begin{itemize}'
ITEM = r'\item {0}\hfill{{}}{1}'
END = r'\end{itemize}'

You can use the magic method iter() to get iterators, for example:

stack = [iter(data['items'])]

or

stack.append(iter(current['items']))

This is more idiomatic than calling the __iter__() method directly. In addition, there is a next() magic method as well which you can use to get the next item from an iterator.

current = next(iterator)

If you are using Python 3.4 or higher, I'd recommend checking out the pathlib module for manipulating file paths over os.path, but in this case using os.path does the job and is probably sufficient.


Project Euler #8 - "largest product in a series" This code works when compiled on gcc 4.8.2.

I would appreciate any comments on my style and the readability of my code, and any improvements that I could make to make the code more readable and/or performant.

Problem:
Largest product in a series

Solution:

/*
 * file:    pe_008.cc
 * title:   Largest product in a grid
 * date:    October 2, 2014
 *
 * note:    The largest possible product of 13 consecutive digits
 *          in a decimal number is 9^13. log_2(9^13) = ~41.2, so we
 *          will need to use a 64-bit integer to represent the product.
 *
 * note 2:  Since some of the elements in the series are 0. The product
 *          of the digits in the 13-digit subseries will be 0 if any of
 *          the elements are 0. This means that if we see a 0, we can
 *          skip ahead by 13 digits, but we have to cache the nonzero
 *          digits that we skip over.
 */

#include <fstream>
#include <iostream>
using namespace std;

const int DIGITS_IN_PRODUCT = 13;

int main() {
  // read series.txt into a string without newline characters
  ifstream ifs("../series.txt");
  string series;
  while (ifs.good()) {
    string line;
    getline(ifs, line);
    series += line;
  }
  ifs.close();
  // take the product of the first 13 elements in the string
  uint64_t product = 1;
  uint64_t max_product;
  uint8_t product_digits[DIGITS_IN_PRODUCT];
  for (size_t i = 0; i < DIGITS_IN_PRODUCT; i++) {
    product_digits[i] = series[i] - '0';
    product *= product_digits[i];
  }
  max_product = product;
  int zero_for_next = 0;
  for (size_t i = DIGITS_IN_PRODUCT; i < series.length(); i++) {
    // use the modulo operator to minimize overwrites
    product_digits[i % DIGITS_IN_PRODUCT] = series[i] - '0';
    if (series[i] == '0') {
      zero_for_next = DIGITS_IN_PRODUCT;
      continue;
    } else if (zero_for_next < 1) {
      product = 1;
      // the order in which the product is taken is not important
      for (uint8_t digit : product_digits) {
        product *= digit;
      }
      if (product > max_product) {
        max_product = product;
      }
    }
    zero_for_next--;
  }
  cout << max_product << endl;
}

This is probably not a good idea:

while (ifs.good()) {
  string line;
  getline(ifs, line);
  series += line;
}

You should be checking the stream itself, not the current state of the stream.  This can be done by putting the stream read (getline() in this case) inside the loop statement.

Also, as getline() (which uses line) will be moved out of scope after this change, you'll need to initialize line before the loop.

string line;

while (getline(ifs, line)) {
    series += line;
}

This will both attempt the read and determine if it was successful.  If it wasn't, specifically if the end of the file has been reached, then it will fail, and the loop will terminate.  See this answer for more info.
Your compiler should've warned you about this type-mismatch:

for (size_t i = 0; i < DIGITS_IN_PRODUCT; i++)

The loop counter type should be an int to match the constant's type.


Ruby Fibonacci(n) recursive computation optimized with reflection The idea is to take the common-known (and awfully bad performing) Fibonacci(n) recursive method:

# recursively computate fibonacci(n)
def fib(n)
  n <= 2 ? 1 : fib(n-2) + fib(n-1)
end

and to optimize it with some Ruby reflection:

require 'benchmark'

def fib(n)
    # if n<= 2 fib(n) = 1
    return 1 if n <= 2
    # if @fib_n is defined it means that another instance of this method
    # has already computed it, so I just return its value
    return instance_variable_get("@fib_#{n}") if instance_variable_get("@fib_#{n}")
    # else I have to set fib_n and return its value
    instance_variable_set("@fib_#{n}", ( fib(n-2) + fib(n-1) ) )
end

Benchmark.bm(30) do |x|
    x.report("fibonacci(#{ARGV[0]}) computation time:") {$fib = fib(ARGV[0].to_i)}
end

puts "fib(#{ARGV[0]}) = #{$fib}"

Since the execution time for fib(36) drops from about 4 sec to 0.000234 sec, I guess it's working! But since I'm a Ruby novice (and since this is my first attempt with reflection), I'd still like to have my code peer-reviewed.

Is my choice to use '@fib_n' instance variables correct or there is a better choice?
Does anyone know a better Ruby optimization for the recursive computation of Fibonacci members? (I know, the most efficient computation for Fibonacci is the iterative one, but here I'm strictly interested in the recursive one).

considering that this is Code Review, I have to say that you probably don't want to use an instance variable. You expose the internal data, and any user could destroy the workings of your method inadvertently. In Ruby 1.9, Ruby added a very interesting feature for just these types of situations that involve infinite sequences. It's called a Fiber.

It's true that Fibers aren't perfect for situations where you have to go backwards in the sequence in addition to forward.

Another option for this is to put the data in a module:

module Fibonacci
  @fib=[0,1,1]
  def self.fib_array(n)
    @fib[n] ||= fib_array(n-2) + fib_array(n-1)
  end
end

And use it like so:

Fibonacci.fib_array(42)


Filtering ASP.NET membership roles without the default attributes I am building an application, and I needed an attribute similar to AuthorizeAttribute, supporting Roles but not using the MembershipProvider roles. (Basically, with the setup I have, I cannot rely on the User.IsInRole method - it's not always accurate, so I manually query the roles in my MasterDbContext instead.)

I have two different parts to it:

public class RequiredRoleAttribute : FilterAttribute, IAuthorizationFilter
{
    public string Roles { get; set; }
    public string Destination { get; set; } = "~/Account/Login";

    public void OnAuthorization(AuthorizationContext filterContext)
    {
        var context = new MasterDbContext();
        var user = context.Users.Find(filterContext.HttpContext.User.Identity.GetUserId());

        var requiredRoles = Roles.Split(',').Select(roleName => context.Roles.First(role => role.Name == roleName).Name).ToList();

        if (!requiredRoles.All(r => user.InRole(r, context)))
        {
            if (Destination == null)
            {
                filterContext.Result = new RedirectToRouteResult("Default", null);
            }
            else
            {
                filterContext.Result = new RedirectResult(Destination);
            }
        }
    }
}

This should be self-explanatory. It does the same thing as [AuthorizeAttribute(Roles = "SomeString")], used in conjunction with that attribute:

[Authorize]
[RequiredRole(Roles = Constants.Roles.Moderators)]
public abstract class BaseController : Controller
{
    public MasterDbContext Context { get; } = new MasterDbContext();
}

So that's easy. As you can see, I can still use [Authorize], I just can't use [Authorize(Roles = ...)].

Then, I wrote User.InRole instead of the User.IsInRole method:

public bool InRole(string roleName, MasterDbContext providedContext = null)
{
    var context = providedContext ?? new MasterDbContext();
    var roleId = context.Roles.First(r => r.Name == roleName).Id;
    return Roles.ToList().Any(r => roleId == r.RoleId);
}

Any/all comments welcome, this is another huge portion of this application and I would love to make it as effective as possible.

public abstract class BaseController : Controller
{
    public MasterDbContext Context { get; } = new MasterDbContext();
}

I really don't like this. Every single controller class derived from BaseController has a public IDisposable dependency. That's taking encapsulation for a little "talk", outside, in a dark alley.

I can't think of a single reason Context would need to be public for. It should be protected.

But that's not the biggest issue. From the outside, say, from a derived class' point of view, inheriting from BaseController seems like a casual thing to do: the name suggests it, strongly.

Every derived controller inherits an IDisposable dependency that it didn't ask for - and since the base class creates it and doesn't take its responsibilities (he who creates an IDisposable instance, should be the one to call Dispose), if every derived type fails to override the controller's Dispose method, every single controller is leaking resources.

So you could implement IDisposable.Dispose in the BaseController.

Or, you could simply get rid of BaseController altogether. Disposable resources are important, they shouldn't be implicit. There's a very solidly (yet far from SOLID) tightly-coupled relationship between every controller and MasterDbContext - and that relationship is toxic.

There has to be a better way. This is more explicit:

[Authorize]
[RequiredRole(Roles = Constants.Roles.Moderator)]
public class AdminController : Controller
{
    private MasterDbContext Context { get; } = new MasterDbContext();

    // controller methods...

    protected override void Dispose(bool disposing)
    {
        if (disposing && Context != null)
        {
            Context.Dispose();
            Context = null;
        }

        base.Dispose(disposing);
    }
}

Now now, I hear you - "but what about DRY? I have to repeat that code in every controller?". No you don't.

What you can do, is make it someone else's job to create (and therefore to dispose) the context: you tell that "someone else" that you need a MasterDbContext, and let them deal with its lifetime.

[Authorize]
[RequiredRole(Roles = Constants.Roles.Moderator)]
public class AdminController : Controller
{
    private readonly MasterDbContext _context

    public AdminController(MasterDbContext context)
    {
        _context = context;
    }

    // controller methods...
}

Now anyone can look at that class and know that it requires a Constants.Roles.Moderator role, and that it has a MasterDbContext dependency. It's still coupled with a concrete type, but at least it's explicit about it. And we don't need to worry about disposing the context anymore - a controller that receives its DbContext through the constructor can happily assume that whoever put it there is going to dispose it when it destroys it.. and exactly when that happens is not its job either: the controller had nothing to do with creating the context.

This poses a problem though: the default controller factory requires a default constructor. And if you add one, then your context is always going to be null... which defeats the purpose of telling your caller that you have a dependency on MasterDbContext.

The solution is nothing less than implementing your own controller factory! Kidding. You can simply take an existing one - ninject, for example, will substitute the default controller factory for one that uses the Ninject IoC container to create an instance per request and inject it into the controllers' constructors.

All that's left to do is to tell Ninject that whenever a controller requests a MasterDbContext, you give it a MasterDbContext instance. The registration code might look like this:

Kernel.Bind<MasterDbContext>().To<MasterDbContext>().InRequestScope();

And from there, you're an inch away from being able to do this instead:

Kernel.Bind<IUnitOfWork>().To<MasterDbContext>().InRequestScope();

Which means your controllers can look like this:

[Authorize]
[RequiredRole(Roles = Constants.Roles.Moderator)]
public class AdminController : Controller
{
    private readonly IUnitOfWork _context

    public AdminController(IUnitOfWork context)
    {
        _context = context;
    }

    // controller methods...
}

And here goes the "D" of SOLID:

  Depend on abstractions, not on concrete types.

The IUnitOfWork interface can be a simple, minimal wrapper for DbContext:

public interface IUnitOfWork : IDisposable
{
    IDbSet<T> Set<T>();
    void Commit();
}

And now you can instantiate an AdminController in a unit test, mock the IUnitOfWork dependency, and write unit tests for every controller method!


Function to split either a string array or string into a string array I am programming a language interpreter in C# and have recently created a set of functions that receive either a string or a string[] and split it by a received string.

For example, with a string:

"Hey:123:hello:456"
":"

It will return this array:

{"hey","123","hello","456"}

And with a string[]:

{"a:b","c","d:e:f"}
":"

It will return this array:

{"a","b","c","d","e","f"}

How can I improve efficiency and maybe make it tidier?

public string[] SplitRows(object thevar, string delimiter) {

            if (delimiter == "<newline>") delimiter = "\n";

            if (thevar.GetType() == typeof(string)) {
                string temp = (string) thevar;
                return temp.Split(new string[] {
                    delimiter
                }, System.StringSplitOptions.None);
            }
            if (thevar.GetType() == typeof(string[])) {
                return stringarraysplitter((string[]) thevar, delimiter);
            }
            return null;
        }

        public string[] stringarraysplitter(string[] arr, string delimiter) {
            string[][] tempr = new string[arr.Length][];
            for (int i = 0; i < arr.Length; i++) {
                tempr[i] = arr[i].Split(new string[] {
                    delimiter
                }, System.StringSplitOptions.None);
            }
            System.Collections.Generic.List < string > templist = new System.Collections.Generic.List < string > ();
            for (int i = 0; i < tempr.Length; i++) {
                for (int j = 0; j < tempr[i].Length; j++) {
                    templist.Add(tempr[i][j]);
                }
            }

            return templist.ToArray();
        }

Side notes:

SplitRows() is the only main function allowed to receive either a string array or string. Even though I use 2 functions, I cannot create 2 for the 2 variable types.
<newline> is a replacement string for \n in my language and can be ignored.

if (delimiter == "<newline>") delimiter = "\n";

Why do you need this? Why not use normal strings?

if (thevar.GetType() == typeof(string)) {

What's wrong with normal method overloading?

if (thevar.GetType() == typeof(string)) {...}
if (thevar.GetType() == typeof(string[])) {...}
return null;

If arguments is not valid throw an ArgumentException. Don't say "I'll just return null, and see where it will go wrong."

temp.Split(new string[] {delimiter}, System.StringSplitOptions.None);

This bit repeats, and should be extracted into a method.

for (int i = 0; i < tempr.Length; i++) {
    for (int j = 0; j < tempr[i].Length; j++) {

Use Linq to simplify loop operations.

Here is the cleaned up version:

    public IEnumerable<string> Split(string str, string delimiter)
    {
        return str.Split(new[]{delimiter}, StringSplitOptions.None);
    }

    public IEnumerable<string> Split(IEnumerable<string> arr, string delimiter) {
        return arr.SelectMany(s => Split(s, delimiter));
    }


Efficient algorithm for counting frequency of a numbers in intervals I need to build a bar graph that illustrate a distribution of pseudorandom numbers that determined by linear congruential method

$$X_n+1 = (a  X_n + c) \mod m$$
$$U = X/m$$

on the interval [0,1]

For example:

Interval           Frequency
[0;0,1]            0,05
[0,1;0,2]          0,15
[0,2;0,3]          0,1
[0,3;0,4]          0,12
[0,4;0,5]          0,1
[0,5;0,6]          0,15
[0,6;0,7]          0,05
[0,7;0,8]          0,08
[0,8;0,9]          0,16
[0,9;1,0]          0,4

I used such a method:

float mas[10] = {0,0,0,0,0,0,0,0,0,0};
void metod1()
{
    int x=-2, m=437, a=33, c=61;
    float u;
    for(int i=0;i<m;i++){
        x=(a*x + c) % m;
        u=(float)x/(float)m;
        int r;
        r = ceil(u*10);
        mas[r] = mas[r] + 1;
    }
    for(i=0;i<10;i++) cout<<"["<<(float)i/10<<";"<<(float)(i+1)/10<<"]"<<" | "<<mas[i]<<"\n-----------------"<<endl;
    return;
}

If you know another efficient algorithm for this problem, that isn't straightforward, I would appreciate it.

I see a number of things that could help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.  Know when to use it and when not to (as when writing include headers).  I don't know for sure that you've done this in your code (since it's just a fragment and not a complete program) but it's an amazingly common thing for especially new C++ programmers to do.

Fix your program bugs

If this code compiled on your machine, your compiler is broken.  The problem is that the definition of i in the first for loop goes out of scope when that loop ends.  That means that the second for loop should start for (int i=0; ... or it isn't really valid C++.  Also, with your \$X_0\$ term being a negative integer, the index for the mas array when you calculate \$X_3\$ will also be negative, which will cause a memory access error (and probably a program crash) when the value of mas is updated.  The initial value of x should be a positive integer.

Use more whitespace for readability

The second for loop line is extremely dense and hard to read.  To make it easier to read, put things on separate lines and use whitespace to make things easier to read and understand:

for (int i = 0; i < 10; i++) {
    std::cout << "[" << (float)i / 10 << ";" << (float)(i + 1) / 10 << "]"
        << " | " << mas[i] << "\n-----------------" << std::endl;
}

Think carefully about the mathematics

The code that is being used to decide which "bin" the psuedo random number goes into is faulty.  It currently uses ceil(u*10) but the problem is that this will incorrectly classify numbers into mas[10] which is out of range.  Instead, the code should use the simpler version:

int r = u * 10;
mas[r]++;

Note also the use of the more idiomatic ++ operator rather than the form that is currently in the code.

Omit the return for a void function

The return at the end of void function metod1 is not needed.  Omit it to make the code cleaner and easier to read.

Pass parameters instead of using global variables

The mas parameter is neither defined within the function nor passed as a parameter, so it must be a global variable.  It's almost always better to explicitly pass the variables instead of using global variables.  The advantage is that the coupling and dependencies of the function are explicit and easily read and understood.


Setting bgcolor in Ruby. What's the proper way to do it? <p style="background-color:#<%= debate.bg_color %>;" >

be_color is a method of Debate that returns a string like 45FFFF.

Although it does what I want, this seems like a tremendously bad idea. What would be a better way to accomplish this? (I'm very new to both Ruby and Rails. And new-ish to web development in general.)

inline styles are bad practice

more elegant way would be assigning class to your  element based on debate properties, maybe creating a helper if this line is used frequently


Is there a better way to count seconds in python def stopWatchSeconds(secondCnt) :
    """ counts second for secondCnt seconds """

    # is there a better way to do this?
    startTime = time.clock()
    ellapsed = 0
    for x in range(0, secondCnt, 1) :
        print "loop cycle time: %f, seconds cnt: %02d" % (ellapsed , x)
        ellapsed = 1 - (time.clock() - startTime)
        time.sleep(ellapsed)
        startTime = time.clock()

I'm reviewing some source code for a buddy of mine and came across this function. I am very new to Python so it is more of a learning experience for me. I see that he put in a comment about is there a better way to do this. I started thinking about it, and I know how I would do it in C#, but since python is so much more laxed it does seem like this function could be improved.

I'm not sure this code does what you think it does, at least on a *nix OS. According to the documentation, time.clock() returns processor time, which basically means that any time spent in time.sleep() is not counted. However, on Windows, time.clock() returns the actual time since it was first called.

A better way to determine the time since the function was first called is to use time.time(), which returns the number of seconds since the epoch. We can still use time.clock() to determine the process time if we are using a *nix OS. (If you are using Windows, you may want to upgrade to Python 3.3 which introduces the platform independent function time.process_time(). (There may be another way to do it if you are using 2.x, but I don't know what it is.)

#!/usr/bin/python
import time

def stopwatch(seconds):
    start = time.time()
    # time.time() returns the number of seconds since the unix epoch.
    # To find the time since the start of the function, we get the start
    # value, then subtract the start from all following values.
    time.clock()
    # When you first call time.clock(), it just starts measuring
    # process time. There is no point assigning it to a variable, or
    # subtracting the first value of time.clock() from anything.
    # Read the documentation for more details.
    elapsed = 0
    while elapsed < seconds:
        elapsed = time.time() - start
        print "loop cycle time: %f, seconds count: %02d" % (time.clock() , elapsed)
        time.sleep(1)
        # You were sleeping in your original code, so I've stuck this in here...
        # You'll notice that the process time is almost nothing.
        # This is because we are spending most of the time sleeping,
        # which doesn't count as process time.
        # For funsies, try removing "time.sleep()", and see what happens.
        # It should still run for the correct number of seconds,
        # but it will run a lot more times, and the process time will
        # ultimately be a lot more.
        # On my machine, it ran the loop 2605326 times in 20 seconds.
        # Obviously it won't run it more than 20 times if you use time.sleep(1)

stopwatch(20)

If you find the comments more confusing than helpful, here's a non-commented version...

#!/usr/bin/python
import time

def stopwatch(seconds):
    start = time.time()
    time.clock()
    elapsed = 0
    while elapsed < seconds:
        elapsed = time.time() - start
        print "loop cycle time: %f, seconds count: %02d" % (time.clock() , elapsed)
        time.sleep(1)

stopwatch(20)


Return different type of response based on Accept header I'm a novice to Scala and Akka-Http and am experimenting with Akka-Http for writing rest services. I have to return JSON or protobuf based on the Accept-Header.

optionalHeaderValueByName("Accept"){ contentType =>
  if(contentType == Some(protoEncode)) {
    complete {
      NewsService.getNewsList().map {
        case stories: List[Story] =>    HttpResponse(entity = HttpEntity(ContentType(protoEncoding),  StoryList(stories).toProto().build().toByteArray))
      }
    }
  } else {
      complete {
        NewsService.getNewsList().map {
          case stories: List[Story] => StoryList(stories)
          }
        }
  }

As you can see, the code repetition is happening. Can anyone suggest what could be the best way to optimise and generalise the design to avoid such a situation?

I don't know Akka-Http API well enough, but it looks like the if condition is misplaced. I think that it can be substituted with a matcher under case stories:

optionalHeaderValueByName("Accept") { contentType =>
  complete {
    NewsService.getNewsList().map {
      case stories: List[Story] => {
        contentType match {
          case Some(protoEncode) =>
            HttpResponse(entity = HttpEntity(ContentType(protoEncoding), StoryList(stories).toProto().build().toByteArray))
          case None => StoryList(stories)
        }
      }
    }
  }
}

It seems that it is a bit too deeply nested, but at least there is no repetition on NewsService.getNewsList().map.


Pseudo-truly random number generator I am creating what strives to be a truly random number generator. As of right now, the implementation uses a mix of mainly nonsense system data and some pseudo-random numbers to generate numbers that are different and use the last half of the number (which, statistically, I believe is sufficiently random). The advice I would like:

Obviously, a general "review", as always.
How can I optimize this code to be faster? Currently, generating ten 1000 digit numbers takes two seconds, ten 10000 digit numbers takes 35-37 seconds, and ten 100000 digit numbers is exponentially slower.
How practical is this? Currently the generator uses some methods which may be considered "hacky"; ergo, using the last half of a number, or including pseudo-random numbers in the algorithm. Is this OK? I believe it should be, as long as the numbers generated are indeed random, but I would like other's opinions on this.

Another thing to note is that when testing some sample batches with this online tester, I got p-values around 0.24, which I believe is the optimal range. If you answer, don't feel pressured to answer all three questions.

import os, re, time, random
from functools import reduce

def rand_num(digits=3):
  nums = []
  strs = []
  rand = []
  add = True
  all_info = re.sub('(.*?: *)','',os.popen("cat /proc/meminfo").read()).replace('kB','').splitlines()+list(filter(None,re.findall(r'(\w*)',os.popen('ps').read())))
  all_info = list(x.strip() for x in all_info)
  nums.append(random.sample(range(digits*70), digits*35))
  nums = nums[0]
  for x in all_info:
    if x.isdigit():
      nums.append(int(x))
    else:
      strs.append(x)
  jugglenum = 0
  for x in nums:
    if add == True:
      jugglenum += x
      add = False
    else:
      jugglenum -= x
      add = True
  jugglenum = abs(jugglenum)
  for i in range(digits):
    rand.append(int(time.time()*(os.getpid()%jugglenum)))
  for i, x in enumerate(rand):
    rand[i] = ''.join(random.sample(str(x), len(str(x))))
  rand = [int(x)//10 for x in rand]
  largenum = str(reduce(lambda x, y: x*y, rand))[:digits]
  return ''.join(random.sample(random.sample(largenum,len(largenum)),len(largenum)))

An executable program on repl.it.

Yes, I know, this is similar to random.SystemRandom or os.urandom. This program I am developing now is really my first look into this concept and I don't plan for this to be my final program by any means. This is just something that I thought I would play around with for the time being.

So before you go and tell me that this is pointless and could have no application, I know. This is just my own personal experiment so I can get better at this sort of program.

Couple of minor simplifications:

nums =[]
...
nums.append(random.sample(...))
nums = nums[0]

could be replaced with:

nums = random.sample(...)

The add/jugglenum loop calculation could be done with:

jugglenum = abs(sum(nums[::2]) - sum(nums[1::2]))

This code:

for i, x in enumerate(rand):
    rand[i] = ''.join(random.sample(str(x), len(str(x))))

could be replaced with:

rand = [ ''.join(random.sample(s, len(s)) for s in (str(x) for x in rand)]

which eliminates the enumeration and avoids turning x into a str(x) twice.

I don’t see strs being used anywhere, so the code that calculates it can be removed.


Which approach is preferable string parsing? I want to find total word count in string. I have two methods as mentioned below.

public static int Wordcount(string strName)
{
    bool wordfound;
    int ctr = 0;

    for (int i = 0; i < strName.Length; )
    {
        wordfound = false;

        while ((i < strName.Length) && (strName[i] == ' '))
            i++;

        while ((i < strName.Length) && (strName[i] != ' '))
        {
            if (!wordfound)
            {
                ctr++;
                wordfound = true;
            }
            i++;
        }
    }

    return ctr;
}

public static int Wordcount1(string strName)
{
    string temp = null;
    int ctr = 0;

    for (int i = 0; i < strName.Length; i++)
    {
        if (strName[i] != ' ')
        {
            if (temp == null)
                ctr++;

            temp += strName[i];
        }
        else if (strName[i] == ' ')
            temp = null;
    }

    return ctr;
}

Can anyone please comment on better approach w.r.t. logic, performance and memory utilization?

Let's analyze each approach:

public static int Wordcount1(string strName)
{
    string temp = null;
    int ctr = 0;

    for (int i = 0; i < strName.Length; i++)
    {
        if (strName[i] != ' ')
        {
            if (temp == null)
                ctr++;

            temp += strName[i];
        }
        else if (strName[i] == ' ')
            temp = null;
    }

    return ctr;
}

You are using temp string only as boolean flag (whether its null or not). So, you don't need to store current word in this variable. Actually temp += strName[i] creates many strings in memory, because strings are immutable. So, its performance and memory consuming. Remove it.

Also you don't need second if. If char is not white space, then it is white space. There is no third option. And improve naming - don't use prefixes for variable names.

public static int Wordcount(string text)
{
    bool wordFound = false
    int count = 0;

    for (int i = 0; i < text.Length; i++)
    {
        if (text[i] != ' ' && !wordFound)
        {
            wordFound = true;
            count++
            continue;
        }

        if (wordFound)
            wordFound = false;
    }

    return count;
}

Another approach:

public static int GetWordsCount(string strName)
{
    bool wordfound;
    int ctr = 0;

    for (int i = 0; i < strName.Length; )
    {
        wordfound = false;

        while ((i < strName.Length) && (strName[i] == ' '))
            i++;

        while ((i < strName.Length) && (strName[i] != ' '))
        {
            if (!wordfound)
            {
                ctr++;
                wordfound = true;
            }
            i++;
        }
    }

    return ctr;
}

For me its a little harder to understand, because you are incrementing loop variable in loop body on some conditions inside other loops. But you are not creating strings here as you do in your second approach. So it's definitely better in terms of performance and memory usage, than you original second approach.

In second approach you can eliminate usage of boolean flag, and condition check in second while loop. Also with some comments this code becomes easier to understand:

public static int GetWordsCount(string text)
{
    int count = 0;

    for (int i = 0; i < text.Length; )
    {
        // process to next word
        while ((i < text.Length) && (text[i] == ' '))
            i++;

        if (i < text.Length)
            count++;

        // loop till the word end
        while ((i < text.Length) && (text[i] != ' '))
            i++;
    }

    return count;
}

But of course its better to use already existing functionality:

return text.Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries).Length;

Its not that fast, and it creates array of words which uses memory. But it's easy to understand for other developers. So, instead of doing premature optimization, I'd go with this approach and modified it only in case of performance problems.


Improve this javascript code: It displays hidden text and highlights the selected page number This code (http://jsbin.com/isexez/5/edit) functions as expected:
(1) it displays the page selected; and
(2) it highlights the number of the selected page.

I would appreciate help in improving it.

I am in the process of writing an ebook (http://www.keepcount.net/ebook/01-09/02_tricks.html) and need to improve this code before I get back to my writing.

*  Here's my jQuery code as suggested by @Gisborne *
See also  jsbin.com/iduxol/2/edit

<!DOCTYPE html>

 -->

<meta charset=utf-8 />
<title>JS Bin</title>

 p1
 p2

   text on page 1

   quiz on page 2

Here are some ideas:

Use either tab or, preferably, 4-spaces for indention consistently. Right now the indention is all screwy on JSBin.
Put all your variable declarations at the top, even if you don't define them yet. This'll prevent any confusion from variable hoisting.
Since null is a falsy value, you can change document.getElementById(...) !== null to document.getElementById(...) to check for the existence of an element in you if statements.
Space out your if statements (so if (...) instead of if(...).)
Maybe think about using jQuery? It'll make working with events and elements much easier than using the DOM directly. So for example, hiding all the pages could be written as easy as: $('div').hide().

That's all I have time for now!


A recursive algorithm to find the number of distinct quantities of money from a pile of coins Problem: find the number of distinct quantities of money that a pile of coins can make.

The vector coins contains the values of the coins, and the corresponding vector quantity contains the number of coin there are (let's say coins = {1} and quantity = {5}, this means I have 5 pennies).

Although I have solved this problem, I would like to make it more time efficient. Could someone give me some pointers?

Miscellaneous info: the variable ctq stands for "coins to quantity". I solved this by systematically iterating through all possible combinations of coins and putting their sum into a set. There has to be a better way to solve this problem, because this problem is supposed to be a dynamic programming question and I solved this problem without any knowledge of what that's supposed to be.

#include<iostream>
#include<vector>
#include<map>
#include<set>

int money(std::map<int, int> x)
{
    int sum=0;
    for(auto it=x.begin(); it!=x.end(); ++it)
    {
        sum+=it->first*it->second;
    }
    return sum;
}

int rec
(
    std::set<int> &sums,
    std::map<int, int> ctq,
    std::vector<int> coins,
    std::vector<int> quantity,
    int m
)
{
    for(int k=quantity[m]; k>=0; k--)
    {
        ctq[coins[m]] = k;
        sums.insert(money(ctq));
        if(m != 0)
        {
            rec(sums, ctq, coins, quantity, m-1);
        }
    }
    return sums.size()-1;
}

int possibleSums(std::vector<int> coins, std::vector<int> quantity)
{
    std::map<int, int> ctq;
    for(std::size_t i=0; i<coins.size(); i++)
    {
        if(ctq.find(coins[i]) == ctq.end())
        {
            ctq.insert(std::make_pair(coins[i], quantity[i]));
        }
        else
        {
            ctq[coins[i]] += quantity[i];
        }
    }

    std::vector<int> n_coins, n_quantity;
    for(auto it = ctq.begin(); it != ctq.end(); ++it)
    {
        n_coins.push_back(it->first);
        n_quantity.push_back(it->second);
    }

    std::set<int> sums;
    return rec(sums, ctq, n_coins, n_quantity, n_coins.size()-1);
}

int main()
{
    std::vector<int> coins = {10, 50, 100, 200};
    std::vector<int> quantity = {511, 22, 30, 50};
    std::cout << possibleSums(coins, quantity) << "\n";
    return 0;
}

Instead of calculating each combination recursively, you can build up the list one coinage type at a time.  This would involve fewer iterations in the case that any combinations form the same total, and more importantly wouldn't require adding up each combination separately.  For example:

std::set<int> sums;
for(int c = 0; c < coins.size(); ++c)
{
    // Include zero on the initial list, but not subsequent ones
    if(sums.empty())
    {
        for(int q = 0; q <= quantity[c]; ++q)
        {
            sums.insert(q * coins[c]);
        }
    }
    else
    {
        std::vector<int> current(sums.begin(), sums.end());
        for(int q = 1; q <= quantity[c]; ++q)
        {
            for(int sum : current)
            {
                sums.insert(sum + q * coins[c]);
            }
        }
    }
}

Using a vector instead of a set to accumulate sums (removing duplicates at the end) might be faster, and would eliminate the need for a temporary copy (current).  However, it wouldn't eliminate the redundant loops based off the same total, so some measurements would be needed to see which is better.

std::vector<int> sums;
for(int c = 0; c < coins.size(); ++c)
{
    // Include zero on the initial list, but not subsequent ones
    if(sums.empty())
    {
        for(int q = 0; q <= quantity[c]; ++q)
        {
            sums.push_back(q * coins[c]);
        }
    }
    else
    {
        int oldSize = sums.size();
        for(int q = 1; q <= quantity[c]; ++q)
        {
            for(int i = 0; i < oldSize; ++i)
            {
                sums.push_back(sums[i] + q * coins[c]);
            }
        }
    }
}

std::sort(sums.begin(), sums.end());
sums.erase(std::unique(sums.begin(), sums.end()), sums.end());


Should I use setters with null or no setter when null value? I would like to know what are the drawbacks of setting values this way:

class PersonFactory
{
    public function createPerson($firstName = null, $lastName = null, $separator = null)
    {
        $person = new Person();
        $person
            ->setFirstName($firstName)
            ->setLastName($lastName)
            ->setSeparator($separator);

        return $person;
    }
}

And if this way would be more clear, sustainable or efficient

class PersonFactory
{
    public function createPerson($firstName = null, $lastName = null, $separator = null)
    {
        $person = new Person();
        if (!null === $firstName) {
            $person->setFirstName($firstName);
        }

        if (!null === $lastName) {
            $person->setLastName($lastName);
        }

        if (!null === $separator) {
            $person->setSeparator($separator);
        }

        return $person;
    }
}

Example call:

class PersonController extends Controller
{
    public function indexAction()
    {
        // get the PersonFactory instance
        $person = $this->get('person.factory')
            ->createPerson('Bob', 'Whatever', '-');

        return $this->render('some/file.html.twig', [
            'person' => $person
        ]);
    }
}

It feels like they are the same as null is already the value of an undefined variable, at least in PHP (looking at you Javascript).

So, would the only difference would be to call the setters even if it's not useful ?

First off i'm a little confused by what  you are trying to do, and why you are doing this that way. I don't really grasp the full context of what and why you are doing it that way.

IMO since you are returning the same type of person, you don't need a person factory (update). You would need a person factory class if you were returning different types of People. Have a look at the method of polymorphism employed here: https://gist.github.com/BKSpurgeon/db4f8062569181d871f1565c89103e7e notice how we have different types of cities: LondonCity, SanFranCity - they all supply slightly different behaviour, but they are all cities.
setLastName not instance methods within the person class? Please have a look at the code below for what i mean. You probably should make these method private if you don't intend to call them.
"that's a no-no for OOP diehards"

this is what i mean.

# as OOP programmers we don't want to be doing things like this:

if x == 2 THEN do this
If x == 3 THEN do this
if x  == 4 THEN do this.
# As OOP programmers this is all we want to do:
#
cww = ClassWhichDoesWork.new(x)
cww.DoWork()
#
# and then we use a Factory method and polymorphism to supply the correct behaviour.
# we have no conditionals which say if x == 2 THEN do this. all of that is taken care of by polymorphism.
# if you do it that way you code will be very easy to maintain and to make changes, and to test as well.

Here is a gist which is probably how I would structure the above code given my very limited understanding of what you are trying to do


Finding missing items in an int list Here is a problem I am trying to solve: trying to find missing photographs from a sequence of filenames. The problem boils down to: given an unsorted list of integers, return a sorted list of missing numbers. Below is my code.

What I am looking for are:

Are there more efficient algorithms?
Is there any performance problem if the list is large (tens of thousands)
Corner cases which does not work?

def find_missing_items(int_list):
    '''
    Finds missing integer within an unsorted list and return a list of
    missing items

    >>> find_missing_items([1, 2, 5, 6, 7, 10])
    [3, 4, 8, 9]

    >>> find_missing_items([3, 1, 2])
    []
    '''

    # Put the list in a set, find smallest and largest items
    original_set  = set(int_list)
    smallest_item = min(original_set)
    largest_item  = max(original_set)

    # Create a super set of all items from smallest to largest
    full_set = set(xrange(smallest_item, largest_item + 1))

    # Missing items are the ones that are in the full_set, but not in
    # the original_set
    return sorted(list(full_set - original_set))

if __name__ == '__main__':
    import doctest
    doctest.testmod()

For this

full_set = set(xrange(smallest_item, largest_item + 1))

I'd suggest inlining the min and max:

full_set = set(xrange(min(original), max(original) + 1))

You don't need to convert to a list before using sorted so:

sorted(list(full_set - original_set))

Can be written as:

sorted(full_set - original_set)


Directory listing, sorting and filtering in F# Here is my first try at F#. My only functional experience is from JS.

This program takes a path as an argument and presents the user with numbered subdirs in that path. The user then selects one of the subdirs by entering it's number and the program opens explorer with the newest subdir of the selected subdir.

Any comments will be greatly appreciated — shorter and better syntax, conversions to point-free style, structuring...

open System
open System.IO

let toDirWithCreationDate path =
    (path, Directory.GetCreationTime(path))

let getLatestSubDir =
    Seq.map toDirWithCreationDate
    >> Seq.sortByDescending snd
    >> Seq.head
    >> fst

let getNumberedSubDirTuples =
    let ints = Seq.initInfinite (fun n -> n + 1)
    Directory.GetDirectories
    >> Seq.sort
    >> Seq.zip ints

let printDirMenu =
    Seq.iter (fun (a, b) -> printfn "%A %A" a b)

let openDir (dir:string) =
    ignore (System.Diagnostics.Process.Start(dir))

[<EntryPoint>]
let main argv =
    let path = argv |> Seq.head
    let dirsSeq = getNumberedSubDirTuples path
    printDirMenu dirsSeq
    let dirNr = System.Console.ReadLine()
    let dir =
        dirsSeq
        |> Seq.tryFind(fun (a,b) -> a = int(dirNr))
    let latestSubdir =
        match dir with
            | Some d -> Directory.GetDirectories (snd d) |> getLatestSubDir |> openDir
            | None -> printfn "Invalid input"
    0

I am not sure if I can summarize the changes I made better, but I made several refactorings and better error handling (when inputing a string, for example, your program crashed.) I also chose to make it zero-indexed, rather than 1-indexed, mostly to simplify the code.

open System
open System.IO

let getLatestSubDir = Seq.maxBy Directory.GetCreationTime // Refactored
let sortedSubDirs = Directory.GetDirectories >> Seq.sort // Refactored
let printDirMenu =
    sortedSubDirs >> Seq.iteri (fun idx path -> printfn "%d %s" idx path)
let openDir (dir : string) = System.Diagnostics.Process.Start(dir) |> ignore

[<EntryPoint>]
let main argv =
    let path = argv |> Seq.head
    printDirMenu path
    let input = System.Console.ReadLine()
    match Int32.TryParse(input) with
    | true, n ->
        let dir =
            path
            |> sortedSubDirs
            |> Seq.tryItem n
        match dir with
        | Some d ->
            Directory.GetDirectories(d)
            |> getLatestSubDir
            |> openDir
        | None -> printfn "Invalid input"
    | _ -> printfn "Invalid input"
    Console.ReadKey() |> ignore
    0

It is definitely possible to factor the sorting so it is only done once, rather than twice in my code.

The code also crashes when the chosen folder doesn't have any sub folders.


"Heads or Tails" game I'm a beginner in programming and made a pretty simple heads or tails type game. I would like to know if I have done this efficiently. If not, please let me know what you would have done differently.

#include <iostream>
#include <cstdlib>
#include <ctime>
using namespace std;

void headsOrTails();
int randomNumber();

int main()
{
    headsOrTails();
}

int randomNumber()
{
    int number;
    srand(time(0));
    for(int i = 1; i < 2; i++)
    {
        number = 1 + (rand()%2);
    }
    return number;
}

void headsOrTails()
{
    string prediction;
    cout << "Heads or Tails? ";
    cin >> prediction;

    if((prediction == "Heads") || (prediction == "heads")){
        if(randomNumber() == 1){
            cout << "Heads, you are correct!\n" << endl;
        }
        else if(randomNumber() == 2){
            cout << "Tails, you were wrong.\n" << endl;
        }
    }
    if((prediction == "Tails") || (prediction == "tails")){
        if(randomNumber() == 1){
            cout << "Heads, you were wrong.\n" << endl;
        }
        else if(randomNumber() == 2){
            cout << "Tails, you are correct!\n" << endl;
        }
    }

    char playAgain;
    cout << "Play again?(Y/N) ";
    cin >> playAgain;
    cout << "\n";

    while((playAgain == 'Y') || (playAgain == 'y')){
        headsOrTails();
    }
    while((playAgain == 'N') || (playAgain == 'n')){
        exit(EXIT_FAILURE);
    }
}

Unnecessary loop

I noticed this code:

for(int i = 1; i < 2; i++)
{
    number = 1 + (rand()%2);
}

It's a loop that loops exactly once, and it doesn't even use i inside the loop.  So you can just remove the loop part:

number = 1 + rand()%2;

Unecessary recursion

At the end of headsOrTails(), you have this code:

while((playAgain == 'Y') || (playAgain == 'y')){
    headsOrTails();
}
while((playAgain == 'N') || (playAgain == 'n')){
    exit(EXIT_FAILURE);
}

Each while here could really be an if.  Neither of the loops actually loop.
It's not a good practice use recursion for no reason.  Ideally, you would have a loop in main() that calls headsOrTails() as long as the user wants to keep playing.  That way, you wouldn't need recursion.  The only reason this even works instead of continuing indefinitely is because you call exit() eventually.
Why EXIT_FALURE?  That means your program will always return failure.  I would use EXIT_SUCCESS instead.


NHibernate select with Query Over optimization for user roles base case I have a complicated select procedure that I solved with a 5 query select, so I am posting it here to get suggestions on how I can shorten it up a bit.  I am hoping that someone can help me make it a single query select.
I'm using nhibernate query over here.

I have 7 tables which are

t_user
t_category
t_event
t_announcement
t_event_follower
t_event_register
t_event_subcribe

I will represent them in the classes below:

class User {
  int id;
  string username;
  IList<Registration> registrations;
  IList<Category> subscribes;
  IList<Event> follows;
}

class Category {
  int id;
  string description;
  IList<User> subscribers;
}

class Event{
  int id;
  string title;
  Category category;
  IList<Registration> registrations;
  IList<User> followers;
  IList<Announcement> announcements;
}

class Announcement {
  int id;
  string title;
  Event event;
  bool isForFollower;
  bool isForRegister;
  bool isForSubscriber;
}

class Registration {
  int id;
  Event event;
  User user;
  string additionalInfo;
  RegistrationStatus status;
}

enum UserRole {
  PUBLIC,
  FOLLOWER,
  SUBSCRIBER,
  REGISTERED
}

The goal is to show all announcements in 1 event based on user roles on that event.  What I had done first is populate user role in list with the code below:

User requester = _userRepo.SearchBy(username); // 1 query for user detail

// 3 lazy load objects make nhibernate to load the object from db, i assume it takes 3 query select if objects haven't load in session
bool isFollower = requester.Follows.Any(eventz => eventz.Id == eventId);
bool isSubscriber = requester.Subscribes.Any(category => category.Events.Any(eventz => eventz.Id == eventId));
bool isRegistered = requester.Registrations.Any(registration => registration.Event.Id == eventId);
var userRoleList= new List<UserRole>();

if (isFollower) userRoleList.Add(UserRole.FOLLOWER);
if (isSubscriber) userRoleList.Add(UserRole.SUBSCRIBER);
if (isRegistered) userRoleList.Add(UserRole.REGISTERED);

last with code in repository layer below i create dynamic disjunction for query over :

Disjunction filterRestriction = Restrictions.Disjunction();
// this condition to select announcement which is for public not specific
filterRestriction.Add(Restrictions.Where<Announcement>(a => a.IsForFollower == false
                                                            && a.IsForSubscriber == false
                                                            && a.IsForRegistered == false));

foreach (UserRole role in userRoleList)
{
    switch (role)
    {
        case UserRole.FOLLOWER:
            filterRestriction.Add(Restrictions.Where<Announcement>(a => a.IsForFollower));
            break;
        case UserRole.SUBSCRIBER:
            filterRestriction.Add(Restrictions.Where<Announcement>(a => a.IsForSubscriber));
            break;
        case UserRole.REGISTERED:
            filterRestriction.Add(Restrictions.Where<Announcement>(a => a.IsForRegistered));
            break;
    }
}
//this is the last query transaction, so total 5 times select from db CMIIW,
return _session.QueryOver<Announcement>()
               .Where(filterRestriction)
               .And(a => a.Event.Id == id)
               .List();

In the above code, the query result will show public announcement plus specific announcement based on role that user have in related event.

I admit this is the simplest solution I could think to solve the case.  Could someone give a suggestion or rework this just by 1 simple operation with query over or just remake it with better query?

If this is a web application, you can use an HttpModule to do your session management. Begin every web request by opening a session and beginning a transaction. At the end of every web request, commit the transaction and close the session.

As long as you're wrapping all the hits to the database in a single transaction, you don't need to worry about efficiency too much.


PHP & Mysqli Code - Is this code safe for a simple search? I am writing some code to search my database and show results. Is the below code ok and safe enough? new to PHP and Mysqli so really appreciate any help that I can get on this subject. Thanks

<?php

    $conn = mysqli_connect("localhost", "root", "", "tutorial");

    if(mysqli_connect_errno()){
        echo "Failed to connect: " . mysqli_connect_error();
    }

    error_reporting(0);

    $output = '';

    if(isset($_GET['q']) && $_GET['q'] !== ' '){
        $searchq = $_GET['q'];

        $q = mysqli_query($conn, "SELECT * FROM search WHERE keywords LIKE '%$searchq%' OR title LIKE '%$searchq%'") or die(mysqli_error());
        $c = mysqli_num_rows($q);
        if($c == 0){
            $output = 'No search results for "' . $searchq . '"';
        } else {
            while($row = mysqli_fetch_array($q)){
                $id = $row['id'];
                $title = $row['title'];
                $desc = $row['description'];
                $link = $row['link'];

                $output .= '

' . $title . '

' . $desc . '

                            ';
            }
        }
    } else {
        header("location: ./");
    }
    print("$output");
    mysqli_close($conn);

?>

It is not safe, you are completely open to SQL Injection. You need to use prepared statements.

Apart from that:

don't use short variables names (q, c, etc)
don't just echo an error, but actually handle it (if there is no db connection, there is no need to execute queries)
try to reduce your nesting, eg by including guard clauses:

Example of guard clauses:

if(!isset($_GET['q']) || $_GET['q'] == ' '){
    // return, die, etc
}

// perform query

if($c == 0){
    // return, die, etc
}

// handle query output


Refactoring based on OPEN CLOSE PRINCIPLE- C# I have some code to write Error Logs to different places like Console/SignalR Messages/Text File.

The code is as follows:-

public class Logger
    {
    public void WriteToLog(string message, LogType logType)
    {
        switch (logType)
        {
            case LogType.Console:
                Console.WriteLine(message);
                break;

            case LogType.SignalR:
                // Code to send message to SignalR Hub
                break;

            case LogType.File:
                // Code to write in .txt file
                break;
        }
    }
}

Where my LogType is a simple Enum as below:-

public enum LogType
{
    Console,
    SignalR,
    File
}

But, when I think of Open-Close Principle, I am not getting optimal solution for it.

Edit:

I need to refactor my code as per open close principle. I need to optimize it, so that I don't need to Modify WriteToLog method once any LogType gets added.

You should create an interface from which you can make many implementations, e.g. an ILogger interface with Warning, Error methods etc.
Then you would create an implementation per logging type (one for console, one for SignalR etc). You could then inject the ILogger into any type that requires it (or another approach is to use a static field and resolve the logger through a static log manager type).
You might also want to consider a library such as Common.Logging which already provides an interface and several implementations already available, e.g. NLog, log4net etc.
Another consideration is whether you wish to mix business logic and logging code. Other alternatives are an AOP approach (PostSharp is one option) or semantic logging.


Is this a correct way to use Factory Pattern? I have an abstract class which implements the IDataCreator interface:

public interface IDataCreator
{
    public String getDataString();
}

public abstract class AbstractCreator implements IDataCreator
{
    protected IProject p;

    public AbstractCreator(IProject p) {
       this.p = p;
    }

    public abstract String getDataString();
}

Then I have 5 concrete classes which extend AbstractCreator and implement the getDataString() method:

public class ProjectManagerCreator extends AbstractCreator
{
    public ProjectManagerCreator(IProject p) {
       super(p);
    }

    @Override
    public String getDataString() {
       if(p.getLead() != null) {
           String lead = p.getLead().getName();
           return lead;
       }
       return "";
    }
}

Factory class is implemented like this:

public class ProjectDataCreatorFactory
{
    IProject p = null;

    public ProjectDataCreatorFactory(IProject p) {
       this.p = p;
    }

    /**
     * Factory method which constructs a object from a given string value.
     *
     * @param column string of the column name TODO: Implement a interface which maps the strings into static variables
     * @return object which implements the {@link IDataCreator} interface
     */
    public IDataCreator createFromString(String column) {
        IDataCreator creator = null;
        if(column.equals("name")) {
           return new URLCreator(p);
        } else if(column.equals("status")) {
           return new ProjectStatusCreator(p);
        } else if(column.equals("manager")) {
           return new ProjectManagerCreator(p);
        } else if(column.equals("setupdate")) {
           return new ProjectDateCreator(p, ProjectDateCreator.CREATION);
        } else if(column.equals("updatedate")) {
           return new ProjectDateCreator(p, ProjectDateCreator.UPDATED);
        }
        return creator;
    }
}

The factory class is called like this:

ProjectDataCreatorFactory factory = new ProjectDataCreatorFactory(p);
String s = factory.createFromString("name").getDataString();

Question

Is this the (or a) correct way to use the Factory Pattern? Is the approach with an interface and an abstract class a good design?

Some design considerations:

Returning Null: Your factory method createFromString returns a null creator if the column is unknown. It's good practice to note this in the javadoc or perhaps throw an IllegalArgumentException if a column you can't handle is passed in.
Date Abstraction: ProjectDateCreatorlooks like it could use either its own factory or two separate implementations based on the second constructor argument. That class isn't included here, but it appears CREATION and UPDATED are some sort of public static final type constants. Those would be better expressed as an enum:

// A factory can use this instead!
public enum ProjectDateOption {
    CREATION, UPDATED;
}

Interface Naming: Interfaces in Java aren't typically prefixed with an I, but if it's consistent through your code then that's a minor quibble.
Column Strings: Your TODO mentions this, but the column strings could also be placed into an enum, and then the checking could occur there.


Counting characters in a text file I have a text file and I was curious about what character appears how often in the text.

Any review is appreciated.

public class CountLetters {
    public static void main(String[] args) throws Exception {
        TreeMap<Character, Integer> hashMap = new TreeMap<Character, Integer>();
        File file = new File("C:/text.txt");
        Scanner scanner = new Scanner(file,"utf-8");
        while (scanner.hasNext()) {
            char[] chars = scanner.nextLine().toLowerCase().toCharArray();
            for (Character c : chars) {
                if(!Character.isLetter(c)){
                    continue;
                }
                else if (hashMap.containsKey(c)) {
                    hashMap.put(c, hashMap.get(c) + 1);
                } else {
                    hashMap.put(c, 1);
                }
            }
        }
        for (Map.Entry<Character, Integer> entry : hashMap.entrySet()) {
            System.out.println(entry.getKey() + ": " + entry.getValue());
        }
    }
}

The output will be for example:

a: 1202
b: 311
c: 603
d: 510
e: 2125
f: 373
g: 362
h: 718
i: 1313
j: 5
k: 74
l: 678
m: 332
n: 1129
o: 1173
p: 348
q: 40
r: 812
s: 1304
t: 1893
u: 415
v: 195
w: 314
x: 86
y: 209
z: 9

Resources:
You should start using try-with-resources. This statment does some work for you with resources that implement AutoCloseable. It closes these resources for you, so you don't have to worry about file-locks and remaining database connections:
File file = new File("C:/text.txt");
try(Scanner scanner = new Scanner(file, "utf-8")){
    //your code here ;)
}

You also shouldn't throw Exception in the main method of your program. This can be very confusing to users. Instead you main-method should handle all exceptions "gracefully" by being wrapped into a try-catch-block.
Conditionals:

if(!Character.isLetter(c)){
   continue;
}

This is an early return statement for the purpose of following conditions, meaning you don't have to write else if in your next condition.
Naming
hashMap is not a good name. The map you use is not a Hash-Map, and treeMap would also not explain what the map does, what it contains.
You might want to rename it to characterMap
all else equal, your naming is nice and consistent, and tells exactly what the variables do. You nicely follow camelCase-conventions. Keep it up!
Summary:
Your code reads nicely and is easily understandable. You follow naming conventions and have descriptive and understandable variable names. You should work on your exception handling and the use of resources.


Checking DnD Traits in an enum Overall, I'm working on a simulator for a modified DnD 3.5 system. Right now, I'm trying to write an enum class that handles the Classes/Professions, and am currently trying to handle the profession modifiers for saving throws.

My issue is that everything is extremely boiler-plate, and I can't figure out a better way of doing it. I was thinking of maybe having a constructor for each profession that used true or false to denote if it was a good saving throw or bad one. That's just really bad though, I think, since it doesn't give any information when reading it. I also thought of having goodSaves and badSaves variables in each and then getGoodSaves() and getBadSaves(), but I can't think of an easy way of getting it working without a literal collection like in other languages.

Does anyone have any advice on how I can make this look better?

public enum ProfessionType {
    BARBARIAN {
        @Override
        public int getWillModifier(int level) {
            return badSaveModifier(level);
        }

        @Override
        public int getFortitudeModifier(int level) {
            return goodSaveModifier(level);
        }

        @Override
        public int getReflexModifier(int level) {
            return badSaveModifier(level);
        }
    },
    BARD {
        @Override
        public int getWillModifier(int level) {
            return goodSaveModifier(level);
        }

        @Override
        public int getFortitudeModifier(int level) {
            return badSaveModifier(level);
        }

        @Override
        public int getReflexModifier(int level) {
            return goodSaveModifier(level);
        }
    },

    /* snip containing all the other professions */

    WIZARD {
        @Override
        public int getWillModifier(int level) {
            return goodSaveModifier(level);
        }

        @Override
        public int getFortitudeModifier(int level) {
            return badSaveModifier(level);
        }

        @Override
        public int getReflexModifier(int level) {
            return badSaveModifier(level);
        }
    };

    private static int goodSaveModifier(int level) {
        return ((level / 2) + 2);
    }

    private static int badSaveModifier(int level) {
        return (level / 3);
    }

    /**
     * Calculates the Profession's Fortitude modifier.
     *
     * @param level Character's current level
     * @return Profession's modifier of the Will save
     */
    abstract int getWillModifier(int level);

    /**
     * Calculates the Profession's Fortitude modifier.
     *
     * @param level Character's current level
     * @return Profession's modifier of the Fortitude save
     */
    abstract int getFortitudeModifier(int level);

    /**
     * Calculates the Profession's Reflex modifier.
     *
     * @param level Character's current level
     * @return Profession's modifier of the Reflex save
     */
    abstract int getReflexModifier(int level);

    @Override
    public String toString() {
        return name().substring(0, 1).toUpperCase() + name().substring(1).toLowerCase();
    }
}

I was thinking of maybe having a constructor for each profession that used true or false to denote if it was a good saving throw or bad one. That's just really bad though, I think, since it doesn't give any information when reading it.

I suppose you mean that this would not be particularly readable:

enum ProfessionType {
    BARBARIAN(false, true, false),
    BARD(true, false, true),
    WIZARD(true, false, false);
    // ...
}

Instead of booleans, you could use enums, like Will.STRONG, Fortitude.WEAK, and so on.
The result will be more readable and less boilerplate.

With this refactoring,
the methods like getWillModifier will be no longer abstract,
and would contain the logic (probably a switch) to get the right value given the property of the enum, and the level.

In general, enums are not so well-suited to have behaviors (custom methods),
they are much better as plain values,
possibly with some common behavior (non-abstract methods).

Another alternative world be to use the builder pattern to build attributes, with a syntax like this:

BARBARIAN(Attributes.builder().will(WEAK).fortitude(STRONG).build()),

Where WEAK and STRONG could come from a Defense enum.

That said, the other answer makes an excellent point that perhaps the attributes should not be hardcoded, but read from a configuration file. You would gain a lot of flexibility, such as adding new types or changing the attributes without recompiling the code.


Simple Hangman game - first Python project I created a basic Hangman game that uses a text file to select a secret word. Are there any areas for improvement?

import random

secret_word = ['']
user_list = []
number_of_tries = 5
guessed_letters = []
user_tries = 0
user_guess = ''

def select_word():
    global secret_word, user_list
    with open('secret_words.txt', 'r') as f:
        word = f.read()
        word_list = word.split('\n')
        secret_word = word_list[random.randint(1, len(word_list))]
        user_list = ['-'] * len(secret_word)

def game_over():
    if user_tries == number_of_tries or user_list == list(secret_word):
        return True
    else:
        return False

def user_input():
    global user_guess
    user_guess = input('Guess a letter\n')
    check_guess(user_guess)

def repeated(guess):
    global guessed_letters
    if guess in guessed_letters:
        print('You already guessed that letter!\n')
        return True
    else:
        guessed_letters.append(user_guess)
        return False

def check_guess(guess):
    correct_guess = False
    for x in range(len(secret_word)):
        if guess == secret_word[x]:
            user_list[x] = guess
            correct_guess = True
        elif not correct_guess and x == len(secret_word)-1:
            global user_tries
            user_tries += 1
            print('Wrong guess, you lose one try\n'
                  'Remaining tries : {}\n'.format(number_of_tries - user_tries))
    if correct_guess:
        print('Correct guess!')

def valid_input(user_letter):
    valid_letters = 'qwertyuiopasdfghjklzxcvbnm'
    if user_letter.lower() in list(valid_letters):
        return True
    else:
        print('Invalid input')
        return False

# main code:

print('----HANG MAN----')
print('*Welcome, guess the word\n*you have 5 tries.')
select_word()

while not game_over():
    for x in user_list:
        print(x, end='')
    user_guess = input('\nGuess a letter : ')
    if valid_input(user_guess):
        if repeated(user_guess):
            continue
        else:
            check_guess(user_guess)

if user_list != list(secret_word):
    print('Game over, you died!\ncorrect word was {}'.format(secret_word))
else:
    print('Congratulations! you guessed the correct word\n')

this if statement is a bit silly:

    if user_tries == number_of_tries or user_list == list(secret_word):
        return True
    else:
        return False

It could easily be:

    return user_tries == number_of_tries or user_list == list(secret_word)

since that expression evaluates to True or False, just return it directly
Instead of

secret_word = word_list[random.randint(1, len(word_list))]

you can use the much more readable random.choice:

secret_word = random.choice(word_list)

your user_input routine isn't used at all.  Nuke it or use it.
You use a lot of global state.  This is generally frowned upon because it makes the code less reusable.  You should first off, try to use less global state: put function results into variables and pass them around, instead.  This makes those functions much more reusable.  If you absolute must use global state, still don't: instead make an object that stores that "global" state, and turn the functions that use it into methods on the object.


Iterate over files and convert with a new name / directory I'm pretty new to bash scripting.  I want a script that I can run in a directory that contains .md files, runs pandoc on them, and delivers the output to a sibling folder html, i.e. my folder structure is like this:

.
├── html
└── md

Here is what I have working, I just don't know if I'm following good practices or if there's a nicer / more readable way of doing what I'm doing.

#! /bin/bash
# run this from a folder containing markdown files
# it delivers html to the sibling html folder
if hash pandoc 2>/dev/null; then
  for file in ./*.md ; do
    echo Converting $file
    cat "$file" | pandoc -o "$(echo $file | sed 's/^./..\/html/' |  sed 's/.md$/.html/')"
  done
else
  echo "I need pandoc installed"
fi

UUOC. pandoc takes the input file name as an argument. echo $file | pandoc -o outfile is equivalent to pandoc -o outfile $file. One process invocation down.
UUOS (sed in this case). bash has very rich string transformation features built in. For example, ${file%md} (see Parameter expansion section of man bash) will strip the md suffix form the filename. So consider

for file in *.md; do
    pandoc -o ../html/${file%md}html
done

Two more process invocations down.
If by any chance an .md filename contains funny characters (like whitespace), the substitution will produce unexpected results. It is safer to use double quotes, as in `"${....}".

PS: It might be beneficial to abandon the shell script altogether in favor of make, with a Makefile having a single stem rule

../html/%.html: %.md
    pandoc -o $@ $<


Syllabification function for Turkish words I wrote an NLP script for processing Turkish language. Yesterday I added syllabication but I wonder if it could be done better. It is kinda hard-coded, so I would like to know if I can improve it.

Here is the syllabication part.

def syllabicate(self, word):
    """
    :param word: The word to be syllabicated
    :return: The syllabicated list that contains syllabs
    """
    word = word.lower()
    syllabs = []
    syllab = ""
    keep_index = 0
    last_was_vowel = False
    next_is_vowel = False

    for let_ind in range(len(word)):
        if let_ind != len(word) - 1:
            if word[let_ind + 1] in self.vowels:
                next_is_vowel = True
            else:
                next_is_vowel = False
        else:
            syllab = word[keep_index:]
            syllabs.append(syllab)
            break

        if next_is_vowel and not last_was_vowel and syllab:
            syllabs.append(syllab)
            syllab = ""
            keep_index = let_ind

        elif next_is_vowel and word[let_ind] not in self.vowels and syllab:
            syllabs.append(syllab)
            syllab = ""
            keep_index = let_ind

        syllab += word[let_ind]

        if word[let_ind] in self.vowels:
            last_was_vowel = True
        else:
            last_was_vowel = False

    return syllabs

First of all you should know that the script doesn't syllabize properly for every word. For example if you give the word authenticated the function returns ['aut', 'hen', 'ti', 'ca', 'ted'] which is incorrect. The correct case would be ['au','then','ti','cat','ed']. However I do not know how to fix that.

Edit: The point made above doesn't stand since this script isn't for the english language.

Second you have a lot of ifs where you check if a character is a vowel like this:

if word[let_ind] in self.vowels:
        last_was_vowel = True
    else:
        last_was_vowel = False

They could be written better like this:

last_was_vowel = word[let_ind] in self.vowels

Or even better I would have written a function that checks if a letter is a vowel.

def is_vowel(self, char):
    return char in self.vowels

The first if in your for loop

if let_ind != len(word) - 1:
    if word[let_ind + 1] in self.vowels:
        next_is_vowel = True
    else:
        next_is_vowel = False
else:
    syllab = word[keep_index:]
    syllabs.append(syllab)
    break

checks if the next one is a vowel except the last char.
It would be more good looking like this:

for pos, char in enumerate(word[:-1]):
    next_is_vowel = self.is_vowel(word[pos + 1])
    ...

syllab = word[keep_index:]
syllabs.append(syllab)

We enumerate through the word to get the position of the letter and
the letter itself. The word[:-1] means we are going to iterate the whole word except its last letter.

Lastly you got 2 ifs here that do the same thing. You also don't need the keep_index.

if next_is_vowel and not last_was_vowel and syllab:
    syllabs.append(syllab)
    syllab = ""

elif next_is_vowel and word[let_ind] not in self.vowels and syllab:
    syllabs.append(syllab)
    syllab = ""

The could be easily written as one if like this:

if next_is_vowel and syllab and not (last_was_vowel and self.is_vowel(char)):
    syllabs.append(syllab)
    syllab = ""

The place where the keep index was used can be replaced like this
from syllab = word[keep_index:]  to syllab += word[-1]

With these changes in mind the code looks now like this.

def is_vowel(self, char):
    """

    :param char: the character to check
    :return: bool depending if the char is a vowel
    """
    return char in self.vowels

def syllabicate(self, word):
    """
    :param word: The word to be syllabicated
    :return: The syllabicated list that contains syllabs
    """
    word = word.lower()
    syllabs = []
    syllab = ""
    last_was_vowel = False
    # we don't want the last character

    for pos, char in enumerate(word[:-1]):
        next_is_vowel = self.is_vowel(word[pos + 1])

        if next_is_vowel and syllab and not (last_was_vowel and self.is_vowel(char)):

            syllabs.append(syllab)
            syllab = ""

        syllab += char
        last_was_vowel = self.is_vowel(char)

    syllab += word[-1]
    syllabs.append(syllab)

    return syllabs


Square code encode and decode I am doing some python practice and wrote a square code encoder/decoder. As described in drill #4 of Programming Practice Problems, the characters of a message are written left-to-right on a square grid, and each line of the encoded message is read by going top to bottom.

I'd like some feedback on what I wrote to see if there is anything I could have done better:

import math

def encode(message):
    message = message.replace(" ", "")
    square = math.ceil(math.sqrt(len(message)))

    # Create the "grid"
    rows = []
    for row in range(square):
        pos = row * square
        code = message[pos:pos + square]
        rows.append(code)

    print("The grid:\n{}".format("\n".join(rows)))

    # Encode the message
    coded_msg = ""
    for col in range(square):
        for row in range(square):
            # The last column of the last row will throw an out of range error, so handle that
            if col < len(rows[row]):
                coded_msg += rows[row][col]
        coded_msg += " "

    return coded_msg.rstrip()

def decode(message):
    square = math.sqrt(len(message))
    col_count = math.ceil(square)
    row_count = math.ceil(len(message) / col_count)

    cols = message.split(" ")

    decoded_msg = ""
    for row in range(row_count):
        for col in cols:
            if row < len(col):
                decoded_msg += col[row]

    return decoded_msg

if __name__ == "__main__":

    print("Gimme a message:")
    in_message = input()

    coded_message = encode(in_message)
    print("--\nSquare message:\n{}".format(coded_message))
    decoded_message = decode(coded_message)
    #print("--\nDecoded message:\n{}".format(decoded_message))

    print("--\ninput: {}\noutput: {}".format(in_message, decoded_message))

Couple of pointers.

In general it's good practice to avoid putting anything other than a call to another function (such as main()) in the if __name__ == "__main__" function, just to aid in ease of unit testing.

Document strings for functions are also nice to have:

def foo():
    '''This function does foo-all, always returns None'''
    pass

A nice (ish) one liner for the decode function might be something like this:

def encode(message):
    message = message.replace(" ", "")
    length = math.ceil(math.sqrt(len(message)))
    ''.join(message[i::length] i in range(length))

Although I admit it needs a bit of refactoring as its a tad long for one line (EDIT adapted a very nice improvement from 200_success 's answer, which they explain well). It does demonstrate that you can skip building the grid though.