JSON loader with event bus, cache, and session I have an Android application which needs to retrieve JSON files from a server and then display the information in my app. To accomplish this I have the following architecture:

The main activity adds a job to the job queue requesting certain information from the server.
The job gets run, retrieving the specified information and caching it in case the same request is made again in the future.
The job on completion adds an event containing the information to the event bus.
The event bus then relays the event back to the main activity which renders the information.

While I was developing other parts of the application I simply provided the job queue, event bus and cache via constants, i.e.

public class Dependencies {
  public static final JobQueue JOB_QUEUE = new JobQueue();
  public static final EventBus EVENT_BUS = new EventBus();
  public static final Cache CACHE = new Cache();
}

I knew at some point I wanted to use dependency injection (via the Dagger 2 library) to improve the style and make my code more modular and testable. I have now begun the process of migrating my code over to this style but I am discovering some problems. Here is my new code (some parts omitted):

public class MainActivity extends Activity {

  @Inject EventBus mEventBus;
  @Inject JobQueue mJobQueue;
  @Inject Cache mCache;

  private Session mSession;

  @Override
  protected void onCreate(final Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);

    /*
    * Dagger 2 injection
    * */
    ((MyApplication) getApplication()).getDependenciesComponent().inject(this);

    final HttpUser httpUser = new HttpUser("xxxxxx@xxxxxxxx.com", "xxxxxxxx");
    startSession(httpUser);
  }

  @Override
  protected void onResume() {
    super.onResume();
    mEventBus.register(this);
  }

  @Override
  protected void onPause() {
    super.onPause();
    mEventBus.unregister(this);
  }

  @Subscribe
  public void onSessionStarted(final SessionStartedEvent sessionStartedEvent) {
    final boolean didExceptionOccur = sessionStartedEvent.didExceptionOccur();
    if(didExceptionOccur) {
      final Throwable throwable = sessionStartedEvent.getThrowable();
      handleSessionStartFailedThrowable(throwable);
    }else {
      mSession = sessionStartedEvent.get();
      loadTasks(mSession);
    }
  }

  @Subscribe
  public void onTasksLoaded(final TasksLoadedEvent tasksLoadedEvent) {
    final boolean didExceptionOccur = tasksLoadedEvent.didExceptionOccur();
    if(didExceptionOccur) {
      final Throwable throwable = tasksLoadedEvent.getThrowable();
      handleLoadTasksFailed(throwable);
    }else {
      final Task[] tasks = tasksLoadedEvent.get();
      final String taskArrayStr = Arrays.toString(tasks);
      Log.i(TAG, "tasks loaded: " + taskArrayStr);
      setTasksOnFragments(tasks);
    }
  }

  private void startSession(final HttpUser httpUser) {
    final Job startSessionJob = new StartSessionJob(this, mEventBus, httpUser);
    mJobQueue.addJobInBackground(startSessionJob);
    Log.d(TAG, "added start session job to job queue");
  }

  private void loadTasks(final Session Session) {
    final Job loadTasksJob = new LoadTasksJob(this, mEventBus, mCache, session);
    mJobQueue.addJobInBackground(loadTasksJob);
    Log.d(TAG, "added load tasks job to job queue");
  }

}

My primary focus here is on the job objects. Whereas before I would simply access the job queue, event bus, and cache from the static singletons, I now have to inject those dependencies into their constructors. As you can see for the "load tasks" job

final Job loadTasksJob = new LoadTasksJob(this, mEventBus, mCache, session);

I now have a four argument constructor, whereas before it was only two. I have read in "Clean Code" that a 4-arg constructor is an abomination and typically a sign you need to refactor your code. Is this the case here, am I doing something wrong structuring my code in this way?

The solution I developed was to inject the dependencies into the job queue instead of the jobs. This way the injection only has to happen once and can be accomplished nicely using Dagger 2 (note: because I was using the android-priority-jobqueue library I had to wrap it in my own class to allow for this functionality).

This is my Dagger module:

import android.app.Application;

import com.treemetrics.taskmanager3.util.AppCache;
import com.treemetrics.taskmanager3.work.eventbus.EventBus;
import com.treemetrics.taskmanager3.work.jobqueue.JobQueue;

import javax.inject.Singleton;

import dagger.Module;
import dagger.Provides;

@Module
public class AppModule {

  private final Application mApplication;

  public AppModule(final Application application) {
    mApplication = application;
  }

  @Provides
  @Singleton
  Application providesApplication() {
    return mApplication;
  }

  @Provides
  @Singleton
  EventBus providesEventBus() {
    return new EventBus();
  }

  @Provides
  @Singleton
  AppCache providesCache() {
    return new AppCache();
  }

  @Provides
  @Singleton
  JobQueue providesJobQueue(final Application app, final EventBus bus, final AppCache cache) {
    return new JobQueue(app, bus, cache);
  }

}

Dagger component:

import javax.inject.Singleton;

import dagger.Component;

@Singleton
@Component(modules = AppModule.class)
public interface AppComponent {

  void inject(MainActivity activity);

}

My wrapper class for the android-priority-jobqueue JobManager class which accepts the dependencies:

import android.content.Context;
import android.support.annotation.Nullable;

import com.birbit.android.jobqueue.JobManager;
import com.birbit.android.jobqueue.config.Configuration;
import com.treemetrics.taskmanager3.util.AppCache;
import com.treemetrics.taskmanager3.work.eventbus.EventBus;

public class JobQueue {

  private static final int DEF_PRIORITY = 1;

  private final Context mContext;
  private final JobManager mJobManager;
  private final EventBus mEventBus;
  private final AppCache mAppCache;

  public JobQueue(final Context context, final EventBus eventBus, final AppCache appCache) {
    mContext = context;
    final Configuration jobManagerConfig = new Configuration.Builder(context).build();
    mJobManager = new JobManager(jobManagerConfig);
    mEventBus = eventBus;
    mAppCache = appCache;
  }

  public void addJob(final Job job) {
    addJob(job, null);
  }

  public void addJob(final Job job, @Nullable final CharSequence groupId) {
    final String groupIdStr = (groupId != null) ? groupId.toString() : null;
    final BirbitJob birbitJob = getBirbitJob(job, DEF_PRIORITY, groupIdStr);
    mJobManager.addJobInBackground(birbitJob);
  }

  private BirbitJob getBirbitJob(final Job job, final int priority, @Nullable final String groupId) {
    return new BirbitJob(job, mContext, mEventBus, mAppCache, priority, groupId);
  }

}

I also have a new Job interface which my JobQueue wrapper class accepts instead of the Job class from the android-priority-jobqueue library. These jobs when submitted to my new JobQueue get translated to "birbit jobs" (the job class which the library accepts) and have the dependencies injected into them at that time. Here is my new Job interface and the BirbitJob class which is used to translate my jobs into the jobs accepted by android-priority-jobqueue.

(Notice the dependencies are provided in the onRun method.)

import android.content.Context;

import com.treemetrics.taskmanager3.util.AppCache;
import com.treemetrics.taskmanager3.work.eventbus.EventBus;

public interface Job {

  void onRun(Context context, EventBus bus, AppCache cache) throws Throwable;

  void onCancel(Throwable throwable, EventBus bus);

}

My translator job class BirbitJob:

import android.content.Context;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;

import com.birbit.android.jobqueue.Params;
import com.birbit.android.jobqueue.RetryConstraint;
import com.treemetrics.taskmanager3.util.AppCache;
import com.treemetrics.taskmanager3.work.eventbus.EventBus;

class BirbitJob extends com.birbit.android.jobqueue.Job {

  private final Job mJob;

  private final Context mContext;
  private final EventBus mEventBus;
  private final AppCache mAppCache;

  public BirbitJob(final Job job,
                   final Context context,
                   final EventBus eventBus,
                   final AppCache appCache,
                   final int priority) {
    this(job, context, eventBus, appCache, priority, null);
  }

  public BirbitJob(final Job job,
                   final Context context,
                   final EventBus eventBus,
                   final AppCache appCache,
                   final int priority,
                   final String groupId) {
    super(new Params(priority).requireNetwork().groupBy(groupId));
    mJob = job;
    mContext = context;
    mEventBus = eventBus;
    mAppCache = appCache;
  }

  @Override
  public void onAdded() {
  }

  @Override
  public void onRun() throws Throwable {
    mJob.onRun(mContext, mEventBus, mAppCache);
  }

  @Override
  protected void onCancel(final int cancelReason, @Nullable final Throwable throwable) {
    mJob.onCancel(throwable, mEventBus);
  }

  @Override
  protected RetryConstraint shouldReRunOnThrowable(
      @NonNull final Throwable throwable, final int runCount, final int maxRunCount) {
    return RetryConstraint.CANCEL;
  }

}


Dynamically changing available select options I have the following code working here on JSFIDDLE:

<!DOCTYPE html>

    <meta http-equiv="X-UA-Compatible" content="IE=edge">

<input class="radio-button-selector" type="radio" name="account" id="opt1" value="opt1">Opt1
<input class="radio-button-selector" type="radio" name="account" id="opt2" value="opt2">Opt2

     <select id="term" name="term" class="form-control">
          <option value="0">Please select</option>
          <option value="6">6 months</option>
          <option value="9">9 months</option>
          <option value="10">10 months</option>
          <option value="12">12 months</option>
          <option value="18">18 months</option>
          <option value="24">24 months</option>
          <option value="30">30 months</option>
          <option value="36">36 months</option>
          <option value="42">42 months</option>
          <option value="48">48 months</option>
    </select>

Questions:

Any way to improve the jQuery code? I saw a lot of times what surprisingly elegant things can be done in JS
Any way to improve the html code?
I only use the class "input-button-selector" for the JavaScript, is this actually needed or could I select the radio buttons in a different way?

Note that this code won't work on IE 8/9/10 (jQuery is not loading)

This is a classic example of counting on the UI to keep track of state and model.

Because you know there are only 2 options, and you know how to go from one option to another, you simply hide and add options. Nowhere in your code is explicitly declared what durations are linked to which option.

If you dont care about that, you could optimize the code doing something like this:

$(document).ready(function() {
  var monthsDifference = $("#term option[value='48'], #term option[value='42']");

    $("input.radio-button-selector").change(function(){
        if ( $(this).val() == "opt2" ) {
            monthsDifference.hide();
        }
        else if ( $(this).val() == "opt1" ) {
            monthsDifference.show();
        }
        });
    });

I would advise you to care though, and have the code know what the months are and build selectors for each option. Something like this (untested):

$(document).ready(function() {
  var optionMonths = {
      opt1: [6,9,10,12,18,24,30,36,42,48],
      opt2: [6,9,10,12,18,24,30,36]
  }
  //Cache all options
  var $options =  $("#term option");

    $("input.radio-button-selector").change(function(){
        //Hide all options first
        $options.hide();
        //Then show relevant options
        showOptionMonths( optionMonths[ $(this).val()] );
    });
});

