Copying sections of one picture to another I have two solutions (in Jython) to take two pictures and copy and paste alternate sections of one picture to the other, like so:

for x in range (0,w):
  for y in range (0,h):
    sourcePx=getPixel(pic1,x,y)
    targetPx=getPixel(pic2,x,y)
    if (y>=0 and y<20) or (y>=40 and y<60)or (y>=80 and y<=100):
      setColor(targetPx, getColor(sourcePx))
repaint (pic2)

for y in range(0,h):
  leaf = (y>=0 and y<20) or (y>=40 and y<60)or (y>=80 and y<=100):
  if leaf:
    for x in range(width):
      sourcePx = getPixel(pic1, x, y)
      targetPx = getPixel(pic2, x, y)
      setColor(targetPx, getColor(sourcePx))
  repaint(pic2)

I have been trying to work this out and have two questions about this:

Which is the more efficient way to loop through the pixels and why?
Does it make any difference to efficiency (and if so how), creating the variable leaf and then using that in the if statement?

The second one is more efficient. Simply because in the first one you are calculating the if w * h times but in the second you are calculating it h times.

Also it can be made more readable and efficient just by using this. It eliminates leaf which you aren't using anywhere. Readability doesn't need explaining.

for y in range(0,h):
  if (20 > y >= 0) or (60 > y >= 40)or (100 >= y >= 80):
    for x in range(width):
      sourcePx = getPixel(pic1, x, y)
      targetPx = getPixel(pic2, x, y)
      setColor(targetPx, getColor(sourcePx))
  repaint(pic2)

Update 1

As limelights has suggested, use of xrange is better than use of range in Python2. If your h or w are big then using range would return a list which occupies memory while use of xrange returns a generator which is much more memory efficient. Performance might suffer a bit but in most cases it is worth it.


Hide Excel Rows faster I've built a VBA Excel macro that works on a spreadsheet of duplicated files, and displays only groups in which at least one duplication is owned by the current user.

The spreadsheet lists file metadata, one file per row, grouped with other files whose content is identical, i.e. "duplicated". Each such duplication group is preceded by a title row in bold.

The macro uses text boldness to determine duplication groups, and checks within each group whether any file's owner (represented in column 7) is the currently logged user.
All groups with no file owned by the user get hidden.
(auto-filtering would not solve this problem because I want the user to see the other files in the duplication group, i.e. see files from other owners with the same content as theirs)

It works exactly as intended, but too slowly. It takes one or two minutes to run through a spreadsheet about 80 thousand lines long. Anyone can see a way to speed it up?

I had the idea of maybe hiding every row first (which I assume can be done much faster) and then de-hide what should be shown. But I couldn't find a command to mass-hide.

Also, since we're here, what best practices am I missing? I'm a complete novice to VBA, though I'm an experienced programmer in other languages.

Grateful for any help.

'Operates on .xslx report of network file duplication, as output by TreeSize tool.
'Hides all Duplication Groups not involving the currently logged user as a file Owner
'(regular filtering does not work because it hides the other duplicates which have a different owner)
'TODO: speed it up. It currently takes ~1min to run. Probably hide-all then unhide-selected will be faster.
Private Sub My_duplicates_Click()
    Dim hidIt As Boolean
    Dim foundInGroup As Boolean

    Application.ScreenUpdating = False
    Application.EnableEvents = False
    login = Environ("Username")
    ownerCol = 7
    rowNum = 3 'skip header
    Do While Not IsEmpty(Cells(rowNum, 1))
        If Cells(rowNum, ownerCol).Font.Bold = True Then 'bold text indicates the beginning of a duplication group
            foundInGroup = False
            groupMemberRow = rowNum + 1
            Do While Not IsEmpty(Cells(groupMemberRow, 1)) And Not Cells(groupMemberRow, ownerCol).Font.Bold = True
                If Cells(groupMemberRow, ownerCol).Value2 = login Then
                    foundInGroup = True
                End If
                groupMemberRow = groupMemberRow + 1
            Loop
            If foundInGroup = False Then
                hidIt = True
            Else
                hidIt = False
            End If
            Cells(rowNum, ownerCol).EntireRow.Hidden = hidIt
            groupMemberRow = rowNum + 1
            Do While Not IsEmpty(Cells(groupMemberRow, 1)) And Not Cells(groupMemberRow, ownerCol).Font.Bold = True
                Cells(groupMemberRow, ownerCol).EntireRow.Hidden = hidIt
                groupMemberRow = groupMemberRow + 1
            Loop
            rowNum = groupMemberRow - 1 'counting on next increment to place it back on the next group header
        End If
        rowNum = rowNum + 1
    Loop
End Sub

Auto Filtering

  (auto-filtering would not solve this problem because I want the user to see the other files in the duplication group, i.e. see files from other owners with the same content as theirs)

Auto Filtering could definitely solve this problem.  The way to go about it would be to add a helper column, set the values in the column to "Show" if you want to show the column and then filter the helper column by the word "Show".  You could even hide this column.

Hiding Rows

  I had the idea of maybe hiding every row first (which I assume can be done much faster) and then de-hide what should be shown. But I couldn't find a command to mass-hide.

Yes, this undoubtedly faster.  Consider that you have 80K rows and you only need to see 1K rows.  Instead of performing 80K + 1 show/hide operations, hiding all the rows and then unhiding the rows you want will reduce the number of operations to  1K + 1.  Union() is much faster than Show/Hide. It would be even faster if you were to Union() all the ranges that need to be shown together and only have 1 hide operation amd 1 show operation.

Note: Concatenating the Range addresses is considerably faster than Union().  This can get somewhat complicated because there is a limit to the address length.  To maximize performance, use a combination of address concatenation and Union().

Option Explicit

MSDN - Option Explicit Statement

  Use Option Explicit to avoid incorrectly typing the name of an existing variable or to avoid confusion in code where the scope of the variable is not clear.

One of the best of the "Best Practices" is to always declare and Type your variables.  The more information that you give the compiler the faster the compiler can catch errors.

Declaring Constants

MSDN - Declaring Constants

  By declaring a constant, you can assign a meaningful name to a value. You use the Const statement to declare a constant and set its value. After a constant is declared, it cannot be modified or assigned a new value.

A variable should be used when its value is either not know prior to compiling or will vary throughout the course of code execution.  A value that will remain always remain constant should be declared as a such.

 Const ownerCol As Long = 7

A Faster Way

Sub ShowUserDuplicates()
    Application.Calculation = xlCalculationManual
    Application.ScreenUpdating = False
    Application.EnableEvents = False

    Const OWNER_COLUMN As Long = 7
    Dim cell As Range, VisibleRows As Range
    Dim flagUser As Boolean
    Dim login As String

    login = Environ("Username")
    With Worksheets("Sheet1")
        'Hide All the Rows
        .UsedRange.Offset(2).EntireRow.Hide
        'Iterate though the cells in the Owner Column
        For Each cell In Range("A3", .Range("A" & .Rows.Count).End(xlUp)).Offset(0, OWNER_COLUMN - 1)
            If cell.Font.Bold Then flagUser = cell.Value = login

            If flagUser Then
                If VisibleRows Is Nothing Then
                    Set VisibleRows = cell
                Else
                    Set VisibleRows = Union(VisibleRows, cell)
                End If
            End If
        Next
        If Not VisibleRows Is Nothing Then VisibleRows.EntireRow.Hidden = False
    End With

    Application.Calculation = xlCalculationAutomatic
    Application.ScreenUpdating = False
    Application.EnableEvents = False
End Sub


Dockerfile of Vim + Plugins and .vimrc What follows is a Dockerfile for my Vim settings, so I could use it anywhere.
While it is public, it is not intended for mass usage, and so the face that it creates the password in the build process isn't a problem.
There is no COMMAND at the end because it is intended to be run with mount. The command I run it with docker run -itv /:/tmp/real_root uberhumus/vim-anywhere:latest vi <Filename>
Was I supposed to COPY the .vimrc later? Or use ADD instead? Am I too splurgy with the layers? Is there a smarter way to do the RUN, WORKDIR, RUN situations I ran into?
Thanks!

ENV DEBIAN_FRONTEND=noninteractive
ARG TZ=Asia/Jerusalem
RUN apt-get update; \
    apt-get dist-upgrade -y && \
    apt-get autoremove -y && \
    apt-get autoclean -y && \
    apt-get install -y vim tmux bash-completion shellcheck ssh git pylint flake8 python sudo expect curl cmake build-essential python3-dev golang npm openjdk-11-jre exuberant-ctags && \
    echo vim-anywhere > /etc/hostname

ARG USERNAME=yotam
RUN PASSWORD=$(openssl rand -base64 16 | tr -d "=") && \
    useradd -m -s /bin/bash $USERNAME && \
    usermod -aG sudo $USERNAME && \
    echo "$USERNAME:$PASSWORD" | chpasswd && \
    echo $PASSWORD

WORKDIR /home/$USERNAME
COPY .vimrc .
RUN mkdir -p ./.vim/pack/plugins/start
WORKDIR /home/$USERNAME/.vim/pack/plugins/start
RUN git clone https://github.com/tpope/vim-fugitive.git/ && \
    git clone --depth 1 https://github.com/dense-analysis/ale.git && \
    git clone https://github.com/ycm-core/YouCompleteMe.git
WORKDIR /home/$USERNAME/.vim/pack/plugins/start/YouCompleteMe
RUN git submodule update --init --recursive && \
    python3 install.py --all && \
    chown -R $USERNAME:$USERNAME /home/$USERNAME
USER $USERNAME```

The best I've got is:
FROM ubuntu:20.04

ENV DEBIAN_FRONTEND=noninteractive
ARG TZ=Asia/Jerusalem
ARG USERNAME=yotam
RUN apt-get update; \
    apt-get dist-upgrade -y && \
    apt-get autoremove -y && \
    apt-get autoclean -y && \
    apt-get install -y vim tmux bash-completion shellcheck ssh git pylint flake8 python sudo expect curl cmake build-essential python3-dev golang npm openjdk-11-jre exuberant-ctags && \
    useradd -m -s /bin/bash $USERNAME && \
    usermod -aG sudo $USERNAME && \
    echo vim-anywhere > /etc/hostname

WORKDIR /home/$USERNAME
COPY .vimrc .
RUN mkdir -p ./.vim/pack/plugins/start
WORKDIR /home/$USERNAME/.vim/pack/plugins/start
RUN git clone https://github.com/tpope/vim-fugitive.git/ && \
    git clone --depth 1 https://github.com/dense-analysis/ale.git && \
    git clone https://github.com/ycm-core/YouCompleteMe.git
WORKDIR /home/$USERNAME/.vim/pack/plugins/start/YouCompleteMe
RUN git submodule update --init --recursive && \
    python3 install.py --all && \
    chown -R $USERNAME:$USERNAME /home/$USERNAME && \
    PASSWORD=$(openssl rand -base64 16 | tr -d "=") && \
    echo "$USERNAME:$PASSWORD" | chpasswd && \
    echo $PASSWORD
WORKDIR /home/$USERNAME
USER $USERNAME

This gives me only minor advantages which are:

The password is echoed at the end of the build and is easier to find in the output.
The return to the homedir at the end is more what you'd intuitively expect.


Showing object's data on GUI and HTML without accessors and mutable objects I have been searching for a solution to show data of class with three assumptions:

Class Employee is not responsible for showing itself
Class Employee does not expose its data with getters
Classes responsible for showing data are immutable

So I created Employee class with its View interface:

public final class Employee {

  private String name;
  private String salary;

  public interface View {
    View addData(String name, String value);
    void show();
  }

  public void showOn(View view) {
    view.addData("Name", name.toString())
        .addData("Salary", salary.toString())
        .show();
  }
}

And two classes to show it to user. HTML:

final class HtmlPage implements Employee.View {
  private final String content;

  public HtmlPage() {
    content = "";
  }

  public HtmlPage(String str) {
    content = str;
  }

  @Override
  public Employee.View addData(String name, String value) {
    String newData = "\t
";
    newData += name;
    newData += "	";
    newData += value;
    newData += "
\n";

    HtmlPage updated = new HtmlPage(content + newData);
    return updated;
  }

  @Override
  public void show() {
    System.out.println("
\n" + content + "
");
  }
}

and GUI:

import javax.swing.*;
import java.awt.*;
class OutputWindow implements Employee.View {

  private final JComponent panel;

  public OutputWindow() {
    this.panel = new JPanel();
  }

  public OutputWindow(JComponent panel) {
    this.panel = panel;
  }

  @Override
  public Employee.View addData(String name, String value) {
    panel.add(new JLabel(name));
    panel.add(new JLabel(value));

    return this;
  }

  @Override
  public void show() {
    JFrame outputFrame = new JFrame("Okienko do wypisania danych");
    outputFrame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    outputFrame.getContentPane().add(this.getJComponent());
    outputFrame.pack();
    outputFrame.show();
  }

  JComponent getJComponent() {
    panel.setLayout(new GridLayout(3, 2));
    return panel;
  }
}

Here is the usage:

public static void main(String[] args) {
    Employee Piotr = new Employee();

    OutputWindow outputWindow = new OutputWindow();
    Piotr.showOn(outputWindow);

    Employee Ewelina = new Employee();

    HtmlPage htmlPage = new HtmlPage();
    Ewelina.showOn(htmlPage);
}

Your outset is wrong. The articles you've linked to present a nice story, but what you're running into here is the issue with that ideology:

When your data structures AND your views are rigid, you can't change the system to do anything else.

You're now stuck!

Say I had to show a listing of Employees. I can't! There is no way to use the data stored in Employee to make a screen that contains a listing of employees. I also cannot make a REST API with this Employee object without some serious magic - I'd have to make a "View" which takes multiple Employees and ignores calls to "show" unless they're from an authorized source.

You say "Class Employee is not responsible for showing itself".

public final class Employee {

  //...
  public void showOn(View view) {
    view.addData("Name", name.toString())
        .addData("Salary", salary.toString())
        .show();
  }
}

What's the show() doing here? Why does the Employee decide when the view is ready? Maybe I have a page consisting of a Project and the Employees that work on it. I'd have to make custom subviews.

You're right in that the Employee does not decide HOW it is shown, but it does decide WHAT is shown and WHEN it is shown. Which is not a desired property.

That, or I'd have to alter Employee to add View logic to support multiple or partial employee views. Which is bad, because standards constantly change, and you don't want to alter all your model classes to be able to give a view that can be used for that standard (e.g. something that you can convert to JSON).

Additionally, by limiting yourself to those constraints, you can no longer leverage certain libraries! It's fun and all to harp on the gigantic projects and code cruft that comes with using big libraries like Hibernate and Spring and such, but if you need to build a REST API and a phone app, being able to have libraries handle your object conversion to json and back (and in case of android, maybe even SHARE the model objects as a library!)... you can't do it in your setup! There are no getters, objects are immutable, libraries don't support this.

Employee.View is oddly specific class; it doesn't make any mention of Employee:

  public interface View {
    View addData(String name, String value);
    void show();
  }

Let's say we had projects later - will there also be Project.View? What methods will it contain? The same? Mostly the same?

This leads to code duplication, which is really bad!

In your HTML page you use a table, and for that, String name, String value will do. But for non table situations, like a stackexchange profile, you have graphs for reputation, lists of answers and questions, it's partially sortable, some are numbers, others are text, some of the numbers are rounded (reputation is shown as ##.#k, rather than a 5 digit amount)...

All of that is either HARD or NOT POSSIBLE just because you're following the dogma of "You should never have Getters/Setters near your code.".

Such an idea is DANGEROUS in this case!

But you link to more than just 1 article of this guy, so maybe you really like what he says and I won't get through to you by just pointing at your code. Let's point at his:

Dog dog = new Dog();
dog.setBall(new Ball());

You could say this can be improved; Dog can have 0 to many objects to play with. Maybe it's carrying up to one object at a time. You could either use setCarriedObject (but that's secretly setBall), or, perhaps better, take(PlayObject). I call it PlayObject but basically anything that you could give to the dog to carry around would work. We're not using a very strict object hierarchy here.

Then, of course, if we're gonna play fetch, we'd have to retrieve the ball.

Dog dog = new Dog();
Ball ball = dog.getBall();

Because we've rephrased setBall to take (the dog takes the ball... because you've ordered him to), getBall is obviously going to become give.

But they're still getters and setters, we just call them differently now. They do the same.

The author comes to the same conclusion:

Dog dog = new Dog();
dog.take(new Ball());
Ball ball = dog.give();

So far, I get the point; getters and setters show that you might not have thought about the usecases and are treating your objects as dumb things, and maybe you want to emphasize that there's actual work being done.

But then the author says this:

  Now, we're treating the dog as a real animal, who can take a ball from us and can give it back, when we ask. Worth mentioning is that the dog can't give NULL back. Dogs simply don't know what NULL is :) Object thinking immediately eliminates NULL references from your code.

How?!

If I do this:

Ball ballInMyHand = new Ball();
Dog dog = new Dog();
Ball otherBall = dog.give();
dog.take(ballInMyHand);

What's otherBall gonna be? The author says it's not going to be null. In that case, the options we've got left are these:

Null-representative value; the dog gives you "air" or a NonExistantBall
A new ball; the dog steals one from the neighborhood
The ball that was previously in my hand; the dog knows where all the balls are and can use reflection to obtain any ball
NoBallException; the dog goes mad and dies, possibly killing you (The program)
Infinite loop; the dog searches for a ball forever, and we're relying on the inherent multi-threaded way that the world works for a person to tell the dog to stop searching (but this doesn't happen, so you're stuck forever)
Program exits with fault code; the universe stops existing

And, save for the "air" option, they're all really bad choices! Exceptions for regular cases, infinite loops, program exits... what's so bad about null?

Back to Employees for a moment; say I want to generate a report that tells me all the costs I spend on Employees per month. That's salary, compensations, bonuses and all that. I sure hope there's an alternative data source out there, because the only way I'm gonna get to that data is by pretending I'm going to display it. I guess adding up all the salaries is gonna be done by casting String to int again.

It's also weird that I have to delete the old employee if I want to give them a raise.

You also have the issue that your views are stringly typed: if you wanted to prominently display Job title at the top, you'd have to filter via if(name.equals("Job title")). And if we ever change it to Job Title or Title or Role or Occupation then guess what, we're going to have to go back and change everything again! IDE's may or may not support this refactoring, so chances are you'd have to do all this by hand.

To summarize - what you've done presents multiple problems:

Views and Model are tightly coupled
New views require model alteration (violation of open-closed and SRP)
Less library support means more manual work
View class is likely to be duplicated
Model giving data rather than providing data means views are limited in what they can do

and I see no real benefits.

But lets say your boss says "I've read this article, and this stuff is amazing! You programmers should work to build our apps like this. Now, I've already discussed this with the managers and we're going with this, so if you don't like it, there's the door." There's no way you're going to convince your boss, he has decided that this way is the way. And you need your job, whilst quitting does sound nice, you need the money, and if you don't work you don't get paid.

What do you do?

In that case, take immutability up another level.

Instead of treating your objects like forms to be filled in and stored, like HtmlPage, create printers of a sort.

Employee goes in, EmployeeReport comes out. If you want a different report, either feed the Employee into a different ReportMaker, give the ReportMaker different settings, or pass more Employees into ReportMaker.

Pure functional programming like that exists, and whilst Java isn't the best language for it, Java 8 made things a lot better. You don't have to carry state around all the time. Once you have the report you need, you can drop all instances of objects and just request them new later.


CMS for allowing users to delete or edit a post I have been working on a personal CMS for a site I've been running, and right now I'm working on a page that lists all entries, or posts from a database. I have given the user, with checkboxes and a drop down menu, the option to either delete or edit a post.

    //query database
    $query = mysql_query("SELECT * FROM userPosts");

    //turn query results into an array
    while($rows = mysql_fetch_array($query)) :

    //List all posts in a table (other table markup not shown).
    $posts = $rows['userTexts'];
    $id = $rows['ID'];
    echo "
\n" . $id . "	\n" . $posts . "	\n<input type='checkbox' name='chkBox' value='chk[". $id . "]'>	\n
\n";
endwhile

This part is not the problem, but I figure it is necessary for the question. Is there a way to make this code cleaner, without so many if statements?

$chkWhat = $_REQUEST["chkBox"];
$options = $_REQUEST["options"];

if(isset($_REQUEST["submit"])) {
    if($chkWhat != "chk[" .$id . "]") {
        echo "
Please select a post.
";
    } else {
        if( //more than one check box is checked but I still have to figure this...) {
            echo "
For now, please just select 1 post you would like to edit.
";
        } else {
            //edit your post
            echo "You will edit your post";
        }
        if($options == "delete") {
            //delete your post(s) here
            echo "You will delete your post(s)";
        }
    }
}

Sorry about the second if statement there, but I am still researching that.

Usually you can use switch statements to clean up abundant if statements, but only if those if statements are similar; Such as checking for different values of the same variable. Since that is not the case here, there isn't much you can change in this area. Sometimes you just need a few if statements. I would, however, suggest that you not use the global $_REQUEST as it is rather insecure. Even if you think you are the only one who is ever going to use it, what if someone got in, you just removed one more step of security from your site making it that much more easier for someone to destroy your hard work. Even you could make a mistake later by accidentally duplicating a POST variable as a GET variable, or vice-versa. Or someone with malicious intent does it on purpose, you could run into a lot of problems there. You should also sanitize those. Check out filter_input (>=PHP 5.2) to get started, or check other forums on the best way to sanitize your form variables.

Your delima with checking for multiple checkboxes should be rather easy. You've already got your form set up to make them into an array after post. Or almost right. You need to change name to what you have for your value. That should work, but if it doesn't try making the value the $id and just leaving the name chk[], should probably do that anyways as it won't leave empty or useless values. After that just check the count.

$checkboxes = $_POST['chk[]'];
if(count($checkboxes) > 1) { etc... }

There is a minor change you should make to your if statements. Since you are checking $options for a "delete" value, you should also check it for an "edit" value, or move that if statement into the else block and give it an else for the edit block. Otherwise, as you can imagine, you'll end up editing and then deleting your posts and that would take up unneeded resources.

if( //more than one check box is checked but I still have to figure this...) {
    echo "
For now, please just select 1 post you would like to edit.
";
} else {
    if($options == "delete") { /*delete your post*/ }
    else { /*edit your post*/ }
}

I don't see anything else really. So good luck!


Subclass of AtomicReference with atomic function application Update: Now that https://github.com/bionicspirit/scala-atomic is available, I have less of a need for this.

Am I doing anything bad? I wrote this class to allow modifying an atomic reference with the assurance that two threads modifying it will not step on each other's toes. To illustrate:

val x = new AtomicRef(List.empty[String])
def compute(s: String) = { Thread.sleep(1000); s }
def updateX(s: String) = x.transform(strings => compute(s) :: strings)

The idea is to guarantee that if two threads call updateX many times, all the strings passed to updateX will wind up in x.

run is like transform but it lets you return a value to the caller other than the value you're setting into the reference.

Here is the class:

/**
 * A subclass of java.util.concurrent.atomic.AtomicReference[A]
 * that adds the ability to atomically modify its value relative to itself.
 */
class AtomicRef[A](init: A) extends java.util.concurrent.atomic.AtomicReference[A](init) {
  /**
   * Update the value atomically by applying a function to it.
   * Uses `compareAndSet` and will keep trying until it succeeds.
   */
 final def transform(f: A => A): A = run { a =>
    val ret = f(a)
    (ret, ret)
 }
  /**
   * Update the value atomically by applying a computation to it.
   * The computation returns a pair of values, of which the
   * `AtomicRef` is updated to the first.
   * Uses `compareAndSet` and will keep trying until it succeeds.
   * @return the second value of the computation
   */
  @annotation.tailrec final def run[B](f: A => (A, B)): B = {
    val a = get
    val (a2, b) = f(a)
    if(compareAndSet(a, a2)) b
    else {
      Thread.sleep(10)
      run(f)
    }
  }
}

To put it in different terms, this sort of abstracts over doing something like this (in Java; List here is a fictional immutable list):

AtomicReference<List<String>> ref = new AtomicReference<List<String>>(new List());
void update(String s) {
  while(true) {
    List<String> a = ref.get;
    // now do "f(a)"
    Thread.sleep(1000); // pretend it takes a long time to run the computation
    List<String> b = a.prepend(s);
    if(compareAndSet(a, b)) return b;
    else {
      Thread.sleep(10);
      // loop the while instead of tail recursion
    }
  }
}

Your approach is definitely valid, I've seen something similar used in GHC's implementation of atomicModifyIORef.

However I have some doubts if such a class is really useful. If the computations are potentially long running, the performance advantage of using AtomicReference instead of some standard synchronization primitive is negligible. Moreover, if there are several updating threads, it will result in repeated recomputation, wasting CPU power on it. So using plain synchronized on some private lock object (so that no other part of code can lock on it) seems simpler, safer and with better performance.


Passing C++ objects by reference for participants in a simulated battle I just started watching C++ videos couple of days ago. I tried taking everything I learned about classes and passing by references vs values. The main question I what would like to know is this the correct way to change the values of private class members or is there a more 'professional' way of doing it. (Or standard).

Entity.cpp:

void Entity::playerAttack(Entity &Player, Entity &Enemy) {
    cout << "\nPlayer is Attacking!" << endl;
    cout << "Enemy's health was at: " << Enemy.getHealth() << endl;
    totalHealth = Enemy.getHealth() - Player.getDamage();
    Enemy.setHealth(totalHealth);

    if (Enemy.getHealth() <= 0) {
        Enemy.setHealth(0);
        cout << "\nEnemies is 0 or below" << endl;
        cout << "Players health is at: " << Player.getHealth() << endl;
    }
    else {
        cout << "Enemies health is now at: " << Enemy.getHealth() << endl;
    }
}

void Entity::setHealth(float x) {
    health = x;
}

void Entity::setDamage(float x) {
    damage = x;
}

float Entity::getDamage() {
    return (damage);
}

float Entity::getHealth() {
    return (health);
}

Entity.h:

#pragma once
class Entity
{
public:
    void setHealth(float x);
    void setDamage(float x);
    void playerAttack(Entity &Player, Entity &Enemy);
    float getHealth();
    float getDamage();

    Entity();
    ~Entity();

private:
    float health;
    float damage;
    float healAbility;
    float totalHealth;
};

source.cpp:

Entity constructE(float damage, float health, float regen) {
    Entity Entity;
    Entity.setDamage(damage);
    Entity.setHealth(health);
    Entity.setRegen(regen);

    return (Entity);
}

int main() {
    Entity Player = constructE(10, 100, 10);
    Entity Monster = constructE(5, 20, 5);
    Player.playerAttack(Player, Monster);
}

I have a couple of issues with how you are doing this (it is very Java like not C++ like).

Constructors

After the constructor finishes your object should be in a valid consistent state. Having to call multiple "setters" to finish initialization is a big red flag.

Entity constructE(float damage, float health, float regen) {
    Entity Entity;
    Entity.setDamage(damage);
    Entity.setHealth(health);
    Entity.setRegen(regen);

    return (Entity);
}

Why not just have a "constructor" that takes these arguments.

Entity Player = constructE(10, 100, 10);

// Should look like:
Entity Player(10, 100, 10);

Passing an object to itself?

Player.playerAttack(Player, Monster);

You are passing player as an argument? As you are calling a method on the object "player" you already have access to the player (via this). So there is no need to pass the player here.

player.attack(monster);

Identifier conventions.

One thing I notice about your code is that everything starts with a capitol letter. This makes it hard to tell the difference between types and objects. One of the most important things in C++ is type.

So the usual convention is that "User Defined Types" have an initial capitol letter. While objects (variables) have an initial lower case letter.

player.attack(monster);
^ Object      ^ Object

While:

Player.attack(monster);
|             ^ object
^ Looks like a type.
  So attack looks likes a static member function (ie a class method).

Getters/Setters

Getters and Setters break encapsulation. They expose the internal implementation details of the class. If you see the pattern

 auto x = object.getValue();
 auto y = performSomeAction(x);
 object.setValue(y);

Then this is usually an indication that you have not designed your class well as you expose internal details to calculate a value. A better design would be to provide a method that performs an ACTION on the class.

 object.someAction();   // internally it can update its own state.

Re-Design

class Entity
{
public:
     Entity(int damage, int helath, int regen)
         : damage(damage)
         , health(health)
         , healAbility(regen)
    {}
    // Destructor not needed.

    // Check if an entity is alive.
    operator bool() const
    {
        return health > 0;
    }
private:
    // Internal use only by attack()
    int weaponDamage() const
    {
        return myRandomNumber() % 10;  // Damage in the range 0..9
    }

    // Take damage
    Entity& takeDamage(int amount)
    {
        // Can not go below zero.
        health = max(0, health - amount);
        return *this;
    }
public:

    // Attack an Enemy
    void attack(Entity &enemy)
    {
        std::cout << "Player:     " << *this
                  << "Attacking:  " << enemy;

        // Damage the enemy and check if he is alive in one line.
        if (enemy.takeDamage(weaponDamage()) {
            // Enemy Alive
            cout << "E. Status: " << enemy;
        }
        else {
            // Enemy Dead
            cout << "Enemies is Dead" << "\n";
            cout << "P: Status: " << *this;
        }
    }

    virtual std::ostream& print(std::ostream& str) const
    {
        return str << "health: "       << health
                   << " damage: "      << damage
                   << " healAbility: " << healAbility
                   << "\n";
    }
    friend std::ostream& operator<<(std::ostream& str, Entity const& data)
    {
        return data.print(str);
    }
private:

private:
    float health;
    float damage;
    float healAbility;
};


Create index-less array product Source: careercup.com

  You are given an array of integers(with all valid input) You have to
  write a function which will produce another array, where the value in
  each index of the array will be the product of all values in the given
  array exccept that index.

  Example

  Array 1: 1 2 3 4 5

  Array 2: 120 60 40 30 24

  Come up with a solution of \$O(n^2)\$ can you improve it?

I would like to get code review comments for my code:

void createIndexlessArrayProduct()
{
   int arr1[]= {1,2,3,4,5};
   int arr2[5];

   arr2[0] = 1;

   for(auto& val : arr1)
   {
       arr2[0] *= val;
   }

   for(int i = 1; i < 5; ++i)
   {
       arr2[i] = arr2[0] / (i+1);
   }

   for( auto& val : arr2)
   {
      std::cout << val << "\n";
   }
}

Isn't it quicker to compute the full product in linear time and then divide by each N?

EDIT: bugfix - (O3N) time with zero check:

#include <numeric>
#include <array>
#include <vector>
#include <iostream>

namespace notstd {

    template<class T>
    auto to_vector(std::initializer_list<T> il) {
        return std::vector<T>(il);
    }
}

static constexpr struct {
    template<class Iter>
    void operator()(Iter first, Iter last) const {
        if (first != last) {
            auto zeros = std::count(first, last, 0);
            switch (zeros) {
                case 0: {
                    auto accum = std::accumulate(std::next(first), last,
                                                 *first, std::multiplies<>());
                    std::transform(first, last,
                                   first,
                                   [accum](auto &&v) {
                                       return accum / v;
                                   });
                }
                    break;

                case 1: {
                    auto maybe_multiply = [](auto &&x, auto &&y) { return y == 0 ? x : x * y; };
                    auto accum = std::accumulate(first, last,
                                                 1, maybe_multiply);
                    std::transform(first, last,
                                   first,
                                   [accum](auto &&v) {
                                       if (v == 0)
                                           return accum;
                                       else
                                           return 0;
                                   });
                }
                    break;

                default: {
                    std::fill(first, last, 0);
                } break;
            }

        }
    }
} other_products {};

template<class Container, class Algo>
auto mutate_copy(Container c, Algo algo) {
    algo(std::begin(c), std::end(c));
    return c;
};

template<class T, class Algo>
auto mutate_copy(std::initializer_list<T> il, Algo &&algo) {
    return mutate_copy(notstd::to_vector(il),
                       std::forward<Algo>(algo));
};

template<class T, std::size_t N, class Algo>
auto mutate_copy(std::array<T, N> const &a, Algo &&algo) {
    return mutate_copy(std::vector<T>(a.begin(), a.end()),
                       std::forward<Algo>(algo));
};

template<class Container, class Algo>
auto &mutate_inplace(Container &c, Algo algo) {
    algo(std::begin(c), std::end(c));
    return c;
};

template<class Container>
std::ostream &emit(std::ostream &os, Container &&c) {
    auto impl = [&os](auto first, auto last) {
        using value_type = typename std::iterator_traits<decltype(first)>::value_type;
        std::copy(first, last,
                  std::ostream_iterator<value_type>(os, ", "));
    };
    impl(std::begin(c), std::end(c));
    return os;
}

int main() {

    emit(std::cout, mutate_copy({1, 2, 0, 4, 5}, other_products)) << std::endl;
    emit(std::cout, mutate_copy({2, 3, 4, 5, 6}, other_products)) << std::endl;
    emit(std::cout, mutate_copy({6, 5, 4, 3, 2}, other_products)) << std::endl;

    std::array<int, 5> ar{10, 20, 30, 40, 50};
    emit(std::cout, mutate_copy(ar, other_products)) << std::endl;
    emit(std::cout, mutate_inplace(ar, other_products)) << std::endl;

    auto in = std::vector<int> {10, 11, 12, 13, 14};
    emit(std::cout, mutate_copy(in, other_products)) << std::endl;

    std::vector<int> foo{};
    emit(std::cout, mutate_inplace(foo, other_products)) << std::endl;
}

previous code:

I believe this would give O(2N) time.

#include <numeric>
#include <vector>
#include <iostream>
#include <algorithm>
#include <functional>
#include <iterator>

template<class Iter>
auto other_products(Iter first, Iter last)
{
    using type = typename std::iterator_traits<Iter>::value_type;
    auto mega_product = std::accumulate(first, last,
                                        type(1), std::multiplies<>());

    std::vector<type> result;
    result.reserve(std::distance(first, last));
    std::transform(first, last,
                   std::back_inserter(result),
                   [mega_product](auto&& v)
                   {
                       return mega_product / v;
                   });
    return result;
}

//
// convenience specialisation for any container
//
template<class Container>
auto other_products(Container&& c)
{
    return other_products(std::begin(c), std::end(c));
}

template<class Value>
auto other_products(std::initializer_list<Value> c)
{
    return other_products(std::begin(c), std::end(c));
}

template<class Container>
std::ostream& emit(std::ostream& os, Container&& c)
{
    auto impl = [&os](auto first, auto last)
    {
        using value_type = typename std::iterator_traits<decltype(first)>::value_type;
        std::copy(first, last,
                  std::ostream_iterator<value_type>(os, ", "));
    };
    impl(std::begin(c), std::end(c));
    return os;
}

int main()
{

    emit(std::cout, other_products({1, 2, 3, 4, 5})) << std::endl;
    emit(std::cout, other_products({2, 3, 4, 5, 6})) << std::endl;
    emit(std::cout, other_products({6, 5, 4, 3, 2})) << std::endl;

    auto in = std::vector<int> { 10, 11, 12, 13, 14 };
    emit(std::cout, other_products(in)) << std::endl;

}

As a further refinement, I had a go are separating the concerns of 'in-place' or 'copy' operations from the actual mutating algorithm.

See what you think:

#include <numeric>
#include <array>
#include <vector>
#include <iostream>
#include <algorithm>
#include <functional>
#include <iterator>
#include <initializer_list>

namespace notstd {

    template<class T>
    auto to_vector(std::initializer_list<T> il)
    {
        return std::vector<T>(il);
    }
}

static constexpr struct
{
    template<class Iter>
    void operator()(Iter first, Iter last) const
    {
        if (first != last) {
            auto accum = std::accumulate(std::next(first), last,
                                         *first, std::multiplies<>());
            std::transform(first, last,
                           first,
                           [accum](auto&& v)
                           {
                               return accum / v;
                           });
        }
    }
} other_products {};

template<class Container, class Algo>
auto mutate_copy(Container c, Algo algo)
{
    algo(std::begin(c), std::end(c));
    return c;
};

template<class T, class Algo>
auto mutate_copy(std::initializer_list<T> il, Algo&& algo)
{
    return mutate_copy(notstd::to_vector(il),
                       std::forward<Algo>(algo));
};

template<class T, std::size_t N, class Algo>
auto mutate_copy(std::array<T, N> const& a, Algo&& algo)
{
    return mutate_copy(std::vector<T>(a.begin(), a.end()),
                       std::forward<Algo>(algo));
};

template<class Container, class Algo>
auto& mutate_inplace(Container& c, Algo algo)
{
    algo(std::begin(c), std::end(c));
    return c;
};

template<class Container>
std::ostream& emit(std::ostream& os, Container&& c)
{
    auto impl = [&os](auto first, auto last)
    {
        using value_type = typename std::iterator_traits<decltype(first)>::value_type;
        std::copy(first, last,
                  std::ostream_iterator<value_type>(os, ", "));
    };
    impl(std::begin(c), std::end(c));
    return os;
}

int main()
{

    emit(std::cout, mutate_copy({1, 2, 3, 4, 5}, other_products)) << std::endl;
    emit(std::cout, mutate_copy({2, 3, 4, 5, 6}, other_products)) << std::endl;
    emit(std::cout, mutate_copy({6, 5, 4, 3, 2}, other_products)) << std::endl;

    std::array<int, 5> ar { 10, 20, 30 , 40, 50 };
    emit(std::cout, mutate_copy(ar, other_products)) << std::endl;
    emit(std::cout, mutate_inplace(ar, other_products)) << std::endl;

    auto in = std::vector<int> {10, 11, 12, 13, 14};
    emit(std::cout, mutate_copy(in, other_products)) << std::endl;
}


Next node of a node in Binary search tree To find the node next to a given node we need to consider two cases.
case 1: if the node has a right subtree then return the min node in its right subtree.
case 2: if the node doesn't have a right subtree then return the ancestor of the node whose left subtree contains the node.

class NodeWithParent {
    int data;
    NodeWithParent left;
    NodeWithParent right;
    NodeWithParent parent;

    NodeWithParent(int x) {
        data = x;
    }
}

public class nextNodeOfANodeInBST {

    public static NodeWithParent getMin(NodeWithParent root) {
        while (root.left != null) {
            root = root.left;
        }
        ;
        return root;
    }

    public static NodeWithParent nextNode(NodeWithParent curNode) {
        if (curNode == null) {
            return null;
        }
        if (curNode.right != null) {
            return getMin(curNode.right);
        } else if (curNode.data > curNode.parent.data) // its the right child
        {
            // find an ancestor for which the curNode is in the left subtree
            while (curNode.parent != null && curNode.data > curNode.parent.data) {
                curNode = curNode.parent;
            }
            if (curNode.parent != null && curNode.parent.data < curNode.data)
                return curNode;
            else
                return curNode.parent;

        } else
            return curNode.parent;
    }

    public static void main(String[] args) {
        // TODO Auto-generated method stub
        NodeWithParent root = new NodeWithParent(15);
        root.parent = null;
        root.left = new NodeWithParent(10);
        root.left.parent = root;
        root.right = new NodeWithParent(20);
        root.right.parent = root;
        root.left.left = new NodeWithParent(8);
        root.left.left.parent = root.left;
        root.right.right = new NodeWithParent(25);
        root.right.right.parent = root.right;
        root.right.left = new NodeWithParent(17);
        root.right.left.parent = root.right;
        root.left.right = new NodeWithParent(12);
        root.left.right.parent = root.left;
        root.left.left.left = new NodeWithParent(6);
        root.left.left.left.parent = root.left.left;
        root.left.right.left = new NodeWithParent(11);
        root.left.right.left.parent = root.left.right;
        root.right.left.left = new NodeWithParent(16);
        root.right.left.left.parent = root.right.left;
        root.right.right.right = new NodeWithParent(27);
        root.right.right.right.parent = root.right.right;
        NodeWithParent nextNode = nextNode(root.right.left);
        if (nextNode != null)
            System.out.println(nextNode.data);
        else
            System.out.println("No next node for " + root.right.left.data);
    }
}

1

NodeWithParent does not sound funky to me: why not try TreeNode.

2

You can drastically simplify (and rename) the nextNode method:

public static final TreeNode getSuccessorNode(TreeNode node) {
    if (node == null) {
        return null;
    }

    if (node.right != null) {
        return getMinimumNode(node.right);
    }

    TreeNode parent = node.parent;

    while (parent != null && parent.right == node) {
        node = parent;
        parent = parent.parent;
    }

    return parent;
}

3

Also, it is conventional to start a class name with a capital letter (NextNodeOfANodeInBST). Actually, a better name for you class may be something as BSTUtils or, say, BinarySearchTreeUtils.

4

In your main you have root.parent = null; You don't have to: by default all reference fields in a class are initialized with null.

5

In case your class contains only static methods, it's customary to declare the constructor of such a class as private: it makes no sense to instantiate an object that does nothing in its own right.

Hope that helps.


A simple assembly program to count the number of ones in a register I made a simple assembly program to count the number of ones in a register. Since this is basically my first ever, I would like to hear what I can improve or if there are some major flaws in this one:

#PURPOSE:  Simple program which counts the number of ones in the edx register and returns them as a status code

#INPUT:    none

#OUTPUT:   Returns the number of ones as a status code.  This can be viewed
#          by typing
#
#          echo $?
#
#          after running the program

#VARIABLES:
#          %eax holds the carry flag bits and eventually the status code
#          %ebx holds the final return value
#          %ecx stores the loop index
#      %edx stores the register whose digits are examined
#      %edi accumulates the sum of ones

.section .data

.section .text
.globl _start
_start:

movl $255, %edx         # Initialize edx to some value
movl $0, %ecx       # Initialize ecx to zero
movl $0, %edi       # Initialize edi to zero
movl $0, %eax       # Initialize eax to zero

start_loop:
    cmpl $32, %ecx      # Loop runs over all 32 bits of the register
    jge loop_exit
    inc %ecx        # Increase the index variable
    shrl $1, %edx       # Right shift edx so the LSB moves into the carry-flag
    pushf           # Get the flags register
    pop %ax         # Put the least 16 bit of the flags into ax register
    and $01, %ax        # ... but store only the LSB
    add %ax, %di        # Now add the carry bit to the result
    jmp start_loop

loop_exit:
    movl $1, %eax       # We dont need eax any more at this point so we can use it for the syscall
    movl %edi, %ebx     # Return value will be edi
    int $0x80

For some more advanced ideas, check out https://stackoverflow.com/a/109025/2189500.

But expanding on CWallach's code, how about:

mov edx, 255 ; setup edx with test value
xor eax, eax ; count

start_loop:
bt edx, 0 ; sets carry flag if bit 0 is set
adc al, 0 ; add with carry
shr edx, 1 ; drop off bit
jnz start_loop ; exit if edx is now 0

xor is a smaller instruction for setting a register to zero.
bt sets the carry flag for use in
adc adds 0 to al, but also includes the contents of the carry flag.

And once you've got your head wrapped around that, how about:

mov edx, 255 ; setup edx with test value
xor eax, eax ; count

shr edx, 1 ; move rightmost bit to carry flag
start_loop:
adc al, 0 ; add with carry
shr edx, 1 ; move rightmost bit to carry flag
jnz start_loop ; exit if edx is now 0
adc al, 0 ; count last bit

This takes advantage of the fact that when shr shifts, it moves the shifted bit into the carry flag.  And while there are more instructions overall (5 vs 6), there are fewer within the loop (4 vs 3).


Calculate fibonacci in O(log n) This program calculates the \$n\$th fibonacci number, in \$O(\log n)\$ time. I'm looking for code review, optimizations, and best practices.

public final class Fibo {

    private Fibo() { }

    public static int getNthfibo(int n) {
        if (n < 0) {
            throw new IllegalArgumentException("The fibo value cannot be negative");
        }

        if (n <= 1) return n;

        int[][] result = {{1, 0}, {0, 1}}; // identity matrix.
        int[][] fiboM = {{1, 1}, {1, 0}};

        while (n > 0) {
            if (n%2 == 1) {
                multMatrix(result, fiboM);
            }
            n = n / 2;
            multMatrix(fiboM, fiboM);
        }

        return result[1][0];
    }

    private static void multMatrix(int[][] m, int [][] n) {
        int a = m[0][0] * n[0][0] +  m[0][1] * n[1][0];
        int b = m[0][0] * n[0][1] +  m[0][1] * n[1][1];
        int c = m[1][0] * n[0][0] +  m[1][1] * n[0][1];
        int d = m[1][0] * n[0][1] +  m[1][1] * n[1][1];

        m[0][0] = a;
        m[0][1] = b;
        m[1][0] = c;
        m[1][1] = d;
    }

    public static void main(String[] args) {
        for (int i = 0; i < 10; i++) {
            System.out.println(getNthfibo(i));
        }
    }
}

Your code seems to be working properly so there is not much to say on this.

The only changes I would perform would be to make things easier to understand through different little steps.

Multiplication could return a result

At the moment, your multiplication procedure stores the result in the first argument. First issue is that it cannot be generalised to product of non-square matrices. The second issue is that it can make things a bit hard to understand when looking at the code using your function (getNthfibo in your case). It is a fairly easy change to perform :

private static int[][] multMatrix(int[][] m, int [][] n) {
    int a = m[0][0] * n[0][0] +  m[0][1] * n[1][0];
    int b = m[0][0] * n[0][1] +  m[0][1] * n[1][1];
    int c = m[1][0] * n[0][0] +  m[1][1] * n[0][1];
    int d = m[1][0] * n[0][1] +  m[1][1] * n[1][1];
    int[][] ret = {
        {a, b},
        {c, d}};
    return ret;
}

and in your main function:

result = multMatrix(result, fiboM);` and `fiboM = multMatrix(fiboM, fiboM);

The signature of the multMatrix method lets us think that is works for any matrices.

Actually, it only works for 2*2 matrices. This should be documented and checked at runtime (because it doesn't seem to be possible to do so at compilation time).
Making Mathematics a bit more obvious

Values can be reformatted in such a way that the mathematics behind are easier to understand. Also, a little bit of comment can help.

    int[][] result = {
        {1, 0},
        {0, 1}};

    /*         n
     * [ 1 1 ]     [ F(n+1) F(n)   ]
     * [ 1 0 ]   = [ F(n)   F(n-1) ]
     */
    int[][] fiboM = {
        {1, 1},
        {1, 0}};


How to improve WCF client proxy based on CreateChannel (not generated from WSDL) in C# When invoking a Web Service from C# (where the external service might be WCF, ASMX, Java, etc. - you just don't know), it is common practice to point your modern development tool of choice (Visual Studio in my case) at the service's WSDL to generate a proxy class for calling the service. You can also create a proxy "by hand" using CreateChannel. This code review posting is to seek feedback on a simple proxy class built using CreateChannel. A couple of supporting classes are shown for completeness & context.

Interface for Service & Proxy:

This interface is also passsed to ChannelFactory.

public interface IService
{
   ServiceResponse DoSomething(ServiceRequest request);
}

Example usage:

var proxy = new ServiceProxy("http://123.45.67.89/foo");
var request = new ServiceRequest { Number = 33, Name = "Larry Bird" };
ServiceResponse response = proxy.DoSomething(request);

Code of interest:  - Any comments/improvements/feedback appreciated

For readers wondering about the use of factory.Abort in the finally block, see answer to this question. Regarding the following casting business (which appears twice): ((IClientChannel)service).Close(); see MSDN document on How To: Use the ChannelFactory which demonstrates its use. The two are hacks that appear to be related and date from early WCF/Indigo days.

public class ServiceProxy : IService
{
   public string EndpointUrl { get; set; }
   public ServiceProxy(string endpointUrl) { EndpointUrl = endpointUrl; }

   public ServiceResponse DoSomething(ServiceRequest request)
   {
      Contract.Requires(request != null);
      Contract.Requires(! String.IsNullOrEmpty(EndpointUrl));

      var endpointAddress = new EndpointAddress(EndpointUrl);
      ChannelFactory<IService> factory = null;
      IService service = null;

      try
      {
         // Just picking a simple binding here to focus on other aspects
         Binding binding = new BasicHttpBinding();
#if DEBUG // boost timeouts, otherwise can cause lots of timeouts in the debugger
         binding.SendTimeout = binding.ReceiveTimeout = TimeSpan.FromMinutes(10);
#endif
         factory = new ChannelFactory<IService>(binding);
         service = factory.CreateChannel(endpointAddress);
         if (service == null)
         {
             throw new CommunicationException(
                String.Format("Unable to connect to service at {0}", endpointUrl));
         }

         ServiceResponse response = service.DoSomething(request);
         ((IClientChannel)service).Close();

         service = null;
         factory.Close();
         factory = null;

         return response;
      }
      finally
      {
         if (service != null) ((IClientChannel)service).Close();
         if (factory != null) factory.Abort();
      }
   }
}

Supporting Objects (for completeness)

Not much of interest in ServiceRequest and ServiceResponse classes, other than this is the prevailing style of calling services - one object in, one object out.

public class ServiceRequest // stuff we send to the service
{
   public int Number { get; set; }
   public string Name { get; set; }
}

public class ServiceResponse // stuff we get back from the service
{
   public int SomeStat { get; set; }
   public string SomeString { get; set; }
}

Instead of wrapping the proxy creation in a concrete service proxy implementation, I'd write a concrete service factory instead which in turn uses the ChannelFactory. The service factory class could also use an interface for creation, so you can have multiple concrete factories.

http://en.wikipedia.org/wiki/Factory_pattern

That way your code can continue to depend on the interface only, and you can change the way the proxy is created without writing concrete proxies. You could also have a "local service factory" which doesn't go over the wire.

Usage would be something like this instead:

var service = ServiceFactory.Create("http://127.0.0.1/svc");
var result = service.DoSomething(request);
// if you need cleanup..
ServiceFactory.Release(service);


Is coding to an interface with regards to PDO the right thing to do? Am I on the right lines for connecting to my Database? If not could somebody help me improve it or point me in the right direction?

Index.php:

use Acme\Foo\Database\Connector;
use Acme\Foo\Database\MySQLConnector;

$obj = new Connector(new MySQLConnector);
echo $obj->db->connect();

DatabaseInterface.php:

interface DatabaseInterface {
    public function connect();
}

MySQLConnector.php:

use Acme\Foo\Database\DatabaseInterface;
use PDO;

class MySQLConnector implements DatabaseInterface
{
    public function connect()
    {
        $user = 'root';
        $pass = 'root';
        try {
        return new PDO('mysql:host=localhost;dbname=test', $user, $pass);
        }
        catch (\PDOException $e)
        {
            throw new \Exception($e->getMessage());
        }
    }
}

Connector.php:

class Connector
{
    public function __construct(DatabaseInterface $db)
    {
        $this->db = $db;
        $this->db->connect();
    }
}

This feels quite over-engineered to me, when i can do the work of all three classes in one line of code.

echo new PDO('mysql:host=localhost;dbname=test', 'root', 'root');

The big question to ask here is: What do you gain?

Every class, every function, every line of code should be able to justify its existence.  More code means more cognitive load -- that's more stuff to have to think about whenever you want to change/add/fix something, so you don't want to add stuff willy-nilly.  It should either reduce overall complexity, add flexibility, or in some other way make the application better.

Here, you add two classes and an interface (adding complexity) and hard-code the connection params (reducing flexibility).  So what makes these classes an improvement over the one line above?  What do you gain from that indirection, that justifies the cost?

"Coding to an interface" doesn't necessarily mean using the interface keyword.  "Interface" here is used in the language-agnostic sense, and basically refers to an API.   When you code to an interface, you're simply using an API that doesn't make the consumer rely on implementation details.  When you use PDO, to some extent, you are already doing that.  The whole point of PDO is to hide a lot of the DBMS-specific stuff behind a common API, so all you have to know is the PDO API and general SQL stuff.

Since you're returning PDO objects, you're not really hiding much (if any) of the implementation.  I still have to know PDO, on top of how to use your API to get a usable PDO object.  You can't really use anything but PDO, because you've married your API to it.  And if i want to use a different database, i have to create a new class.

A more useful abstraction would hide the PDO objects from the user as well, so that someone using this code only needs to know two things -- your API, and general SQL stuff.  But at that point, you're starting to reinvent PDO.

As for the code itself, if you really want to keep it...

The name MySQLConnector suggests that it is a special type of Connector.  It's not -- it's a special type of DatabaseInterface.  Consider a different name.  I might suggest TestDatabase if you're going to keep the connection info hard-coded, or MySQLDatabase if you make it general enough to be used for other databases.
Never throw new \Exception.  There is never a good reason, other than laziness.  And here, even laziness wouldn't suffice -- if you're not handling an exception, you can simply not bother to catch it.  By catching it and throwing a whole new exception that's as generic as you can legally make an exception, you may be tossing out essential info -- like the previous exception's stack trace! -- that could save you days of debugging.

If you don't want the user to have to know that a PDO exception may occur, then create your own type and wrap the exception in that.

class DatabaseException extends \RuntimeException { }

...

try {
    return new \PDO ($dsn, $user, $pass);
}
catch (\PDOException $e) {
    throw new DatabaseException("Unable to connect", 0, $e);
}

Now the caller can react specifically to database exceptions, if it chooses.  It might try the connection again, for example.  When everything is an Exception, on the other hand, exceptions become all but indistinguishable, unless you resort to some ugliness like parsing the exception message (which is really meant for humans, not code).
$obj->db->connect() is a violation of what's called the Law of Demeter (LoD).  The Law basically says that the only things your code should be talking to are the things it can access without traversing more than one object other than $this*.  You should be telling $obj to connect, rather than having to know about $obj->db at all.  This keeps it easier to change one class without having to change code in dozens of different places.

* Most languages boil LoD down to "no more than one dot".  But that is a bit oversimplified -- particularly in PHP, which doesn't allow for an implicit $this.


String template using concrete implementation instead of external library Q1: What are the benefits of using any external library e.g Antlr3.StringTemplate?

Q2: What are the potential issues with the below implementation?

public class StringTemplate
{
    private string text = string.Empty;
    public StringTemplate(string template)
    {
        this.text = template;
    }

    public StringTemplate SetAttribute(string attribute, object value)
    {
        text = text.Replace($"${attribute}$", value?.ToString() ?? "");
        return this;
    }
    public override string ToString()
    {
        return text.ToString();
    }
}

And use it like below

 private void writeMsg()
 {
     var email = "Dear $name$...";//Loading from html file place holder used pattern $placeholder$.

     var template =
         new StringTemplate(email)
         .SetAttribute("name", "John");

     Console.WriteLine(template.ToString());
 }

One probable issue is performance, suppose there are 30 tokens to be replaced. And email template has a size of 50 kb. strings are immutable in C#/.Net. That means whenever you call StringTemplate.SetAttribute it creates a new string by manipulating existing and return the new string. suppose there are 30 tokens so it will create 50KB * 29 waste string. It will create more pressure on GC and GC will collect more often which will lead to poor performance. Better to use List<char> to do this kind of operation.


Logging module for use in other projects I'm trying to make a simple module I can paste into my various projects to provide basic logging capabilities. I'm hoping to become less dependent on print because:

Using logging will allow me to quickly enable and disable what's being logged
logging has built-in file and formatting utilities
The built-in levels of logging are helpful

Here's the log.py module:

import argparse
import logging
import sys

def parse_log_args(parser: argparse.ArgumentParser):
    default_level = 'INFO'
    parser.add_argument("-l", "--log", dest="loglevel",
                        choices=['DEBUG', 'INFO', 'WARNING', 'ERROR', 'CRITICAL'],
                        default=default_level,
                        help=f"Set the logging level (default: {default_level})",
                        type=str)

    parser.add_argument(
        '-lf', '--logfile',
        help="File for logging",
        dest="logfile",
        type=str
    )

    return parser

def conf_logging(logger: logging.Logger, loglevel: str, path=None):

    if loglevel.upper() not in ('CRITICAL', 'ERROR', 'WARNING', 'INFO', 'DEBUG'):
        raise ValueError("Invalid logging level")

    if path is None:
        console_formatter = logging.Formatter('[%(levelname)s] %(message)s')
        handler = logging.StreamHandler(sys.stdout)
        handler.setFormatter(console_formatter)
    else:
        file_formatter = logging.Formatter(
            ('%(asctime)s [%(levelname)s] %(name)s.%(funcName)s'
             '@ L%(lineno)d\n  %(message)s'))
        handler = logging.FileHandler(path, encoding='utf-8')
        handler.setFormatter(file_formatter)

    l_name = logging.getLevelName(loglevel)
    handler.setLevel(l_name)
    logger.setLevel(l_name)

    logger.addHandler(handler)

    return logger

Here's an example of its usage:

import logging
import argparse
from log import parse_log_args, conf_logging

logger = logging.getLogger(__name__)

parser = argparse.ArgumentParser()

parser = parse_log_args(parser)
args = parser.parse_args()
logger = conf_logging(logger, args.loglevel, args.logfile)

logger.debug("What")
logger.info("okay")

Which can be called as follows:

> python basic.py --log INFO
[INFO] okay
> python basic.py --log DEBUG
[DEBUG] What
[INFO] okay

I'm not super familiar with the logging module, so I was a little surprised I had to have to set the logging lever for both the logger and the handler. Is there a better, more explicit way of writing this?

It's more natural to say "--loglevel", ... than "--log", dest="loglevel", .... A log switch suggests that it takes an argument of a logfile output filename.

Not sure why you need dest for the logfile argument.

You're specifying type of str which is already the default, but maybe you did it to help with type annotations?

Raising "Invalid logging level" is maybe superfluous, better to let the underlying logging library deal with it. Looks like you're reaching across layers.

Naming your module log is perhaps a bit adventurous, as sometimes folks concisely write log.info() instead of logger.info().

In the example, I recommend deleting:

logger = logging.getLogger(__name__)

in favor of conf_logging(logging.getLogger(__name__), args.loglevel, args.logfile).
Perhaps you'd like to make it more convenient for callers, by defaulting loglevel, and letting a caller just pass in __name__ if that's all that is interesting.

Overall, it looks like a useful utility.


Python script using Minneapolis / St. Paul Metro Transit's API to tell you how long till next bus I have written a program which will tell you how long it is until the next bus on “BUS ROUTE” leaving from “BUS STOP NAME” going “DIRECTION” using the api defined at http://svc.metrotransit.org/.

Example:

$ metro.py "metro blue line" "target field station platform 1" "south"
6 minutes

I am very new to Python and wondering how this code could be better — more efficient, follow best practices, etc. What would you do differently?

import requests, sys, time

# need to have each: route, stop & direction
if len(sys.argv) != 4:
    sys.exit("Please be sure to enter a route, stop, and direction.")

route = sys.argv[1]
stop = sys.argv[2]
direction = sys.argv[3]

url = "http://svc.metrotransit.org/NexTrip/"

def getData (uri, key, value, arg):
    resp = requests.get(url + uri + "?format=json")
    data = resp.json()

    # default in case of invalid input
    dataID = -1

    while True:
        try: sys.exit(uri + str(resp.status_code))

        except:
            for item in data:
                if item[key].lower().find(arg) > -1:
                    dataID = item[value]
                    break
            return dataID

routeID = getData("Routes", "Description", "Route", route)

if routeID < 0:
    sys.exit(route + " is not a valid route.")

uri = routeID
directionID =  getData("Directions/" + uri, "Text", "Value", direction)

if directionID < 0:
    sys.exit(route + " does not go " + direction + ".")

uri += "/" + directionID
stopID = getData("Stops/" + uri, "Text", "Value", stop)

if stopID < 0:
    sys.exit(stop + " is not along " + route + " going " + direction + ".")

uri += "/" + stopID
timeID = getData(uri, "RouteDirection", "DepartureTime", direction)

if timeID != -1:
    #Get 10 digit timestamp from response, - from current time, and / by 60 to get minutes as an integer
    time = int((float(timeID[6:16]) - time.time()) // 60)
    if time > 1:
        print str(time) + " minutes"
    else:
        print "1 minute or less"

As Ludisposed said in his answer, yours is a very top-down approach. The minimum you should do is put this into a function, so you can re-use it. Here I rewrote your code quite significantly. Instead of returning -1, use the fact that a Python function returns None by default, which is a better safe value than -1.

I also completely re-wrote your getData function. Honestly, I have no idea what half of that was trying to do. All you need to do is iterate over the data. No need for a try: sys.exit(...) except: (which will always call what is inside the except, so why bother?). Also no need for a while True loop, if you are only ever going to go through it once.

I added the simple argparse parser and added the calling code under a if __name__ == "__main__": guard.

import argparse
import time
import sys
import requests

URL = "http://svc.metrotransit.org/NexTrip/{}?format=json"

def get(path, key, value, arg):
    resp = requests.get(URL.format(path))
    data = resp.json()
    for item in data:
        if arg in item[key].lower():
            return item[value]

def next_bus(route, stop, direction):
    route_id = get("Routes", "Description", "Route", route)
    if route_id is None:
        raise RuntimeError("{} is not a valid route.".format(route))

    direction_id = get("Directions/{}".format(route_id),
                           "Text", "Value", direction)
    if direction_id is None:
        raise RuntimeError("{} does not go {}.".format(route, direction))

    stop_id = get("Stops/{}/{}".format(route_id, direction_id), "Text", "Value", stop)
    if stop_id is None:
        raise RuntimeError("{} is not along {} going {}.".format(
            stop, route, direction))

    time_ = get("{}/{}/{}".format(route_id, direction_id, stop_id),
                    "RouteDirection", "DepartureTime", direction)
    if time_ is None:
        raise RuntimeError("Could not get next connection.")
    return int((float(time_[6:16]) - time.time()) // 60)

if __name__ == "__main__":
    parser = argparse.ArgumentParser()
    parser.add_argument("route")
    parser.add_argument("stop")
    parser.add_argument("direction")
    args = parser.parse_args()
    time = next_bus(**vars(args))
    if time > 1:
        print "{} minutes".format(time)
    else:
        print "1 minute or less"

Alternatively, you could make this a class and use property for the different ids. This would allow you to have an object like trip_home = Connection(route, direction, stop), on which you could call trip_home.next to get the latest connection.

The different ids are looked up upon the initialization of the class. Calls to trip_home.next will only need to get the next connection. This assumes that the ids do not change during the runtime of the program, of course.

import argparse
import time
import sys
import requests

class Connection:
    url = "http://svc.metrotransit.org/NexTrip/{}?format=json"

    def __init__(self, route, stop, direction):
        self.route = route
        self.stop = stop
        self.direction = direction
        self.route_id = self.get_route_id()
        self.direction_id = self.get_direction_id()
        self.stop_id = self.get_stop_id()

    def __str__(self):
        return "{self.route} from {self.stop} due {self.direction}. Next connection in {self.next} minutes.".format(self=self)

    def get(self, path, key, value, arg):
        resp = requests.get(self.url.format(path))
        data = resp.json()
        for item in data:
            if arg in item[key].lower():
                return item[value]

    def get_route_id(self):
        id_ = self.get("Routes", "Description", "Route", self.route)
        if id_ is None:
            raise RuntimeError(
                "{self.route} is not a valid route.".format(self=self))
        return id_

    def get_direction_id(self):
        id_ = self.get(
            "Directions/{self.route_id}".format(self=self), "Text", "Value", self.direction)
        if id_ is None:
            raise RuntimeError("{self.route} does not go {self.direction}.".format(
                self=self))
        return id_

    def get_stop_id(self):
        id_ = self.get(
            "Stops/{self.route_id}/{self.direction_id}".format(self=self), "Text", "Value", self.stop)
        if id_ is None:
            raise RuntimeError(
                "{self.stop} is not along {self.route} going {self.direction}.".format(self=self))
        return id_

    @property
    def next(self):
        time_ = self.get("{self.route_id}/{self.direction_id}/{self.stop_id}".format(self=self),
                         "RouteDirection", "DepartureTime", self.direction)
        if time_ is None:
            raise RuntimeError(
                "Could not find next connection on route {self.route} from {self.stop} due {self.direction}".format(self=self))
        return int((float(time_[6:16]) - time.time()) // 60)

if __name__ == "__main__":
    parser = argparse.ArgumentParser()
    parser.add_argument("route")
    parser.add_argument("stop")
    parser.add_argument("direction")
    args = parser.parse_args()
    trip_home = Connection(args.route, args.stop, args.direction)
    print trip_home


Filter by multiple options - also only if not empty Here's how my json looks like. I'm trying to write a function which filters based on participantCriteria.criteriaType & participantCriteria._id

participantCriteria ": [ {
  "_id": "9be6c6299cca48f597fe71bc99c37b2f",
  "caption": "XXXXXXXXXXX",
  "criteriaType": "YYYYYY",
  "inputSources": [{
      "_id": "66be4486ffd3431eb60e6ea6326158fe",
      "caption": "docId",
      "participantCriteriaId": "9be6c6299cca48f597fe71bc99c37b2f",
      "referenceId": null,
      "type": "identifier"
    },
    {
      "_id": "1ecdf410b3314865be2b52ca9b4c8539",
      "caption": "limit",
      "participantCriteriaId": "9be6c6299cca48f597fe71bc99c37b2f",
      "referenceId": null,
      "type": "number"
    }
  ]
},

Here is the function I have written to filter out the JSON above.

filterFunction(id) // returns the above JSON
.then((criteria) => {
  let filteredCriteria = (participantType ? criteria.filter(({ criteriaType }) =>
    criteriaType === participantType) : criteria); // if participantType is provided, filter the JSON where criteriaType === participantType

  filteredCriteria = (participantCriteriaId ? filteredCriteria.filter(({ _id }) =>
    _id === participantCriteriaId) : filteredCriteria) // if participantCriteriaId is provided, filter the json where _id === participantCriteriaId
    .map(({ _objectId: _id, caption, criteriaType, inputSources }) => ({
      _id,
      caption,
      criteriaType,
      inputSources: inputSources
        .map(({ _objectId, caption: inputCaption, isLookup, lookupId, picklistId, type }) => {
          let inputSourceType = type;
          if (type === 'identifier') { inputSourceType = (isLookup && lookupId) ? 'lookup' : 'key'; }
          return {
            _id: _objectId,
            caption: inputCaption,
            participantCriteriaId: _id,
            referenceId: (lookupId || picklistId) || null,
            type: inputSourceType,
          };
        }),
    }));
  res.json({
    data: {
      participantCriteria: filteredCriteria,
    },
  });
  return undefined;
})

Is there a better way to achieve this filtering ? or Maybe a way to chain the two filters I have added above (the lines where I have added comments).

Is this what you're after?

criteria.filter({ _id, criteriaType } => (
    (!participantType || participantType === criteriaType)
    &&
    (!participantCriteriaId || participantCriteriaId === _id)
));

If participantType is empty, it will be coerced to true by the negation, which short-circuits the || operator, so it won't check the right-hand side of the condition. Same for participantCriteriaId.

Note: This type of condition is a common pattern in boolean logic. Generally speaking, conditions that can be phrased as:

  If A is true, then check if B is true; otherwise, ignore this condition

Can be transcribed as:

  (not A) or B

Which in code is:

!a || b

In your case, the condition is "if the search filter is populated, then check for equality, otherwise ignore", which is transcribed as !filter || property === filter.


A list builder or list accumulator Imagine a UTF-8 decoder that takes a list of bytes and returns human-readable code points like:

> (utf-8->human-readable-code-points '(32 32 195 160 160))
("u+0020" "u+0020" "u+00E0" ("Error: trailing byte without preceding start byte" 160))

This list builder functionality is meant for that function:

;; a list builder will allow us to build a list
;; by successively adding to the end without retracing
;; the list each time. And without having a stack of
;; cons calls to unwind. Will allow constructing a list
;; in order in a tail call fashion
;; (list-builder 'add! item) -- returns list so far
;; (list-builder 'list) -- returns list so far
(define (make-list-builder)
  (let [(list-under-construction '())
        (last-cons-cell '())]
    (lambda (dispatch . parameters)
      (case dispatch
        [(add!) (if (null? list-under-construction)
                   (begin
                     (set! list-under-construction (list (car parameters)))
                     (set! last-cons-cell list-under-construction))
                   (let ((new-cons-cell (list (car parameters))))
                     (set-cdr! last-cons-cell new-cons-cell)
                     (set! last-cons-cell new-cons-cell)))
         list-under-construction]
        [(list) list-under-construction]
        [else (error "unmatched dispatch" dispatch)]))))

Sample use:

> (define lb (make-list-builder))
> (lb 'list)
()
> (lb 'add! "here's an atom")
("here's an atom")
> (lb 'add! '(here's a list))
("here's an atom" (here 's a list))

Questions for review:

Is there a standard scheme function or way of doing this that I missed? The problem I am trying to ultimately solve can't be solved by a map because a map produces a list of the same size as the input, and map does not guarantee the order that elements of a Lisp or processed. Googling and searching Stack Overflow for scheme list builder or scheme list accumulater did not return useful results.
Is make-list-builder expressed in idiomatic Scheme?
How would you improve make-list-builder, or what would you do differently?

Wow, so sorry I missed seeing this question! (Normally I get emails when someone posts something tagged with scheme, but didn't see this one till now. :-()

So, the idiomatic way to build a list in Scheme is to cons elements to the front each time, then reverse the whole list at the end. This is still O(n) on the number of items to insert, so you're no worse off compared to the set-cdr! solution.

However, for this situation, a better solution (IMHO) in your case is actually to use unfold, which takes any given object (not necessarily a list) and results in a list. This is perfect for your situation because you are processing variable numbers of items in your input list at each step, which would make it a poor fit for map, fold, and the like (which processes exactly one item at each step).

Here's an example (require SRFIs 1, 8, and 60):

(define (decode-utf8 bytes)
  (define (invalid byte)
    (list 'invalid byte))
  (define (non-shortest val)
    (list 'non-shortest val))
  (define (end-of-input val)
    (list 'end-of-input val))

  (define (decode-start bytes)
    (define byte (car bytes))
    (define rest (cdr bytes))
    (cond ((< byte #x80) (values (integer->char byte) rest))
          ((< byte #xC0) (values (invalid byte) rest))
          ((< byte #xE0) (decode-rest 1 #x80 (logand byte #x1F) rest))
          ((< byte #xF0) (decode-rest 2 #x800 (logand byte #x0F) rest))
          ((< byte #xF8) (decode-rest 3 #x10000 (logand byte #x07) rest))
          (else (values (invalid byte) rest))))
  (define (decode-rest count minval val bytes)
    (cond ((zero? count) (values ((if (< val minval)
                                      non-shortest
                                      integer->char) val) bytes))
          ((null? bytes) (values (end-of-input val) bytes))
          (else
            (let ((byte (car bytes))
                  (rest (cdr bytes)))
              (cond ((< byte #x80) (values (invalid byte) rest))
                    ((< byte #xC0)
                     (decode-rest (- count 1) minval
                                  (+ (ash val 6) (logand byte #x3F))
                                  rest))
                    (else (values (invalid byte) rest)))))))

  (define (decode bytes)
    (receive (value rest) (decode-start bytes)
      value))
  (define (next bytes)
    (receive (value rest) (decode-start bytes)
      rest))

  (unfold null? decode next bytes))

The real action happens in decode-start and decode-rest, of course; both functions return two values:

the character decoded (or an error)
a reference to the next byte to process

The decode function extracts the first value, while next extracts the second.

If you still want to use a list builder, that's fine too. Here's a simple version: it builds the list when given no arguments, and otherwise appends the arguments given:

(define (make-list-builder)
  (define cur '())
  (case-lambda
   (() (reverse cur))
   ((item) (set! cur (cons item cur)))    ; redundant
   (items (set! cur (append-reverse items cur)))))

(where append-reverse comes from SRFI 1). Note that append-reverse with only one item is identical to cons, so the cons line above is redundant; it just demonstrates how you would do it if you want to add items one by one.

If you want to be able to send messages like in your version, here's how one might implement it:

(define (make-list-builder)
  (define cur '())
  (lambda (msg . args)
    (case msg
      ((add) (set! cur (append-reverse args cur)))
      ((get) (reverse cur))
      ((clear) (set! cur '())))))

In both cases, we keep track of a single variable only, not the start and end of list, because we are prepending elements rather than appending them.

There is still a valid use for the start-and-end approach you had, though: it's a common approach for implementing a queue. You enqueue by using the end reference, set-cdr!ing the new value in, then resetting the end reference; and of course, you dequeue by using the start reference. Here's an example (zero arguments means dequeue; otherwise enqueue the given arguments):

(define (make-queue)
  (define start (list 'queue-head))
  (define end start)
  (case-lambda
   (() (when (null? (cdr start))
         (error "empty queue"))
       (let ((result (cadr start)))
         (set-cdr! start (cddr start))
         (when (null? (cdr start))
           (set! end start))
         result))
   (items (set-cdr! end items)
          (set! end (last-pair end)))))

(here, last-pair comes from SRFI 1).


Replace first occurrence of pattern in file Here's my attempt to replace first occurrence of pattern in file with spaces. Effectively deleting it, and hopefully will allow me to "delete" it in a large file, without rewriting it.

#define MAX_LINE_LENGTH
int removeFirstOccurenceInFile(const char* fileName, const char* pattern) {
  FILE* f = fopen(fileName,"r+");
  if (f == NULL) {
     perror("Can't open input file");
     return 0;
  }
  char buf[MAX_LINE_LENGTH] = {'\0'};
  char spaces[MAX_LINE_LENGTH];

  int unreadBytes = 0;
  char* patInBuf = NULL;
  while (1) {
    if (patInBuf != NULL) break;
    unreadBytes += strlen(buf);
    if (fgets(buf,sizeof(buf),f) == NULL) break;
    patInBuf = strstr(buf,pattern);
  }
  if (patInBuf == NULL) {
    fprintf(stderr,"No '%s' found in '%s'\n",pattern,fileName);
    fclose(f);
    return 0;
  }
  int delFrom = patInBuf-buf;

  memset(spaces,' ',delFrom);
  fseek(f,unreadBytes+delFrom, SEEK_SET);
  fwrite(spaces,1,strlen(pattern),f);

  fclose(f);
  return 1;
}

I'm not sure about the error handling. I considered using goto instead of the usual error handling:

  if (patInBuf == NULL) {
    fprintf(stderr,"No '%s' found in '%s'\n",pattern,fileName);
    retval = -1;
    goto cleanup;
  }
...
cleanup:
  fclose(f);
  return retval;

Are there other problems? Will that indeed be efficient?

I think you have the error codes the other way around.  It's more common to return 0 on success and 1 (or any non-zero number) on failure, especially for debugging. If you actually need to deviate from this, then comments could help with specification.

As for the goto, it is good to avoid using it whenever possible.  Based on that little snippet, it looks like you could've used a function instead, at least if other this were needed in other places.


callback function with no dynamic memory allocation I tried to make a callback type for embedded, I know std::function and lambda but I know that there is some dynamic memory allocation that I want to avoid, I came to this implementation:

#pragma once
namespace interfaces
{
    template <class returnType, class ... parameterType>
    class ICallable
   {
       public:
       virtual returnType execute(parameterType ... args) = 0;
   };
}

static callback :

#pragma once

#include "ICallable.hpp"

template <class ReturnType, class ... ParameterType>
class StaticCallback : public interfaces::ICallable <ReturnType,ParameterType...>
{
   public:
      typedef ReturnType (*CallbackFunction)(ParameterType ...);

      constexpr StaticCallback(CallbackFunction function) : m_function(function)
      {
      }

   virtual ReturnType execute(ParameterType ... args) override final
   {
       return m_function(args...);
   }

 private:
    CallbackFunction m_function;
};

and callback with an object

#pragma once
#include "volund/interfaces/system/ICallable.hpp"

 template <class objectType,class returnType,class ... parameterType>
 class ObjectCallback : public interfaces::ICallable<returnType, parameterType ...>
 {
    public:
      typedef returnType (objectType::*Callbackfunction)(parameterType ... args);
      constexpr ObjectCallback(objectType& self, Callbackfunction function) : m_object(&self), m_functionToCall(function)
      {
      }
      constexpr ObjectCallback(objectType *self, Callbackfunction function) : m_object(self), m_functionToCall(function)
      {
      }
      virtual returnType execute(parameterType ... params) override final
      {
          return (*m_object.*m_functionToCall)(params...);
      }

    private:
       objectType* m_object;
       Callbackfunction m_functionToCall;
  };

I'm not sure about the reason of dynamic allocation in std::function, so I'm wondering if there is no pitfall I didn't see in my solution.
If someone have any tips or advice that would be great =)

EDIT :

This is how i use it, typically from a driver that trigger an interrupt and user code register a callback to define some code to be executed on interrupt :

class driver
{
    public:
      void setCallback(ICallable* callback)
      {
         m_callback = callback;
      }

      driver& get()
      {
         return s_self;
      }

     private:
       static s_self;

       Icallable* m_callback;

       static void interrupt()
       {
          s_self.m_callback->execute();
       }
  }

  class user
  {
      public:
        void task()
        {
            driver::get().setCallback(&m_callFromDriver);
            while(true)
            {
               //do something
            }
        }
       private:
         ObjectCallback m_callFromDriver = ObjectCallback(this, &user::onEvent);

        void onEvent()
        {
            //do something
        }
  }

lambda's don't alloc, std::function only allocates memory if the object it is wrapping is 'too large'.  The VC++ 2019 std::function impl defines 'too large' as being more than 7 * sizeof(void*) bytes in size.  Other impl may use a different limit.  This is big enough to handle many (most ?) cases w/o allocating.

You could make your own function class, based on std::function, and put static_assert's in the constructors and assignment op's to ensure none of your code that uses your function would trigger an alloc; or if it would you could increase the 'too large' size of your function class.  This would give you something as flexible as std::function, and the control over alloc that you want.

[EDIT]

If you don't want to use the VC++ version, the gcc one is at:

https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/bits/std_function.h

To start you can copy the file and put static_assert's before lines 160 and 237 to catch any cases that would require an alloc (likely also have to check dependencies in other gcc .h files).

... it looks like this version doesn't reserve much space i.e. would alloc in many cases.  You'd have to make a couple other tweaks to increase the no_copy size.


Read until EOF and realloc as needed This is a function that reads until EOF and reallocs as it goes.

char *
read_to_end(int fd, int *bread)
{
    ssize_t size, nread, rc;
    char *buf, *tmp;

    nread = 0;
    size = MINLEN;
    buf = malloc(size);

    while ((rc = read(fd, buf + nread, size - nread))) {
        if (rc < 0) {
            if (errno == EINTR)
                continue;

            goto error;
        }

        nread += rc;

        /* See if we need to expand. */
        if (size - nread < MINLEN) {
            size *= 2;

            /* Make sure realloc doesn't fail. */
            if (!(tmp = realloc(buf, size)))
                goto error;

            buf = tmp;
        }
    }

    /* Shrink if necessary. */
    if (size != nread && !(tmp = realloc(buf, nread)))
        goto error;

    buf = tmp;
    *bread = nread;
    return buf;

error:
    free(buf);
    return NULL;
}

Do you have any improvement suggestions?

Seeing this is 'unanswered' here's my tuppence worth:

If you are reading a file I suggest you get the file size first, eg using fstat(), allocate a buffer of the correct size and read into that.

Alternatively, if the file is big and you have OS support, consider mapping the file into memory using mmap().  This maps the file into virtual memory but does not read it immediately.  As you access the memory into which the file is mapped the file is read into that address space for you (in the background) by the OS.

On your code, I have a few comments:

If you have a stderr, perhaps add a call to perror() on error.
It looks as if you always re-alloc, even if the initial buffer is big enough.
If zero bytes are read, you will realloc(buf, 0).  realloc man page says this is ok.
Add a bracket around size != nread - I always have to check operator precedence in statements like this and I guess I am not alone.  Brackets save the reader the trouble.
I would prefer *bread to be set to 0 on failure.  Should bread be size_t ?


(sort of) casting types in Ruby: changing class of an instance I defined the class Rectangle:

class Rectangle
  attr_reader :b, :h
  def initialize(b, h)
    @b = b
    @h = h
  end
  def area
    @b*@h
  end
  def to_s
    "Rectangle #{@b}x{@h}"
  end
end

and its subclass Square:

class Square < Rectangle

  attr_reader :s
  def initialize(s)
    @s = s
    super(@s, @s)
  end
  def to_s
    "Square of side #{@s}"
  end
end

Now, let's say I defined a Rectangle object r = Rectangle.new(5, 5), but actually it should be a Square because I'd like r.to_s to return "Square of side 5".

I can define a to_square method in the Rectangle class that returns a Square object equivalent to r, but, is it possible to write a to_square! method that would actually change the r class to Square without returning another object, so that r.class would now return Square instead of Rectangle?

And, what if I'd like:

Square(r)

to return the Square equivalent object of r, just like:

Integer("3")

which returns the Integer 3?

Is that possible? And, if so, how?

First off, why bother with a separate Square class?

Personally, I'd make a Rectangle class' to_square! method simply set both width and height to Math.sqrt(area). The result is still a Rectangle, just a square one.

The Rectangle's initializer method could also be written with an optional second argument; If there's only one argument, use that for both width and height, and you have a square.

Furthermore, I'd add a square? method. Checking obj.square? is, in my view, preferable to checking obj.class == Square. You could also use square? in to_s to return one string or the other.

To answer your question, though, I don't know of any way to just change an instance's class. You could however experiment with including different modules depending on whether the rectangle in question is a square or not. But that all seems pretty complex.

Finally, if you want a Square method, simply define one:

def Square(side)
  Rectangle.new(side, side)
end

Even if you have a Square class already, you can still have a method of the same name:

class Square
  # ...
end

def Square(side) # no naming conflict
  Square.new(side)
end


Find Duplicate with 2 criteria My code below find the duplicates based on 2 criteria:

The first criteria is the Name in Column A
The second criteria is the Country in Column D

Sheet("RawData") :

Sheets("Conso") after the macro:

What I am looking is to increase the speed of this code, because I am working with more than 150K rows and it takes hours.

Sub MDMDuplicates()

Dim WB As Workbook
Dim wsRawData As Worksheet, wsConso As Worksheet
Dim i As Long, j As Long, Lastrow As Long, LastrowConso as Long
Dim SupNameToCheck As String, ConsoSupplierDUNS As String, ConsoSupplierMDM As String, ConsoSupplierNAME As String

Set WB = ThisWorkbook
Set wsRawData = WB.Sheets("RawData")
Set wsConso = WB.Sheets("Conso")

Lastrow = wsRawData.Range("A" & Rows.Count).End(xlUp).Row

Application.ScreenUpdating = False
With wsRawData

For i = 2 To Lastrow

SupNameToCheck = .Cells(i, "A").Value
SupCountryToCheck = .Cells(i, "D")
ConsoSupplierDUNS = ""
ConsoSupplierMDM = ""
ConsoSupplierNAME = ""

j = Lastrow
    Do
        If i <> j And SupNameToCheck = .Cells(j, "A") And SupCountryToCheck = .Cells(j, "D") Then

            If ConsoSupplierNAME = "" Then

            ConsoSupplierDUNS = .Cells(i, "B") & "," & .Cells(j, "B")
            ConsoSupplierMDM = .Cells(i, "C") & "," & .Cells(j, "C")
            ConsoSupplierNAME = SupNameToCheck & "," & .Cells(j, "A")

            Else

            ConsoSupplierDUNS = .Cells(j, "B") & "," & ConsoSupplierDUNS
            ConsoSupplierMDM = .Cells(j, "C") & "," & ConsoSupplierMDM
            ConsoSupplierNAME = .Cells(j, "A") & "," & ConsoSupplierNAME

            End If

           .Cells(j, "A").EntireRow.Delete

        End If

        j = j - 1

      Loop Until j = 1

LastrowConso = wsConso.Range("A" & Rows.Count).End(xlUp).Row + 1
If Not ConsoSupplierNAME = "" Then
        wsConso.Cells(LastrowConso, "B") = ConsoSupplierDUNS
        wsConso.Cells(LastrowConso, "C") = ConsoSupplierMDM
        wsConso.Cells(LastrowConso, "A") = ConsoSupplierNAME
    Else
        wsConso.Cells(LastrowConso, "B") = .Cells(i, "B")
        wsConso.Cells(LastrowConso, "C") = .Cells(i, "C")
        wsConso.Cells(LastrowConso, "A") = SupNameToCheck
 End If

Next i
Application.ScreenUpdating = True

End With
End Sub

The first thing to do before making any changes to this code, is to improve its readability. I saw .Cells and thought "oh we're in a With block" ...and then had to look three times to find the With statement.

The keyword here, is indentation.

Sub DoSomething()
....
....With SomeObject
....|...
....|...If SomeCondition Then
....|...|...DoActionOne
....|...Else
....|...|...DoActionTwo
....|...End If
....|...
....|...Do
....|...|...DoActionThree
....|...Loop
....|...
....End With
....
....For i = 1 To 10
....|....
....|....DoActionFour
....|....
....Next
....
End Sub

When the Else blocks don't line up with the corresponding If statement, or when a Loop keyword doesn't line up with its corresponding Do keyword, or when nested blocks line up in column 1, you basically set yourself up for making a change that introduces a bug.

Proper indentation cannot be underestimated.

You have redundant object references:

If WB.Sheets("RawData") has CodeName Sheet1, name it RawDataSheet and use that reference instead.
If WB.Sheets("Conso") has CodeName Sheet2, name it ConsoSheet and use that reference instead.

"CodeName" is a property of all sheet objects in Excel VBA; there's a global object reference pointing to these, readily available for you to use - no need to fetch it from WB.Sheets collection, which by the way could give you non-worksheet objects, since the Sheets collection includes Charts, among other sheet types. You probably meant to use the WB.Worksheets collection instead. But then again, you don't need it - just use the global object VBA gives you for free instead.

Application.ScreenUpdating = False

Whenever you set that value to False, you need to handle runtime errors and make sure that whatever happens, the method can't exit without setting it back to True. Otherwise Excel will look "frozen" when it's actually completely responsive, just not redrawing itself... because you told it not to.

Don't assume things won't blow up. They always do.

Sub DoSomething()

    On Error GoTo CleanFail

    Application.ScreenUpdating = False

    '...

CleanExit:
    Application.ScreenUpdating = True
    Exit Sub
CleanFail:
    'handle errors
    Resume CleanExit
End Sub

Now that's great, but you're deleting rows, which should trigger a recalculation if calculation is set to xlAutomatic. Consider not only turning off ScreenUpdating, but also setting Calculation to xlManual while you're doing your thing.