function showOptionMonths( months ){
    months.forEach( function( month ){
       $("#term option[value='" + month + "']").show();
    });
}

Of course, I would try to find a better name than opt1 or opt2, but I dont have enough knowledge of what you are trying to achieve to suggest something more meaningful.

Finally, you could cache the selectors for each option, I leave that as an exercise for the reader.


Avoiding using strings when using Spring.Net by incorporating instantiation in class Totally new to MVP and Spring, and I have what seems to be typical MVP application that uses Spring, where the presenter is instantiated like:

IApplicationContext ctx = ContextRegistry.GetContext();
presenter = (clsPresenter)ctx.GetObject("clsPresenter");

I hate using strings to call code, really, I hate it.  So I'm thinking of putting something like

static private readonly string _clsName = typeof(ClsPresenter).Name.ToCamelCase();
public static ClsPresenter getNewPresenterFromSpring()
{
    IApplicationContext ctx = ContextRegistry.GetContext();
    return (ClsPresenter)ctx.GetObject(_clsName);
}

into all of the presenters.  So that instead of the the two lines above which use a string to find the presenter, it would be one line using the static method:

presenter = ClsPresenter.getNewPresenterFromSpring();

I know this violates the SRP, and it might look to be introducing some coupling between the Presenter classes and Spring, but it seems to me that it's not really coupled any more than it was before, it's just now type (and typo) safe, while living in a somewhat weird place.  If I stop using Spring, I simply replace this call with whatever else gets used instead.

Am I missing something that makes this a really bad idea?  Is there a better way of avoiding the usage of strings?  Should I just suck it and make sure I don't spell "clsPresenter" as "classPresenter"?

Not sure if that is just the case in your example but it's rather uncommon in C# to prefix classes with cls or class. In fact the only commonly used type identifier as part of the type name is usually the I prefix for interfaces.
Method names in C# are usually PascalCase naming convention. camelCase is used for local variables.
Not entirely sure about the whole structure of your code but to me it looks like you should be able to easily move the code into a separate helper class like this:

public static class SpringHelper
{
    public static T GetNewPresenter<T>()
    {
        IApplicationContext ctx = ContextRegistry.GetContext();
        return (T)ctx.GetObject(typeof(T).Name.ToCamelCase());
    }
}

You would call it like this then:

SpringHelper.GetNewPresenter<ClsPresenter>();

If all your presenters have a base class your could also add a where clause to restrict the types.

This would at least remove the dependency out of the presenter class.


Python text based game room to room movement I'm new to coding and working on a text based game moving from room to room. The code works in pycharm but according to an instant feedback program I entered it into, it gave some tips on it and I am not exactly sure how how to improve upon it. Here is what it gave me:

The Great Hall string is a key in main dictionary. Revise code so it is not a key in main dictionary.
It said it can not classify my code (not sure what that means) do I need a def main() command?
Consolidate multiple print commands into one function
Make condition simpler by using non-complex conditions. With simple operators that don't look for a contain within the string
5.Better practice to use while True: and the break reserved word to stop the loop when you require it to stop

data setup
rooms = {'Great Hall': {'name': 'Great Hall', 'South': 'Bedroom', 'North': 'Dungeon', 'West': 'Library', 'East': 'Kitchen'},
     'Bedroom': {'name': 'Bedroom', 'East': 'Cellar', 'North': 'Great Hall'},
     'Cellar': {'name': 'Cellar', 'West': 'Bedroom'},
     'Library': {'name': 'Library', 'East': 'Great Hall'},
     'Kitchen': {'name': 'Kitchen', 'West': 'Great Hall', 'North': 'Dining Room'},
     'Dining Room': {'name': 'Dining Room', 'South': 'Kitchen'},
     'Dungeon': {'name': 'Dungeon', 'South': 'Great Hall', 'East': 'Gallery'},
     'Gallery': {'name': 'Gallery', 'West': 'Dungeon'},
     }
directions = ['North', 'South', 'East', 'West']
current_room = rooms['Great Hall']

# game loop
while True:
   # display current location
   print()
   print('You are in the {}.'.format(current_room['name']))

  # get user input
  command = input('\nWhat direction do you want to go? ').strip()
  # movement
  if command in directions:
     if command in current_room:
         current_room = rooms[current_room[command]]
     else:
        # bad movement
        print("You can't go that way.")
  # Exit game
  elif command.lower() in ('q', 'quit'):
      break
  # bad command
  else:
     print("I don't understand that command.")

Overall I don't think the code is that bad, considering what is the purpose; but if you want to expand your game I think the greatest issue is the data structure.
It'd be very easy to misplace some room or call it in a different way if you have to repeat yourself every time, so I suggest you to use a class to represent a Room:
class Room:
    name: str
    north: 'Room'
    east: 'Room'
    south: 'Room'
    west: 'Room'

    def __init__(self, name, north=None, east=None, south=None, west=None):
        self.name = name
        self.north = north
        self.east = east
        self.west = west
        self.south = south

        if north:
            north.south = self
        if east:
            east.west = self
        if south:
            south.north = self
        if west:
            west.east = self

    def go_to(self, direction):
        if direction in ['north','east','south','west']:
            return getattr(self, direction)
        else:
            return None

    def __str__(self):
        return self.name

I'm using types because is very helpful to find out problems before it's too late, in Python if you have a recursive type (like Room) you need to use the quotes, but it just a syntactic notation.
By default a room does not have any north,east,south,west Room (hence the =None) but the important thing that happens in the __init__ is that when you add a room it automatically set the opposite direction. So if a room goes to another by east, the other room does the opposite by west. Thus we are reducing errors in directions. If this will not be the case for some room, you will be able to override that using another class (SelfLockingRoom that closes all the other directions when you enter, for example).
The __str__ method is just to handle the display of the room to the room itself; at one point you could have more than just the name to display.
I'm adding also a go_to method, it is the room responsibility to decide where to go given the direction; in the future you could have a TrapRoom that extends Room and in case of direction "East" will do nasty things, for example. The getattr is just to avoid having to write if direction=='north': return self.north but it would be the same and maybe even clearer.
Actually, if the game develops further, you may need more complex rules to decide what do to given a direction so probably you will need a House class:
class House:
    current_room: Room
    rooms: list[Room]  # at the moment is not used but could be in the future?

    def __init__(self, current_room: Room, rooms: list[Room]):
        self.current_room = current_room
        self.rooms = rooms

    def go_to(self, direction):
        if next_room := self.current_room.go_to(direction):
            self.current_room = next_room
        return next_room

At the moment is not very useful, but I think it's going to be.
To initialize the house you just create the rooms:
house_rooms = [cellar := Room('Cellar'), library := Room('Library'), dining_room := Room('Dining Room'),
               gallery := Room('Gallery'), bedroom := Room('Bedroom', east=cellar),
               dungeon := Room('Dungeon', east=gallery),
               kitchen := Room('Kitchen', north=dining_room),
               great_hall := Room('Great Hall', south=bedroom, north=dungeon, west=library, east=kitchen)]

house = House(current_room=great_hall, rooms=house_rooms)

(As you can see I'm not repeating names or directions)
The game loop is OK, we can collect some helpful methods, just in case you want to expand them in the future:
def prompt(text: str):
    print(text)

def ask(text: str) -> str:
    return input(text).strip()

and this would be your game loop (I'm always lowering the direction so you can write East east or EAST).
commands = {
    "directions": ['north', 'south', 'east', 'west'],
    "quit": ['q', 'quit']
}

def game_loop():
    prompt(f"\nYou are in the {house.current_room}")
    command = ask('\nWhat direction do you want to go? ').lower()
    if command in commands["directions"]:
        if not house.go_to(command):
            prompt("You can't go that way.")
    elif command in commands["quit"]:
        return False
    else:
        prompt("I don't understand that command.")
    return True

if __name__ == '__main__':
    while game_loop():
        pass


Binary Search in Ruby I want to write a search that raises an error if the list supplied is not sorted.

class BinarySearch

  attr_accessor :list

  def initialize(orig)
    raise ArgumentError unless sorted? orig
    self.list = orig
  end

  def search_for(n)
    s_list = self.list
    offset = 0
    loop do
      mid = s_list.length/2
      left = s_list[0..mid-1]
      right = s_list[mid+1..s_list.length-1]
      mid_val = s_list[mid]
      if n == mid_val
        return offset+mid
      elsif n > mid_val
        offset += mid+1
        s_list = right
      else
        s_list = left
      end
      raise RuntimeError if s_list.empty?
    end
  end

  def middle
   self.list.length/2
  end

  private

  def sorted?(orig)
    for i in 1..orig.length-1
        return false if orig[i] < orig[i-1]
    end
    return true
  end

end

Same concept as yours, but slimmed down a bit, and avoiding an explicit loop.

You might also consider making this a utility function, or a mixin with which you can dynamically extend Enumerable objects.  Making it a class which creates a special object wrapper over a list, for the purpose of adding a single method, has a clunky java-esque feel.

Also, keep in the mind the below is optimized for readability and brevity, not for speed.  In particular, left_of and right_of are making unnecessary copies of the array halves.

class BinarySearch

  attr_accessor :list

  def initialize(orig)
    raise ArgumentError unless sorted? orig
    @list = orig
  end

  def search_for(n, s_list = list, offset = 0)

    raise RuntimeError if s_list.empty?

    mid = s_list.length / 2
    mid_val = s_list[mid]

    return mid + offset if mid_val == n
    return search_for(n, left_of(s_list, mid), offset) if mid_val > n
    search_for(n, right_of(s_list, mid), offset+mid+1)

  end

  private

  def left_of(s_list, i)
    s_list[0..i-1]
  end

  def right_of(s_list, i)
    s_list[i+1..-1]
  end

  def sorted?(orig)
    orig.each_cons(2).all? { |a,b| (a <=> b) <= 0 }
  end

end


Uploading multiple images I have this code which works for uploading multiple files (images). However, it seems inefficient due to having to a function for every single file upload (for 2 uploads, but there will be about 12 in total).

function readURL1(input) {
    if (input.files && input.files[0]) {
        var reader = new FileReader();

        reader.onload = function (e) {
            jQuery('#div1').attr('src', e.target.result);
        }

        reader.readAsDataURL(input.files[0]);
    }
}

function readURL2(input) {
    if (input.files && input.files[0]) {
        var reader = new FileReader();

        reader.onload = function (e) {
            jQuery('#div2').attr('src', e.target.result);
        }

        reader.readAsDataURL(input.files[0]);
    }
}

Then...

jQuery("#img1").change(function(){
    readURL1(this);
});

jQuery("#img2").change(function(){
    readURL2(this);
});

No need for a loop. The only difference between the functions is the target image (I assume it's an image, since DIVs don't have a srcattribute), so change the function to get the target from an data attribute of the input field:

function readURL(input) {
    if (input.files && input.files[0]) {
        var image = jQuery(input).data('image-target');

        var reader = new FileReader();

        reader.onload = function (e) {
            jQuery(image).attr('src', e.target.result);
        }

        reader.readAsDataURL(input.files[0]);
    }
}

Add those data attributes to the HTML:

Image 1: <input type="file" data-image-target="#div1">
Image 2: <input type="file" data-image-target="#div2">

As you may notice the id of the input fields have gone. Instead of assigning them each their own separate event handler, assign one handler for all (or use a class):

jQuery("input[type=file]").change(function() {
   readURL(this);
});


Create New C++ Class I'm currently developing a new version of ExpressGenGen Using TDD. While I'm rewriting the new version of my Code Generator Generator.
I'm using its old version to generate code to be used by the new version.
This is how I'm creating new classes: when a new class is created,
it will generate a .cpp and .h file and then add them to CMakeLists.txt,
and also TestHelper.h is updated with new headers.

What I want reviewed are the bash scripts. (You are also welcome to review the ExpressGenGen templates)

Bash Scripts

gen

#!/bin/bash

cd ..
cat Samples/template_bash_CMakeLists.txt | xgengen bash | bash - > CMakeLists.txt
cat Samples/template_bash_TestHelper.txt | xgengen bash | bash - > Tests/TestHelper.h

new_class

#!/bin/bash

echo 'Create New Class'
echo '--------------------------'
echo 'Type Class Name : '
read _class
# convert Upper Camel case to header guard, ex: NewClass -> _NEW_CLASS_H_
_guard=`echo $_class | sed -e 's/\([A-Z]\)/_\1/g' | awk '{printf toupper($0)} END{printf "_H_";}'`
# export the variables so they are visible to the child shell
export guard=$_guard
export class=$_class
cat template_bash_CPP.txt | xgengen bash | bash - > "../Src/$class.cpp"
cat template_bash_H.txt | xgengen bash | bash - > "../Src/$class.h"
./gen

Templates

template_bash_CMakeLists.txt

project(ExpressGenGen)

cmake_minimum_required(VERSION 2.6)
include_directories($ENV{BOOST_ROOT} $ENV{LUA_ROOT} $ENV{GMOCK_HOME}/include $ENV{GMOCK_HOME}/gtest/include .)
link_directories($ENV{LUA_ROOT} $ENV{BOOST_ROOT}/stage/lib $ENV{GMOCK_HOME}/build $ENV{GMOCK_HOME}/build/gtest)
set(CMAKE_CXX_FLAGS "${CMAXE_CXX_FLAGS} -Wall -Wpedantic -std=gnu++11")

set(sources
$$$find . -name '*.cpp' | sed 's/\.\//  /'
    $$$find . -name '*.h' | sed 's/\.\//  /'
  WindowsRsrc/WindowsRes.rc)
add_executable(xgengen ${sources})
target_link_libraries(xgengen lua)
target_link_libraries(xgengen boost_program_options)
target_link_libraries(xgengen pthread)
target_link_libraries(xgengen gmock)
target_link_libraries(xgengen gtest)

template_bash_CPP.txt

//--------------------------------------------
// Definition
#include "{$$$class$$}.h"
//--------------------------------------------
// Uses
//
//--------------------------------------------

{$$$class$$}::{$$$class$$}()
{

}

//--------------------------------------------

template_bash_H.txt

#ifndef {$$$guard$$}
#define {$$$guard$$}

//--------------------------------------------
// Has
//
//--------------------------------------------
// Inherits
//
//--------------------------------------------
// Uses
//
//--------------------------------------------

class {$$$class$$} {
public:
    {$$$class$$}();
    virtual ~{$$$class$$}() {}

private:
};

//-------------------------------------------

#endif // {$$$guard$$}

template_bash_TestHelper.txt

//--------------------------------------------
// TestHelper
//
// This header is basically an include all type helper for Test*.cpp files
// therefore greatly reducing duplication
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wall"
#pragma GCC diagnostic ignored "-Wpedantic"
#pragma GCC diagnostic ignored "-Wunused-local-typedefs"
#include <gmock/gmock.h>
#pragma GCC diagnostic pop

#include <iostream>
#include <istream>
#include <sstream>
#include <memory>
// AutoGenerated
$$$find Src -name "*.h" | sed -e 's/\(.*\)/#include \"\.\.\/\1\"/g'
// End AutoGenerated
//--------------------------------------------

Use input redirection instead of cat | ...

Instead of cat somefile | somecommand, you should use input redirection, for example:

xgengen bash < Samples/template_bash_CMakeLists.txt | bash - > CMakeLists.txt

The advantage of this is that you're running one fewer process (no more cat).

Use modern style command substitution $(...)

Don't use old style `...` command substitution.
Use the modern and better $(...) style.

Simplify sed + awk pipelines when possible

This looks very complicated, and it's suspicious to have both sed and awk in the same pipeline:

echo $_class | sed -e 's/\([A-Z]\)/_\1/g' | awk '{printf toupper($0)} END{printf "_H_";}'

You can simplify using a single awk command:

echo $_class | awk '{gsub("[A-Z]", "_\&"); printf toupper($0) "_H_"}'

Use here-strings instead of echo | ...

Similar to using input redirection instead of cat | ...,
in Bash it's better to replace echo | ... with here-strings using <<<,
rewriting the above example as:

awk '{gsub("[A-Z]", "_\&"); printf toupper($0) "_H_"}' <<< $_class

Use alternative regex pattern separator to improve readability

When a regular expression pattern contains /,
using such patterns with the s/// operator becomes tedious and hard to read,
because of the escapes.
To improve readability, it's often good to use an alternative pattern separator.
So for example instead of this:

sed 's/\.\//  /'

You can do this instead to avoid escaping /:

sed 's|\./|  |'

The benefit is more obvious when you have multiple / in the pattern.
In this example the difference might be negligible.

Simplify regex

The s/// here can be simplified:

sed -e 's/\(.*\)/#include \"\.\.\/\1\"/g'

To this:

sed -e 's/.*/#include "..\/&"/'

That is:

No need to capture .* in \(...\), you can access the matched string directly with &
No need to escape . in replacement, as it has no special meaning there
No need to escape " within single-quoted strings in Bash
Since the pattern matches the entire line, no need for the g flag


Dockerfile for C project This is a Dockerfile which aims to create a complete environment for the bismon project. I have written it as an exercise and to make it easy for other users to begin with bismon.

FROM ubuntu:latest
MAINTAINER Niklas Rosencrantz (niklasrosencrantz@xyz.com)
ENV DEBIAN_FRONTEND noninteractive
RUN apt-get update
RUN apt-get install --yes software-properties-common
RUN apt-add-repository --yes --update ppa:ubuntu-toolchain-r/test
RUN apt-get update
RUN apt-get install --yes git
RUN apt-get install --yes gcc-snapshot
RUN apt-get install --yes build-essential make gcc-8 cmake
RUN apt-get install --yes ninja-build g++-8 gcc-8-plugin-dev libgccjit-8-dev libgtk-3-dev
RUN apt-get install --yes markdown indent astyle tardy texlive texlive-full hevea
RUN adduser --disabled-password --gecos '' newuser
USER newuser
WORKDIR /home/newuser
RUN git clone https://github.com/ianlancetaylor/libbacktrace.git
RUN cd libbacktrace; ./configure ; make
USER root
WORKDIR /home/newuser/libbacktrace
RUN make install
USER newuser
RUN cd; git clone https://github.com/davidmoreno/onion.git; cd onion; mkdir build; cd build; cmake ..; make
USER root
WORKDIR /home/newuser/onion/build
RUN make install; cp /usr/local/lib/libonion* /usr/lib; cp /usr/local/lib/libbacktrace* /usr/lib
USER newuser
RUN cd; git clone https://github.com/bstarynk/bismon.git; cd bismon; make; touch ~/passwords_BM; chmod u+rw,go-rwx ~/passwords_BM
WORKDIR /home/newuser/bismon

Combine as many RUN statements as possible to reduce the number of layers (and size of the image). Each RUN command makes a new layer.

For example,

RUN apt-get update && apt-get install --yes software-properties-common git gcc-snapshot

After these, you should clean the cache with apt-get autoremove && apt-get autoclean.

For example,

RUN apt-get update && apt-get install --yes software-properties-common git gcc-snapshot && apt-get autoremove && apt-get autoclean

This saves space on the image.

FROM ubuntu:latest is not best practice. Sometimes something works with FROM ubuntu:16.04, but not FROM ubuntu:18.04 (for example, software compatibility).


Authenticate users based on both user role, and requested operation I have a function that checks if a user can perform an operation based on access to a specific URL.

The function parameters are the requested URL: url, and user level: lv. The conf object has stored access levels for comparison with the users access level.

function isAuthenticated(url, lv){
    let conf = config.getConfig();

    // Role Editor
    if((url === "/RoleEditor" || url === "/SaveRoles") && lv > conf.roleEditor)
        return true;
    // Queue
    if(url === "/WriteQueueData" && lv > conf.queuedata)
        return true;
    // Listener
    if((url === "/startListener" || url === "/stopListener" || url === "restartListener" || url === "/ListenerConfig" || url === "/SaveListenerConfig") && lv > conf.listener)
        return true;
    // Client details
    if(url === "/ClientInfo" && lv > conf.clientDetails)
        return true;
    // User information
    if(url === "/UserManagement" && lv > conf.userOverview)
        return true;
    // User Edit
    if((url === "/EditUser" || url === "/EditUserToDb") && lv > conf.userEdit)
        return true;

    //
    // Another 6 of this comparisons
    //
    return false;
}

This seems inefficient. For example, if the url is "/RoleEditor" but the power level is too low, all other comparisons are still being made. This could be fixed by changing to this:

if((url === "/RoleEditor" || url === "/SaveRoles"))
    if(lv > conf.roleEditor)
        return true;
    else
        return false;

But that feels even worse (more redundant).

Is there a better way to do this?

If return true, else return false

if(lv > conf.roleEditor)
    return true;
else
    return false;

Before we delve into the contextual considerations, an if that returns true or false is always a redundant if. This can at all times be refactored to:

return lv > conf.roleEditor;

Performance

  I see that, if the url is "/RoleEditor" but the power level is too low, all other comparisons will be made. A fix for this would be changing all to this:

The first question is always "does this performance need to be optimized?" As these are simple value comparisons, the performance cost is likely negligible. Assuming this method isn't being called many thousands of times in a given page load.

When performance cost is negligible, favor readability over (irrelevant) optimizations. In that regard, you're free to not avoid the additional checks if it makes the code more readable and doesn't meaningfully impact performance.

That being said, the code can be improved here, which I will get to. The main takeaway here is that you are not forced to optimize it the way you currently wanted to if there is no proven performance issue.

Authentication vs authorization

As others have already remarked, you're dealing with authorization here, not authentication.

Authentication = Who is this? (it's Bob)
Authorization = Can Bob delete users? (yes)

Secure authorization

You are showing a javascript method. Presumably, this runs in the client's browser. By putting the logic in the browser, your users are able to find it and alter it if they so choose. This is a bad idea for security concerns.

It's not necessarily wrong to perform these checks client-side (e.g. for a fast response and thus good user experience) if they are also backed by server-side security checks (to ensure that users fiddling with the authorization logic client-side are unable to perform actions server-side).

Optimizing the algorithm

With all these sidenotes out of the way, we move on to the actual issue you were asking about, optimizing the ifs. Reading the code, there is a general pattern to be spotted:

if(url == "MyString" && lv > conf.MyStringMinimumLevel)

Sometimes there is more than one allowed string, so we'll account for that, but the repeating pattern is otherwise always the same. I'm going to refer to these two values as "names" and "levels" in the rest of the answer for brevity's sake.

This immediately opens the door to abstraction by mapping these names and levels. In fact, you've already sort of done this with all your conf.roleEditor, conf.queuedata, ... values. You just haven't done it reusably, which is forcing you to manually write all your if checks. If you write it reusable, you can reduce this to a very simple process:

Fetch the level based on the url name
Check if the given user level is greater than the fetched level

var mappings =
{
    {
        "level" : 1
        "names" : [ "RoleEditor", "SaveRoles" ]
    },
    {
        "level" : 2
        "names" : [ "WriteQueueData" ]
    }
};

function isAuthorized(url, lv) {

    var mapping = mappings.find(m => m.names.includes(url.trim('/'));

    return mapping !== undefined
           && lv > mapping.level;
}

To explain:

.find() gives us the first item which conforms to the selection logic (or undefined if no such item exists).
The selection logic (.includes()) checks if a mapping contains a name that matches the url parameter.
By using url.trim('/'), you omit the first and last character (the /) from the URL when it is a / (but will leave any other characters). This just makes it easier for use to not have to constantly put the / in the mapping names.
The !== undefined check ensures that we return false if no mapping exists for the current URL. Your intended behavior may be different here - adjust it as you see fit.

You'll probably want to store this mapping in your conf object, but I'll leave the finer points up to you. This is just a basic example of how you can reduce the method.

Edit: Sidenote

If the conf object should not be altered (for whatever reason), you can work around this by indirectly using the values in the mappings:

var mappings =
{
    {
        "level" : conf.roleEditor,
        "names" : [ "RoleEditor", "SaveRoles" ]
    },
    {
        "level" : conf.queuedata,
        "names" : [ "WriteQueueData" ]
    }
};

// The function is unchanged

This is less ideal (it's better to just change the conf object) but if you can't change it, you can at least still work around it.


Java console version Conway's Game of Life I did a super simple version of Conway's Game of Life and would like a review.

public class Conway {
        String [][] grid;
        int [][] neighbors;
        int [][] directions = {{0,1},{1,1},{1,0},{1,-1},{0,-1},{-1,-1},{-1,0},{-1,1}};
        Conway (int gridSize) {
                this.neighbors = new int [gridSize][gridSize];
                this.grid = new String[gridSize][gridSize];
                for (int row = 0; row < this.grid.length; row++) {
                        for (int cell = 0; cell < this.grid[row].length; cell++) {
                                this.grid[row][cell] = (Math.random() > .9) ? "x|" : " |";
                                System.out.print(this.grid[row][cell]);
                        }
                        System.out.println();
                }
                this.getNeihbors();
        }
        void getNeihbors() {
                int i = 0;
                for (int row = 0; row < this.grid.length; row++ ) {
                        for (int col = 0; col < this.grid[row].length; col++) {
                                for (int drow = 0; drow < this.directions.length; drow++) {
                                        int x = (this.directions[drow][this.directions[drow].length-this.directions[drow].length]);
                                        int y = (this.directions[drow][this.directions[drow].length - 1]);
                                        x = row + x;
                                        y = col + y;
                                        if (x < 0) {
                                                x += this.grid.length;
                                        } else if (x + 1 > this.grid.length) {
                                                 x -= x;
                                        }
                                        if (y < 0) {
                                                y += this.grid[row].length;
                                        } else if (y + 1 > this.grid[row].length) {
                                                 y -= y;
                                        }
                                        if (this.grid[x][y] == "x|") i++;
                                }
                                this.neighbors[row][col] = i;
                                i=0;
                        }
                }
                this.showGrid();
        }
        void showGrid() {
                for (int row = 0; row < this.grid.length; row++) {
                        for (int cell = 0; cell < this.grid[row].length; cell++) {
                                if (this.grid[row][cell] == " |" && this.neighbors[row][cell] == 3) {
                                        this.grid[row][cell] = "x|";
                                } else if (this.grid[row][cell] == "x|" && this.neighbors[row][cell] > 3) {
                                        this.grid[row][cell] = " |";
                                } else if (this.grid[row][cell] == "x|" && this.neighbors[row][cell] < 2) {
                                        this.grid[row][cell] = " |";
                                }
                                System.out.print(this.grid[row][cell]);
                        }
                        System.out.println();
                }
                this.getNeihbors();
        }
        public static void main(String[] args) {
                Conway c = new Conway(80);
        }
}

Object fields

    String [][] grid;
    int [][] neighbors;

I would make both of these private rather than the default package-protected visibility.  Unless of course you are actually planning to access these from another class in the package.

    private String [][] grid;
    private int [][] neighborCounts;

I also changed the name of neighbors, as I found the original inaccurate.  I would expect a variable called neighbors to hold some kind of reference to the neighbors.  Instead, this counts the neighbors, which is necessary for Conway's Game of Life.  So let's call it what it is.

I'd also consider changing the grid to be some other kind of object rather than String.  But that's something of a judgment call.  One advantage of doing that would be that you could get rid of neighbors altogether.  Instead, each cell would track its own neighbors.

Class constants

        int [][] directions = {{0,1},{1,1},{1,0},{1,-1},{0,-1},{-1,-1},{-1,0},{-1,1}};

You make this a regular object field, but it never changes and is the same for all instances of the class.

        private static final int [][] directions
                = {{0,1}, {1,1}, {1,0}, {1,-1}, {0,-1}, {-1,-1}, {-1,0}, {-1,1}};

Now only one copy will be initialized for the application.

Constructor initialization

                this.neighbors = new int [gridSize][gridSize];
                this.grid = new String[gridSize][gridSize];

Personally, I think that eight-column indent is a bit much.

            grid = new String[gridSize][gridSize];
            neighborCounts = new int[grid.length][grid[0].length];

Four-column indent is more common in Java.

If you do use an eight-column indent, consider adopting the same rule as the Linux source:  only two levels of indent inside a function.  That will help keep function contents from shooting off the edge.  It also helps keep functions simple, without too much nested logic.

I prefer not to use the this. with object fields.  Java doesn't require it, so I only use it when necessary to disambiguate from parameters with the same name.

You want neighborCounts to be the same size as grid.  So make sure of that by setting it to the same dimensions dependently rather than in parallel.  Parallel logic is fragile.

Don't run inside the constructor

At the end of the constructor, you

                this.getNeihbors();

First, that's misspelled (missing a 'g').  It should be getNeighbors.

Second, it's not idiomatic.  I would expect a method called getNeighbors to return the contents of an object field neighbors which holds the neighbors of something.  This fills out the neighbors multidimensional array.  I would probably call it countNeighbors.

Third, if you start the simulation from the constructor, then you have no way of constructing an object without starting the simulation.

Consider removing all current calls to getNeighbors and showGrid and instead adding to main something like

        while (true) {
            c.countNeighbors();
            c.showGrid();
        }

That also has the advantage of not filling up the stack with recursive calls.

Simplifying

                                for (int drow = 0; drow < this.directions.length; drow++) {
                                        int x = (this.directions[drow][this.directions[drow].length-this.directions[drow].length]);
                                        int y = (this.directions[drow][this.directions[drow].length - 1]);
                                        x = row + x;
                                        y = col + y;

This could be as simple as

                for (int[] direction : directions) {
                    int y = row + direction[0];
                    int x = col + direction[1];

And

                                        } else if (x + 1 > this.grid.length) {
                                                 x -= x;

could be

                    } else if (x >= grid.length) {
                        x -= grid.length;

Now if you change the rules to allow neighbors that aren't adjacent, this will still work.

And we don't have to do an extra, unnecessary math operation.

Descriptive names

                int i = 0;

I would find this easier to follow as

        int count = 0;

When I see a variable named i, I expect it to be a loop iteration variable.  This isn't.  We're counting the neighbors.  Why not name it based on that?


Integer input validation I'm fairly new to Java and I'd like to know if there's any way to improve or refactor the prompt on validating integer values for best practices.
import java.util.Scanner;

public class Triangle {
 public static void main(String[] args) {
  int a, b, c;
  Scanner in = new Scanner(System.in);

  // Prompt for a
  System.out.print("a: ");
  if (!in.hasNextInt() || (a = in.nextInt()) <= 0) {
   System.out.println("Input must be a positive integer!");
   return;
  }

  // Prompt for b
  System.out.print("b: ");
  if (!in.hasNextInt() || (b = in.nextInt()) <= 0) {
   System.out.println("Input must be a positive integer!");
   return;
  }

  // Prompt for c
  System.out.print("c: ");
  if (!in.hasNextInt() || (c = in.nextInt()) <= 0) {
   System.out.println("Input must be a positive integer!");
   return;
  }

  if (a + b > c && a + c > b && b + c > a) {
   System.out.println("It's a traingle.");
  } else {
   System.out.println("Invalid lengths for a traingle.");
  }
 }
}

Code Repetition
In your program, you are using the following code three times:
System.out.print("a: ");
if (!in.hasNextInt() || (a = in.nextInt()) <= 0) {
 System.out.println("Input must be a positive integer!");
 return;
}

You should use a method for this:
public static int getUserInput() {
 Scanner in = new Scanner(System.in);
 int number;
 if (!in.hasNextInt() || (number = in.nextInt()) <= 0) {
  System.out.println("Input must be a positive integer!");
  return -1; //Returns -1 for illegal input
 }
 return number;
}

Input validation
It's good that you control, whether the user made a valid input, but this point can be improved further: Your program stops, when the user makes an invalid input. That's not what an user expects:
public static int getUserInput(String var) {
 Scanner sc = new Scanner(System.in);
 int number;
 while(true) {
  try {
   System.out.print(var + ": ");
   number = sc.nextInt();
   if(number <= 0) {
    throw new InputMismatchException();
   }
   break;
  }
  catch(InputMismatchException e) {
   System.out.println("Enter a number > 0.");
   sc.nextLine();
  }
 }
 return number;
}

By using this code, the user will be able to make inputs until making a valid input. If you don't know about try-catch yet, i suggest reading this explanation.

Logic
I would create a seperate method for the "triangle-validation":
public static boolean isTriangle(int a, int b, int c) {
 if (a + b > c && a + c > b && b + c > a) {
  return true;
 } else {
  return false;
 }
}

Style
Please consider using more than one space for indentation. My suggestion is two or even four spaces. This improves the legibility.

Final code
import java.util.Scanner;
import java.util.InputMismatchException;

public class Triangle {
    public static void main(String[] args) {
        int a, b, c;
        Scanner in = new Scanner(System.in);

        // Prompt for a
        a = getUserInput("a");

        // Prompt for b
        b = getUserInput("b");

        // Prompt for c
        c = getUserInput("c");

        if (isTriangle(a, b, c)) {
            System.out.println("It's a triangle.");
        } else {
            System.out.println("Invalid lengths for a triangle.");
        }
    }

    public static boolean isTriangle(int a, int b, int c) {
        if (a + b > c && a + c > b && b + c > a) {
            return true;
        } else {
            return false;
        }
    }

    public static int getUserInput(String var) {
        Scanner sc = new Scanner(System.in);
        int number;
        while(true) {
            try {
                System.out.print(var + ": ");
                number = sc.nextInt();
                if(number <= 0) {
                    throw new InputMismatchException();
                }
                break;
            }
            catch(InputMismatchException e) {
                System.out.println("Enter a number > 0.");
                sc.nextLine();
            }
        }
        return number;
    }
}


Displaying data from from many-to-many table The goal is to display all posts with their categories underneath them.  The code below accomplishes this, however I'm thinking it's bad and was hoping someone could review and offer some feedback.

Because each post can have many categories and because each category can belong to many posts, I used a many-to-many table.

I have three tables:

table 1: Posts
columns: | id | post_name | post_text |
table 2: categories
columns: | id | category_name |
table 3: cats_to_posts
columns: | cats_id | post_id |

index.php:

$posts = Posts::read_all();

foreach ( $posts as $post )
{
    echo $post->post_name ."
";
    echo $post->post_text ."
";

    $categories_array = Posts::get_cats_by_post_id($post->id);

    foreach ($categories_array as $category)
    {
        echo "&bull;". $category->category_name ."
";
    }
}

Posts class:

public static function get_cats_by_post_id($id)
{
    $db = new Database();

    $sql = "SELECT  cats_to_posts.cats_id
            FROM    cats_to_posts
            WHERE   cats_to_posts.post_id = {$id}";

    $result = $db->exe_query($sql);

    $cat_id_array = array(); // stores array of category ids that match post ids

    while ( $record = $result->fetch_object() )
    {
        $cat_id_array[] = $record;
    }

    $cat_name_array = array(); // stores array of category names that match category ids
    foreach($cat_id_array as $cat_id)
    {
        $new_sql = "SELECT  category_name
                    FROM    categories
                    WHERE   id = ". $cat_id->cats_id;

        $new_result = $db->exe_query($new_sql);

        while ( $new_record = $new_result->fetch_object() )
        {
            $cat_name_array[] = $new_record;
        }

    }

    return $cat_name_array;
}

My main curiosity is on the get_cats_by_post_id() method inside the Posts class. Is there is a better way to get all category names based on the post_id than using two SQL commands like I am doing?

By the looks of it, you

get the category IDs using the post ID
get the categories according to the category IDs you just had

Well, you can use JOINS to bridge the tables:

SELECT c.category_name       //select category name
FROM cats_to_posts cp        //from cats_to_posts
    INNER JOIN categories c  //joined with categories
        ON cp.cats_id=c.id   //where the cats_to_posts id  = category id
WHERE cp.post_id = {$id}     //get for post with ID

This single query should return an array of all category names for that certain post ID. The INNER JOIN joins the cats_to_posts table with categories table and returns only those categories that have an equivalent ID in cats_to_posts


A weird looking class crawler I have written a crawler in python with the combination of class and function. Few days back I saw a scraper in a tutorial more or less similar to what I did here. I found it hard the necessity of using class here. However, I decided to create one. My scraper is able to traverse all the next pages and print the collected results errorlesly. If there is any suggestion or input to give this scraper a better look, I'm ready to comply with that. Thanks in advance.

Here is what I've written:

import requests
from urllib.request import urljoin
from lxml.html import fromstring

main_link = "https://www.yify-torrent.org/search/western/"

class movie_doc(object):
    def __init__(self,name,genre,quality,size,rating):
        self.name = name
        self.genre = genre
        self.quality = quality
        self.size = size
        self.rating = rating

film_storage = [] # global variable

def get_links(link):
    root = fromstring(requests.get(link).text)
    for item in root.cssselect(".mv"):
        name = item.cssselect("h3 a")[0].text
        genre = item.cssselect("li b:contains('Genre:')")[0].tail
        quality = item.cssselect("li b:contains('Quality:')")[0].tail
        size = item.cssselect("li b:contains('Size:')")[0].tail
        rating = item.cssselect("li b:contains('Rating:')")[0].tail
        film_details = movie_doc(name,genre,quality,size,rating)
        film_storage.append(film_details)

    next_page = root.cssselect(".pager a:contains('Next')")[0].attrib['href'] if root.cssselect(".pager a:contains('Next')") else ""
    if next_page:
        full_link = urljoin(link,next_page)
        get_links(full_link)

if __name__ == '__main__':
    get_links(main_link)
    for item in film_storage: # as film_storage variable is declared global I called here directly
        print(item.name,item.genre,item.quality,item.size,item.rating)

For this kind of a simple "storage" class, it might be more appropriate to use either a simple dictionary or a collections.namedtuple():

from collections import namedtuple

MovieDocument = namedtuple('MovieDocument', ['name', 'genre', 'quality', 'size', 'rating'])

Note, style-wise, the class name should be defined in PascalCase starting with a capital letter.

Also, consider making get_links() a generator and renaming it to, say, crawl() as it is not actually getting the links but crawling and returning movie objects.

I would also use an EAFP approach for the next page link retrieval.

The code with all the changes applied:

from collections import namedtuple
from urllib.request import urljoin

import requests
from lxml.html import fromstring

Movie = namedtuple('MovieDocument', ['name', 'genre', 'quality', 'size', 'rating'])

def crawl(link):
    root = fromstring(requests.get(link).text)
    for item in root.cssselect(".mv"):
        name = item.cssselect("h3 a")[0].text
        genre = item.cssselect("li b:contains('Genre:')")[0].tail
        quality = item.cssselect("li b:contains('Quality:')")[0].tail
        size = item.cssselect("li b:contains('Size:')")[0].tail
        rating = item.cssselect("li b:contains('Rating:')")[0].tail

        yield Movie(name, genre, quality, size, rating)

    try:
        next_page = root.cssselect(".pager a:contains('Next')")[0].attrib['href']
        full_link = urljoin(link,next_page)
        yield from crawl(full_link)
    except IndexError:
        pass

if __name__ == '__main__':
    for movie in crawl("https://www.yify-torrent.org/search/western/"):
        print(movie.name, movie.genre, movie.quality, movie.size, movie.rating)

There is, though, still this repetition in the CSS selectors - consider extracting it into a reusable function parameterized with a text you then use in contains().


Add to a variable multiple in cells variables I have code that is used to add all cells of a tab to a var:

#include <stdio.h>
#include <stdlib.h>
#include <assert.h>
#include <malloc.h>

#define NB 10000

// Instruction RDTSC du Pentium
static __inline__ unsigned long long RDTSC(void) {
    unsigned hi, lo;
    __asm__ __volatile__ ("rdtsc" : "=a"(lo), "=d"(hi));
    return ((unsigned long long) lo) | (((unsigned long long) hi) << 32);
}

// IS HERE TO CONTAIN THE TAB, IT WILL CONTAIN ALL THE NUMBERS I WILL HAVE TO ADD TO MY FINAL INT NAMES "somme" IN MY MAIN.
// Initialisation de la matrice d'entiers
void InitTab(int **tab) {
    int i, j;

    for (i = 0; i < NB; i++) {
        tab[i] = (int *) malloc(NB * sizeof(int));
        assert (tab[i] != NULL);
    }

    for (i = 0; i < NB; i++)
        for (j = 0; j < NB; j++)
            tab[i][j] = rand()%100000;
}

// IT WILL RETURN ME THE MINIMUM OF A COLUMN OF MY TABLE BECAUSE WHE WANT TO ADD THE MINIMUM OF ALL COLUMNS OF THE TABLE TO THE FINAL INT NAMES "somme" IN MY MAIN.
// Calcul du minimum d'une colonne
int MinColonne(int **tab, int colonne) {
    int i, min;

    min = tab[0][colonne];
    for (i = 1; i < NB; i++)
        if (min > tab[i][colonne]) min = tab[i][colonne];

    return (min);
}

// MAIN
int main(void) {
    int somme = 0;
    int i,z;
    int **tab;

    unsigned long long t1, t2; // chronometrage
    srand(1);

    tab = (int **) malloc(NB * sizeof(int *));
    assert(tab != NULL); // exit si erreur d'allocation

    InitTab(tab);// permet aussi de mettre le processeur en pleine puissance

    t1 = RDTSC();
    for (z=0; z<10; z++) // pour avoir une moyenne du temps d'execution
    for (i = 0; i < NB; i++)
         somme += MinColonne(tab, i);
t2 = RDTSC();

    printf("Somme des minima de chaque colonne : %d \n", somme);

    printf("Temps en cycles d'horloge : %llu \n", (t2 - t1));

    return EXIT_SUCCESS;
}

I have to optimize this code but I don't find any for to break, while to optimize and no multiple if ... (These are the only things I know how to optimize)

Do you know how to?

I can't touch at InitTab(tab); line and the double for.

Utilizing the cache
The reason your function is so slow is because you are traversing your matrix by rows instead of by columns.  This means that every time you load a value from a new row, you are loading it from a different cache line, which is slow compared to reading nearby values (from the same row and cache line).
I modified your function to take the whole table as input and traverse it in linear fashion, reading all entries in one row before going to the next row.  Your program took 11 seconds, and mine took 1.2 seconds, for an almost 10x speedup.
Here are my changes:
int FindSumOfMinOfColumns(int **tab, int size)
{
    int *minVals = malloc(size * sizeof(*minVals));
    int  row     = 0;
    int  col     = 0;
    int  sum     = 0;

    // Initialize with 0th element of each column.
    memcpy(minVals, tab[0], size * sizeof(*minVals));

    // Compute the minimum of each column.
    for (row = 1; row < size; row++) {
        int *pRow = tab[row];
        for (col = 0; col < size; col++) {
            if (pRow[col] < minVals[col])
                minVals[col] = pRow[col];
        }
    }

    // Sum up the minimum of each column and return it.
    for (col = 0; col < size; col++)
        sum += minVals[col];
    free(minVals);
    return sum;
}

int main(void) {
    // ...

    t1 = RDTSC();
    for (z=0; z<10; z++) // pour avoir une moyenne du temps d'execution
        somme += FindSumOfMinOfColumns(tab, NB);
    t2 = RDTSC();

    // ...
}


Generate all possible combination of n-pair parentheses The task is taken from LeetCode

  Given n pairs of parentheses, write a function to generate all combinations of well-formed parentheses.

  For example, given n = 3, a solution set is:

[
  "((()))",
  "(()())",
  "(())()",
  "()(())",
  "()()()"
]

My solution

is based on backtracking. Maybe there's a better approach than that? The reason I do this task is because I want to improve my skills. So, if you got any improvement suggestions or an alternative approach - no matter how fancy it is - feel free to post it here.

/**
 * @param {number} n
 * @return {string[]}
 */
var generateParenthesis = function(n) {
  const addParanthesis = (cur, open, close) => {
    if (cur.length === n * 2) {
      res.push(cur);
      return;
    }
    if (open < n) { addParanthesis(cur + '(', open + 1, close); }
    if (close < open) { addParanthesis(cur + ')', open, close + 1); }
  };
  const res = [];
  addParanthesis('', 0, 0);
  return res;
};

Names

The plural of parenthesis is parentheses (as used in the specification). So I think it would make more sense to call the top level function generateParentheses, or even generateBalancedParentheses.

The inner function should be addParenthesis (with an e substituted for the third a).

Opinions will vary on cur as an abbreviation for current. I think it's common enough to be easily recognised, although given the meaning of the value it names perhaps prefix would be better.

open and close read to me as verbs, although open could also be a count of the number of parentheses which have been opened and not closed (i.e. the surplus of ( over ) in cur). I've reverse engineered that they count, respectively, the number of ( and ) in cur. Perhaps opened and closed would be clearer, along with a comment explaining what they are.

Scope

Although JavaScript is very forgiving of the order of declarations, humans tend to read code in order. I would find it clearer if res were defined before addParenthesis, since that references it.

Correctness

Although the code looks correct to me, I would like to see some comments to justify why it's correct. Why are the if guards necessary and sufficient?

With these comments, you would also be able to justify a slight simplification:

  const addParenthesis = (prefix, opened, closed) => {
    if (closed === n) { res.push(prefix); }
    if (opened < n) { addParenthesis(prefix + '(', opened + 1, closed); }
    if (closed < opened) { addParenthesis(prefix + ')', opened, closed + 1); }
  };


Is there a better or more concise way to do this? My goal was to make a list dynamically list itself into two columns, no matter the length of the list. I know this is possible by just floating the li nodes, but I wanted to keep the li nodes in the same vertical order. I would really like to hear from people who know more about JavaScript and jQuery than I do if this code looks good or if there is a better or more concise method for implementing this sort of thing.

Demo: http://jsfiddle.net/mkimitch/ZEL5x/

HTML:

Australia

Brazil

Canada

Chile

China

France

India

Italy

Malaysia

Norway

Russia

United Kingdom

United States

JavaScript:

var colLength = $('.columned li').length;
var colHeight = $('.columned').height();
var liHeight = $('.columned li').height();
if ($('.columned li').length % 2 != 0) {
    var half = (Math.round(colHeight / 2)) + liHeight / 2;
} else {
    var half = Math.round(colHeight / 2);
}
var firstrow = Math.ceil(colLength / 2);
var secondrow = firstrow + 1;
$('.columned li:nth-child(-n+' + firstrow + ')').addClass('column1');
$('.columned li:nth-child(n+' + secondrow + ')').addClass('column2');
$('.columned li:nth-child(' + secondrow + ')').css('margin-top', -half);

the best javascript is no javascript ;-)

you could use css3 columns, see http://jsfiddle.net/bjelline/JWnaz/ for the result

ul {
-moz-column-count: 2;
-moz-column-gap: 20px;
-webkit-column-count: 2;
-webkit-column-gap: 20px;
column-count: 2;
column-gap: 20px;

}

if you are worried about browser support: there's a polyfill for that
http://www.csscripting.com/css-multi-column/
that falls back to javascript if the browser doesn't honor the css yet.


Epilogue to a binary search to find the range of matching indexes I am trying to find out the best practice for writing a loop which does one check and at the same time has to increment a value so that the while loop will eventually fail. Performance is important that's why I thought of using the least amount of variables.

Use case

In a decently low level method I do a binarySearch(array, key) which guarantees to find a key value in a sorted array if the key can be found. However it does not guarantees to find the first or last key first. But since I need the range of indexes in which the key excists (for other code later on) I try to find the last and first element in the array that matches. With this code I am trying to let lastSucceedingIndex hold the last index which holds the key:

while (lastSucceedingIndex < tailIndex && (array[++lastSucceedingIndex] == key)) {
}
lastSucceedingIndex--;

There is one situation in which a bug exists, namely when the lastSucceedingIndex is equal to the tailIndex: the while loop will fail and the ++lastSucceedingIndex will not happen. The lastSucceedingIndex-- is there in the end to fix the ++lastSucceedingIndex when it's not equal to the key, but since this does not happen in the last case it will decrement lastSucceedingIndex incorrectly. I wrote this loop for performance and I take this little bug for granted, it happens so rarely that it will effect less than 0.001% of all cases. This way it seems like I only need one counter and one variable, the loop could be executed as much as 17000 times before it finds its lastSucceedingIndex and the array is very large with possibly up to a million entries.

Are there any alternatives for performance and readability?

I analysed my code by the build in code analysis of my IDE and I get the remark

  while statement has empty body at line

Which seems to indicate that having a loop with no lines in them is considered a possible bug by the compiler or at least bad practice. Now my question is, is having an empty loop considered bad practice? I could move the lastSucceedingIndex to the body and make it bug-free but that would require one extra addition like so:

while (lastSucceedingIndex < tailIndex && array[lastSucceedingIndex + 1] == key) {
    lastSucceedingIndex++;
}

1 less bug for millions upon millions of extra assembly instructions seems a hard trade-off to make.

There are three alternatives you should consider. Depending on circumstances, I have used all three.

The most simple, is to comment the block.

while (lastSucceedingIndex < tailIndex && (array[++lastSucceedingIndex] == key)) {
    //loop condition terminates when key is not found, or data runs out.
}
lastSucceedingIndex--;

The comment should shut down the error.

The second approach is to simplify the loop condition to match your code and to make the dependent code part of the loop. The pre-increment is essentially your "work" which should be in the block... but, because it is a pre-increment, it is messy... Still, your code would become...

while (lastSucceedingIndex < tailIndex) {
    lastSucceedingIndex++;
    if (array[lastSucceedingIndex] != key) {
        break;
    }
}
lastSucceedingIndex--;

That's still not as neat as I would like, but it reduces the complexity of the while condition and makes the break-out process more apparent.

This last alternative is simpler in other ways:

while (lastSucceedingIndex < tailIndex && array[lastSucceedingIndex + 1] == key) {
    lastSucceedingIndex++;
}

Now there is no need for the corrective decrement.


Implement a blocking counter with a reader/writer lock I'm trying to implement a blocking counter that may have several readers and writers at the same time:

class BlockingCounter {
public:
    BlockingCounter(int count) : count_(count) {
        // init lock_;
    }
    ~BlockingCounter() {
        // destroy lock_;
    }

    void Increment() {
        pthread_rwlock_wrlock(&lock_);
        count_++;
        pthread_rwlock_unlock(&lock_);

    }

    void Decrement() {
        pthread_rwlock_wrlock(&lock_);
        count_--;
        pthread_rwlock_unlock(&lock_);

    }

    int GetCount() {
        pthread_rwlock_rdlock(&lock_);
        int count = count_;
        pthread_rwlock_unlock(&lock_);
        return count;
    }

private:
    pthread_rwlock_t lock_;
    int count_;

};

Does this code have problems under multi-threading circumstances?

I intended to use it like this:

Main thread:

BlockingCounter *counter = new BlockingCounter(10);

// Here 10 threads are started. See code below.
while (counter->GetCount() > 0) {
  ConditionVar.wait();
}
// Main thread resumes execution.

Other 10 thread(s):

// Do stuff
counter.Decrement();
ConditionVar.notify();

You are really writing C
C++ has its own language specific features to handle multi threading.
Your interface is broken.
The only way to test the value of the counter is via GetCount() but by the time you can use the value the counter may have changed.
 Thread1                      Thread2                    Thread3
 if (b.GetCount() == 0) {     if (b.GetCount() == 0) {   if (b.GetCount() == 0) {
     b.Increment();                b.Increment();             b.Increment();
     // I have the lock.
     Do stuff                      Do stuff                   Do stuff
 }                            }                          }

As you can see your mechanism does not stop all three threads from doing something. The only real use for the counter is to print the count (and even that may not be accurate but at least it wont break anything).


Simple string repeater program I'm new to C, want to know if I'm doing this right.

//repstring.c
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main(int argc, char **argv)
{
    //Initialize variables and pointers
     int i;
     int loop;
     char *deststr;
     char *str;

     //Change second argument to integer
     loop = atoi(argv[2]);

    //If user requests to repeat string more than 9999 times, terminate
    if(loop > 9999)
    {
        printf("Error: Asked to repeat too many times.\n");
        printf("Hardcoded maximum: 9999\n");
        return 1;
    }

    if(argc == 3)
    {

        //Dynamically allocate memory to strings
        deststr = malloc(1024 * 1024 * 1024  * sizeof(char));
        str = malloc(1024 * 1024 * 1024  * sizeof(char));

        if(str == NULL || deststr == NULL)
        {
            printf("Error allocating memory!\n");
            free(deststr);
            free(str);
            return 1;
        }

        //Copy string in first argument to str
        strncpy(str, argv[1], 50);

        //Append str to deststr for amount of times specified in second argument
        for(i = 0; i < loop ; i++)
        {
            strcat(deststr, str);
        }

        //Print repeated string to stdout
        printf("%s\n", deststr);

        //Free allocated memory
        free(deststr);
        free(str);

    }
    else
    {
        printf("Usage: repstring <string> <amount of times to repeat>\n");
        return 1;
    }

 return 0;
}

Is there anything that could be otherwise improved? Am I handling memory correctly?
Review would be very appreciated.

Standard guarantees sizeof(char) to be 1.
argv[1] is a perfectly good string. There is no need to copy it anywhere, let alone allocate a megabyte for it.
Length of a destination string is strlen(argv[1]) * loop + 1. Do not hardcode a megabyte.
strcat has to find the end of destination string over and over again, making the execution time quadratic. Trace the end of the destination string as you go and strcpy to the right location instead.


Calculates the Average Score of Given Number of Scores in C I have started with C and I made this code and as the title suggests this program can calculate the average score of a given number of scores. Now my major concern is, is it more efficient to get the number of scores as a Command Line Argument or is the current approach better incase if sometime in the future someone designs a Interactive Front-End for it?
Also I am using the cs50 library I am not sure if it is allowed here please tell me if it's not so I'll keep that in mind in the future.
#include <cs50.h>
#include <stdio.h>
float average(int length, int array[]);

int main(void)

{
    int i, j;
    printf("This Program will Help you Compute the Average Score \n");         //Welcome Message
    int Total = get_int("How Many Scores do you wish to Use: ");               //Asks User for How Many Scores
    int scores[Total];                                                         //Stores Scores in an Array
    for (i=0; i < Total; i++)                                                  //A Loop to keep Asking for Scores
    {
        scores[i]=get_int("Enter Score %d \n", i+1);                           //i+1 is used as it would be better to start asking from Score 1 so that it avoids confusion
    }
    printf("The Average score is %f", average(Total, scores));
}

float average(int length, int array[])
  {  int sum = 0;
    for (int j=0; j < length; j++)
    {
        sum += array[j];
    }
    return sum/(float)length;
  }

Reduce the number of loops
The most time-consuming part of your program is to loop through the array. Notice that you have done this twice, why can't you keep a track of the total sum while taking the input?
#include <stdio.h>

float average(int sum,int number);

int main(void)
{
    int number_of_integers,number,sum = 0;
    printf("Enter the number of integers: ");
    scanf("%d",&number_of_integers);
    for(int i = 0;i < number_of_integers;i++)
    {
        printf("Enter: ");
        scanf("%d",&number);
        sum+=number;
    }
    printf("Average = %d",average(sum,number_of_integers));
}

float average(int sum, int number)
{
    return sum/(float)number;
}

This reduces the number of loops from 2 into 1. And completely removes the need of having an array of integers. Making your program both memory efficient and fast.


Find two values that add up to the sum I was working on a practice Java exam online:

  Write a function that, given a list and a target sum, returns
  zero-based indices of any two distinct elements whose sum is equal to
  the target sum. If there are no such elements, the function should
  return null.
       For example, findTwoSum(new int[] { 1, 3, 5, 7, 9 }, 12) should return a single dimensional array with two elements and contain any of
  the following pairs of indices:

1 and 4 (3 + 9 = 12)
2 and 3 (5 + 7 = 12)
3 and 2 (7 + 5 = 12)
4 and 1 (9 + 3 = 12)

Here is my solution:

public class TwoSum {

    public static int[] findTwoSum(int[] list, int sum) {
        int[] twoSum = null;
        if(list != null){
            int listSize = list.length;
            int mod = listSize % 2;
            int half = listSize/2;
            int stop = mod > 0 ? half+1 : half;
            if(listSize >= 2){
                outer: for(int currentIndex=0; currentIndex<listSize; currentIndex++){
                            if(currentIndex == stop){
                                break;
                            }
                            for(int otherIndex=0; otherIndex<listSize; otherIndex++){
                                if(currentIndex < otherIndex){
                                    int current = list[currentIndex];
                                    int other = list[otherIndex];
                                    int currentSum = current + other;
                                    if(currentSum == sum){
                                        twoSum = new int[]{currentIndex, otherIndex};
                                        break outer;
                                    }
                                }
                            }
                        }
            }
        }
        return twoSum;
    }

    public static void main(String[] args) {
        int[] indices = findTwoSum(new int[] { 1, 3, 5, 7, 9 }, 12);
        System.out.println(indices[0] + " " + indices[1]);
    }
}

My code passed all the unit tests, however, they have another check for a large list, and apparently my code runs too slow when the list size grows. Apart from me trying to decipher what their definition of too slow is, simply put, are there any optimizations I can make to have my code run faster? I tried to find the 'tipping point' in comparing numbers where I could stop checking, and I added that optimization, but apparently, that isn't good enough.

I would find this easier to read if the first thing in the method was

        if (list == null || list.length < 2) {
            return null;
        }

This gets rid of the unlikely cases off the bat and saves us having to indent the rest of the logic inside two if statements.

            int listSize = list.length;
            int mod = listSize % 2;
            int half = listSize/2;
            int stop = mod > 0 ? half+1 : half;

This could just be

            int stop = (list.length + 1) / 2;

You never use mod or half elsewhere and can just replace listSize with list.length.  This will return the same results, as if the list has an odd size, adding one will give it an even size.  Then dividing by two will return the same result as half + 1, since half is rounded down.  If even, then adding one before dividing by two does nothing, as the odd number will be rounded down.

                outer: for(int currentIndex=0; currentIndex<listSize; currentIndex++){
                            if(currentIndex == stop){
                                break;
                            }

This could just be

          for (int currentIndex = 0; currentIndex < stop; currentIndex++) {

Although either way it returns an incorrect result for an input like

new int[] { 1, 2, 9, 5, 7 }, 12

You do

                                    int current = list[currentIndex];

inside the inner loop.

Do it before the inner loop instead.  Then you can just do it once rather than once for each iteration of the inner loop.  On the bright side, the compiler probably compiled this out anyway.

                                        twoSum = new int[]{currentIndex, otherIndex};
                                        break outer;

This could be just

                            return new int[]{currentIndex, otherIndex};

Then you don't need the labeled loop.  And

        return twoSum;

could be just

        return null;

No need for the twoSum variable at all.

Alternatives

This is an \$\mathcal{O}(n^2)\$ solution.  But this problem is solvable in linear time.  Consider

public static int[] findTwoSum(int[] numbers, int target) {
    if (numbers == null) {
        return null;
    }

    Integer[] complements = new Integer[target];

    for (int i = 0; i < numbers.length; i++) {
        if (complements[numbers[i]] != null) {
            return new int[]{complements[numbers[i]], i};
        }

        complements[target - numbers[i]] = i;
    }

    return null;
}

This works fine for modest sums where all the inputs are positive (like your example).  It will be much faster for large inputs.

If those constraints don't work for you, try

public static int[] findTwoSum(int[] numbers, int target) {
    if (numbers == null) {
        return null;
    }

    Map<Long, Integer> complements = new HashMap<>();

    for (int i = 0; i < numbers.length; i++) {
        Integer complementIndex = complements.get((long)numbers[i]);
        if (complementIndex != null) {
            return new int[]{complementIndex, i};
        }

        // it's possible that for large targets and a negative input
        // that we might overflow the int type
        long complement = (long)target - numbers[i];
        complements.put(complement, i);
    }

    return null;
}

Now it doesn't matter if the target sum is large or some of the numbers are negative.

Either version works by storing the locations of the complements for each value.  We only need to loop once and get/put are constant time operations for a HashMap.


Rock, Paper, Scissors game made in python I added Rock, Paper, Scissors to one of my programs. I'm new to python and PEP 8, so I want to see how to improve my work.

def rock_paper_scissors(name):
    play_rps = True
    while play_rps:
        rps = ["rock", 'paper', 'scissors']
        player_rps = input("Rock, Paper, or Scissors: ").lower()
        com_rps = rps[random.randint(0,3)]
        print(com_rps)

        if com_rps == player_rps:
            print('Tie')

        if com_rps == 'rock' and player_rps == "scissors":
            print("Chatter Bot Wins!")

        if com_rps == 'scissors' and player_rps == "paper":
            print("Chatter Bot Wins!")

        if com_rps == 'paper' and player_rps == "rock":
            print("Chatter Bot Wins!")

        if player_rps == 'rock' and com_rps == "scissors":
            print(f"{name} Wins!")

        if player_rps == 'sicssors' and com_rps == "paper":
            print(f"{name} Wins!")

        if player_rps == 'paper' and com_rps == "rock":
            print(f"{name} Wins!")

        yn = input("Do you want to play again. Y/N: ").lower()

        if yn == 'n' or yn == 'no':
            play_rps = False

Move more of your code out of the function. Make a find_winner function.
You don't need to add _rps to the end of every variable name. It only makes it harder to read the code.
PEP 8 doesn't say which string delimiter to use, but says to stick to one. I use ', you may prefer ".
You can use random.choice rather than random.randint to select from a list.
There is a bug in your code. I suggest you keep using string literals to a minimum to prevent this.

player_rps == 'sicssors' and com_rps == "paper"

You can simplify your yn check by using in.

This in all got me:

WIN_VALUES = {
    'rock': 'scissors',
    'paper': 'rock',
    'scissors': 'paper'
}

def find_winner(choice1, choice2):
    if choice1 == choice2:
        return 0
    if WIN_VALUES[choice1] == choice2:
        return 1
    if WIN_VALUES[choice2] == choice1:
        return 2

def rock_paper_scissors(name):
    play = True
    while play:
        player = input('Rock, Paper, or Scissors: ').lower()
        com = random.choice(list(WIN_VALUES))
        print(com)

        winner = find_winner(com, player)
        if winner == 0:
            print('Tie')
        elif winner == 1:
            print('Chatter Bot Wins!')
        elif winner == 2:
            print(f'{name} Wins!')
        else:
            print(f'Invalid input {player}')

        yn = input('Do you want to play again. Y/N: ').lower()
        if yn in ('n', 'no'):
            play = False


Implementation of binary search I am new to the divide and conquer paradigm.  I wanted to verify that there's nothing wrong with this implementation of binary search.

I ran it a few times and it does print the correct result, but I also wanted a sanity check and any possible improvements.

def search(array, item):

    if len(array) == 1:
        if array[0] == item:
            return 1
        return 0

    mid = len(array) // 2
    x = array[:mid]
    y = array[mid:]

    found = []

    found.append(search(x, item))
    found.append(search(y, item))

    for elm in found:
        if elm ==  1:
            return True

    return False

print(search([1,2,3,4], 8))
print(search([1,2,3,4], 3))

Also, the running time of this is \$O(n log(n))\$, correct? \$O(log(n))\$ being the recursion and \$O(n)\$ being the loop that checks the result list.

It looks almost correct. But it can be improved.

Bug

What will happen if the input array is empty? Stack overflow.

Boolean values

Why return 1 for true and 0 for false instead of using boolean values?

if array[0] == item:
    return 1
return 0

In fact you could return the result of the boolean expression directly:

return array[0] == item

Similarly, what's happening here?

found = []

found.append(search(x, item))
found.append(search(y, item))

for elm in found:
    if elm ==  1:
        return True

return False

You append 0-1 values into found. If found contains a 1, return True,
otherwise return False. All this could be written on a single line:

return search(left, item) or search(right, item)

I also renamed the meaningless x and y to left and right.

Use doctests

Doctests are awesome. They are easy to write and execute and can greatly increase your confidence in your code and make it possible to refactor the code safely. For example:

def search(array, item):
    """
    >>> search([], 1)
    False

    >>> search([1,2,3,4], 8)
    False

    >>> search([1,2,3,4], 0)
    False

    >>> search([1,2,3,5], 4)
    False

    >>> search([1,2,3,5], 3)
    True

    >>> search([1,2,3,5], 5)
    True

    >>> search([1,2,3,5], 1)
    True

    """

    if not array:
        return False

    if len(array) == 1:
        return array[0] == item

    mid = len(array) // 2
    left = array[:mid]
    right = array[mid:]

    return search(left, item) or search(right, item)

You can run this with python -m doctest script.py.
If any of the test cases fails, you'll get a nice report (otherwise no output).


Object-oriented approach to sieve of Eratosthenes I was trying to implement the sieve of Eratosthenes entirely by myself, using only what I have learned so far. Now I'm wondering what I can do to improve my code's readability. Optimizing the algorithm is not my goal for now.

#include <iostream>
#include <cmath>

using namespace std;

class num {
private:
    static unsigned obj_count; //simplifying initialization of the array, performance is not my main concern
    unsigned n;
    bool flagged;
public:
    num();
    void setn(unsigned);
    void setflagged(bool);
    bool isflagged();
    unsigned getn();
};

void Eratosthenes_sieve(num*, unsigned);

//Initialization of static member
unsigned num::obj_count = 0;

//Implementation of member functions
num::num() {
    flagged = false;
    ++obj_count; //simply declaring an array of class num will result in initialized values for n
    n = obj_count;
}

void num::setn(unsigned n) {
    this->n = n;
}

void num::setflagged(bool flagged) {
    this->flagged = flagged;
}

bool num::isflagged() {
    return flagged;
}

unsigned num::getn() {
    return n;
}

int main()
{
    unsigned n = 100;
    num arr[n];

    Eratosthenes_sieve(arr, n);

    return 0;
}

void Eratosthenes_sieve(num *arr, unsigned n) {
    unsigned i;

    for (i = 1; i < sqrt(n); ++i) { //minor optimization, as there will be no multiples or sqrt(arr[i]) that are lower than n
        if ( !(arr[i].isflagged()) ) { //if the number is not flagged, proceed to flag it
            for (unsigned j = 1; j < ( n / (arr[i].getn()) ); ++j) {
                arr[i + j * arr[i].getn()].setflagged(true); //flags every arr[i]-th number in the array.
            }
        }
    }

    cout << "Prime numbers up to " << n << ": \n";
    for (i = 1; i < n; ++i) {
        if ( !(arr[i].isflagged()) ) {
            cout << arr[i].getn() << " ";
        }
    }
}

Is "n" really a good idea?

I don't like arr[i].getn().  The whole algorithm relies on the fact that arr[i].getn() is exactly i+1.  If this is ever not true, then the sieve will not work.  Given that the array index is inextricably linked to the value of n, I wouldn't even bother with n at all.

Using constant expressions in loop conditions

In two places, you use constant expressions in your loop condition:

for (i = 1; i < sqrt(n); ++i) // sqrt() on every loop
for (unsigned j = 1; j < ( n / (arr[i].getn()) ); ++j) // division on every loop

What's bad about this is that you will be calling sqrt() or doing a division on every loop, even though the value of the expression will never change.  It's possible the compiler could optimize this for you, but it's not a certainty.  It's easy enough to just move the expression out of the loop yourself:

int sqrtN = sqrt(n);
for (i = 1; i < sqrtN; ++i)

Simplifying the loop

Even after getting rid of getn(), the second loop can still be simplified.  The current loop is this:

for (unsigned j = 1; j < (n / (i+1)); ++j) {
    arr[i + j * (i+1)].setflagged(true);
}

It can be simplified to this:

for (unsigned j = i+(i+1); j < n; j += (i+1)) {
    arr[j].setflagged(true);
}

In other words, instead of computing the array index using a multiplication, you can just have j be the array index.  This way avoids any multiplications and divisions and in my opinion is also simpler to read.  It would be even simpler if arr[i] stood for the number i instead of i+1.


Return a random line from a file when user enters a specific text I am a beginner trying to fetch a random line from a file and wondering if this code can be improved. Should I use try and catch in this case? Should I take a parameter for the function "quote"? How can I take several alternatives for searchText? (E.g. "hello", "hi" or "hey" but one is enough to return true.)

def getInputFromUser(inputText, verifier, error=None):
    """
    General function to get input from the user, repeating the question until verifier returns true
    """
    while True:
        userInput = input(inputText)
        if not verifier or verifier(userInput):
            return userInput
        elif error:
            print(error)

def quote():
    """
    Return a random line from a file if user enters the text "hello"
    """
    searchText = "hello"
    text = getInputFromUser("Enter a sentence with the words 'hello', 'hi' or 'hey': ", lambda inputText: inputText)

    if searchText in text:
        lineFetched = random.choice(open('myquotes.txt').readlines())
        print("My quote: ", lineFetched)
    else:
        quote()

When working with files, you should wrap open calls in a with statement like this:

if search_text in text:
    with open('myquotes.txt') as fh:
        line_fetched = random.choice(fh.readlines())
    print("My quote: ", line_fetched)
else:
    quote()

If you want to make it possible to match any of multiple words like "hello", "hi",
then you'd need a helper function, for example:

def text_contains_any_of(text, words):
    for term in words:
        if term in text:
            return True
    return False

And then change your code to use this:

words = ("hello", "hi")

if text_contains_any_of(text, words):
    with open('myquotes.txt') as fh:
        line_fetched = random.choice(fh.readlines())
    print("My quote: ", line_fetched)

In the above examples I renamed the variable and method names to follow PEP8.


java.sql.Timestamps: There's gotta be an easier way I have a function that counts how many "assists" a team performs during the course of their day.  The way I've written it feels bulky and inefficient using GregorianCalendar.

Basically, the function counts specific types of assistance then returns the information in an object.

Does anyone know of a cleaner way to do this?

public static AssistReportData getAssistReportByDate(int teamId, Date date) throws ClassNotFoundException, SQLException {
    AssistReportData ard = null;
    Connection conn = getConnection();

    String query = "SELECT assistanceProvided FROM fauassist WHERE teamId=? AND timeOut BETWEEN ? AND ?;";

    PreparedStatement ps = conn.prepareStatement(query);
    ps.setInt(1, teamId);
    GregorianCalendar gc = new GregorianCalendar();
    gc.setTime(date);
    gc.set(Calendar.HOUR, 0);
    gc.set(Calendar.MINUTE, 0);
    gc.set(Calendar.SECOND, 0);
    ps.setTimestamp(2, new java.sql.Timestamp(gc.getTime().getTime()));
    GregorianCalendar gc2 = new GregorianCalendar();
    gc2.setTime(date);
    gc.set(Calendar.HOUR, 59);
    gc.set(Calendar.MINUTE, 59);
    gc.set(Calendar.SECOND, 59);
    ps.setTimestamp(3, new java.sql.Timestamp(gc2.getTime().getTime()));

    ResultSet rs = ps.executeQuery();
    while (rs.next()) {
        ard = new AssistReportData();
        ard.setTeam(teamId);
        switch (rs.getString("assistanceProvided")) {
            case ("TEAM EFFORT"):
                ard.addTeamEffort(1);
                break;
            case ("OUTSIDE AGENCY"):
                ard.addOutsideAgency(1);
                break;
            case("SEARCH WARRANT"):
                ard.addSearchWarrant(1);
                break;
            case("TRANSPORT"):
                ard.addTransport(1);
            default:
        }
    }
    return ard;
}

Bug

gc.set(Calendar.HOUR, 59);

I suppose you mean gc.set(Calendar.HOUR, 23); here?

Potential Bug

while (rs.next()) {
    ard = new AssistReportData();
    // ...
}

I suppose this works currently because you only get a single result row, but please be aware that if you have multiple rows, you will simply be re-referencing until the last row of your ResultSet.

Getting dates

All you need is a nice method that converts Date to Timestamp:

// using shortened variable names for brevity
private static Timestamp convertAndReset(Date date, int hr, int min, int sec, int nano) {
    GregorianCalendar calendar = new GregorianCalendar();
    calendar.setTime(date);
    calendar.set(Calendar.HOUR, hr);
    calendar.set(Calendar.MINUTE, min);
    calendar.set(Calendar.SECOND, sec);
    Timestamp result = new Timestamp(calendar.getTimeInMillis());
    result.setNanos(nano);
    return result;
}

You can them call it as such:

ps.setInt(1, teamId);
ps.setTimestamp(2, convertAndReset(date, 0, 0, 0, 0));
ps.setTimestamp(3, convertAndReset(date, 23, 59, 59, 0));

Java 8?

If you happen to be on Java 8, then congratulations: the new java.time.* classes ('Time APIs') are much better for chronological representations.

You can then consider rewriting the same method as such:

private static Timestamp convertAndReset(Date date, int hr, int min, int sec, int nano) {
    return Timestamp.valueOf(date.toInstant().atZone(ZoneId.systemDefault())
                                    .toLocalDate().atTime(hr, min, sec, nano));
}

Convert Date to Instant via Date.toInstant().
Convert to ZonedDateTime via Instant.atZone(ZoneId).
Convert to either a LocalDate or LocalDateTime. Using the former because...
Setting the time via LocalDate.atTime() is arguably more fluent, which gives us a LocalDateTime instance with the desired time in another 'step'.
Finally, call Timestamp.valueOf(LocalDateTime) to get the Timestamp instance.


MySQL vs PDO execution time I'm having some issues understanding why PDO is 6X slower executing a query than the depreciated MySQL query function. Here's my code:

$conn = new PDO('mysql:host=localhost;dbname=work', 'root', 'Montreal');

protected function query_s($table, $col = '*', $where_col , $where, $return_col)
{

              $q = $conn->query("SELECT $col FROM $table WHERE $where_col='".$where."' LIMIT 1");
              $f = $q->fetch();
              $result = $f[$return_col];
              return $result;

}

Which retrieves 1 piece of information for a specific column.

The old code:

protected function query_s($table, $col = '*', $where_col , $where, $return_col)
{

    $SQL = mysql_query("SELECT $col FROM ".$table." WHERE $where_col='$where' LIMIT 1") or die(mysql_error());
            $data = mysql_fetch_array($SQL);
            $this->query_rows = mysql_num_rows($SQL);

            return $data[$return_col];
}

As soon as I run the PDO version in a while loop through a database search (user tries to find something), it takes almost 6 seconds to retrieve 50 rows... Normally, it would take around 0.02 seconds with the MySQL function

I'm fairly new with PDO and I'm trying to write new code with it. I'm missing something here.

Change your connection string from host=localhost to host=127.0.0.1, for some reason PDO is slow at resolving the host name, just when using PDO always use the IP address of the server.

Also why are you fetching one row at a time when you need 50?